Re: [edk2-devel] [PATCH v5 05/10] SecurityPkg: Remove duplicated functions from SecureBootConfigDxe.

["Yao, Jiewen" <jiewen.yao@intel.com>]

Sorry. My mistake.
It should be Reviewed-by: Jiewen Yao <Jiewen.yao@intel.com>

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Yao, Jiewen
> Sent: Monday, July 12, 2021 7:46 PM
> To: Grzegorz Bernacki <gjb@semihalf.com>; devel@edk2.groups.io
> Cc: leif@nuviainc.com; ardb+tianocore@kernel.org; Samer El-Haj-Mahmoud
> <Samer.El-Haj-Mahmoud@arm.com>; Sunny Wang <Sunny.Wang@arm.com>;
> mw@semihalf.com; upstream@semihalf.com; Wang, Jian J
> <jian.j.wang@intel.com>; Xu, Min M <min.m.xu@intel.com>;
> lersek@redhat.com; Sami Mujawar <Sami.Mujawar@arm.com>;
> afish@apple.com; Ni, Ray <ray.ni@intel.com>; Justen, Jordan L
> <jordan.l.justen@intel.com>; rebecca@bsdio.com; grehan@freebsd.org;
> Thomas Abraham <thomas.abraham@arm.com>; Chiu, Chasel
> <chasel.chiu@intel.com>; Desimone, Nathaniel L
> <nathaniel.l.desimone@intel.com>; gaoliming@byosoft.com.cn; Dong, Eric
> <eric.dong@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Sun,
> Zailiang <zailiang.sun@intel.com>; Qian, Yi <yi.qian@intel.com>;
> graeme@nuviainc.com; rad@semihalf.com; pete@akeo.ie
> Subject: Re: [edk2-devel] [PATCH v5 05/10] SecurityPkg: Remove duplicated
> functions from SecureBootConfigDxe.
> 
> Signed-off-by: Jiewen Yao <Jiewen.yao@intel.com>
> 
> > -----Original Message-----
> > From: Grzegorz Bernacki <gjb@semihalf.com>
> > Sent: Thursday, July 1, 2021 5:18 PM
> > To: devel@edk2.groups.io
> > Cc: leif@nuviainc.com; ardb+tianocore@kernel.org; Samer El-Haj-Mahmoud
> > <Samer.El-Haj-Mahmoud@arm.com>; Sunny Wang <Sunny.Wang@arm.com>;
> > 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>; lersek@redhat.com; Sami Mujawar
> > <Sami.Mujawar@arm.com>; afish@apple.com; Ni, Ray <ray.ni@intel.com>;
> > Justen, Jordan L <jordan.l.justen@intel.com>; rebecca@bsdio.com;
> > grehan@freebsd.org; Thomas Abraham <thomas.abraham@arm.com>; Chiu,
> > Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L
> > <nathaniel.l.desimone@intel.com>; gaoliming@byosoft.com.cn; Dong, Eric
> > <eric.dong@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>;
> Sun,
> > Zailiang <zailiang.sun@intel.com>; Qian, Yi <yi.qian@intel.com>;
> > graeme@nuviainc.com; rad@semihalf.com; pete@akeo.ie; Grzegorz Bernacki
> > <gjb@semihalf.com>
> > Subject: [PATCH v5 05/10] SecurityPkg: Remove duplicated functions from
> > SecureBootConfigDxe.
> >
> > This commit removes functions which were added
> > to SecureBootVariableLib. It also adds dependecy
> > on that library.
> >
> > Signed-off-by: Grzegorz Bernacki <gjb@semihalf.com>
> > ---
> >
> >
> SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDx
> > e.inf |   1 +
> >
> >
> SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigIm
> > pl.c  | 189 +-------------------
> >  2 files changed, 2 insertions(+), 188 deletions(-)
> >
> > diff --git
> >
> a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfig
> > Dxe.inf
> >
> b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfig
> > Dxe.inf
> > index 573efa6379..30d9cd8025 100644
> > ---
> >
> a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfig
> > Dxe.inf
> > +++
> >
> b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfig
> > Dxe.inf
> > @@ -54,6 +54,7 @@
> >    DevicePathLib
> >    FileExplorerLib
> >    PeCoffLib
> > +  SecureBootVariableLib
> >
> >  [Guids]
> >    ## SOMETIMES_CONSUMES      ## Variable:L"CustomMode"
> > diff --git
> >
> a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigI
> > mpl.c
> >
> b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigI
> > mpl.c
> > index e82bfe7757..67e5e594ed 100644
> > ---
> >
> a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigI
> > mpl.c
> > +++
> >
> b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigI
> > mpl.c
> > @@ -9,6 +9,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> >  #include "SecureBootConfigImpl.h"
> >  #include <Library/BaseCryptLib.h>
> > +#include <Library/SecureBootVariableLib.h>
> >
> >  CHAR16              mSecureBootStorageName[] =
> > L"SECUREBOOT_CONFIGURATION";
> >
> > @@ -237,168 +238,6 @@ SaveSecureBootVariable (
> >    return Status;
> >  }
> >
> > -/**
> > -  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 when finish
> > using it.
> > -
> > -  @retval EFI_SUCCESS              Create time based payload successfully.
> > -  @retval EFI_OUT_OF_RESOURCES     There are not enough memory
> resources
> > 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;
> > -  }
> > -
> > -  //
> > -  // In Setup mode or Custom mode, 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;
> > -}
> > -
> > -/**
> > -  Internal helper function to delete a Variable given its name and GUID, NO
> > authentication
> > -  required.
> > -
> > -  @param[in]      VariableName            Name of the Variable.
> > -  @param[in]      VendorGuid              GUID of the Variable.
> > -
> > -  @retval EFI_SUCCESS              Variable deleted successfully.
> > -  @retval Others                   The driver failed to start the device.
> > -
> > -**/
> > -EFI_STATUS
> > -DeleteVariable (
> > -  IN  CHAR16                    *VariableName,
> > -  IN  EFI_GUID                  *VendorGuid
> > -  )
> > -{
> > -  EFI_STATUS              Status;
> > -  VOID*                   Variable;
> > -  UINT8                   *Data;
> > -  UINTN                   DataSize;
> > -  UINT32                  Attr;
> > -
> > -  GetVariable2 (VariableName, VendorGuid, &Variable, NULL);
> > -  if (Variable == NULL) {
> > -    return EFI_SUCCESS;
> > -  }
> > -  FreePool (Variable);
> > -
> > -  Data     = NULL;
> > -  DataSize = 0;
> > -  Attr     = EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_RUNTIME_ACCESS
> |
> > EFI_VARIABLE_BOOTSERVICE_ACCESS
> > -             | EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS;
> > -
> > -  Status = CreateTimeBasedPayload (&DataSize, &Data);
> > -  if (EFI_ERROR (Status)) {
> > -    DEBUG ((EFI_D_ERROR, "Fail to create time-based data payload: %r",
> Status));
> > -    return Status;
> > -  }
> > -
> > -  Status = gRT->SetVariable (
> > -                  VariableName,
> > -                  VendorGuid,
> > -                  Attr,
> > -                  DataSize,
> > -                  Data
> > -                  );
> > -  if (Data != NULL) {
> > -    FreePool (Data);
> > -  }
> > -  return Status;
> > -}
> > -
> > -/**
> > -
> > -  Set the platform secure boot mode into "Custom" or "Standard" mode.
> > -
> > -  @param[in]   SecureBootMode        New secure boot mode:
> > STANDARD_SECURE_BOOT_MODE or
> > -                                     CUSTOM_SECURE_BOOT_MODE.
> > -
> > -  @return EFI_SUCCESS                The platform has switched to the special
> mode
> > successfully.
> > -  @return other                      Fail to operate the secure boot mode.
> > -
> > -**/
> > -EFI_STATUS
> > -SetSecureBootMode (
> > -  IN     UINT8         SecureBootMode
> > -  )
> > -{
> > -  return gRT->SetVariable (
> > -                EFI_CUSTOM_MODE_NAME,
> > -                &gEfiCustomModeEnableGuid,
> > -                EFI_VARIABLE_NON_VOLATILE |
> > EFI_VARIABLE_BOOTSERVICE_ACCESS,
> > -                sizeof (UINT8),
> > -                &SecureBootMode
> > -                );
> > -}
> > -
> >  /**
> >    This code checks if the encode type and key strength of X.509
> >    certificate is qualified.
> > @@ -646,32 +485,6 @@ ON_EXIT:
> >    return Status;
> >  }
> >
> > -/**
> > -  Remove the PK variable.
> > -
> > -  @retval EFI_SUCCESS    Delete PK successfully.
> > -  @retval Others         Could not allow to delete PK.
> > -
> > -**/
> > -EFI_STATUS
> > -DeletePlatformKey (
> > -  VOID
> > -)
> > -{
> > -  EFI_STATUS Status;
> > -
> > -  Status = SetSecureBootMode(CUSTOM_SECURE_BOOT_MODE);
> > -  if (EFI_ERROR (Status)) {
> > -    return Status;
> > -  }
> > -
> > -  Status = DeleteVariable (
> > -             EFI_PLATFORM_KEY_NAME,
> > -             &gEfiGlobalVariableGuid
> > -             );
> > -  return Status;
> > -}
> > -
> >  /**
> >    Enroll a new KEK item from public key storing file (*.pbk).
> >
> > --
> > 2.25.1
> 
> 
> 
> 
>