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

["gaoliming" <gaoliming@byosoft.com.cn>]

Greg:
 Yes. You can submit another BZ for code optimization. 

Thanks
Liming
> -----邮件原件-----
> 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Grzegorz
> Bernacki
> 发送时间: 2021年6月25日 13:46
> 收件人: devel@edk2.groups.io; gaoliming@byosoft.com.cn
> 抄送: Sunny Wang <Sunny.Wang@arm.com>; leif@nuviainc.com;
> ardb+tianocore@kernel.org; Samer El-Haj-Mahmoud
> <Samer.El-Haj-Mahmoud@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>
> 主题: Re: [edk2-devel] [PATCH v3 8/8] MdeModulePkg: Use
> SecureBootVariableLib in PlatformVarCleanupLib.
> 
> Hi Liming,
> Adding support for DXE_DRIVER and UEFI_APPLICATION to AuthVariableLib
> causes problem with PlatformSecureLib and VariablePolicyLib which does
> not support that modules. I don't want to change to many modules just
> to remove one function in PlatformVarCleanupLib. As I wrote before I
> think that the best solution would be to leave PlatformVarCleanupLib
> unchanged. What do you think?
> 
> thanks,
> greg
> 
> czw., 24 cze 2021 o 02:59 gaoliming <gaoliming@byosoft.com.cn> napisał(a):
> >
> > I agree that AuthVariableLib can support UEFI_DRIVER, DXE_DRIVER and
> UEFI_APPLICATION. There is no limitation for this library.
> >
> > Thanks
> > Liming
> > > -----邮件原件-----
> > > 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Grzegorz
> > > Bernacki
> > > 发送时间: 2021年6月23日 18:39
> > > 收件人: devel@edk2.groups.io; Sunny Wang <Sunny.Wang@arm.com>
> > > 抄送: gaoliming@byosoft.com.cn; leif@nuviainc.com;
> > > ardb+tianocore@kernel.org; Samer El-Haj-Mahmoud
> > > <Samer.El-Haj-Mahmoud@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>
> > > 主题: Re: [edk2-devel] [PATCH v3 8/8] MdeModulePkg: Use
> > > SecureBootVariableLib in PlatformVarCleanupLib.
> > >
> > > Hi,
> > >
> > > AuthVariableLib must support DXE_DRIVER and UEFI_APPLICATION.
> Adding
> > > it leads to errors for libraries which are used by AuthVariableLib and
> > > which does not support DXE_DRIVER or UEFI_APPLICATION. This changes
> > > causes a lot of minor changes in another libraries.
> > > The whole problem exists because I wanted to remove a duplicate of
> > > CreateTimeBasedPayload() from PlatformVarCleanupLib. I just leave it
> > > there. This module is not used anyway. I think it can be safely
> > > removed, but I don't want this change to be a part of this patchset.
> > > thanks,
> > > greg
> > >
> > > śr., 23 cze 2021 o 05:33 Sunny Wang <Sunny.Wang@arm.com>
> napisał(a):
> > > >
> > > > 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.
> > > >
> > > >
> > > >
> > > >
> > > >
> > >
> > >
> > >
> > >
> >
> >
> >
> >
> >
> >
> >
> >
> 
> 
> 
>