Re: [PATCH] Reallocate TPM Active PCRs based on platform support.

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

Would you please confirm if you have run CI and got a PASS result?


> -----Original Message-----
> From: Gonzalez Del Cueto, Rodrigo <rodrigo.gonzalez.del.cueto@intel.com>
> Sent: Friday, November 5, 2021 2:07 AM
> To: devel@edk2.groups.io
> Cc: Gonzalez Del Cueto, Rodrigo <rodrigo.gonzalez.del.cueto@intel.com>;
> Wang, Jian J <jian.j.wang@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
> Subject: [PATCH] Reallocate TPM Active PCRs based on platform support.
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3515
> 
> In V3: Cleaned up comments, debug prints and updated patch to use the
> new debug ENUM definitions.
> 
> - Replaced EFI_D_INFO with DEBUG_INFO.
> - Replaced EFI_D_VERBOSE with DEBUG_VERBOSE.
> 
> In V2: Add case to RegisterHashInterfaceLib logic
> 
> RegisterHashInterfaceLib needs to correctly handle registering the HashLib
> instance supported algorithm bitmap when PcdTpm2HashMask is set to zero.
> 
> The current implementation of SyncPcrAllocationsAndPcrMask() triggers
> PCR bank reallocation only based on the intersection between
> TpmActivePcrBanks and PcdTpm2HashMask.
> 
> When the software HashLibBaseCryptoRouter solution is used, no PCR bank
> reallocation is occurring based on the supported hashing algorithms
> registered by the HashLib instances.
> 
> Need to have an additional check for the intersection between the
> TpmActivePcrBanks and the PcdTcg2HashAlgorithmBitmap populated by the
> HashLib instances present on the platform's BIOS.
> 
> Signed-off-by: Rodrigo Gonzalez del Cueto
> <rodrigo.gonzalez.del.cueto@intel.com>
> 
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> ---
>  SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterDxe.c
> |  6 +++++-
>  SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c |
> 6 +++++-
>  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c                                        | 67
> ++++++++++++++++++++++++++++++++++++++++++-------------------------
>  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf                                      |  1 +
>  4 files changed, 53 insertions(+), 27 deletions(-)
> 
> diff --git
> a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterDxe.
> c
> b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterDxe.
> c
> index 7a0f61efbb..0821159120 100644
> ---
> a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterDxe.
> c
> +++
> b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterDxe.
> c
> @@ -230,13 +230,17 @@ RegisterHashInterfaceLib (
>  {
>    UINTN              Index;
>    UINT32             HashMask;
> +  UINT32             Tpm2HashMask;
>    EFI_STATUS         Status;
> 
>    //
>    // Check allow
>    //
>    HashMask = Tpm2GetHashMaskFromAlgo (&HashInterface->HashGuid);
> -  if ((HashMask & PcdGet32 (PcdTpm2HashMask)) == 0) {
> +  Tpm2HashMask = PcdGet32 (PcdTpm2HashMask);
> +
> +  if ((Tpm2HashMask != 0) &&
> +      ((HashMask & Tpm2HashMask) == 0)) {
>      return EFI_UNSUPPORTED;
>    }
> 
> diff --git
> a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c
> b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c
> index 42cb562f67..6ae51dbce4 100644
> ---
> a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c
> +++
> b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c
> @@ -327,13 +327,17 @@ RegisterHashInterfaceLib (
>    UINTN              Index;
>    HASH_INTERFACE_HOB *HashInterfaceHob;
>    UINT32             HashMask;
> +  UINT32             Tpm2HashMask;
>    EFI_STATUS         Status;
> 
>    //
>    // Check allow
>    //
>    HashMask = Tpm2GetHashMaskFromAlgo (&HashInterface->HashGuid);
> -  if ((HashMask & PcdGet32 (PcdTpm2HashMask)) == 0) {
> +  Tpm2HashMask = PcdGet32 (PcdTpm2HashMask);
> +
> +  if ((Tpm2HashMask != 0) &&
> +      ((HashMask & Tpm2HashMask) == 0)) {
>      return EFI_UNSUPPORTED;
>    }
> 
> diff --git a/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c
> b/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c
> index 93a8803ff6..582b9377e5 100644
> --- a/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c
> +++ b/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c
> @@ -1,7 +1,7 @@
>  /** @file
>    Initialize TPM2 device and measure FVs before handing off control to DXE.
> 
> -Copyright (c) 2015 - 2020, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2015 - 2021, Intel Corporation. All rights reserved.<BR>
>  Copyright (c) 2017, Microsoft Corporation.  All rights reserved. <BR>
>  SPDX-License-Identifier: BSD-2-Clause-Patent
> 
> @@ -253,7 +253,7 @@ EndofPeiSignalNotifyCallBack (
> 
>  /**
>    Make sure that the current PCR allocations, the TPM supported PCRs,
> -  and the PcdTpm2HashMask are all in agreement.
> +  PcdTcg2HashAlgorithmBitmap and the PcdTpm2HashMask are all in
> agreement.
>  **/
>  VOID
>  SyncPcrAllocationsAndPcrMask (
> @@ -262,52 +262,68 @@ SyncPcrAllocationsAndPcrMask (
>  {
>    EFI_STATUS                        Status;
>    EFI_TCG2_EVENT_ALGORITHM_BITMAP   TpmHashAlgorithmBitmap;
> +  EFI_TCG2_EVENT_ALGORITHM_BITMAP   BiosHashAlgorithmBitmap;
>    UINT32                            TpmActivePcrBanks;
>    UINT32                            NewTpmActivePcrBanks;
>    UINT32                            Tpm2PcrMask;
>    UINT32                            NewTpm2PcrMask;
> 
> -  DEBUG ((EFI_D_ERROR, "SyncPcrAllocationsAndPcrMask!\n"));
> +  DEBUG ((DEBUG_ERROR, "SyncPcrAllocationsAndPcrMask!\n"));
> 
>    //
>    // Determine the current TPM support and the Platform PCR mask.
>    //
>    Status = Tpm2GetCapabilitySupportedAndActivePcrs
> (&TpmHashAlgorithmBitmap, &TpmActivePcrBanks);
> +
>    ASSERT_EFI_ERROR (Status);
> 
> +  DEBUG ((DEBUG_INFO, "Tpm2GetCapabilitySupportedAndActivePcrs -
> TpmHashAlgorithmBitmap: 0x%08x\n", TpmHashAlgorithmBitmap));
> +  DEBUG ((DEBUG_INFO, "Tpm2GetCapabilitySupportedAndActivePcrs -
> TpmActivePcrBanks 0x%08x\n", TpmActivePcrBanks));
> +
>    Tpm2PcrMask = PcdGet32 (PcdTpm2HashMask);
>    if (Tpm2PcrMask == 0) {
>      //
> -    // if PcdTPm2HashMask is zero, use ActivePcr setting
> +    // If PcdTpm2HashMask is zero, use ActivePcr setting.
> +    // Only when PcdTpm2HashMask is initialized to 0, will it be updated to
> current Active Pcrs.
>      //
>      PcdSet32S (PcdTpm2HashMask, TpmActivePcrBanks);
>      Tpm2PcrMask = TpmActivePcrBanks;
>    }
> +  DEBUG ((DEBUG_INFO, "Tpm2PcrMask 0x%08x\n", Tpm2PcrMask));
> 
>    //
> -  // Find the intersection of Pcd support and TPM support.
> -  // If banks are missing from the TPM support that are in the PCD, update the
> PCD.
> -  // If banks are missing from the PCD that are active in the TPM, reallocate the
> banks and reboot.
> -  //
> -
> -  //
> -  // If there are active PCR banks that are not supported by the Platform mask,
> -  // update the TPM allocations and reboot the machine.
> +  // The Active PCRs in the TPM need to be a strict subset of the hashing
> algorithms supported by BIOS.
>    //
> -  if ((TpmActivePcrBanks & Tpm2PcrMask) != TpmActivePcrBanks) {
> -    NewTpmActivePcrBanks = TpmActivePcrBanks & Tpm2PcrMask;
> -
> -    DEBUG ((EFI_D_INFO, "%a - Reallocating PCR banks from 0x%X to 0x%X.\n",
> __FUNCTION__, TpmActivePcrBanks, NewTpmActivePcrBanks));
> +  // * Find the intersection of Pcd support and TPM active PCRs. If banks are
> missing from the TPM support
> +  // that are in the PCD, update the PCD.
> +  // * Find intersection of TPM Active PCRs and BIOS supported algorithms. If
> there are active PCR banks
> +  // that are not supported by the platform, update the TPM allocations and
> reboot.
> +  // Note: When the HashLibBaseCryptoRouter solution is used, the hash
> algorithm support from BIOS is reported
> +  //       by Tcg2HashAlgorithmBitmap, which is populated by HashLib instances
> at runtime.
> +  BiosHashAlgorithmBitmap = PcdGet32 (PcdTcg2HashAlgorithmBitmap);
> +  DEBUG ((DEBUG_INFO, "Tcg2HashAlgorithmBitmap: 0x%08x\n",
> BiosHashAlgorithmBitmap));
> +
> +  if (((TpmActivePcrBanks & Tpm2PcrMask) != TpmActivePcrBanks) ||
> +      ((TpmActivePcrBanks & BiosHashAlgorithmBitmap) != TpmActivePcrBanks)) {
> +    DEBUG ((DEBUG_INFO, "TpmActivePcrBanks & Tpm2PcrMask = 0x%08x\n",
> (TpmActivePcrBanks & Tpm2PcrMask)));
> +    DEBUG ((DEBUG_INFO, "TpmActivePcrBanks & BiosHashAlgorithmBitmap =
> 0x%08x\n", (TpmActivePcrBanks & BiosHashAlgorithmBitmap)));
> +    NewTpmActivePcrBanks = TpmActivePcrBanks;
> +    NewTpmActivePcrBanks &= Tpm2PcrMask;
> +    NewTpmActivePcrBanks &= BiosHashAlgorithmBitmap;
> +    DEBUG ((DEBUG_INFO, "NewTpmActivePcrBanks 0x%08x\n",
> NewTpmActivePcrBanks));
> +
> +    DEBUG ((DEBUG_INFO, "%a - Reallocating PCR banks from 0x%X to 0x%X.\n",
> __FUNCTION__, TpmActivePcrBanks, NewTpmActivePcrBanks));
>      if (NewTpmActivePcrBanks == 0) {
> -      DEBUG ((EFI_D_ERROR, "%a - No viable PCRs active! Please set a less
> restrictive value for PcdTpm2HashMask!\n", __FUNCTION__));
> +      DEBUG ((DEBUG_ERROR, "%a - No viable PCRs active! Please set a less
> restrictive value for PcdTpm2HashMask!\n", __FUNCTION__));
>        ASSERT (FALSE);
>      } else {
> +      DEBUG ((DEBUG_ERROR, "Tpm2PcrAllocateBanks
> (TpmHashAlgorithmBitmap: 0x%08x, NewTpmActivePcrBanks: 0x%08x)\n",
> TpmHashAlgorithmBitmap, NewTpmActivePcrBanks));
>        Status = Tpm2PcrAllocateBanks (NULL, (UINT32)TpmHashAlgorithmBitmap,
> NewTpmActivePcrBanks);
>        if (EFI_ERROR (Status)) {
>          //
>          // We can't do much here, but we hope that this doesn't happen.
>          //
> -        DEBUG ((EFI_D_ERROR, "%a - Failed to reallocate PCRs!\n",
> __FUNCTION__));
> +        DEBUG ((DEBUG_ERROR, "%a - Failed to reallocate PCRs!\n",
> __FUNCTION__));
>          ASSERT_EFI_ERROR (Status);
>        }
>        //
> @@ -324,13 +340,14 @@ SyncPcrAllocationsAndPcrMask (
>    if ((Tpm2PcrMask & TpmHashAlgorithmBitmap) != Tpm2PcrMask) {
>      NewTpm2PcrMask = Tpm2PcrMask & TpmHashAlgorithmBitmap;
> 
> -    DEBUG ((EFI_D_INFO, "%a - Updating PcdTpm2HashMask from 0x%X to
> 0x%X.\n", __FUNCTION__, Tpm2PcrMask, NewTpm2PcrMask));
> +    DEBUG ((DEBUG_ERROR, "%a - Updating PcdTpm2HashMask from 0x%X to
> 0x%X.\n", __FUNCTION__, Tpm2PcrMask, NewTpm2PcrMask));
>      if (NewTpm2PcrMask == 0) {
> -      DEBUG ((EFI_D_ERROR, "%a - No viable PCRs supported! Please set a less
> restrictive value for PcdTpm2HashMask!\n", __FUNCTION__));
> +      DEBUG ((DEBUG_ERROR, "%a - No viable PCRs supported! Please set a less
> restrictive value for PcdTpm2HashMask!\n", __FUNCTION__));
>        ASSERT (FALSE);
>      }
> 
>      Status = PcdSet32S (PcdTpm2HashMask, NewTpm2PcrMask);
> +    DEBUG ((DEBUG_ERROR, "Set PcdTpm2Hash Mask to 0x%08x\n",
> NewTpm2PcrMask));
>      ASSERT_EFI_ERROR (Status);
>    }
>  }
> @@ -365,7 +382,7 @@ LogHashEvent (
>    RetStatus = EFI_SUCCESS;
>    for (Index = 0; Index < sizeof(mTcg2EventInfo)/sizeof(mTcg2EventInfo[0]);
> Index++) {
>      if ((SupportedEventLogs & mTcg2EventInfo[Index].LogFormat) != 0) {
> -      DEBUG ((EFI_D_INFO, "  LogFormat - 0x%08x\n",
> mTcg2EventInfo[Index].LogFormat));
> +      DEBUG ((DEBUG_INFO, "  LogFormat - 0x%08x\n",
> mTcg2EventInfo[Index].LogFormat));
>        switch (mTcg2EventInfo[Index].LogFormat) {
>        case EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2:
>          Status = GetDigestFromDigestList (TPM_ALG_SHA1, DigestList,
> &NewEventHdr->Digest);
> @@ -476,7 +493,7 @@ HashLogExtendEvent (
>    }
> 
>    if (Status == EFI_DEVICE_ERROR) {
> -    DEBUG ((EFI_D_ERROR, "HashLogExtendEvent - %r. Disable TPM.\n", Status));
> +    DEBUG ((DEBUG_ERROR, "HashLogExtendEvent - %r. Disable TPM.\n",
> Status));
>      BuildGuidHob (&gTpmErrorHobGuid,0);
>      REPORT_STATUS_CODE (
>        EFI_ERROR_CODE | EFI_ERROR_MINOR,
> @@ -1011,7 +1028,7 @@ PeimEntryMA (
>    }
> 
>    if (GetFirstGuidHob (&gTpmErrorHobGuid) != NULL) {
> -    DEBUG ((EFI_D_ERROR, "TPM2 error!\n"));
> +    DEBUG ((DEBUG_ERROR, "TPM2 error!\n"));
>      return EFI_DEVICE_ERROR;
>    }
> 
> @@ -1075,7 +1092,7 @@ PeimEntryMA (
>        for (PcrIndex = 0; PcrIndex < 8; PcrIndex++) {
>          Status = MeasureSeparatorEventWithError (PcrIndex);
>          if (EFI_ERROR (Status)) {
> -          DEBUG ((EFI_D_ERROR, "Separator Event with Error not Measured.
> Error!\n"));
> +          DEBUG ((DEBUG_ERROR, "Separator Event with Error not Measured.
> Error!\n"));
>          }
>        }
>      }
> @@ -1106,7 +1123,7 @@ PeimEntryMA (
> 
>  Done:
>    if (EFI_ERROR (Status)) {
> -    DEBUG ((EFI_D_ERROR, "TPM2 error! Build Hob\n"));
> +    DEBUG ((DEBUG_ERROR, "TPM2 error! Build Hob\n"));
>      BuildGuidHob (&gTpmErrorHobGuid,0);
>      REPORT_STATUS_CODE (
>        EFI_ERROR_CODE | EFI_ERROR_MINOR,
> diff --git a/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
> b/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
> index 06c26a2904..17ad116126 100644
> --- a/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
> +++ b/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
> @@ -86,6 +86,7 @@
>    ## SOMETIMES_CONSUMES
>    ## SOMETIMES_PRODUCES
>    gEfiSecurityPkgTokenSpaceGuid.PcdTpm2HashMask
> +  gEfiSecurityPkgTokenSpaceGuid.PcdTcg2HashAlgorithmBitmap                  ##
> CONSUMES
> 
>  [Depex]
>    gEfiPeiMasterBootModePpiGuid AND
> --
> 2.33.1.windows.1