[edk2-devel] [PATCH] Reallocate TPM Active PCRs based on platform support.
Rodrigo Gonzalez del Cueto
rodrigo.gonzalez.del.cueto at intel.com
Tue Aug 10 06:26:51 UTC 2021
Hi Jiewen,
Indeed, this bug has existed for a long time in this code. What recently changed are the TPM configurations we are testing and exposed the issue; this can be reproduced when the BIOS supported algorithms are a strict subset of the PCRs currently active in the TPM.
Now that we are using TPM configurations with support for additional PCR banks (ex. SHA384 and SM3) the bug has been exposed when compiling a BIOS without support for these PCR banks which are active by default in the some of the TPMs.
Regards,
-Rodrigo
________________________________
From: Yao, Jiewen <jiewen.yao at intel.com>
Sent: Sunday, August 8, 2021 6:13 PM
To: Gonzalez Del Cueto, Rodrigo <rodrigo.gonzalez.del.cueto at intel.com>; devel at edk2.groups.io <devel at edk2.groups.io>
Cc: Wang, Jian J <jian.j.wang at intel.com>
Subject: RE: [PATCH] Reallocate TPM Active PCRs based on platform support.
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 at intel.com>
> Sent: Thursday, August 5, 2021 7:28 AM
> To: devel at edk2.groups.io
> Cc: Gonzalez Del Cueto, Rodrigo <rodrigo.gonzalez.del.cueto at intel.com>;
> Wang, Jian J <jian.j.wang at intel.com>; Yao, Jiewen <jiewen.yao at 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 at intel.com>
>
> Cc: Jian J Wang <jian.j.wang at intel.com>
> Cc: Jiewen Yao <jiewen.yao at 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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#79019): https://edk2.groups.io/g/devel/message/79019
Mute This Topic: https://groups.io/mt/84674454/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/edk2-devel-archive/attachments/20210810/cd6c3817/attachment.htm>
More information about the edk2-devel-archive
mailing list