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

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

Hi Rodrigo
I don't understand the problem statement.

This code has been there for long time. What is changed recently ?

Thank you
Yao Jiewen


> -----Original Message-----
> From: Gonzalez Del Cueto, Rodrigo <rodrigo.gonzalez.del.cueto@intel.com>
> Sent: Thursday, August 5, 2021 7:28 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 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                                        | 18
> +++++++++++++++++-
>  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf                                      |  1 +
>  4 files changed, 28 insertions(+), 3 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..5ad6a45cf3 100644
> --- a/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c
> +++ b/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c
> @@ -262,6 +262,7 @@ SyncPcrAllocationsAndPcrMask (
>  {
>    EFI_STATUS                        Status;
>    EFI_TCG2_EVENT_ALGORITHM_BITMAP   TpmHashAlgorithmBitmap;
> +  EFI_TCG2_EVENT_ALGORITHM_BITMAP   BiosHashAlgorithmBitmap;
>    UINT32                            TpmActivePcrBanks;
>    UINT32                            NewTpmActivePcrBanks;
>    UINT32                            Tpm2PcrMask;
> @@ -273,16 +274,27 @@ SyncPcrAllocationsAndPcrMask (
>    // Determine the current TPM support and the Platform PCR mask.
>    //
>    Status = Tpm2GetCapabilitySupportedAndActivePcrs
> (&TpmHashAlgorithmBitmap, &TpmActivePcrBanks);
> +
>    ASSERT_EFI_ERROR (Status);
> +
> +  DEBUG ((EFI_D_INFO, "Tpm2GetCapabilitySupportedAndActivePcrs -
> TpmHashAlgorithmBitmap: 0x%08x\n", TpmHashAlgorithmBitmap));
> +  DEBUG ((EFI_D_INFO, "Tpm2GetCapabilitySupportedAndActivePcrs -
> TpmActivePcrBanks 0x%08x\n", TpmActivePcrBanks));
> 
>    Tpm2PcrMask = PcdGet32 (PcdTpm2HashMask);
>    if (Tpm2PcrMask == 0) {
>      //
>      // if PcdTPm2HashMask is zero, use ActivePcr setting
>      //
> +    DEBUG ((EFI_D_VERBOSE, "Initializing PcdTpm2HashMask to
> TpmActivePcrBanks 0x%08x\n", TpmActivePcrBanks));
>      PcdSet32S (PcdTpm2HashMask, TpmActivePcrBanks);
> +    DEBUG ((EFI_D_VERBOSE, "Initializing Tpm2PcrMask to TpmActivePcrBanks
> 0x%08x\n", Tpm2PcrMask));
>      Tpm2PcrMask = TpmActivePcrBanks;
>    }
> +
> +  BiosHashAlgorithmBitmap = PcdGet32 (PcdTcg2HashAlgorithmBitmap);
> +  DEBUG ((EFI_D_INFO, "PcdTcg2HashAlgorithmBitmap 0x%08x\n",
> BiosHashAlgorithmBitmap));
> +  DEBUG ((EFI_D_INFO, "Tpm2PcrMask 0x%08x\n", Tpm2PcrMask)); // Active
> PCR banks from TPM input
> +  DEBUG ((EFI_D_INFO, "TpmActivePcrBanks & BiosHashAlgorithmBitmap =
> 0x%08x\n", NewTpmActivePcrBanks));
> 
>    //
>    // Find the intersection of Pcd support and TPM support.
> @@ -294,9 +306,12 @@ SyncPcrAllocationsAndPcrMask (
>    // If there are active PCR banks that are not supported by the Platform mask,
>    // update the TPM allocations and reboot the machine.
>    //
> -  if ((TpmActivePcrBanks & Tpm2PcrMask) != TpmActivePcrBanks) {
> +  if (((TpmActivePcrBanks & Tpm2PcrMask) != TpmActivePcrBanks) ||
> +      ((TpmActivePcrBanks & BiosHashAlgorithmBitmap) != TpmActivePcrBanks)) {
>      NewTpmActivePcrBanks = TpmActivePcrBanks & Tpm2PcrMask;
> +    NewTpmActivePcrBanks &= BiosHashAlgorithmBitmap;
> 
> +    DEBUG ((EFI_D_INFO, "NewTpmActivePcrBanks 0x%08x\n",
> NewTpmActivePcrBanks));
>      DEBUG ((EFI_D_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__));
> @@ -331,6 +346,7 @@ SyncPcrAllocationsAndPcrMask (
>      }
> 
>      Status = PcdSet32S (PcdTpm2HashMask, NewTpm2PcrMask);
> +    DEBUG ((EFI_D_INFO, "Setting PcdTpm2Hash Mask to 0x%08x\n",
> NewTpm2PcrMask));
>      ASSERT_EFI_ERROR (Status);
>    }
>  }
> 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.31.1.windows.1