Re: [edk2-devel] [PATCH v3 8/8] MdeModulePkg: Use SecureBootVariableLib in PlatformVarCleanupLib.

["Sunny Wang" <Sunny.Wang@arm.com>]

Hi Greg,
Why can't we make AuthVariableLib support DXE_DRIVER?

Best Regards,
Sunny Wang

-----Original Message-----
From: Grzegorz Bernacki <gjb@semihalf.com>
Sent: Monday, June 21, 2021 5:59 PM
To: devel@edk2.groups.io; Grzegorz Bernacki <gjb@semihalf.com>
Cc: gaoliming@byosoft.com.cn; leif@nuviainc.com; ardb+tianocore@kernel.org; Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>; Sunny Wang <Sunny.Wang@arm.com>; Marcin Wojtas <mw@semihalf.com>; upstream@semihalf.com; Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Xu, Min M <min.m.xu@intel.com>; Laszlo Ersek <lersek@redhat.com>; Sami Mujawar <Sami.Mujawar@arm.com>; afish@apple.com; ray.ni@intel.com; jordan.l.justen@intel.com; rebecca@bsdio.com; grehan@freebsd.org; Thomas Abraham <thomas.abraham@arm.com>; chasel.chiu@intel.com; nathaniel.l.desimone@intel.com; eric.dong@intel.com; michael.d.kinney@intel.com; zailiang.sun@intel.com; yi.qian@intel.com; graeme@nuviainc.com; Radosław Biernacki <rad@semihalf.com>; Pete Batard <pete@akeo.ie>
Subject: Re: [edk2-devel] [PATCH v3 8/8] MdeModulePkg: Use SecureBootVariableLib in PlatformVarCleanupLib.

Hi,

I moved CreateTimeBasedPayload() to AuthVariableLib, but then I cannot
use it in SecureBootConfigDxe, cause AuthVariableLib does not support
DXE_DRIVER.
So:
- having that function only in AuthVariableLib does not work
- having that function only in SecureBootVariableLib, causes a lot of
changes in platform DSCs files and also causes MdeModulePkg to be
depended on SecurityPkg

Right now I tend to roll back the changes related to
CreateTimeBasedPayload() and just let the modules to have its own copy
of that function. What do you think?
thanks,
greg

pt., 18 cze 2021 o 10:03 Grzegorz Bernacki via groups.io
<gjb=semihalf.com@groups.io> napisał(a):
>
> Hi,
>
> Thanks for the comment, I will move that function to AuthVariableLib.
> greg
>
> czw., 17 cze 2021 o 04:35 gaoliming <gaoliming@byosoft.com.cn> napisał(a):
> >
> > Grzegorz:
> >   MdeModulePkg is generic base package. It should not depend on SecurityPkg.
> >
> >   I agree CreateTimeBasedPayload() is the generic API. It can be shared in
> > the different modules.
> >   I propose to add it into MdeModulePkg AuthVariableLib.
> >
> > Thanks
> > Liming
> > > -----邮件原件-----
> > > 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Grzegorz
> > > Bernacki
> > > 发送时间: 2021年6月14日 17:43
> > > 收件人: devel@edk2.groups.io
> > > 抄送: leif@nuviainc.com; ardb+tianocore@kernel.org;
> > > Samer.El-Haj-Mahmoud@arm.com; sunny.Wang@arm.com;
> > > mw@semihalf.com; upstream@semihalf.com; jiewen.yao@intel.com;
> > > jian.j.wang@intel.com; min.m.xu@intel.com; lersek@redhat.com;
> > > sami.mujawar@arm.com; afish@apple.com; ray.ni@intel.com;
> > > jordan.l.justen@intel.com; rebecca@bsdio.com; grehan@freebsd.org;
> > > thomas.abraham@arm.com; chasel.chiu@intel.com;
> > > nathaniel.l.desimone@intel.com; gaoliming@byosoft.com.cn;
> > > eric.dong@intel.com; michael.d.kinney@intel.com; zailiang.sun@intel.com;
> > > yi.qian@intel.com; graeme@nuviainc.com; rad@semihalf.com; pete@akeo.ie;
> > > Grzegorz Bernacki <gjb@semihalf.com>
> > > 主题: [edk2-devel] [PATCH v3 8/8] MdeModulePkg: Use
> > > SecureBootVariableLib in PlatformVarCleanupLib.
> > >
> > > This commits removes CreateTimeBasedPayload() function from
> > > PlatformVarCleanupLib and uses exactly the same function from
> > > SecureBootVariableLib.
> > >
> > > Signed-off-by: Grzegorz Bernacki <gjb@semihalf.com>
> > > ---
> > >  MdeModulePkg/Library/PlatformVarCleanupLib/PlatformVarCleanupLib.inf |
> > > 2 +
> > >  MdeModulePkg/Library/PlatformVarCleanupLib/PlatVarCleanup.h
> > > |  1 +
> > >  MdeModulePkg/Library/PlatformVarCleanupLib/PlatVarCleanupLib.c
> > > | 84 --------------------
> > >  3 files changed, 3 insertions(+), 84 deletions(-)
> > >
> > > diff --git
> > > a/MdeModulePkg/Library/PlatformVarCleanupLib/PlatformVarCleanupLib.inf
> > > b/MdeModulePkg/Library/PlatformVarCleanupLib/PlatformVarCleanupLib.inf
> > > index 8d5db826a0..493d03e1d8 100644
> > > ---
> > > a/MdeModulePkg/Library/PlatformVarCleanupLib/PlatformVarCleanupLib.inf
> > > +++
> > > b/MdeModulePkg/Library/PlatformVarCleanupLib/PlatformVarCleanupLib.inf
> > > @@ -34,6 +34,7 @@
> > >  [Packages]
> > >    MdePkg/MdePkg.dec
> > >    MdeModulePkg/MdeModulePkg.dec
> > > +  SecurityPkg/SecurityPkg.dec
> > >
> > >  [LibraryClasses]
> > >    UefiBootServicesTableLib
> > > @@ -44,6 +45,7 @@
> > >    PrintLib
> > >    MemoryAllocationLib
> > >    HiiLib
> > > +  SecureBootVariableLib
> > >
> > >  [Guids]
> > >    gEfiIfrTianoGuid                  ## SOMETIMES_PRODUCES   ##
> > > GUID
> > > diff --git a/MdeModulePkg/Library/PlatformVarCleanupLib/PlatVarCleanup.h
> > > b/MdeModulePkg/Library/PlatformVarCleanupLib/PlatVarCleanup.h
> > > index c809a7086b..94fbc7d2a4 100644
> > > --- a/MdeModulePkg/Library/PlatformVarCleanupLib/PlatVarCleanup.h
> > > +++ b/MdeModulePkg/Library/PlatformVarCleanupLib/PlatVarCleanup.h
> > > @@ -18,6 +18,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> > >  #include <Library/MemoryAllocationLib.h>
> > >  #include <Library/HiiLib.h>
> > >  #include <Library/PlatformVarCleanupLib.h>
> > > +#include <Library/SecureBootVariableLib.h>
> > >
> > >  #include <Protocol/Variable.h>
> > >  #include <Protocol/VarCheck.h>
> > > diff --git
> > > a/MdeModulePkg/Library/PlatformVarCleanupLib/PlatVarCleanupLib.c
> > > b/MdeModulePkg/Library/PlatformVarCleanupLib/PlatVarCleanupLib.c
> > > index 3875d614bb..204f1e00ad 100644
> > > --- a/MdeModulePkg/Library/PlatformVarCleanupLib/PlatVarCleanupLib.c
> > > +++ b/MdeModulePkg/Library/PlatformVarCleanupLib/PlatVarCleanupLib.c
> > > @@ -319,90 +319,6 @@ DestroyUserVariableNode (
> > >    }
> > >  }
> > >
> > > -/**
> > > -  Create a time based data payload by concatenating the
> > > EFI_VARIABLE_AUTHENTICATION_2
> > > -  descriptor with the input data. NO authentication is required in this
> > > function.
> > > -
> > > -  @param[in, out] DataSize          On input, the size of Data buffer in
> > > bytes.
> > > -                                    On output, the size of data
> > > returned in Data
> > > -                                    buffer in bytes.
> > > -  @param[in, out] Data              On input, Pointer to data buffer to
> > > be wrapped or
> > > -                                    pointer to NULL to wrap an
> > > empty payload.
> > > -                                    On output, Pointer to the new
> > > payload date buffer allocated from pool,
> > > -                                    it's caller's responsibility to free
> > > the memory after using it.
> > > -
> > > -  @retval EFI_SUCCESS               Create time based payload
> > > successfully.
> > > -  @retval EFI_OUT_OF_RESOURCES      There are not enough memory
> > > resourses to create time based payload.
> > > -  @retval EFI_INVALID_PARAMETER     The parameter is invalid.
> > > -  @retval Others                    Unexpected error happens.
> > > -
> > > -**/
> > > -EFI_STATUS
> > > -CreateTimeBasedPayload (
> > > -  IN OUT UINTN      *DataSize,
> > > -  IN OUT UINT8      **Data
> > > -  )
> > > -{
> > > -  EFI_STATUS                        Status;
> > > -  UINT8                             *NewData;
> > > -  UINT8                             *Payload;
> > > -  UINTN                             PayloadSize;
> > > -  EFI_VARIABLE_AUTHENTICATION_2     *DescriptorData;
> > > -  UINTN                             DescriptorSize;
> > > -  EFI_TIME                          Time;
> > > -
> > > -  if (Data == NULL || DataSize == NULL) {
> > > -    return EFI_INVALID_PARAMETER;
> > > -  }
> > > -
> > > -  //
> > > -  // At user physical presence, the variable does not need to be signed
> > but
> > > the
> > > -  // parameters to the SetVariable() call still need to be prepared as
> > > authenticated
> > > -  // variable. So we create EFI_VARIABLE_AUTHENTICATED_2 descriptor
> > > without certificate
> > > -  // data in it.
> > > -  //
> > > -  Payload     = *Data;
> > > -  PayloadSize = *DataSize;
> > > -
> > > -  DescriptorSize = OFFSET_OF (EFI_VARIABLE_AUTHENTICATION_2,
> > > AuthInfo) + OFFSET_OF (WIN_CERTIFICATE_UEFI_GUID, CertData);
> > > -  NewData = (UINT8 *) AllocateZeroPool (DescriptorSize + PayloadSize);
> > > -  if (NewData == NULL) {
> > > -    return EFI_OUT_OF_RESOURCES;
> > > -  }
> > > -
> > > -  if ((Payload != NULL) && (PayloadSize != 0)) {
> > > -    CopyMem (NewData + DescriptorSize, Payload, PayloadSize);
> > > -  }
> > > -
> > > -  DescriptorData = (EFI_VARIABLE_AUTHENTICATION_2 *) (NewData);
> > > -
> > > -  ZeroMem (&Time, sizeof (EFI_TIME));
> > > -  Status = gRT->GetTime (&Time, NULL);
> > > -  if (EFI_ERROR (Status)) {
> > > -    FreePool (NewData);
> > > -    return Status;
> > > -  }
> > > -  Time.Pad1       = 0;
> > > -  Time.Nanosecond = 0;
> > > -  Time.TimeZone   = 0;
> > > -  Time.Daylight   = 0;
> > > -  Time.Pad2       = 0;
> > > -  CopyMem (&DescriptorData->TimeStamp, &Time, sizeof (EFI_TIME));
> > > -
> > > -  DescriptorData->AuthInfo.Hdr.dwLength         = OFFSET_OF
> > > (WIN_CERTIFICATE_UEFI_GUID, CertData);
> > > -  DescriptorData->AuthInfo.Hdr.wRevision        = 0x0200;
> > > -  DescriptorData->AuthInfo.Hdr.wCertificateType =
> > > WIN_CERT_TYPE_EFI_GUID;
> > > -  CopyGuid (&DescriptorData->AuthInfo.CertType, &gEfiCertPkcs7Guid);
> > > -
> > > -  if (Payload != NULL) {
> > > -    FreePool (Payload);
> > > -  }
> > > -
> > > -  *DataSize = DescriptorSize + PayloadSize;
> > > -  *Data     = NewData;
> > > -  return EFI_SUCCESS;
> > > -}
> > > -
> > >  /**
> > >    Create a counter based data payload by concatenating the
> > > EFI_VARIABLE_AUTHENTICATION
> > >    descriptor with the input data. NO authentication is required in this
> > > function.
> > > --
> > > 2.25.1
> > >
> > >
> > >
> > >
> > >
> >
> >
> >
> >
> >
> >
> >
> >
>
>
> 
>
>
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.