Re: [PATCH V4 6/8] QuarkPlatformPkg/PlatformBootManager: Add capsule/recovery handling.

["Kinney, Michael D" <michael.d.kinney@intel.com>]

Jiewen,

Can the code that checks for the use of a test key be moved into a common BDS lib or module?
Maybe in MdeModulePkg\Universal\BdsDxe\BdsEntry.c right before the call to 
PlatformBootManagerAfterConsole()?  The logic in BdsEntry.c can do the check and set the
PcdTestKeyUsed PCD and can go a DEBUG() message for the use of a test key.

With the current design, you depend on a platform specific BDS library to include the test 
key check, and we want to make sure the check for the use of a test key is always performed.

Also, the test key check against PcdRsa2048Sha256PublicKeyBuffer is incomplete. The DEC file
description of this PCD is as follows:

  ## Provides one or more SHA 256 Hashes of the RSA 2048 public keys used to verify Recovery and Capsule Update images
  #  WARNING: The default value is treated as test key. Please do not use default value in the production.
  # @Prompt One or more SHA 256 Hashes of RSA 2048 bit public keys used to verify Recovery and Capsule Update images
  #
  gEfiSecurityPkgTokenSpaceGuid.PcdRsa2048Sha256PublicKeyBuffer|{0x91, 0x29, 0xc4, 0xbd, 0xea, 0x6d, 0xda, 0xb3, 0xaa, 0x6f, 0x50, 0x16, 0xfc, 0xdb, 0x4b, 0x7e, 0x3c, 0xd6, 0xdc, 0xa4, 0x7a, 0x0e, 0xdd, 0xe6, 0x15, 0x8c, 0x73, 0x96, 0xa2, 0xd4, 0xa6, 0x4d}|VOID*|0x00010013

Since this PCD provides one or more SHA 256 Hashes, the check for the use of a test key needs to get the 
Size, determine how many hashes are in this PCD, and compare the test key value against each entry in
this array.

Thanks,

Mike

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Jiewen Yao
> Sent: Saturday, October 22, 2016 7:32 PM
> To: edk2-devel@lists.01.org
> Cc: Tian, Feng <feng.tian@intel.com>; Gao, Liming <liming.gao@intel.com>; Zeng, Star
> <star.zeng@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Zhang, Chao B
> <chao.b.zhang@intel.com>
> Subject: [edk2] [PATCH V4 6/8] QuarkPlatformPkg/PlatformBootManager: Add
> capsule/recovery handling.
> 
> 1) Add capsule and recovery boot path handling in platform BDS.
> 2) Add check if the platform is using default test key for recovery or update.
> Produce PcdTestKeyUsed to indicate if there is any
> test key used in current BIOS, such as recovery key,
> or capsule update key.
> Then the generic UI may consume this PCD to show warning information.
> 
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Kelly Steele <kelly.steele@intel.com>
> Cc: Feng Tian <feng.tian@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Chao Zhang <chao.b.zhang@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>
> ---
>  QuarkPlatformPkg/Library/PlatformBootManagerLib/PlatformBootManager.c      | 131
> +++++++++++++++++++-
>  QuarkPlatformPkg/Library/PlatformBootManagerLib/PlatformBootManager.h      |   9 +-
>  QuarkPlatformPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf |  16 ++-
>  3 files changed, 151 insertions(+), 5 deletions(-)
> 
> diff --git a/QuarkPlatformPkg/Library/PlatformBootManagerLib/PlatformBootManager.c
> b/QuarkPlatformPkg/Library/PlatformBootManagerLib/PlatformBootManager.c
> index 19ff3d0..f327c89 100644
> --- a/QuarkPlatformPkg/Library/PlatformBootManagerLib/PlatformBootManager.c
> +++ b/QuarkPlatformPkg/Library/PlatformBootManagerLib/PlatformBootManager.c
> @@ -2,7 +2,7 @@
>  This file include all platform action which can be customized
>  by IBV/OEM.
> 
> -Copyright (c) 2015, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2015 - 2016, Intel Corporation. All rights reserved.<BR>
>  This program and the accompanying materials
>  are licensed and made available under the terms and conditions of the BSD License
>  which accompanies this distribution.  The full text of the license may be found at
> @@ -205,6 +205,8 @@ PlatformBootManagerBeforeConsole (
>    EFI_INPUT_KEY                 Enter;
>    EFI_INPUT_KEY                 F2;
>    EFI_BOOT_MANAGER_LOAD_OPTION  BootOption;
> +  ESRT_MANAGEMENT_PROTOCOL      *EsrtManagement;
> +  EFI_BOOT_MODE                 BootMode;
>    EFI_ACPI_S3_SAVE_PROTOCOL     *AcpiS3Save;
>    EFI_HANDLE                    Handle;
>    EFI_EVENT                     EndOfDxeEvent;
> @@ -246,6 +248,40 @@ PlatformBootManagerBeforeConsole (
>    //
>    PlatformRegisterFvBootOption (&mUefiShellFileGuid, L"UEFI Shell",
> LOAD_OPTION_ACTIVE);
> 
> +  Status = gBS->LocateProtocol(&gEsrtManagementProtocolGuid, NULL, (VOID
> **)&EsrtManagement);
> +  if (EFI_ERROR(Status)) {
> +    EsrtManagement = NULL;
> +  }
> +
> +  BootMode = GetBootModeHob();
> +  switch (BootMode) {
> +  case BOOT_ON_FLASH_UPDATE:
> +    DEBUG((EFI_D_INFO, "ProcessCapsules Before EndOfDxe ......\n"));
> +    Status = ProcessCapsules ();
> +    DEBUG((EFI_D_INFO, "ProcessCapsules %r\n", Status));
> +    break;
> +  case BOOT_IN_RECOVERY_MODE:
> +    break;
> +  case BOOT_ASSUMING_NO_CONFIGURATION_CHANGES:
> +  case BOOT_WITH_MINIMAL_CONFIGURATION:
> +  case BOOT_ON_S4_RESUME:
> +    if (EsrtManagement != NULL) {
> +      //
> +      // Lock ESRT cache repository before EndofDxe if ESRT sync is not needed
> +      //
> +      EsrtManagement->LockEsrtRepository();
> +    }
> +    break;
> +  default:
> +    //
> +    // Require to sync ESRT from FMP in a new boot
> +    //
> +    if (EsrtManagement != NULL) {
> +      EsrtManagement->SyncEsrtFmp();
> +    }
> +    break;
> +  }
> +
>    //
>    // Prepare for S3
>    //
> @@ -303,7 +339,64 @@ PlatformBootManagerAfterConsole (
>    VOID
>    )
>  {
> -  EFI_STATUS  Status;
> +  EFI_STATUS                     Status;
> +  EFI_BOOT_MODE                  BootMode;
> +  ESRT_MANAGEMENT_PROTOCOL       *EsrtManagement;
> +  VOID                           *Buffer;
> +  UINTN                          Size;
> +
> +  Status = gBS->LocateProtocol(&gEsrtManagementProtocolGuid, NULL, (VOID
> **)&EsrtManagement);
> +  if (EFI_ERROR(Status)) {
> +    EsrtManagement = NULL;
> +  }
> +
> +  BootMode = GetBootModeHob();
> +  switch (BootMode) {
> +  case BOOT_ON_FLASH_UPDATE:
> +    DEBUG((EFI_D_INFO, "Capsule Mode detected\n"));
> +    if (FeaturePcdGet(PcdSupportUpdateCapsuleReset)) {
> +      EfiBootManagerConnectAll ();
> +      EfiBootManagerRefreshAllBootOption ();
> +
> +      //
> +      // Always sync ESRT Cache from FMP Instances after connect all and before
> capsule process
> +      //
> +      if (EsrtManagement != NULL) {
> +        EsrtManagement->SyncEsrtFmp();
> +      }
> +
> +      DEBUG((EFI_D_INFO, "ProcessCapsules After ConnectAll ......\n"));
> +      Status = ProcessCapsules();
> +      DEBUG((EFI_D_INFO, "ProcessCapsules %r\n", Status));
> +    }
> +    break;
> +
> +  case BOOT_IN_RECOVERY_MODE:
> +    DEBUG((EFI_D_INFO, "Recovery Mode detected\n"));
> +    // Passthrough
> +
> +  case BOOT_ASSUMING_NO_CONFIGURATION_CHANGES:
> +  case BOOT_WITH_MINIMAL_CONFIGURATION:
> +  case BOOT_WITH_FULL_CONFIGURATION:
> +  case BOOT_WITH_FULL_CONFIGURATION_PLUS_DIAGNOSTICS:
> +  case BOOT_WITH_DEFAULT_SETTINGS:
> +  default:
> +    EfiBootManagerConnectAll ();
> +    EfiBootManagerRefreshAllBootOption ();
> +
> +    //
> +    // Sync ESRT Cache from FMP Instance on demand after Connect All
> +    //
> +    if ((BootMode != BOOT_ASSUMING_NO_CONFIGURATION_CHANGES) &&
> +        (BootMode != BOOT_WITH_MINIMAL_CONFIGURATION) &&
> +        (BootMode != BOOT_ON_S4_RESUME)) {
> +      if (EsrtManagement != NULL) {
> +        EsrtManagement->SyncEsrtFmp();
> +      }
> +    }
> +
> +    break;
> +  }
> 
>    Print (
>      L"\n"
> @@ -313,6 +406,40 @@ PlatformBootManagerAfterConsole (
>      );
> 
>    //
> +  // Check if the platform is using test key.
> +  //
> +  Status = GetSectionFromAnyFv(
> +             PcdGetPtr(PcdEdkiiRsa2048Sha256TestPublicKeyFileGuid),
> +             EFI_SECTION_RAW,
> +             0,
> +             &Buffer,
> +             &Size
> +             );
> +  if (!EFI_ERROR(Status)) {
> +    if ((Size == PcdGetSize(PcdRsa2048Sha256PublicKeyBuffer)) &&
> +        (CompareMem(Buffer, PcdGetPtr(PcdRsa2048Sha256PublicKeyBuffer), Size) == 0)) {
> +      Print(L"WARNING: Recovery Test Key is used.\n");
> +      PcdSetBoolS(PcdTestKeyUsed, TRUE);
> +    }
> +    FreePool(Buffer);
> +  }
> +  Status = GetSectionFromAnyFv(
> +             PcdGetPtr(PcdEdkiiPkcs7TestPublicKeyFileGuid),
> +             EFI_SECTION_RAW,
> +             0,
> +             &Buffer,
> +             &Size
> +             );
> +  if (!EFI_ERROR(Status)) {
> +    if ((Size == PcdGetSize(PcdPkcs7CertBuffer)) &&
> +        (CompareMem(Buffer, PcdGetPtr(PcdPkcs7CertBuffer), Size) == 0)) {
> +      Print(L"WARNING: Capsule Test Key is used.\n");
> +      PcdSetBoolS(PcdTestKeyUsed, TRUE);
> +    }
> +    FreePool(Buffer);
> +  }
> +
> +  //
>    // Use a DynamicHii type pcd to save the boot status, which is used to
>    // control configuration mode, such as FULL/MINIMAL/NO_CHANGES configuration.
>    //
> diff --git a/QuarkPlatformPkg/Library/PlatformBootManagerLib/PlatformBootManager.h
> b/QuarkPlatformPkg/Library/PlatformBootManagerLib/PlatformBootManager.h
> index 7413883..395f78b 100644
> --- a/QuarkPlatformPkg/Library/PlatformBootManagerLib/PlatformBootManager.h
> +++ b/QuarkPlatformPkg/Library/PlatformBootManagerLib/PlatformBootManager.h
> @@ -1,7 +1,7 @@
>  /** @file
>  Head file for BDS Platform specific code
> 
> -Copyright (c) 2015, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2015 - 2016, Intel Corporation. All rights reserved.<BR>
>  This program and the accompanying materials
>  are licensed and made available under the terms and conditions of the BSD License
>  which accompanies this distribution.  The full text of the license may be found at
> @@ -21,6 +21,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR
> IMPLIED.
>  #include <Protocol/FirmwareVolume2.h>
>  #include <Protocol/AcpiS3Save.h>
>  #include <Protocol/DxeSmmReadyToLock.h>
> +#include <Protocol/EsrtManagement.h>
>  #include <Guid/DebugAgentGuid.h>
>  #include <Guid/EventGroup.h>
>  #include <Guid/PcAnsi.h>
> @@ -32,9 +33,13 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR
> IMPLIED.
>  #include <Library/DevicePathLib.h>
>  #include <Library/MemoryAllocationLib.h>
>  #include <Library/UefiBootServicesTableLib.h>
> +#include <Library/UefiRuntimeServicesTableLib.h>
>  #include <Library/UefiLib.h>
>  #include <Library/UefiBootManagerLib.h>
> -
> +#include <Library/PrintLib.h>
> +#include <Library/HobLib.h>
> +#include <Library/CapsuleLib.h>
> +#include <Library/DxeServicesLib.h>
> 
>  typedef struct {
>    EFI_DEVICE_PATH_PROTOCOL  *DevicePath;
> diff --git a/QuarkPlatformPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> b/QuarkPlatformPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> index d59f14a..eadf1fe 100644
> --- a/QuarkPlatformPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> +++ b/QuarkPlatformPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> @@ -1,7 +1,7 @@
>  ## @file
>  #  Include all platform action which can be customized by IBV/OEM.
>  #
> -#  Copyright (c) 2012 - 2015, Intel Corporation. All rights reserved.<BR>
> +#  Copyright (c) 2012 - 2016, Intel Corporation. All rights reserved.<BR>
>  #  This program and the accompanying materials
>  #  are licensed and made available under the terms and conditions of the BSD License
>  #  which accompanies this distribution.  The full text of the license may be found at
> @@ -38,6 +38,8 @@
>    IntelFrameworkPkg/IntelFrameworkPkg.dec
>    IntelFrameworkModulePkg/IntelFrameworkModulePkg.dec
>    SourceLevelDebugPkg/SourceLevelDebugPkg.dec
> +  QuarkPlatformPkg/QuarkPlatformPkg.dec
> +  SecurityPkg/SecurityPkg.dec
> 
>  [LibraryClasses]
>    BaseLib
> @@ -49,11 +51,16 @@
>    UefiBootServicesTableLib
>    UefiLib
>    UefiBootManagerLib
> +  PrintLib
> +  HobLib
> +  CapsuleLib
> +  DxeServicesLib
> 
>  [Protocols]
>    gEfiFirmwareVolume2ProtocolGuid
>    gEfiAcpiS3SaveProtocolGuid
>    gEfiDxeSmmReadyToLockProtocolGuid
> +  gEsrtManagementProtocolGuid
> 
>  [Guids]
>    gEfiPcAnsiGuid
> @@ -70,3 +77,10 @@
>    gEfiMdePkgTokenSpaceGuid.PcdUartDefaultStopBits
>    gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType
>    gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdBootState
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdSupportUpdateCapsuleReset
> +  gQuarkPlatformTokenSpaceGuid.PcdEdkiiRsa2048Sha256TestPublicKeyFileGuid
> +  gQuarkPlatformTokenSpaceGuid.PcdEdkiiPkcs7TestPublicKeyFileGuid
> +  gEfiSecurityPkgTokenSpaceGuid.PcdRsa2048Sha256PublicKeyBuffer
> +  gEfiSecurityPkgTokenSpaceGuid.PcdPkcs7CertBuffer
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdTestKeyUsed
> +
> --
> 2.7.4.windows.1
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel