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

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

Jiewen,

Thanks.  That makes sense.

Let's make sure the array is handled correctly in this platform specific solution and the new tool feature.

Mike

From: Yao, Jiewen
Sent: Wednesday, October 26, 2016 6:48 PM
To: Kinney, Michael D <michael.d.kinney@intel.com>; edk2-devel@lists.01.org
Cc: Tian, Feng <feng.tian@intel.com>; Gao, Liming <liming.gao@intel.com>; Zeng, Star <star.zeng@intel.com>; Zhang, Chao B <chao.b.zhang@intel.com>
Subject: RE: [edk2] [PATCH V4 6/8] QuarkPlatformPkg/PlatformBootManager: Add capsule/recovery handling.

Yes, Mike. You are right.

We do have plan to make it generic. As we discussed before, we will enhance the tool to detect such info and set a PCD at build time. Then we can include that in the UiApp driver to report such error directly.

The tool is not ready yet. So current platform solution is just a temporary solution.

We do have plan to migrate to the new solution after the tool is ready. Then we can clean up the platform BDS code.

Thank you
Yao Jiewen


From: Kinney, Michael D
Sent: Thursday, October 27, 2016 7:36 AM
To: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
Cc: Tian, Feng <feng.tian@intel.com<mailto:feng.tian@intel.com>>; Gao, Liming <liming.gao@intel.com<mailto:liming.gao@intel.com>>; Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com>>; Zhang, Chao B <chao.b.zhang@intel.com<mailto:chao.b.zhang@intel.com>>
Subject: RE: [edk2] [PATCH V4 6/8] QuarkPlatformPkg/PlatformBootManager: Add capsule/recovery handling.

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<mailto:edk2-devel@lists.01.org>
> Cc: Tian, Feng <feng.tian@intel.com<mailto:feng.tian@intel.com>>; Gao, Liming <liming.gao@intel.com<mailto:liming.gao@intel.com>>; Zeng, Star
> <star.zeng@intel.com<mailto:star.zeng@intel.com>>; Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Zhang, Chao B
> <chao.b.zhang@intel.com<mailto: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<mailto:michael.d.kinney@intel.com>>
> Cc: Kelly Steele <kelly.steele@intel.com<mailto:kelly.steele@intel.com>>
> Cc: Feng Tian <feng.tian@intel.com<mailto:feng.tian@intel.com>>
> Cc: Star Zeng <star.zeng@intel.com<mailto:star.zeng@intel.com>>
> Cc: Liming Gao <liming.gao@intel.com<mailto:liming.gao@intel.com>>
> Cc: Chao Zhang <chao.b.zhang@intel.com<mailto:chao.b.zhang@intel.com>>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jiewen Yao <jiewen.yao@intel.com<mailto: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<mailto:edk2-devel@lists.01.org>
> https://lists.01.org/mailman/listinfo/edk2-devel