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

["Grzegorz Bernacki" <gjb@semihalf.com>]

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.
>
>
> 
>
>