Thanks Abner.

I will correct the description for the function AcpiLocateTableBySignature
in the v3.

Best regards,
Nhi

On 08/09/2021 11:53, Chang, Abner (HPS SW/FW Technologist) wrote:

After below comments are addressed,

Reviewed-by: Abner Chang <abner.chang@...>

-----Original Message-----
From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
Nhi Pham via groups.io
Sent: Friday, September 3, 2021 11:44 PM
To: devel@edk2.groups.io
Cc: patches@...; Nhi Pham
<nhi@...>; Leif Lindholm <leif@...>; Ard
Biesheuvel <ardb+tianocore@...>; Chang, Abner (HPS SW/FW
Technologist) <abner.chang@...>; Schaefer, Daniel
<daniel.schaefer@...>
Subject: [edk2-devel] [PATCH v2 1/1] EmbeddedPkg/AcpiLib: Add more
helper functions

This adds more helper functions that assist in calculating the checksum,
locating an ACPI table by signature, and updating an AML integer object.

Cc: Leif Lindholm <leif@...>
Cc: Ard Biesheuvel <ardb+tianocore@...>
Cc: Abner Chang <abner.chang@...>
Cc: Daniel Schaefer <daniel.schaefer@...>
Signed-off-by: Nhi Pham <nhi@...>
---
Changes since v1:
+ Add copyright [Abner]
+ Improve the AcpiLocateTableBySignature function to remove the caution
for the usage of SSDT table. [Abner]
+ AcpiAmlObjectUpdateInteger: Use the AcpiSdtProtocol->SetOption to
update
the value of data object. [Abner]

EmbeddedPkg/Library/AcpiLib/AcpiLib.inf | 3 +
EmbeddedPkg/Include/Library/AcpiLib.h | 69 +++++++
EmbeddedPkg/Library/AcpiLib/AcpiLib.c | 214 ++++++++++++++++++++
3 files changed, 286 insertions(+)

diff --git a/EmbeddedPkg/Library/AcpiLib/AcpiLib.inf
b/EmbeddedPkg/Library/AcpiLib/AcpiLib.inf
index 538fe09cca29..01b12c9423a9 100644
--- a/EmbeddedPkg/Library/AcpiLib/AcpiLib.inf
+++ b/EmbeddedPkg/Library/AcpiLib/AcpiLib.inf
@@ -1,6 +1,7 @@
#/** @file
#
# Copyright (c) 2014, ARM Ltd. All rights reserved.
+# Copyright (c) 2021, Ampere Computing LLC. All rights reserved.
#
# SPDX-License-Identifier: BSD-2-Clause-Patent
#
@@ -23,6 +24,8 @@ [Packages]
EmbeddedPkg/EmbeddedPkg.dec

[LibraryClasses]
+ BaseLib
+ BaseMemoryLib
DebugLib
UefiBootServicesTableLib

diff --git a/EmbeddedPkg/Include/Library/AcpiLib.h
b/EmbeddedPkg/Include/Library/AcpiLib.h
index c142446d9d59..933582b7f607 100644
--- a/EmbeddedPkg/Include/Library/AcpiLib.h
+++ b/EmbeddedPkg/Include/Library/AcpiLib.h
@@ -2,6 +2,7 @@
Helper Library for ACPI

Copyright (c) 2014-2016, ARM Ltd. All rights reserved.
+ Copyright (c) 2021, Ampere Computing LLC. All rights reserved.

SPDX-License-Identifier: BSD-2-Clause-Patent

@@ -13,6 +14,7 @@
#include <Uefi.h>

#include <IndustryStandard/Acpi10.h>
+#include <Protocol/AcpiSystemDescriptionTable.h>

//
// Macros for the Generic Address Space
@@ -128,4 +130,71 @@ LocateAndInstallAcpiFromFv (
IN CONST EFI_GUID* AcpiFile
);

+/**
+ This function calculates and updates a UINT8 checksum
+ in an ACPI description table header.
+
+ @param Buffer Pointer to buffer to checksum
+ @param Size Number of bytes to checksum
+
+ @retval EFI_SUCCESS The function completed successfully.
+ @retval EFI_INVALID_PARAMETER Invalid parameter.
+
+**/
+EFI_STATUS
+EFIAPI
+AcpiUpdateChecksum (
+ IN OUT UINT8 *Buffer,
+ IN UINTN Size
+ );
+
+/**
+ This function uses the ACPI SDT protocol to locate an ACPI table
+ with a given signature that only have a single instance.
+
+ @param AcpiTableSdtProtocol Pointer to ACPI SDT protocol.
+ @param TableSignature ACPI table signature.
+ @param Index The zero-based index of the table where to
search the table.
+ @param Table Pointer to the table.
+ @param TableKey Pointer to the table key.
+
+ @return EFI_SUCCESS The function completed successfully.
+ @return EFI_INVALID_PARAMETER At least one of parameters is invalid.
+ @retval EFI_NOT_FOUND The requested index is too large and a
table was not found.
+
+**/
+EFI_STATUS
+EFIAPI
+AcpiLocateTableBySignature (
+ IN EFI_ACPI_SDT_PROTOCOL *AcpiSdtProtocol,
+ IN UINT32 TableSignature,
+ IN OUT UINTN *Index,
+ OUT EFI_ACPI_DESCRIPTION_HEADER **Table,
+ OUT UINTN *TableKey
+ );
+
+/**
+ This function updates the integer value of an AML Object.
+
+ @param AcpiTableSdtProtocol Pointer to ACPI SDT protocol.
+ @param TableHandle Points to the table representing the starting
point
+ for the object path search.
+ @param AsciiObjectPath Pointer to the ACPI path of the object being
updated.
+ @param Value New value to write to the object.
+
+ @return EFI_SUCCESS The function completed successfully.
+ @return EFI_INVALID_PARAMETER At least one of parameters is invalid
or the data type
+ of the ACPI object is not an integer value.
+ @retval EFI_NOT_FOUND The object is not found with the given path.
+
+**/
+EFI_STATUS
+EFIAPI
+AcpiAmlObjectUpdateInteger (
+ IN EFI_ACPI_SDT_PROTOCOL *AcpiSdtProtocol,
+ IN EFI_ACPI_HANDLE TableHandle,
+ IN CHAR8 *AsciiObjectPath,
+ IN UINTN Value
+ );
+
#endif // __ACPI_LIB_H__
diff --git a/EmbeddedPkg/Library/AcpiLib/AcpiLib.c
b/EmbeddedPkg/Library/AcpiLib/AcpiLib.c
index ff7d678433d5..393133f54381 100644
--- a/EmbeddedPkg/Library/AcpiLib/AcpiLib.c
+++ b/EmbeddedPkg/Library/AcpiLib/AcpiLib.c
@@ -1,6 +1,7 @@
/** @file
*
* Copyright (c) 2014-2015, ARM Limited. All rights reserved.
+* Copyright (c) 2021, Ampere Computing LLC. All rights reserved.
*
* SPDX-License-Identifier: BSD-2-Clause-Patent
*
@@ -9,9 +10,12 @@
#include <Uefi.h>

#include <Library/AcpiLib.h>
+#include <Library/BaseLib.h>
+#include <Library/BaseMemoryLib.h>
#include <Library/DebugLib.h>
#include <Library/UefiBootServicesTableLib.h>

+#include <Protocol/AcpiSystemDescriptionTable.h>
#include <Protocol/AcpiTable.h>
#include <Protocol/FirmwareVolume2.h>

@@ -170,3 +174,213 @@ LocateAndInstallAcpiFromFv (
{
return LocateAndInstallAcpiFromFvConditional (AcpiFile, NULL);
}
+
+/**
+ This function calculates and updates a UINT8 checksum
+ in an ACPI description table header.
+
+ @param Buffer Pointer to buffer to checksum
+ @param Size Number of bytes to checksum
+
+ @retval EFI_SUCCESS The function completed successfully.
+ @retval EFI_INVALID_PARAMETER Invalid parameter.
+
+**/
+EFI_STATUS
+EFIAPI
+AcpiUpdateChecksum (
+ IN OUT UINT8 *Buffer,
+ IN UINTN Size
+ )
+{
+ UINTN ChecksumOffset;
+
+ if (Buffer == NULL || Size == 0) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ ChecksumOffset = OFFSET_OF (EFI_ACPI_DESCRIPTION_HEADER,
Checksum);
+
+ //
+ // Set checksum to 0 first
+ //
+ Buffer[ChecksumOffset] = 0;
+
+ //
+ // Update checksum value
+ //
+ Buffer[ChecksumOffset] = CalculateCheckSum8 (Buffer, Size);
+
+ return EFI_SUCCESS;
+}
+
+/**
+ This function uses the ACPI SDT protocol to locate an ACPI table
+ with a given signature that only have a single instance.

Is the description correct? I think this function can be used to search
the given signature which could have multiple instances, right?

+
+ @param AcpiTableSdtProtocol Pointer to ACPI SDT protocol.
+ @param TableSignature ACPI table signature.
+ @param Index The zero-based index of the table where to
search the table.

Could you please mention that the index will be updated to the next instance
if the table is found with the matched TableSignature?

+ @param Table Pointer to the table.
+ @param TableKey Pointer to the table key.
+
+ @return EFI_SUCCESS The function completed successfully.
+ @return EFI_INVALID_PARAMETER At least one of parameters is invalid.
+ @retval EFI_NOT_FOUND The requested index is too large and a
table was not found.
+
+**/
+EFI_STATUS
+EFIAPI
+AcpiLocateTableBySignature (
+ IN EFI_ACPI_SDT_PROTOCOL *AcpiSdtProtocol,
+ IN UINT32 TableSignature,
+ IN OUT UINTN *Index,
+ OUT EFI_ACPI_DESCRIPTION_HEADER **Table,
+ OUT UINTN *TableKey
+ )
+{
+ EFI_STATUS Status;
+ EFI_ACPI_SDT_HEADER *TempTable;
+ EFI_ACPI_TABLE_VERSION TableVersion;
+ UINTN TableIndex;
+
+ if (AcpiSdtProtocol == NULL
+ || Table == NULL
+ || TableKey == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ Status = EFI_SUCCESS;
+
+ //
+ // Search for ACPI Table with matching signature
+ //
+ TableVersion = 0;
+ TableIndex = *Index;
+ while (!EFI_ERROR (Status)) {
+ Status = AcpiSdtProtocol->GetAcpiTable (
+ TableIndex,
+ &TempTable,
+ &TableVersion,
+ TableKey
+ );
+ if (!EFI_ERROR (Status)) {
+ TableIndex++;
+
+ if (((EFI_ACPI_DESCRIPTION_HEADER *)TempTable)->Signature ==
TableSignature) {
+ *Table = (EFI_ACPI_DESCRIPTION_HEADER *)TempTable;
+ *Index = TableIndex;
+ break;
+ }
+ }
+ }
+
+ return Status;
+}
+
+/**
+ This function updates the integer value of an AML Object.
+
+ @param AcpiTableSdtProtocol Pointer to ACPI SDT protocol.
+ @param TableHandle Points to the table representing the starting
point
+ for the object path search.
+ @param AsciiObjectPath Pointer to the ACPI path of the object being
updated.
+ @param Value New value to write to the object.
+
+ @return EFI_SUCCESS The function completed successfully.
+ @return EFI_INVALID_PARAMETER At least one of parameters is invalid
or the data type
+ of the ACPI object is not an integer value.
+ @retval EFI_NOT_FOUND The object is not found with the given path.
+
+**/
+EFI_STATUS
+EFIAPI
+AcpiAmlObjectUpdateInteger (
+ IN EFI_ACPI_SDT_PROTOCOL *AcpiSdtProtocol,
+ IN EFI_ACPI_HANDLE TableHandle,
+ IN CHAR8 *AsciiObjectPath,
+ IN UINTN Value
+ )
+{
+ EFI_STATUS Status;
+ EFI_ACPI_HANDLE ObjectHandle;
+ EFI_ACPI_HANDLE DataHandle;
+ EFI_ACPI_DATA_TYPE DataType;
+ UINT8 *Buffer;
+ UINTN BufferSize;
+ UINTN DataSize;
+
+ if (AcpiSdtProtocol == NULL || AsciiObjectPath == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ ObjectHandle = NULL;
+ Status = AcpiSdtProtocol->FindPath (TableHandle, AsciiObjectPath,
&ObjectHandle);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ Status = AcpiSdtProtocol->GetOption (ObjectHandle, 0, &DataType, (VOID
*)&Buffer, &BufferSize);
+ if (EFI_ERROR (Status)) {
+ Status = EFI_NOT_FOUND;
+ goto Exit;
+ }
+ ASSERT (DataType == EFI_ACPI_DATA_TYPE_OPCODE);
+ ASSERT (Buffer != NULL);
+
+ if (Buffer[0] != AML_NAME_OP) {
+ Status = EFI_NOT_FOUND;
+ goto Exit;
+ }
+
+ //
+ // Get handle of data object
+ //
+ DataHandle = NULL;
+ Status = AcpiSdtProtocol->GetChild (ObjectHandle, &DataHandle);
+ ASSERT_EFI_ERROR (Status);
+
+ Status = AcpiSdtProtocol->GetOption (DataHandle, 0, &DataType, (VOID

Ok, that is fine to use 0 as you mentioned AML_OP_PARSE_INDEX_GET_OPCODE was defined privately. The better way
is to define this in EmbeddedPkg/Library/AcpiLib.h. Sorry I don't have chance to reply your last mail.

Abner

*)&Buffer, &BufferSize);
+ ASSERT (DataType == EFI_ACPI_DATA_TYPE_OPCODE);
+ ASSERT (Buffer != NULL);
+
+ if (Buffer[0] == AML_ZERO_OP || Buffer[0] == AML_ONE_OP) {
+ Status = AcpiSdtProtocol->SetOption (DataHandle, 0, (VOID *)&Value,
sizeof (UINT8));
+ ASSERT_EFI_ERROR (Status);
+ } else {
+ //
+ // Check the size of data object
+ //
+ switch (Buffer[0]) {
+ case AML_BYTE_PREFIX:
+ DataSize = sizeof (UINT8);
+ break;
+
+ case AML_WORD_PREFIX:
+ DataSize = sizeof (UINT16);
+ break;
+
+ case AML_DWORD_PREFIX:
+ DataSize = sizeof (UINT32);
+ break;
+
+ case AML_QWORD_PREFIX:
+ DataSize = sizeof (UINT64);
+ break;
+
+ default:
+ // The data type of the ACPI object is not an integer
+ Status = EFI_INVALID_PARAMETER;
+ goto Exit;
+ }
+
+ Status = AcpiSdtProtocol->SetOption (DataHandle, 1, (VOID *)&Value,
DataSize);
+ ASSERT_EFI_ERROR (Status);
+ }
+
+Exit:
+ AcpiSdtProtocol->Close (DataHandle);
+ AcpiSdtProtocol->Close (ObjectHandle);
+
+ return Status;
+}
--
2.17.1

-----Original Message-----
From: Gonzalez Del Cueto, Rodrigo <rodrigo.gonzalez.del.cueto@...>
Sent: Friday, December 17, 2021 10:48 AM
To: devel@edk2.groups.io
Cc: Gonzalez Del Cueto, Rodrigo <rodrigo.gonzalez.del.cueto@...>;
Wang, Jian J <jian.j.wang@...>; Yao, Jiewen <jiewen.yao@...>
Subject: [PATCH] SecurityPkg: TPM must go to Idle state on CRB command
completion

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3463

In V2: Fixed patch format and uncrustify cleanup

In V1: To follow the TCG CRB protocol specification, on every CRB TPM
command completion the TPM should return to Idle state, regardless of
the CRB Idle Bypass capability reported by the TPM device.

See: TCG PC Client Device Driver Design Principles for TPM 2.0,
Version 1.0, Rev 0.27

Signed-off-by: Rodrigo Gonzalez del Cueto
<rodrigo.gonzalez.del.cueto@...>
Cc: Jian J Wang <jian.j.wang@...>
Cc: Jiewen Yao <jiewen.yao@...>
---
SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2Ptp.c | 14 ++------------
1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2Ptp.c
b/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2Ptp.c
index 40ab998004..1d99beaa10 100644
--- a/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2Ptp.c
+++ b/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2Ptp.c
@@ -1,7 +1,7 @@
/** @file
PTP (Platform TPM Profile) CRB (Command Response Buffer) interface used by
dTPM2.0 library.

-Copyright (c) 2015 - 2018, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2015 - 2021, Intel Corporation. All rights reserved.<BR>
Copyright (c), Microsoft Corporation.
SPDX-License-Identifier: BSD-2-Clause-Patent

@@ -320,7 +320,7 @@ PtpCrbTpmCommand (
// Command completed, but buffer is not enough
//
Status = EFI_BUFFER_TOO_SMALL;
- goto GoReady_Exit;
+ goto GoIdle_Exit;
}

*SizeOut = TpmOutSize;
@@ -340,16 +340,6 @@ PtpCrbTpmCommand (
DEBUG ((DEBUG_VERBOSE, "\n"));
DEBUG_CODE_END ();

-GoReady_Exit:
- //
- // Goto Ready State if command is completed successfully and TPM support
IdleBypass
- // If not supported. flow down to GoIdle
- //
- if (GetCachedIdleByPass () == 1) {
- MmioWrite32 ((UINTN)&CrbReg->CrbControlRequest,
PTP_CRB_CONTROL_AREA_REQUEST_COMMAND_READY);
- return Status;
- }
-
//
// Do not wait for state transition for TIMEOUT_C
// This function will try to wait 2 TIMEOUT_C at the beginning in next call.
--
2.26.2.windows.1

-----Original Message-----
From: Gonzalez Del Cueto, Rodrigo <rodrigo.gonzalez.del.cueto@...>
Sent: Friday, December 17, 2021 10:48 AM
To: devel@edk2.groups.io
Cc: Gonzalez Del Cueto, Rodrigo <rodrigo.gonzalez.del.cueto@...>;
Wang, Jian J <jian.j.wang@...>; Yao, Jiewen <jiewen.yao@...>
Subject: [PATCH] SecurityPkg: Reallocate TPM Active PCRs based on platform
support

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3515

In V4: Fixed patch format and uncrustify cleanup

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@...>
Cc: Jian J Wang <jian.j.wang@...>
Cc: Jiewen Yao <jiewen.yao@...>
---
SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterDxe.c
| 11 ++++++++---
SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c |
11 ++++++++---
SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c | 43
+++++++++++++++++++++++++++++++------------
SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf | 1 +
4 files changed, 48 insertions(+), 18 deletions(-)

diff --git
a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterDxe.
c
b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterDxe.
c
index 59639d0538..ee8fe6e06e 100644
---
a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterDxe.
c
+++
b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterDxe.
c
@@ -3,7 +3,7 @@
hash handler registered, such as SHA1, SHA256.
Platform can use PcdTpm2HashMask to mask some hash engines.

-Copyright (c) 2013 - 2018, Intel Corporation. All rights reserved. <BR>
+Copyright (c) 2013 - 2021, Intel Corporation. All rights reserved. <BR>
SPDX-License-Identifier: BSD-2-Clause-Patent

**/
@@ -234,13 +234,18 @@ RegisterHashInterfaceLib (
{
UINTN Index;
UINT32 HashMask;
+ UINT32 Tpm2HashMask;
EFI_STATUS Status;

//
// Check allow
//
- HashMask = Tpm2GetHashMaskFromAlgo (&HashInterface->HashGuid);
- if ((HashMask & PcdGet32 (PcdTpm2HashMask)) == 0) {
+ HashMask = Tpm2GetHashMaskFromAlgo (&HashInterface->HashGuid);
+ 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 e21103d371..eeb424b6c3 100644
---
a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c
+++
b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c
@@ -3,7 +3,7 @@
hash handler registered, such as SHA1, SHA256.
Platform can use PcdTpm2HashMask to mask some hash engines.

-Copyright (c) 2013 - 2018, Intel Corporation. All rights reserved. <BR>
+Copyright (c) 2013 - 2021, Intel Corporation. All rights reserved. <BR>
SPDX-License-Identifier: BSD-2-Clause-Patent

**/
@@ -330,13 +330,18 @@ 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) {
+ HashMask = Tpm2GetHashMaskFromAlgo (&HashInterface->HashGuid);
+ 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 a97a4e7f2d..0da89b795e 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

@@ -252,7 +252,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 (
@@ -261,6 +261,7 @@ SyncPcrAllocationsAndPcrMask (
{
EFI_STATUS Status;
EFI_TCG2_EVENT_ALGORITHM_BITMAP TpmHashAlgorithmBitmap;
+ EFI_TCG2_EVENT_ALGORITHM_BITMAP BiosHashAlgorithmBitmap;
UINT32 TpmActivePcrBanks;
UINT32 NewTpmActivePcrBanks;
UINT32 Tpm2PcrMask;
@@ -274,33 +275,50 @@ SyncPcrAllocationsAndPcrMask (
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;
}

- //
- // 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.
- //
+ DEBUG ((DEBUG_INFO, "Tpm2PcrMask 0x%08x\n", Tpm2PcrMask));

//
- // 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;
+ // * 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 ((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)) {
//
@@ -331,6 +349,7 @@ SyncPcrAllocationsAndPcrMask (
}

Status = PcdSet32S (PcdTpm2HashMask, NewTpm2PcrMask);
+ DEBUG ((DEBUG_ERROR, "Set 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.26.2.windows.1

-----Original Message-----
From: Gonzalez Del Cueto, Rodrigo <rodrigo.gonzalez.del.cueto@...>
Sent: Friday, December 17, 2021 10:47 AM
To: devel@edk2.groups.io
Cc: Gonzalez Del Cueto, Rodrigo <rodrigo.gonzalez.del.cueto@...>; Yao,
Jiewen <jiewen.yao@...>; Wang, Jian J <jian.j.wang@...>
Subject: [PATCH] SecurityPkg: Debug code to audit BIOS TPM extend operations

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2858

In V2: Fixed patch format and uncrustify cleanup

In V1: Add debug functionality to examine TPM extend operations
performed by BIOS and inspect the PCR 00 value prior to
any BIOS measurements.

Signed-off-by: Rodrigo Gonzalez del Cueto
<rodrigo.gonzalez.del.cueto@...>
Cc: Jiewen Yao <jiewen.yao@...>
Cc: Jian J Wang <jian.j.wang@...>
---
SecurityPkg/Include/Library/Tpm2CommandLib.h | 33
+++++++++++++++++++++++++--------
SecurityPkg/Library/Tpm2CommandLib/Tpm2Integrity.c | 190
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c | 9 ++++++++-
3 files changed, 222 insertions(+), 10 deletions(-)

diff --git a/SecurityPkg/Include/Library/Tpm2CommandLib.h
b/SecurityPkg/Include/Library/Tpm2CommandLib.h
index 2e83a2f474..a2fb97f18d 100644
--- a/SecurityPkg/Include/Library/Tpm2CommandLib.h
+++ b/SecurityPkg/Include/Library/Tpm2CommandLib.h
@@ -1,7 +1,7 @@
/** @file
This library is used by other modules to send TPM2 command.

-Copyright (c) 2013 - 2018, Intel Corporation. All rights reserved. <BR>
+Copyright (c) 2013 - 2021, Intel Corporation. All rights reserved. <BR>
SPDX-License-Identifier: BSD-2-Clause-Patent

**/
@@ -503,9 +503,9 @@ Tpm2PcrExtend (
EFI_STATUS
EFIAPI
Tpm2PcrEvent (
- IN TPMI_DH_PCR PcrHandle,
- IN TPM2B_EVENT *EventData,
- OUT TPML_DIGEST_VALUES *Digests
+ IN TPMI_DH_PCR PcrHandle,
+ IN TPM2B_EVENT *EventData,
+ OUT TPML_DIGEST_VALUES *Digests
);

/**
@@ -522,10 +522,10 @@ Tpm2PcrEvent (
EFI_STATUS
EFIAPI
Tpm2PcrRead (
- IN TPML_PCR_SELECTION *PcrSelectionIn,
- OUT UINT32 *PcrUpdateCounter,
- OUT TPML_PCR_SELECTION *PcrSelectionOut,
- OUT TPML_DIGEST *PcrValues
+ IN TPML_PCR_SELECTION *PcrSelectionIn,
+ OUT UINT32 *PcrUpdateCounter,
+ OUT TPML_PCR_SELECTION *PcrSelectionOut,
+ OUT TPML_DIGEST *PcrValues
);

/**
@@ -1113,4 +1113,21 @@ GetDigestFromDigestList (
OUT VOID *Digest
);

+/**
+ This function will query the TPM to determine which hashing algorithms and
+ get the digests of all active and supported PCR banks of a specific PCR
register.
+
+ @param[in] PcrHandle The index of the PCR register to be read.
+ @param[out] HashList List of digests from PCR register being read.
+
+ @retval EFI_SUCCESS The Pcr was read successfully.
+ @retval EFI_DEVICE_ERROR The command was unsuccessful.
+**/
+EFI_STATUS
+EFIAPI
+Tpm2PcrReadForActiveBank (
+ IN TPMI_DH_PCR PcrHandle,
+ OUT TPML_DIGEST *HashList
+ );
+
#endif
diff --git a/SecurityPkg/Library/Tpm2CommandLib/Tpm2Integrity.c
b/SecurityPkg/Library/Tpm2CommandLib/Tpm2Integrity.c
index 8dde5f34a2..94e93b2642 100644
--- a/SecurityPkg/Library/Tpm2CommandLib/Tpm2Integrity.c
+++ b/SecurityPkg/Library/Tpm2CommandLib/Tpm2Integrity.c
@@ -1,7 +1,7 @@
/** @file
Implement TPM2 Integrity related command.

-Copyright (c) 2013 - 2018, Intel Corporation. All rights reserved. <BR>
+Copyright (c) 2013 - 2021, Intel Corporation. All rights reserved. <BR>
SPDX-License-Identifier: BSD-2-Clause-Patent

**/
@@ -138,6 +138,23 @@ Tpm2PcrExtend (
&Digests->digests[Index].digest,
DigestSize
);
+
+ DEBUG_CODE_BEGIN ();
+ UINTN Index2;
+ DEBUG ((
+ DEBUG_VERBOSE,
+ "Tpm2PcrExtend - Hash = 0x%04x, Pcr[%02d], digest = ",
+ Digests->digests[Index].hashAlg,
+ (UINT8)PcrHandle
+ ));
+
+ for (Index2 = 0; Index2 < DigestSize; Index2++) {
+ DEBUG ((DEBUG_VERBOSE, "%02x ", Buffer[Index2]));
+ }
+
+ DEBUG ((DEBUG_VERBOSE, "\n"));
+ DEBUG_CODE_END ();
+
Buffer += DigestSize;
}

@@ -172,6 +189,11 @@ Tpm2PcrExtend (
return EFI_DEVICE_ERROR;
}

+ DEBUG_CODE_BEGIN ();
+ DEBUG ((DEBUG_VERBOSE, "Tpm2PcrExtend: PCR read after extend...\n"));
+ Tpm2PcrReadForActiveBank (PcrHandle, NULL);
+ DEBUG_CODE_END ();
+
//
// Unmarshal the response
//
@@ -705,3 +727,169 @@ Done:
ZeroMem (&LocalAuthSession.hmac, sizeof (LocalAuthSession.hmac));
return Status;
}
+
+/**
+ This function will query the TPM to determine which hashing algorithms and
+ get the digests of all active and supported PCR banks of a specific PCR
register.
+
+ @param[in] PcrHandle The index of the PCR register to be read.
+ @param[out] HashList List of digests from PCR register being read.
+
+ @retval EFI_SUCCESS The Pcr was read successfully.
+ @retval EFI_DEVICE_ERROR The command was unsuccessful.
+**/
+EFI_STATUS
+EFIAPI
+Tpm2PcrReadForActiveBank (
+ IN TPMI_DH_PCR PcrHandle,
+ OUT TPML_DIGEST *HashList
+ )
+{
+ EFI_STATUS Status;
+ TPML_PCR_SELECTION Pcrs;
+ TPML_PCR_SELECTION PcrSelectionIn;
+ TPML_PCR_SELECTION PcrSelectionOut;
+ TPML_DIGEST PcrValues;
+ UINT32 PcrUpdateCounter;
+ UINT8 PcrIndex;
+ UINT32 TpmHashAlgorithmBitmap;
+ TPMI_ALG_HASH CurrentPcrBankHash;
+ UINT32 ActivePcrBanks;
+ UINT32 TcgRegistryHashAlg;
+ UINTN Index;
+ UINTN Index2;
+
+ PcrIndex = (UINT8)PcrHandle;
+
+ if ((PcrIndex < 0) ||
+ (PcrIndex >= IMPLEMENTATION_PCR))
+ {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ ZeroMem (&PcrSelectionIn, sizeof (PcrSelectionIn));
+ ZeroMem (&PcrUpdateCounter, sizeof (UINT32));
+ ZeroMem (&PcrSelectionOut, sizeof (PcrSelectionOut));
+ ZeroMem (&PcrValues, sizeof (PcrValues));
+ ZeroMem (&Pcrs, sizeof (TPML_PCR_SELECTION));
+
+ DEBUG ((DEBUG_INFO, "ReadPcr - %02d\n", PcrIndex));
+
+ //
+ // Read TPM capabilities
+ //
+ Status = Tpm2GetCapabilityPcrs (&Pcrs);
+
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "ReadPcr: Unable to read TPM capabilities\n"));
+ return EFI_DEVICE_ERROR;
+ }
+
+ //
+ // Get Active Pcrs
+ //
+ Status = Tpm2GetCapabilitySupportedAndActivePcrs (
+ &TpmHashAlgorithmBitmap,
+ &ActivePcrBanks
+ );
+
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "ReadPcr: Unable to read TPM capabilities and
active PCRs\n"));
+ return EFI_DEVICE_ERROR;
+ }
+
+ //
+ // Select from Active PCRs
+ //
+ for (Index = 0; Index < Pcrs.count; Index++) {
+ CurrentPcrBankHash = Pcrs.pcrSelections[Index].hash;
+
+ switch (CurrentPcrBankHash) {
+ case TPM_ALG_SHA1:
+ DEBUG ((DEBUG_VERBOSE, "HASH_ALG_SHA1 Present\n"));
+ TcgRegistryHashAlg = HASH_ALG_SHA1;
+ break;
+ case TPM_ALG_SHA256:
+ DEBUG ((DEBUG_VERBOSE, "HASH_ALG_SHA256 Present\n"));
+ TcgRegistryHashAlg = HASH_ALG_SHA256;
+ break;
+ case TPM_ALG_SHA384:
+ DEBUG ((DEBUG_VERBOSE, "HASH_ALG_SHA384 Present\n"));
+ TcgRegistryHashAlg = HASH_ALG_SHA384;
+ break;
+ case TPM_ALG_SHA512:
+ DEBUG ((DEBUG_VERBOSE, "HASH_ALG_SHA512 Present\n"));
+ TcgRegistryHashAlg = HASH_ALG_SHA512;
+ break;
+ case TPM_ALG_SM3_256:
+ DEBUG ((DEBUG_VERBOSE, "HASH_ALG_SM3 Present\n"));
+ TcgRegistryHashAlg = HASH_ALG_SM3_256;
+ break;
+ default:
+ //
+ // Unsupported algorithm
+ //
+ DEBUG ((DEBUG_VERBOSE, "Unknown algorithm present\n"));
+ TcgRegistryHashAlg = 0;
+ break;
+ }
+
+ //
+ // Skip unsupported and inactive PCR banks
+ //
+ if ((TcgRegistryHashAlg & ActivePcrBanks) == 0) {
+ DEBUG ((DEBUG_VERBOSE, "Skipping unsupported or inactive bank:
0x%04x\n", CurrentPcrBankHash));
+ continue;
+ }
+
+ //
+ // Select PCR from current active bank
+ //
+ PcrSelectionIn.pcrSelections[PcrSelectionIn.count].hash =
Pcrs.pcrSelections[Index].hash;
+ PcrSelectionIn.pcrSelections[PcrSelectionIn.count].sizeofSelect =
PCR_SELECT_MAX;
+ PcrSelectionIn.pcrSelections[PcrSelectionIn.count].pcrSelect[0] = (PcrIndex <
8) ? 1 << PcrIndex : 0;
+ PcrSelectionIn.pcrSelections[PcrSelectionIn.count].pcrSelect[1] = (PcrIndex >
7) && (PcrIndex < 16) ? 1 << (PcrIndex - 8) : 0;
+ PcrSelectionIn.pcrSelections[PcrSelectionIn.count].pcrSelect[2] = (PcrIndex >
15) ? 1 << (PcrIndex - 16) : 0;
+ PcrSelectionIn.count++;
+ }
+
+ //
+ // Read PCRs
+ //
+ Status = Tpm2PcrRead (
+ &PcrSelectionIn,
+ &PcrUpdateCounter,
+ &PcrSelectionOut,
+ &PcrValues
+ );
+
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "Tpm2PcrRead failed Status = %r \n", Status));
+ return EFI_DEVICE_ERROR;
+ }
+
+ for (Index = 0; Index < PcrValues.count; Index++) {
+ DEBUG ((
+ DEBUG_INFO,
+ "ReadPcr - HashAlg = 0x%04x, Pcr[%02d], digest = ",
+ PcrSelectionOut.pcrSelections[Index].hash,
+ PcrIndex
+ ));
+
+ for (Index2 = 0; Index2 < PcrValues.digests[Index].size; Index2++) {
+ DEBUG ((DEBUG_INFO, "%02x ", PcrValues.digests[Index].buffer[Index2]));
+ }
+
+ DEBUG ((DEBUG_INFO, "\n"));
+ }
+
+ if (HashList != NULL) {
+ CopyMem (
+ HashList,
+ &PcrValues,
+ sizeof (TPML_DIGEST)
+ );
+ }
+
+ return EFI_SUCCESS;
+}
diff --git a/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c
b/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c
index a97a4e7f2d..622989aff3 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

@@ -1106,6 +1106,13 @@ PeimEntryMA (
}
}

+ DEBUG_CODE_BEGIN ();
+ //
+ // Peek into TPM PCR 00 before any BIOS measurement.
+ //
+ Tpm2PcrReadForActiveBank (00, NULL);
+ DEBUG_CODE_END ();
+
//
// Only install TpmInitializedPpi on success
//
--
2.26.2.windows.1