* [PATCH] Reallocate TPM Active PCRs based on platform support.
@ 2021-08-04 23:28 Rodrigo Gonzalez del Cueto
2021-08-09 1:13 ` Yao, Jiewen
0 siblings, 1 reply; 11+ messages in thread
From: Rodrigo Gonzalez del Cueto @ 2021-08-04 23:28 UTC (permalink / raw)
To: devel; +Cc: Rodrigo Gonzalez del Cueto, Jian J Wang, Jiewen Yao
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
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH] Reallocate TPM Active PCRs based on platform support.
2021-08-04 23:28 [PATCH] Reallocate TPM Active PCRs based on platform support Rodrigo Gonzalez del Cueto
@ 2021-08-09 1:13 ` Yao, Jiewen
2021-08-10 6:26 ` Rodrigo Gonzalez del Cueto
0 siblings, 1 reply; 11+ messages in thread
From: Yao, Jiewen @ 2021-08-09 1:13 UTC (permalink / raw)
To: Gonzalez Del Cueto, Rodrigo, devel@edk2.groups.io; +Cc: Wang, Jian J
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] Reallocate TPM Active PCRs based on platform support.
2021-08-09 1:13 ` Yao, Jiewen
@ 2021-08-10 6:26 ` Rodrigo Gonzalez del Cueto
2021-08-11 5:36 ` Yao, Jiewen
0 siblings, 1 reply; 11+ messages in thread
From: Rodrigo Gonzalez del Cueto @ 2021-08-10 6:26 UTC (permalink / raw)
To: Yao, Jiewen, devel@edk2.groups.io; +Cc: Wang, Jian J
[-- Attachment #1: Type: text/plain, Size: 8674 bytes --]
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@intel.com>
Sent: Sunday, August 8, 2021 6:13 PM
To: Gonzalez Del Cueto, Rodrigo <rodrigo.gonzalez.del.cueto@intel.com>; devel@edk2.groups.io <devel@edk2.groups.io>
Cc: Wang, Jian J <jian.j.wang@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@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
[-- Attachment #2: Type: text/html, Size: 15537 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] Reallocate TPM Active PCRs based on platform support.
2021-08-10 6:26 ` Rodrigo Gonzalez del Cueto
@ 2021-08-11 5:36 ` Yao, Jiewen
2021-10-30 0:26 ` Rodrigo Gonzalez del Cueto
0 siblings, 1 reply; 11+ messages in thread
From: Yao, Jiewen @ 2021-08-11 5:36 UTC (permalink / raw)
To: Gonzalez Del Cueto, Rodrigo, devel@edk2.groups.io; +Cc: Wang, Jian J
[-- Attachment #1: Type: text/plain, Size: 9714 bytes --]
OK, Would you please to share the PCD configuration works before and PCD configuration fails now? As well as your DSC file on how to configure the library.
I would like to understand the problem statement from real use case, because the issue description cannot provide useful information to me.
From: Gonzalez Del Cueto, Rodrigo <rodrigo.gonzalez.del.cueto@intel.com>
Sent: Tuesday, August 10, 2021 2:27 PM
To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
Cc: Wang, Jian J <jian.j.wang@intel.com>
Subject: Re: [PATCH] Reallocate TPM Active PCRs based on platform support.
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@intel.com<mailto:jiewen.yao@intel.com>>
Sent: Sunday, August 8, 2021 6:13 PM
To: Gonzalez Del Cueto, Rodrigo <rodrigo.gonzalez.del.cueto@intel.com<mailto:rodrigo.gonzalez.del.cueto@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>>
Cc: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@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@intel.com<mailto:rodrigo.gonzalez.del.cueto@intel.com>>
> Sent: Thursday, August 5, 2021 7:28 AM
> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
> Cc: Gonzalez Del Cueto, Rodrigo <rodrigo.gonzalez.del.cueto@intel.com<mailto:rodrigo.gonzalez.del.cueto@intel.com>>;
> Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Yao, Jiewen <jiewen.yao@intel.com<mailto: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<mailto:rodrigo.gonzalez.del.cueto@intel.com>>
>
> Cc: Jian J Wang <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>
> Cc: Jiewen Yao <jiewen.yao@intel.com<mailto: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
[-- Attachment #2: Type: text/html, Size: 18299 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] Reallocate TPM Active PCRs based on platform support.
2021-08-11 5:36 ` Yao, Jiewen
@ 2021-10-30 0:26 ` Rodrigo Gonzalez del Cueto
2021-11-02 11:07 ` [edk2-devel] " Gerd Hoffmann
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Rodrigo Gonzalez del Cueto @ 2021-10-30 0:26 UTC (permalink / raw)
To: Yao, Jiewen, devel@edk2.groups.io; +Cc: Wang, Jian J
[-- Attachment #1: Type: text/plain, Size: 13406 bytes --]
Hi Jiewen,
In the past most of the TPM devices supported SHA1 and SHA256 hashing algorithms, which we have also supported in BIOS for many years.
What recently changed is the exposure to new TPM devices which support additional hashing algorithms (SHA384 and SM3) and will have such PCR banks active by default, but which are not supported by some BIOS implementations.
With the following example configuration, I will illustrate how we would hit the problematic condition I just described:
* Using a TPM device supporting SM3 hashing algorithm and with the corresponding PCR bank active by default.
HashLib library classes instances registered for Tcg2Config, Tcg2Pei and Tcg2Dxe modules:
* SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
* SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
* SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha384.inf
PCD Configuration:
* gEfiSecurityPkgTokenSpaceGuid.PcdTcg2HashAlgorithmBitmap|0xFFFFFFFF
* gEfiSecurityPkgTokenSpaceGuid.PcdTpm2HashMask|0x0000001F
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 present HashLib instances:
SyncPcrAllocationsAndPcrMask!
Supported PCRs - Count = 00000003
GetSupportedAndActivePcrs - Count = 00000002
SyncPcrAllocationsAndPcrMask - Updating PcdTpm2HashMask from 0x1F to 0x13.
You can see no reallocation is triggered; the unsupported PCR banks are left active and no extend operations occur on them, thus leaving them uncapped.
With the proposed patch set we are fixing two issues:
a) An additional check for the intersection between the TpmActivePcrBanks and the PcdTcg2HashAlgorithmBitmap populated by the BIOS' HashLib instances at runtime.
b) RegisterHashInterfaceLib correctly handles registering the HashLib instance supported algorithm bitmap when PcdTpm2HashMask is set to zero.
This is the BIOS behavior with the proposed patch:
SyncPcrAllocationsAndPcrMask!
Supported PCRs - Count = 00000003
GetSupportedAndActivePcrs - Count = 00000003
Tpm2GetCapabilitySupportedAndActivePcrs - TpmHashAlgorithmBitmap: 0x00000013
Tpm2GetCapabilitySupportedAndActivePcrs - TpmActivePcrBanks 0x00000013
TpmHashAlgorithmBitmap: 0x00000013
Tpm2PcrMask 0x0000001F
TpmActivePcrBanks & Tpm2PcrMask = 0x00000013
TpmActivePcrBanks & BiosHashAlgorithmBitmap = 0x00000003
NewTpmActivePcrBanks 0x00000003
SyncPcrAllocationsAndPcrMask - Reallocating PCR banks from 0x13 to 0x3.
Tpm2PcrAllocateBanks (TpmHashAlgorithmBitmap: 0x00000013, NewTpmActivePcrBanks: 0x00000003)
Tpm2PcrAllocateBanks call Tpm2PcrAllocate - Success
AllocationSuccess - 01
MaxPCR - 00000018
SizeNeeded - 000004E0
SizeAvailable - 00000C60
After the PCR reallocation is triggered, the TPM active PCRs are a strict subset of the hashing algorithms supported by BIOS.
Please let me know if you need any questions regarding the solution or need any further clarification on the problem statement.
Regards,
-Rodrigo
________________________________
From: Yao, Jiewen <jiewen.yao@intel.com>
Sent: Tuesday, August 10, 2021 10:36 PM
To: Gonzalez Del Cueto, Rodrigo <rodrigo.gonzalez.del.cueto@intel.com>; devel@edk2.groups.io <devel@edk2.groups.io>
Cc: Wang, Jian J <jian.j.wang@intel.com>
Subject: RE: [PATCH] Reallocate TPM Active PCRs based on platform support.
OK, Would you please to share the PCD configuration works before and PCD configuration fails now? As well as your DSC file on how to configure the library.
I would like to understand the problem statement from real use case, because the issue description cannot provide useful information to me.
From: Gonzalez Del Cueto, Rodrigo <rodrigo.gonzalez.del.cueto@intel.com>
Sent: Tuesday, August 10, 2021 2:27 PM
To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
Cc: Wang, Jian J <jian.j.wang@intel.com>
Subject: Re: [PATCH] Reallocate TPM Active PCRs based on platform support.
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@intel.com<mailto:jiewen.yao@intel.com>>
Sent: Sunday, August 8, 2021 6:13 PM
To: Gonzalez Del Cueto, Rodrigo <rodrigo.gonzalez.del.cueto@intel.com<mailto:rodrigo.gonzalez.del.cueto@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>>
Cc: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@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@intel.com<mailto:rodrigo.gonzalez.del.cueto@intel.com>>
> Sent: Thursday, August 5, 2021 7:28 AM
> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
> Cc: Gonzalez Del Cueto, Rodrigo <rodrigo.gonzalez.del.cueto@intel.com<mailto:rodrigo.gonzalez.del.cueto@intel.com>>;
> Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Yao, Jiewen <jiewen.yao@intel.com<mailto: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<mailto:rodrigo.gonzalez.del.cueto@intel.com>>
>
> Cc: Jian J Wang <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>
> Cc: Jiewen Yao <jiewen.yao@intel.com<mailto: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
[-- Attachment #2: Type: text/html, Size: 29371 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [edk2-devel] [PATCH] Reallocate TPM Active PCRs based on platform support.
2021-10-30 0:26 ` Rodrigo Gonzalez del Cueto
@ 2021-11-02 11:07 ` Gerd Hoffmann
2021-11-04 13:39 ` Yao, Jiewen
[not found] ` <16B45B855E634563.17679@groups.io>
2 siblings, 0 replies; 11+ messages in thread
From: Gerd Hoffmann @ 2021-11-02 11:07 UTC (permalink / raw)
To: devel, rodrigo.gonzalez.del.cueto; +Cc: Yao, Jiewen, Wang, Jian J
Hi,
> You can see no reallocation is triggered; the unsupported PCR banks are left active and no extend operations occur on them, thus leaving them uncapped.
I have seen this effect too the compiling OVMF with SHA1 support
disabled, the firmware left the SHA1 bank active then ...
take care,
Gerd
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] Reallocate TPM Active PCRs based on platform support.
2021-10-30 0:26 ` Rodrigo Gonzalez del Cueto
2021-11-02 11:07 ` [edk2-devel] " Gerd Hoffmann
@ 2021-11-04 13:39 ` Yao, Jiewen
[not found] ` <16B45B855E634563.17679@groups.io>
2 siblings, 0 replies; 11+ messages in thread
From: Yao, Jiewen @ 2021-11-04 13:39 UTC (permalink / raw)
To: Gonzalez Del Cueto, Rodrigo, devel@edk2.groups.io; +Cc: Wang, Jian J
[-- Attachment #1: Type: text/plain, Size: 14240 bytes --]
Thanks the detail explanation.
I think it makes sense to make "NewTpmActivePcrBanks = TpmActivePcrBanks & PcdTpm2HashMask (hardware config) & PcdTcg2HashAlgorithmBitmap (software config)"
Reviewed-by: Jiewen Yao <Jiewen.yao@intel.com>
From: Gonzalez Del Cueto, Rodrigo <rodrigo.gonzalez.del.cueto@intel.com>
Sent: Saturday, October 30, 2021 8:26 AM
To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
Cc: Wang, Jian J <jian.j.wang@intel.com>
Subject: Re: [PATCH] Reallocate TPM Active PCRs based on platform support.
Hi Jiewen,
In the past most of the TPM devices supported SHA1 and SHA256 hashing algorithms, which we have also supported in BIOS for many years.
What recently changed is the exposure to new TPM devices which support additional hashing algorithms (SHA384 and SM3) and will have such PCR banks active by default, but which are not supported by some BIOS implementations.
With the following example configuration, I will illustrate how we would hit the problematic condition I just described:
* Using a TPM device supporting SM3 hashing algorithm and with the corresponding PCR bank active by default.
HashLib library classes instances registered for Tcg2Config, Tcg2Pei and Tcg2Dxe modules:
* SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
* SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
* SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha384.inf
PCD Configuration:
* gEfiSecurityPkgTokenSpaceGuid.PcdTcg2HashAlgorithmBitmap|0xFFFFFFFF
* gEfiSecurityPkgTokenSpaceGuid.PcdTpm2HashMask|0x0000001F
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 present HashLib instances:
SyncPcrAllocationsAndPcrMask!
Supported PCRs - Count = 00000003
GetSupportedAndActivePcrs - Count = 00000002
SyncPcrAllocationsAndPcrMask - Updating PcdTpm2HashMask from 0x1F to 0x13.
You can see no reallocation is triggered; the unsupported PCR banks are left active and no extend operations occur on them, thus leaving them uncapped.
With the proposed patch set we are fixing two issues:
a) An additional check for the intersection between the TpmActivePcrBanks and the PcdTcg2HashAlgorithmBitmap populated by the BIOS' HashLib instances at runtime.
b) RegisterHashInterfaceLib correctly handles registering the HashLib instance supported algorithm bitmap when PcdTpm2HashMask is set to zero.
This is the BIOS behavior with the proposed patch:
SyncPcrAllocationsAndPcrMask!
Supported PCRs - Count = 00000003
GetSupportedAndActivePcrs - Count = 00000003
Tpm2GetCapabilitySupportedAndActivePcrs - TpmHashAlgorithmBitmap: 0x00000013
Tpm2GetCapabilitySupportedAndActivePcrs - TpmActivePcrBanks 0x00000013
TpmHashAlgorithmBitmap: 0x00000013
Tpm2PcrMask 0x0000001F
TpmActivePcrBanks & Tpm2PcrMask = 0x00000013
TpmActivePcrBanks & BiosHashAlgorithmBitmap = 0x00000003
NewTpmActivePcrBanks 0x00000003
SyncPcrAllocationsAndPcrMask - Reallocating PCR banks from 0x13 to 0x3.
Tpm2PcrAllocateBanks (TpmHashAlgorithmBitmap: 0x00000013, NewTpmActivePcrBanks: 0x00000003)
Tpm2PcrAllocateBanks call Tpm2PcrAllocate - Success
AllocationSuccess - 01
MaxPCR - 00000018
SizeNeeded - 000004E0
SizeAvailable - 00000C60
After the PCR reallocation is triggered, the TPM active PCRs are a strict subset of the hashing algorithms supported by BIOS.
Please let me know if you need any questions regarding the solution or need any further clarification on the problem statement.
Regards,
-Rodrigo
________________________________
From: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>
Sent: Tuesday, August 10, 2021 10:36 PM
To: Gonzalez Del Cueto, Rodrigo <rodrigo.gonzalez.del.cueto@intel.com<mailto:rodrigo.gonzalez.del.cueto@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>>
Cc: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>
Subject: RE: [PATCH] Reallocate TPM Active PCRs based on platform support.
OK, Would you please to share the PCD configuration works before and PCD configuration fails now? As well as your DSC file on how to configure the library.
I would like to understand the problem statement from real use case, because the issue description cannot provide useful information to me.
From: Gonzalez Del Cueto, Rodrigo <rodrigo.gonzalez.del.cueto@intel.com<mailto:rodrigo.gonzalez.del.cueto@intel.com>>
Sent: Tuesday, August 10, 2021 2:27 PM
To: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Cc: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>
Subject: Re: [PATCH] Reallocate TPM Active PCRs based on platform support.
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@intel.com<mailto:jiewen.yao@intel.com>>
Sent: Sunday, August 8, 2021 6:13 PM
To: Gonzalez Del Cueto, Rodrigo <rodrigo.gonzalez.del.cueto@intel.com<mailto:rodrigo.gonzalez.del.cueto@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>>
Cc: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@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@intel.com<mailto:rodrigo.gonzalez.del.cueto@intel.com>>
> Sent: Thursday, August 5, 2021 7:28 AM
> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
> Cc: Gonzalez Del Cueto, Rodrigo <rodrigo.gonzalez.del.cueto@intel.com<mailto:rodrigo.gonzalez.del.cueto@intel.com>>;
> Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Yao, Jiewen <jiewen.yao@intel.com<mailto: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<mailto:rodrigo.gonzalez.del.cueto@intel.com>>
>
> Cc: Jian J Wang <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>
> Cc: Jiewen Yao <jiewen.yao@intel.com<mailto: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
[-- Attachment #2: Type: text/html, Size: 42527 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread[parent not found: <16B45B855E634563.17679@groups.io>]
* Re: [edk2-devel] [PATCH] Reallocate TPM Active PCRs based on platform support.
[not found] ` <16B45B855E634563.17679@groups.io>
@ 2021-11-04 13:59 ` Yao, Jiewen
0 siblings, 0 replies; 11+ messages in thread
From: Yao, Jiewen @ 2021-11-04 13:59 UTC (permalink / raw)
To: devel@edk2.groups.io, Yao, Jiewen, Gonzalez Del Cueto, Rodrigo
Cc: Wang, Jian J
[-- Attachment #1: Type: text/plain, Size: 14897 bytes --]
CI fail: https://github.com/tianocore/edk2/pull/2172
Please fix it, run CI by yourself before send the next version.
Thank you
Yao Jiewen
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Yao, Jiewen
Sent: Thursday, November 4, 2021 9:40 PM
To: Gonzalez Del Cueto, Rodrigo <rodrigo.gonzalez.del.cueto@intel.com>; devel@edk2.groups.io
Cc: Wang, Jian J <jian.j.wang@intel.com>
Subject: Re: [edk2-devel] [PATCH] Reallocate TPM Active PCRs based on platform support.
Thanks the detail explanation.
I think it makes sense to make "NewTpmActivePcrBanks = TpmActivePcrBanks & PcdTpm2HashMask (hardware config) & PcdTcg2HashAlgorithmBitmap (software config)"
Reviewed-by: Jiewen Yao <Jiewen.yao@intel.com<mailto:Jiewen.yao@intel.com>>
From: Gonzalez Del Cueto, Rodrigo <rodrigo.gonzalez.del.cueto@intel.com<mailto:rodrigo.gonzalez.del.cueto@intel.com>>
Sent: Saturday, October 30, 2021 8:26 AM
To: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Cc: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>
Subject: Re: [PATCH] Reallocate TPM Active PCRs based on platform support.
Hi Jiewen,
In the past most of the TPM devices supported SHA1 and SHA256 hashing algorithms, which we have also supported in BIOS for many years.
What recently changed is the exposure to new TPM devices which support additional hashing algorithms (SHA384 and SM3) and will have such PCR banks active by default, but which are not supported by some BIOS implementations.
With the following example configuration, I will illustrate how we would hit the problematic condition I just described:
* Using a TPM device supporting SM3 hashing algorithm and with the corresponding PCR bank active by default.
HashLib library classes instances registered for Tcg2Config, Tcg2Pei and Tcg2Dxe modules:
* SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
* SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
* SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha384.inf
PCD Configuration:
* gEfiSecurityPkgTokenSpaceGuid.PcdTcg2HashAlgorithmBitmap|0xFFFFFFFF
* gEfiSecurityPkgTokenSpaceGuid.PcdTpm2HashMask|0x0000001F
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 present HashLib instances:
SyncPcrAllocationsAndPcrMask!
Supported PCRs - Count = 00000003
GetSupportedAndActivePcrs - Count = 00000002
SyncPcrAllocationsAndPcrMask - Updating PcdTpm2HashMask from 0x1F to 0x13.
You can see no reallocation is triggered; the unsupported PCR banks are left active and no extend operations occur on them, thus leaving them uncapped.
With the proposed patch set we are fixing two issues:
a) An additional check for the intersection between the TpmActivePcrBanks and the PcdTcg2HashAlgorithmBitmap populated by the BIOS' HashLib instances at runtime.
b) RegisterHashInterfaceLib correctly handles registering the HashLib instance supported algorithm bitmap when PcdTpm2HashMask is set to zero.
This is the BIOS behavior with the proposed patch:
SyncPcrAllocationsAndPcrMask!
Supported PCRs - Count = 00000003
GetSupportedAndActivePcrs - Count = 00000003
Tpm2GetCapabilitySupportedAndActivePcrs - TpmHashAlgorithmBitmap: 0x00000013
Tpm2GetCapabilitySupportedAndActivePcrs - TpmActivePcrBanks 0x00000013
TpmHashAlgorithmBitmap: 0x00000013
Tpm2PcrMask 0x0000001F
TpmActivePcrBanks & Tpm2PcrMask = 0x00000013
TpmActivePcrBanks & BiosHashAlgorithmBitmap = 0x00000003
NewTpmActivePcrBanks 0x00000003
SyncPcrAllocationsAndPcrMask - Reallocating PCR banks from 0x13 to 0x3.
Tpm2PcrAllocateBanks (TpmHashAlgorithmBitmap: 0x00000013, NewTpmActivePcrBanks: 0x00000003)
Tpm2PcrAllocateBanks call Tpm2PcrAllocate - Success
AllocationSuccess - 01
MaxPCR - 00000018
SizeNeeded - 000004E0
SizeAvailable - 00000C60
After the PCR reallocation is triggered, the TPM active PCRs are a strict subset of the hashing algorithms supported by BIOS.
Please let me know if you need any questions regarding the solution or need any further clarification on the problem statement.
Regards,
-Rodrigo
________________________________
From: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>
Sent: Tuesday, August 10, 2021 10:36 PM
To: Gonzalez Del Cueto, Rodrigo <rodrigo.gonzalez.del.cueto@intel.com<mailto:rodrigo.gonzalez.del.cueto@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>>
Cc: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>
Subject: RE: [PATCH] Reallocate TPM Active PCRs based on platform support.
OK, Would you please to share the PCD configuration works before and PCD configuration fails now? As well as your DSC file on how to configure the library.
I would like to understand the problem statement from real use case, because the issue description cannot provide useful information to me.
From: Gonzalez Del Cueto, Rodrigo <rodrigo.gonzalez.del.cueto@intel.com<mailto:rodrigo.gonzalez.del.cueto@intel.com>>
Sent: Tuesday, August 10, 2021 2:27 PM
To: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Cc: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>
Subject: Re: [PATCH] Reallocate TPM Active PCRs based on platform support.
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@intel.com<mailto:jiewen.yao@intel.com>>
Sent: Sunday, August 8, 2021 6:13 PM
To: Gonzalez Del Cueto, Rodrigo <rodrigo.gonzalez.del.cueto@intel.com<mailto:rodrigo.gonzalez.del.cueto@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>>
Cc: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@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@intel.com<mailto:rodrigo.gonzalez.del.cueto@intel.com>>
> Sent: Thursday, August 5, 2021 7:28 AM
> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
> Cc: Gonzalez Del Cueto, Rodrigo <rodrigo.gonzalez.del.cueto@intel.com<mailto:rodrigo.gonzalez.del.cueto@intel.com>>;
> Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Yao, Jiewen <jiewen.yao@intel.com<mailto: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<mailto:rodrigo.gonzalez.del.cueto@intel.com>>
>
> Cc: Jian J Wang <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>
> Cc: Jiewen Yao <jiewen.yao@intel.com<mailto: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
[-- Attachment #2: Type: text/html, Size: 47298 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] Reallocate TPM Active PCRs based on platform support.
@ 2021-11-04 18:06 Rodrigo Gonzalez del Cueto
2021-11-05 5:35 ` Yao, Jiewen
0 siblings, 1 reply; 11+ messages in thread
From: Rodrigo Gonzalez del Cueto @ 2021-11-04 18:06 UTC (permalink / raw)
To: devel; +Cc: Rodrigo Gonzalez del Cueto, Jian J Wang, Jiewen Yao
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
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH] Reallocate TPM Active PCRs based on platform support.
2021-11-04 18:06 Rodrigo Gonzalez del Cueto
@ 2021-11-05 5:35 ` Yao, Jiewen
0 siblings, 0 replies; 11+ messages in thread
From: Yao, Jiewen @ 2021-11-05 5:35 UTC (permalink / raw)
To: Gonzalez Del Cueto, Rodrigo, devel@edk2.groups.io; +Cc: Wang, Jian J
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] Reallocate TPM Active PCRs based on platform support.
@ 2021-07-29 17:33 Rodrigo Gonzalez del Cueto
0 siblings, 0 replies; 11+ messages in thread
From: Rodrigo Gonzalez del Cueto @ 2021-07-29 17:33 UTC (permalink / raw)
To: devel; +Cc: Rodrigo Gonzalez del Cueto, Jian J Wang, Jiewen Yao
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3515
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.
Change-Id: I1cdabe14a4fb5adfc289a2dd60f1b467c64282ac
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/Tcg/Tcg2Pei/Tcg2Pei.c | 18 +++++++++++++++++-
SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf | 1 +
2 files changed, 18 insertions(+), 1 deletion(-)
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-11-05 5:35 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-08-04 23:28 [PATCH] Reallocate TPM Active PCRs based on platform support Rodrigo Gonzalez del Cueto
2021-08-09 1:13 ` Yao, Jiewen
2021-08-10 6:26 ` Rodrigo Gonzalez del Cueto
2021-08-11 5:36 ` Yao, Jiewen
2021-10-30 0:26 ` Rodrigo Gonzalez del Cueto
2021-11-02 11:07 ` [edk2-devel] " Gerd Hoffmann
2021-11-04 13:39 ` Yao, Jiewen
[not found] ` <16B45B855E634563.17679@groups.io>
2021-11-04 13:59 ` [edk2-devel] " Yao, Jiewen
-- strict thread matches above, loose matches on Subject: below --
2021-11-04 18:06 Rodrigo Gonzalez del Cueto
2021-11-05 5:35 ` Yao, Jiewen
2021-07-29 17:33 Rodrigo Gonzalez del Cueto
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox