BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3625
DxeTpm2MeasureBootLib supports TPM2 based measure boot. After
Td protocol is introduced, TD based measure boot needs to be supported
in DxeTpm2MeasureBootLib as well.
There are 2 major changes in this commit.
1. MEASURE_BOOT_PROTOCOLS is defined to store the instances of TCG2
protocol and TD protocol. In the DxeTpm2MeasureBootHandler above 2
measure boot protocol instances will be located. Then the located
protocol instances will be called to do the measure boot.
2. TdEvent is similar to Tcg2Event except the MrIndex and PcrIndex.
CreateTdEventFromTcg2Event is used to create the TdEvent based on the
Tcg2Event.
Above 2 changes make the minimize changes to the existing code.
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
---
.../DxeTpm2MeasureBootLib.c | 346 ++++++++++++++----
.../DxeTpm2MeasureBootLib.inf | 1 +
2 files changed, 279 insertions(+), 68 deletions(-)
diff --git a/SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.c b/SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.c
index 92eac715800f..f523a1a7a9d6 100644
--- a/SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.c
+++ b/SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.c
@@ -41,6 +41,12 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#include <Library/PeCoffLib.h>
#include <Library/SecurityManagementLib.h>
#include <Library/HobLib.h>
+#include <Protocol/TdProtocol.h>
+
+typedef struct {
+ EFI_TCG2_PROTOCOL *Tcg2Protocol;
+ EFI_TD_PROTOCOL *TdProtocol;
+} MEASURE_BOOT_PROTOCOLS;
//
// Flag to check GPT partition. It only need be measured once.
@@ -55,6 +61,56 @@ UINTN mTcg2ImageSize;
EFI_HANDLE mTcg2CacheMeasuredHandle = NULL;
MEASURED_HOB_DATA *mTcg2MeasuredHobData = NULL;
+/**
+ Create TdEvent from Tcg2Event.
+
+ TdEvent is similar to Tcg2Event except the MrIndex.
+
+ @param TdProtocol Pointer to the located Td protocol instance.
+ @param Tcg2Event Pointer to the Tcg2Event.
+ @param EventSize Size of the Event.
+
+ @retval Pointer to the created TdEvent.
+**/
+EFI_TD_EVENT *
+CreateTdEventFromTcg2Event (
+ IN EFI_TD_PROTOCOL *TdProtocol,
+ IN EFI_TCG2_EVENT *Tcg2Event,
+ IN UINT32 EventSize
+ )
+{
+ EFI_TD_EVENT *TdEvent;
+ UINT32 MrIndex;
+ EFI_STATUS Status;
+
+ TdEvent = NULL;
+ if (Tcg2Event == NULL || TdProtocol == NULL) {
+ ASSERT (FALSE);
+ return NULL;
+ }
+
+ Status = TdProtocol->MapPcrToMrIndex (TdProtocol, Tcg2Event->Header.PCRIndex, &MrIndex);
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "Cannot map PcrIndex(%d) to MrIndex\n", Tcg2Event->Header.PCRIndex));
+ return NULL;
+ }
+
+ TdEvent = (EFI_TD_EVENT *)AllocateZeroPool (Tcg2Event->Size);
+ if (TdEvent == NULL) {
+ ASSERT (FALSE);
+ return NULL;
+ }
+
+ TdEvent->Size = Tcg2Event->Size;
+ TdEvent->Header.HeaderSize = Tcg2Event->Header.HeaderSize;
+ TdEvent->Header.HeaderVersion = Tcg2Event->Header.HeaderVersion;
+ TdEvent->Header.MrIndex = MrIndex;
+ TdEvent->Header.EventType = Tcg2Event->Header.EventType;
+ CopyMem (TdEvent->Event, Tcg2Event->Event, EventSize);
+
+ return TdEvent;
+}
+
/**
Reads contents of a PE/COFF image in memory buffer.
@@ -109,7 +165,7 @@ DxeTpm2MeasureBootLibImageRead (
Caution: This function may receive untrusted input.
The GPT partition table is external input, so this function should parse partition data carefully.
- @param Tcg2Protocol Pointer to the located TCG2 protocol instance.
+ @param MeasureBootProtocols Pointer to the located MeasureBoot protocol instances (i.e. TCG2/Td protocol).
@param GptHandle Handle that GPT partition was installed.
@retval EFI_SUCCESS Successfully measure GPT table.
@@ -121,8 +177,8 @@ DxeTpm2MeasureBootLibImageRead (
EFI_STATUS
EFIAPI
Tcg2MeasureGptTable (
- IN EFI_TCG2_PROTOCOL *Tcg2Protocol,
- IN EFI_HANDLE GptHandle
+ IN MEASURE_BOOT_PROTOCOLS *MeasureBootProtocols,
+ IN EFI_HANDLE GptHandle
)
{
EFI_STATUS Status;
@@ -134,13 +190,24 @@ Tcg2MeasureGptTable (
UINTN NumberOfPartition;
UINT32 Index;
EFI_TCG2_EVENT *Tcg2Event;
+ EFI_TD_EVENT *TdEvent;
EFI_GPT_DATA *GptData;
UINT32 EventSize;
+ EFI_TCG2_PROTOCOL *Tcg2Protocol;
+ EFI_TD_PROTOCOL *TdProtocol;
if (mTcg2MeasureGptCount > 0) {
return EFI_SUCCESS;
}
+ Tcg2Protocol = MeasureBootProtocols->Tcg2Protocol;
+ TdProtocol = MeasureBootProtocols->TdProtocol;
+
+ if (Tcg2Protocol == NULL && TdProtocol == NULL) {
+ ASSERT (FALSE);
+ return EFI_UNSUPPORTED;
+ }
+
Status = gBS->HandleProtocol (GptHandle, &gEfiBlockIoProtocolGuid, (VOID**)&BlockIo);
if (EFI_ERROR (Status)) {
return EFI_UNSUPPORTED;
@@ -149,6 +216,7 @@ Tcg2MeasureGptTable (
if (EFI_ERROR (Status)) {
return EFI_UNSUPPORTED;
}
+
//
// Read the EFI Partition Table Header
//
@@ -156,6 +224,15 @@ Tcg2MeasureGptTable (
if (PrimaryHeader == NULL) {
return EFI_OUT_OF_RESOURCES;
}
+
+ //
+ // PrimaryHeader->SizeOfPartitionEntry should not be zero
+ //
+ if (PrimaryHeader->SizeOfPartitionEntry == 0) {
+ DEBUG ((DEBUG_ERROR, "SizeOfPartitionEntry should not be zero!\n"));
+ return EFI_BAD_BUFFER_SIZE;
+ }
+
Status = DiskIo->ReadDisk (
DiskIo,
BlockIo->Media->MediaId,
@@ -164,7 +241,7 @@ Tcg2MeasureGptTable (
(UINT8 *)PrimaryHeader
);
if (EFI_ERROR (Status)) {
- DEBUG ((EFI_D_ERROR, "Failed to Read Partition Table Header!\n"));
+ DEBUG ((DEBUG_ERROR, "Failed to Read Partition Table Header!\n"));
FreePool (PrimaryHeader);
return EFI_DEVICE_ERROR;
}
@@ -201,16 +278,18 @@ Tcg2MeasureGptTable (
PartitionEntry = (EFI_PARTITION_ENTRY *)((UINT8 *)PartitionEntry + PrimaryHeader->SizeOfPartitionEntry);
}
+ TdEvent = NULL;
+ Tcg2Event = NULL;
+
//
- // Prepare Data for Measurement
+ // Prepare Data for Measurement (TdProtocol and Tcg2Protocol)
//
EventSize = (UINT32)(sizeof (EFI_GPT_DATA) - sizeof (GptData->Partitions)
+ NumberOfPartition * PrimaryHeader->SizeOfPartitionEntry);
Tcg2Event = (EFI_TCG2_EVENT *) AllocateZeroPool (EventSize + sizeof (EFI_TCG2_EVENT) - sizeof(Tcg2Event->Event));
if (Tcg2Event == NULL) {
- FreePool (PrimaryHeader);
- FreePool (EntryPtr);
- return EFI_OUT_OF_RESOURCES;
+ Status = EFI_OUT_OF_RESOURCES;
+ goto Exit;
}
Tcg2Event->Size = EventSize + sizeof (EFI_TCG2_EVENT) - sizeof(Tcg2Event->Event);
@@ -242,23 +321,56 @@ Tcg2MeasureGptTable (
PartitionEntry =(EFI_PARTITION_ENTRY *)((UINT8 *)PartitionEntry + PrimaryHeader->SizeOfPartitionEntry);
}
+ if (TdProtocol != NULL) {
+ TdEvent = CreateTdEventFromTcg2Event (TdProtocol, Tcg2Event, EventSize);
+ if (TdEvent == NULL) {
+ goto Exit;
+ }
+ }
+
+ //
+ // Measure the GPT data by Tcg2Protocol
+ //
+ if (Tcg2Protocol != NULL) {
+ Status = Tcg2Protocol->HashLogExtendEvent (
+ Tcg2Protocol,
+ 0,
+ (EFI_PHYSICAL_ADDRESS) (UINTN) (VOID *) GptData,
+ (UINT64) EventSize,
+ Tcg2Event
+ );
+ if (!EFI_ERROR (Status)) {
+ mTcg2MeasureGptCount++;
+ }
+ DEBUG ((DEBUG_INFO, "DxeTpm2MeasureBootHandler - Tcg2 MeasureGptTable - %r\n", Status));
+ }
+
+ //
+ // Measure the GPT data by TdProtocol
//
- // Measure the GPT data
- //
- Status = Tcg2Protocol->HashLogExtendEvent (
- Tcg2Protocol,
- 0,
- (EFI_PHYSICAL_ADDRESS) (UINTN) (VOID *) GptData,
- (UINT64) EventSize,
- Tcg2Event
- );
- if (!EFI_ERROR (Status)) {
- mTcg2MeasureGptCount++;
+ if (TdProtocol != NULL) {
+ Status = TdProtocol->HashLogExtendEvent (
+ TdProtocol,
+ 0,
+ (EFI_PHYSICAL_ADDRESS) (UINTN) (VOID *) GptData,
+ (UINT64) EventSize,
+ TdEvent
+ );
+ if (!EFI_ERROR (Status)) {
+ mTcg2MeasureGptCount++;
+ }
+ DEBUG ((DEBUG_INFO, "DxeTpm2MeasureBootHandler - Td MeasureGptTable - %r\n", Status));
}
+Exit:
FreePool (PrimaryHeader);
FreePool (EntryPtr);
- FreePool (Tcg2Event);
+ if (Tcg2Event != NULL) {
+ FreePool (Tcg2Event);
+ }
+ if (TdEvent != NULL) {
+ FreePool (TdEvent);
+ }
return Status;
}
@@ -271,12 +383,12 @@ Tcg2MeasureGptTable (
PE/COFF image is external input, so this function will validate its data structure
within this image buffer before use.
- @param[in] Tcg2Protocol Pointer to the located TCG2 protocol instance.
- @param[in] ImageAddress Start address of image buffer.
- @param[in] ImageSize Image size
- @param[in] LinkTimeBase Address that the image is loaded into memory.
- @param[in] ImageType Image subsystem type.
- @param[in] FilePath File path is corresponding to the input image.
+ @param[in] MeasureBootProtocols Pointer to the located MeasureBoot protocol instances.
+ @param[in] ImageAddress Start address of image buffer.
+ @param[in] ImageSize Image size
+ @param[in] LinkTimeBase Address that the image is loaded into memory.
+ @param[in] ImageType Image subsystem type.
+ @param[in] FilePath File path is corresponding to the input image.
@retval EFI_SUCCESS Successfully measure image.
@retval EFI_OUT_OF_RESOURCES No enough resource to measure image.
@@ -287,7 +399,7 @@ Tcg2MeasureGptTable (
EFI_STATUS
EFIAPI
Tcg2MeasurePeImage (
- IN EFI_TCG2_PROTOCOL *Tcg2Protocol,
+ IN MEASURE_BOOT_PROTOCOLS *MeasureBootProtocols,
IN EFI_PHYSICAL_ADDRESS ImageAddress,
IN UINTN ImageSize,
IN UINTN LinkTimeBase,
@@ -300,9 +412,22 @@ Tcg2MeasurePeImage (
EFI_IMAGE_LOAD_EVENT *ImageLoad;
UINT32 FilePathSize;
UINT32 EventSize;
+ EFI_TD_EVENT *TdEvent;
+ EFI_TD_PROTOCOL *TdProtocol;
+ EFI_TCG2_PROTOCOL *Tcg2Protocol;
Status = EFI_UNSUPPORTED;
ImageLoad = NULL;
+ TdEvent = NULL;
+
+ Tcg2Protocol = MeasureBootProtocols->Tcg2Protocol;
+ TdProtocol = MeasureBootProtocols->TdProtocol;
+
+ if (Tcg2Protocol == NULL && TdProtocol == NULL) {
+ ASSERT (FALSE);
+ return EFI_UNSUPPORTED;
+ }
+
FilePathSize = (UINT32) GetDevicePathSize (FilePath);
//
@@ -334,7 +459,7 @@ Tcg2MeasurePeImage (
break;
default:
DEBUG ((
- EFI_D_ERROR,
+ DEBUG_ERROR,
"Tcg2MeasurePeImage: Unknown subsystem type %d",
ImageType
));
@@ -352,28 +477,124 @@ Tcg2MeasurePeImage (
//
// Log the PE data
//
- Status = Tcg2Protocol->HashLogExtendEvent (
- Tcg2Protocol,
- PE_COFF_IMAGE,
- ImageAddress,
- ImageSize,
- Tcg2Event
- );
- if (Status == EFI_VOLUME_FULL) {
- //
- // Volume full here means the image is hashed and its result is extended to PCR.
- // But the event log can't be saved since log area is full.
- // Just return EFI_SUCCESS in order not to block the image load.
- //
- Status = EFI_SUCCESS;
+ if (Tcg2Protocol != NULL) {
+ Status = Tcg2Protocol->HashLogExtendEvent (
+ Tcg2Protocol,
+ PE_COFF_IMAGE,
+ ImageAddress,
+ ImageSize,
+ Tcg2Event
+ );
+ if (Status == EFI_VOLUME_FULL) {
+ //
+ // Volume full here means the image is hashed and its result is extended to PCR.
+ // But the event log can't be saved since log area is full.
+ // Just return EFI_SUCCESS in order not to block the image load.
+ //
+ Status = EFI_SUCCESS;
+ }
+ DEBUG ((DEBUG_INFO, "DxeTpm2MeasureBootHandler - Tcg2 MeasurePeImage - %r\n", Status));
+ }
+
+ if (TdProtocol != NULL) {
+ TdEvent = CreateTdEventFromTcg2Event (TdProtocol, Tcg2Event, EventSize);
+ if (TdEvent == NULL) {
+ goto Finish;
+ }
+
+ Status = TdProtocol->HashLogExtendEvent (
+ TdProtocol,
+ PE_COFF_IMAGE,
+ ImageAddress,
+ ImageSize,
+ TdEvent
+ );
+ if (Status == EFI_VOLUME_FULL) {
+ //
+ // Volume full here means the image is hashed and its result is extended to PCR.
+ // But the event log can't be saved since log area is full.
+ // Just return EFI_SUCCESS in order not to block the image load.
+ //
+ Status = EFI_SUCCESS;
+ }
+ DEBUG ((DEBUG_INFO, "DxeTpm2MeasureBootHandler - Td MeasurePeImage - %r\n", Status));
}
Finish:
- FreePool (Tcg2Event);
+ if (Tcg2Event != NULL) {
+ FreePool (Tcg2Event);
+ }
+
+ if (TdEvent != NULL) {
+ FreePool (TdEvent);
+ }
return Status;
}
+/**
+ Get the measure boot protocols.
+
+ There are 2 measure boot, TCG2 protocol based and Td protocol based.
+
+ @param MeasureBootProtocols Pointer to the located measure boot protocol instances.
+
+ @retval EFI_SUCCESS Sucessfully locate the measure boot protocol instances (at least one instance).
+ @retval EFI_UNSUPPORTED Measure boot is not supported.
+**/
+EFI_STATUS
+EFIAPI
+GetMeasureBootProtocols (
+ MEASURE_BOOT_PROTOCOLS *MeasureBootProtocols
+ )
+{
+ EFI_STATUS Status;
+ EFI_TCG2_PROTOCOL *Tcg2Protocol;
+ EFI_TD_PROTOCOL *TdProtocol;
+ EFI_TCG2_BOOT_SERVICE_CAPABILITY Tcg2ProtocolCapability;
+ EFI_TD_BOOT_SERVICE_CAPABILITY TdProtocolCapability;
+
+ TdProtocol = NULL;
+ Status = gBS->LocateProtocol (&gEfiTdProtocolGuid, NULL, (VOID **) &TdProtocol);
+ if (EFI_ERROR (Status)) {
+ //
+ // TdTcg2 protocol is not installed.
+ //
+ DEBUG ((DEBUG_VERBOSE, "TdProtocol is not installed. - %r\n", Status));
+ } else {
+ TdProtocolCapability.Size = sizeof (TdProtocolCapability);
+ Status = TdProtocol->GetCapability (TdProtocol, &TdProtocolCapability);
+ if (EFI_ERROR (Status) || !TdProtocolCapability.TdPresentFlag) {
+ DEBUG ((DEBUG_ERROR, "TdPresentFlag=FALSE. %r\n", Status));
+ TdProtocol = NULL;
+ }
+ }
+
+ Tcg2Protocol = NULL;
+ Status = gBS->LocateProtocol (&gEfiTcg2ProtocolGuid, NULL, (VOID **) &Tcg2Protocol);
+ if (EFI_ERROR (Status)) {
+ //
+ // Tcg2 protocol is not installed. So, TPM2 is not present.
+ //
+ DEBUG ((DEBUG_VERBOSE, "Tcg2Protocol is not installed. - %r\n", Status));
+ } else {
+ Tcg2ProtocolCapability.Size = (UINT8) sizeof (Tcg2ProtocolCapability);
+ Status = Tcg2Protocol->GetCapability (Tcg2Protocol, &Tcg2ProtocolCapability);
+ if (EFI_ERROR (Status) || (!Tcg2ProtocolCapability.TPMPresentFlag)) {
+ //
+ // TPM device doesn't work or activate.
+ //
+ DEBUG ((DEBUG_ERROR, "TPMPresentFlag=FALSE %r\n", Status));
+ Tcg2Protocol = NULL;
+ }
+ }
+
+ MeasureBootProtocols->Tcg2Protocol = Tcg2Protocol;
+ MeasureBootProtocols->TdProtocol = TdProtocol;
+
+ return (Tcg2Protocol == NULL && TdProtocol == NULL) ? EFI_UNSUPPORTED: EFI_SUCCESS;
+}
+
/**
The security handler is used to abstract platform-specific policy
from the DXE core response to an attempt to use a file that returns a
@@ -422,9 +643,8 @@ DxeTpm2MeasureBootHandler (
IN BOOLEAN BootPolicy
)
{
- EFI_TCG2_PROTOCOL *Tcg2Protocol;
+ MEASURE_BOOT_PROTOCOLS MeasureBootProtocols;
EFI_STATUS Status;
- EFI_TCG2_BOOT_SERVICE_CAPABILITY ProtocolCapability;
EFI_DEVICE_PATH_PROTOCOL *DevicePathNode;
EFI_DEVICE_PATH_PROTOCOL *OrigDevicePathNode;
EFI_HANDLE Handle;
@@ -435,28 +655,19 @@ DxeTpm2MeasureBootHandler (
EFI_PHYSICAL_ADDRESS FvAddress;
UINT32 Index;
- Status = gBS->LocateProtocol (&gEfiTcg2ProtocolGuid, NULL, (VOID **) &Tcg2Protocol);
+ MeasureBootProtocols.Tcg2Protocol = NULL;
+ MeasureBootProtocols.TdProtocol = NULL;
+
+ Status = GetMeasureBootProtocols(&MeasureBootProtocols);
+
if (EFI_ERROR (Status)) {
- //
- // Tcg2 protocol is not installed. So, TPM2 is not present.
- // Don't do any measurement, and directly return EFI_SUCCESS.
- //
- DEBUG ((EFI_D_VERBOSE, "DxeTpm2MeasureBootHandler - Tcg2 - %r\n", Status));
+ DEBUG ((DEBUG_INFO, "None of Tcg2Protocol/TdProtocol is installed.\n"));
return EFI_SUCCESS;
}
- ProtocolCapability.Size = (UINT8) sizeof (ProtocolCapability);
- Status = Tcg2Protocol->GetCapability (
- Tcg2Protocol,
- &ProtocolCapability
- );
- if (EFI_ERROR (Status) || (!ProtocolCapability.TPMPresentFlag)) {
- //
- // TPM device doesn't work or activate.
- //
- DEBUG ((EFI_D_ERROR, "DxeTpm2MeasureBootHandler (%r) - TPMPresentFlag - %x\n", Status, ProtocolCapability.TPMPresentFlag));
- return EFI_SUCCESS;
- }
+ DEBUG ((DEBUG_INFO, "Tcg2Protocol = %p, TdProtocol = %p\n",
+ MeasureBootProtocols.Tcg2Protocol,
+ MeasureBootProtocols.TdProtocol));
//
// Copy File Device Path
@@ -502,8 +713,8 @@ DxeTpm2MeasureBootHandler (
//
// Measure GPT disk.
//
- Status = Tcg2MeasureGptTable (Tcg2Protocol, Handle);
- DEBUG ((EFI_D_INFO, "DxeTpm2MeasureBootHandler - Tcg2MeasureGptTable - %r\n", Status));
+ Status = Tcg2MeasureGptTable (&MeasureBootProtocols, Handle);
+
if (!EFI_ERROR (Status)) {
//
// GPT disk check done.
@@ -647,14 +858,13 @@ DxeTpm2MeasureBootHandler (
// Measure PE image into TPM log.
//
Status = Tcg2MeasurePeImage (
- Tcg2Protocol,
+ &MeasureBootProtocols,
(EFI_PHYSICAL_ADDRESS) (UINTN) FileBuffer,
FileSize,
(UINTN) ImageContext.ImageAddress,
ImageContext.ImageType,
DevicePathNode
);
- DEBUG ((EFI_D_INFO, "DxeTpm2MeasureBootHandler - Tcg2MeasurePeImage - %r\n", Status));
}
//
@@ -665,7 +875,7 @@ Finish:
FreePool (OrigDevicePathNode);
}
- DEBUG ((EFI_D_INFO, "DxeTpm2MeasureBootHandler - %r\n", Status));
+ DEBUG ((DEBUG_INFO, "DxeTpm2MeasureBootHandler - %r\n", Status));
return Status;
}
diff --git a/SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.inf b/SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.inf
index 2506abbe7c8b..29b62c3ba8fa 100644
--- a/SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.inf
+++ b/SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.inf
@@ -61,6 +61,7 @@
[Protocols]
gEfiTcg2ProtocolGuid ## SOMETIMES_CONSUMES
+ gEfiTdProtocolGuid
gEfiFirmwareVolumeBlockProtocolGuid ## SOMETIMES_CONSUMES
gEfiBlockIoProtocolGuid ## SOMETIMES_CONSUMES
gEfiDiskIoProtocolGuid ## SOMETIMES_CONSUMES
--
2.29.2.windows.2
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#81651): https://edk2.groups.io/g/devel/message/81651
Mute This Topic: https://groups.io/mt/86163959/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Hi Min, Jiewen,
Thank you for this patch.
I think this patch would need updating based on the changes done to
patch 1/3.
Other than that I have some general feedback marked inline as [SAMI].
Regards,
Sami Mujawar
On 08/10/2021 06:21 AM, Min Xu via groups.io wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3625
>
> DxeTpm2MeasureBootLib supports TPM2 based measure boot. After
> Td protocol is introduced, TD based measure boot needs to be supported
> in DxeTpm2MeasureBootLib as well.
>
> There are 2 major changes in this commit.
>
> 1. MEASURE_BOOT_PROTOCOLS is defined to store the instances of TCG2
> protocol and TD protocol. In the DxeTpm2MeasureBootHandler above 2
> measure boot protocol instances will be located. Then the located
> protocol instances will be called to do the measure boot.
>
> 2. TdEvent is similar to Tcg2Event except the MrIndex and PcrIndex.
> CreateTdEventFromTcg2Event is used to create the TdEvent based on the
> Tcg2Event.
>
> Above 2 changes make the minimize changes to the existing code.
>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Signed-off-by: Min Xu <min.m.xu@intel.com>
> ---
> .../DxeTpm2MeasureBootLib.c | 346 ++++++++++++++----
> .../DxeTpm2MeasureBootLib.inf | 1 +
> 2 files changed, 279 insertions(+), 68 deletions(-)
>
> diff --git a/SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.c b/SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.c
> index 92eac715800f..f523a1a7a9d6 100644
> --- a/SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.c
> +++ b/SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.c
> @@ -41,6 +41,12 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> #include <Library/PeCoffLib.h>
> #include <Library/SecurityManagementLib.h>
> #include <Library/HobLib.h>
> +#include <Protocol/TdProtocol.h>
> +
> +typedef struct {
> + EFI_TCG2_PROTOCOL *Tcg2Protocol;
> + EFI_TD_PROTOCOL *TdProtocol;
> +} MEASURE_BOOT_PROTOCOLS;
>
> //
> // Flag to check GPT partition. It only need be measured once.
> @@ -55,6 +61,56 @@ UINTN mTcg2ImageSize;
> EFI_HANDLE mTcg2CacheMeasuredHandle = NULL;
> MEASURED_HOB_DATA *mTcg2MeasuredHobData = NULL;
>
> +/**
> + Create TdEvent from Tcg2Event.
> +
> + TdEvent is similar to Tcg2Event except the MrIndex.
> +
> + @param TdProtocol Pointer to the located Td protocol instance.
> + @param Tcg2Event Pointer to the Tcg2Event.
> + @param EventSize Size of the Event.
> +
> + @retval Pointer to the created TdEvent.
> +**/
> +EFI_TD_EVENT *
> +CreateTdEventFromTcg2Event (
> + IN EFI_TD_PROTOCOL *TdProtocol,
> + IN EFI_TCG2_EVENT *Tcg2Event,
> + IN UINT32 EventSize
> + )
> +{
> + EFI_TD_EVENT *TdEvent;
> + UINT32 MrIndex;
> + EFI_STATUS Status;
> +
> + TdEvent = NULL;
> + if (Tcg2Event == NULL || TdProtocol == NULL) {
> + ASSERT (FALSE);
> + return NULL;
> + }
> +
> + Status = TdProtocol->MapPcrToMrIndex (TdProtocol, Tcg2Event->Header.PCRIndex, &MrIndex);
> + if (EFI_ERROR (Status)) {
> + DEBUG ((DEBUG_ERROR, "Cannot map PcrIndex(%d) to MrIndex\n", Tcg2Event->Header.PCRIndex));
> + return NULL;
> + }
> +
> + TdEvent = (EFI_TD_EVENT *)AllocateZeroPool (Tcg2Event->Size);
> + if (TdEvent == NULL) {
> + ASSERT (FALSE);
> + return NULL;
> + }
> +
> + TdEvent->Size = Tcg2Event->Size;
> + TdEvent->Header.HeaderSize = Tcg2Event->Header.HeaderSize;
> + TdEvent->Header.HeaderVersion = Tcg2Event->Header.HeaderVersion;
> + TdEvent->Header.MrIndex = MrIndex;
> + TdEvent->Header.EventType = Tcg2Event->Header.EventType;
> + CopyMem (TdEvent->Event, Tcg2Event->Event, EventSize);
> +
> + return TdEvent;
> +}
> +
> /**
> Reads contents of a PE/COFF image in memory buffer.
>
> @@ -109,7 +165,7 @@ DxeTpm2MeasureBootLibImageRead (
> Caution: This function may receive untrusted input.
> The GPT partition table is external input, so this function should parse partition data carefully.
>
> - @param Tcg2Protocol Pointer to the located TCG2 protocol instance.
> + @param MeasureBootProtocols Pointer to the located MeasureBoot protocol instances (i.e. TCG2/Td protocol).
> @param GptHandle Handle that GPT partition was installed.
>
> @retval EFI_SUCCESS Successfully measure GPT table.
> @@ -121,8 +177,8 @@ DxeTpm2MeasureBootLibImageRead (
> EFI_STATUS
> EFIAPI
> Tcg2MeasureGptTable (
> - IN EFI_TCG2_PROTOCOL *Tcg2Protocol,
> - IN EFI_HANDLE GptHandle
> + IN MEASURE_BOOT_PROTOCOLS *MeasureBootProtocols,
> + IN EFI_HANDLE GptHandle
> )
> {
> EFI_STATUS Status;
> @@ -134,13 +190,24 @@ Tcg2MeasureGptTable (
> UINTN NumberOfPartition;
> UINT32 Index;
> EFI_TCG2_EVENT *Tcg2Event;
> + EFI_TD_EVENT *TdEvent;
> EFI_GPT_DATA *GptData;
> UINT32 EventSize;
> + EFI_TCG2_PROTOCOL *Tcg2Protocol;
> + EFI_TD_PROTOCOL *TdProtocol;
>
> if (mTcg2MeasureGptCount > 0) {
> return EFI_SUCCESS;
> }
>
> + Tcg2Protocol = MeasureBootProtocols->Tcg2Protocol;
> + TdProtocol = MeasureBootProtocols->TdProtocol;
> +
> + if (Tcg2Protocol == NULL && TdProtocol == NULL) {
> + ASSERT (FALSE);
> + return EFI_UNSUPPORTED;
> + }
> +
> Status = gBS->HandleProtocol (GptHandle, &gEfiBlockIoProtocolGuid, (VOID**)&BlockIo);
> if (EFI_ERROR (Status)) {
> return EFI_UNSUPPORTED;
> @@ -149,6 +216,7 @@ Tcg2MeasureGptTable (
> if (EFI_ERROR (Status)) {
> return EFI_UNSUPPORTED;
> }
> +
> //
> // Read the EFI Partition Table Header
> //
> @@ -156,6 +224,15 @@ Tcg2MeasureGptTable (
> if (PrimaryHeader == NULL) {
> return EFI_OUT_OF_RESOURCES;
> }
> +
> + //
> + // PrimaryHeader->SizeOfPartitionEntry should not be zero
> + //
> + if (PrimaryHeader->SizeOfPartitionEntry == 0) {
> + DEBUG ((DEBUG_ERROR, "SizeOfPartitionEntry should not be zero!\n"));
> + return EFI_BAD_BUFFER_SIZE;
> + }
[SAMI] I think this check is at an incorrect location. Should this be
after the ReadDisk() below? Also, PrimaryHeader would need to be freed
in the error scenario above.
> +
> Status = DiskIo->ReadDisk (
> DiskIo,
> BlockIo->Media->MediaId,
> @@ -164,7 +241,7 @@ Tcg2MeasureGptTable (
> (UINT8 *)PrimaryHeader
> );
> if (EFI_ERROR (Status)) {
> - DEBUG ((EFI_D_ERROR, "Failed to Read Partition Table Header!\n"));
> + DEBUG ((DEBUG_ERROR, "Failed to Read Partition Table Header!\n"));
> FreePool (PrimaryHeader);
> return EFI_DEVICE_ERROR;
> }
> @@ -201,16 +278,18 @@ Tcg2MeasureGptTable (
> PartitionEntry = (EFI_PARTITION_ENTRY *)((UINT8 *)PartitionEntry + PrimaryHeader->SizeOfPartitionEntry);
> }
>
> + TdEvent = NULL;
> + Tcg2Event = NULL;
> +
> //
> - // Prepare Data for Measurement
> + // Prepare Data for Measurement (TdProtocol and Tcg2Protocol)
> //
> EventSize = (UINT32)(sizeof (EFI_GPT_DATA) - sizeof (GptData->Partitions)
> + NumberOfPartition * PrimaryHeader->SizeOfPartitionEntry);
> Tcg2Event = (EFI_TCG2_EVENT *) AllocateZeroPool (EventSize + sizeof (EFI_TCG2_EVENT) - sizeof(Tcg2Event->Event));
> if (Tcg2Event == NULL) {
> - FreePool (PrimaryHeader);
> - FreePool (EntryPtr);
> - return EFI_OUT_OF_RESOURCES;
> + Status = EFI_OUT_OF_RESOURCES;
> + goto Exit;
> }
>
> Tcg2Event->Size = EventSize + sizeof (EFI_TCG2_EVENT) - sizeof(Tcg2Event->Event);
> @@ -242,23 +321,56 @@ Tcg2MeasureGptTable (
> PartitionEntry =(EFI_PARTITION_ENTRY *)((UINT8 *)PartitionEntry + PrimaryHeader->SizeOfPartitionEntry);
> }
>
> + if (TdProtocol != NULL) {
> + TdEvent = CreateTdEventFromTcg2Event (TdProtocol, Tcg2Event, EventSize);
> + if (TdEvent == NULL) {
> + goto Exit;
[SAMI] I think Status should be set to reflect an appropriate error code
here. Also would it be possible to create this event just before calling
TdProtocol->HashLogExtendEvent at line 351?
I am trying to understand why is this done differently in
Tcg2MeasurePeImage() i.e. The TdEvent is created and extended in the
same if (TdProtocol != NULL) block.
[/SAMI]
> + }
> + }
> +
> + //
> + // Measure the GPT data by Tcg2Protocol
> + //
> + if (Tcg2Protocol != NULL) {
> + Status = Tcg2Protocol->HashLogExtendEvent (
> + Tcg2Protocol,
> + 0,
> + (EFI_PHYSICAL_ADDRESS) (UINTN) (VOID *) GptData,
> + (UINT64) EventSize,
> + Tcg2Event
> + );
> + if (!EFI_ERROR (Status)) {
> + mTcg2MeasureGptCount++;
> + }
> + DEBUG ((DEBUG_INFO, "DxeTpm2MeasureBootHandler - Tcg2 MeasureGptTable - %r\n", Status));
> + }
> +
> + //
> + // Measure the GPT data by TdProtocol
> //
> - // Measure the GPT data
> - //
> - Status = Tcg2Protocol->HashLogExtendEvent (
> - Tcg2Protocol,
> - 0,
> - (EFI_PHYSICAL_ADDRESS) (UINTN) (VOID *) GptData,
> - (UINT64) EventSize,
> - Tcg2Event
> - );
> - if (!EFI_ERROR (Status)) {
> - mTcg2MeasureGptCount++;
> + if (TdProtocol != NULL) {
> + Status = TdProtocol->HashLogExtendEvent (
> + TdProtocol,
> + 0,
> + (EFI_PHYSICAL_ADDRESS) (UINTN) (VOID *) GptData,
> + (UINT64) EventSize,
> + TdEvent
> + );
> + if (!EFI_ERROR (Status)) {
> + mTcg2MeasureGptCount++;
> + }
> + DEBUG ((DEBUG_INFO, "DxeTpm2MeasureBootHandler - Td MeasureGptTable - %r\n", Status));
> }
>
> +Exit:
> FreePool (PrimaryHeader);
> FreePool (EntryPtr);
> - FreePool (Tcg2Event);
> + if (Tcg2Event != NULL) {
> + FreePool (Tcg2Event);
> + }
> + if (TdEvent != NULL) {
> + FreePool (TdEvent);
> + }
>
> return Status;
> }
> @@ -271,12 +383,12 @@ Tcg2MeasureGptTable (
> PE/COFF image is external input, so this function will validate its data structure
> within this image buffer before use.
>
> - @param[in] Tcg2Protocol Pointer to the located TCG2 protocol instance.
> - @param[in] ImageAddress Start address of image buffer.
> - @param[in] ImageSize Image size
> - @param[in] LinkTimeBase Address that the image is loaded into memory.
> - @param[in] ImageType Image subsystem type.
> - @param[in] FilePath File path is corresponding to the input image.
> + @param[in] MeasureBootProtocols Pointer to the located MeasureBoot protocol instances.
> + @param[in] ImageAddress Start address of image buffer.
> + @param[in] ImageSize Image size
> + @param[in] LinkTimeBase Address that the image is loaded into memory.
> + @param[in] ImageType Image subsystem type.
> + @param[in] FilePath File path is corresponding to the input image.
>
> @retval EFI_SUCCESS Successfully measure image.
> @retval EFI_OUT_OF_RESOURCES No enough resource to measure image.
> @@ -287,7 +399,7 @@ Tcg2MeasureGptTable (
> EFI_STATUS
> EFIAPI
> Tcg2MeasurePeImage (
> - IN EFI_TCG2_PROTOCOL *Tcg2Protocol,
> + IN MEASURE_BOOT_PROTOCOLS *MeasureBootProtocols,
> IN EFI_PHYSICAL_ADDRESS ImageAddress,
> IN UINTN ImageSize,
> IN UINTN LinkTimeBase,
> @@ -300,9 +412,22 @@ Tcg2MeasurePeImage (
> EFI_IMAGE_LOAD_EVENT *ImageLoad;
> UINT32 FilePathSize;
> UINT32 EventSize;
> + EFI_TD_EVENT *TdEvent;
> + EFI_TD_PROTOCOL *TdProtocol;
> + EFI_TCG2_PROTOCOL *Tcg2Protocol;
>
> Status = EFI_UNSUPPORTED;
> ImageLoad = NULL;
> + TdEvent = NULL;
> +
> + Tcg2Protocol = MeasureBootProtocols->Tcg2Protocol;
> + TdProtocol = MeasureBootProtocols->TdProtocol;
> +
> + if (Tcg2Protocol == NULL && TdProtocol == NULL) {
> + ASSERT (FALSE);
> + return EFI_UNSUPPORTED;
> + }
> +
> FilePathSize = (UINT32) GetDevicePathSize (FilePath);
>
> //
> @@ -334,7 +459,7 @@ Tcg2MeasurePeImage (
> break;
> default:
> DEBUG ((
> - EFI_D_ERROR,
> + DEBUG_ERROR,
> "Tcg2MeasurePeImage: Unknown subsystem type %d",
> ImageType
> ));
> @@ -352,28 +477,124 @@ Tcg2MeasurePeImage (
> //
> // Log the PE data
> //
> - Status = Tcg2Protocol->HashLogExtendEvent (
> - Tcg2Protocol,
> - PE_COFF_IMAGE,
> - ImageAddress,
> - ImageSize,
> - Tcg2Event
> - );
> - if (Status == EFI_VOLUME_FULL) {
> - //
> - // Volume full here means the image is hashed and its result is extended to PCR.
> - // But the event log can't be saved since log area is full.
> - // Just return EFI_SUCCESS in order not to block the image load.
> - //
> - Status = EFI_SUCCESS;
> + if (Tcg2Protocol != NULL) {
> + Status = Tcg2Protocol->HashLogExtendEvent (
> + Tcg2Protocol,
> + PE_COFF_IMAGE,
> + ImageAddress,
> + ImageSize,
> + Tcg2Event
> + );
> + if (Status == EFI_VOLUME_FULL) {
> + //
> + // Volume full here means the image is hashed and its result is extended to PCR.
> + // But the event log can't be saved since log area is full.
> + // Just return EFI_SUCCESS in order not to block the image load.
> + //
> + Status = EFI_SUCCESS;
> + }
> + DEBUG ((DEBUG_INFO, "DxeTpm2MeasureBootHandler - Tcg2 MeasurePeImage - %r\n", Status));
> + }
> +
> + if (TdProtocol != NULL) {
> + TdEvent = CreateTdEventFromTcg2Event (TdProtocol, Tcg2Event, EventSize);
> + if (TdEvent == NULL) {
> + goto Finish;
[SAMI] I think Status should be set to reflect an appropriate error code
here.
> + }
> +
> + Status = TdProtocol->HashLogExtendEvent (
> + TdProtocol,
> + PE_COFF_IMAGE,
> + ImageAddress,
> + ImageSize,
> + TdEvent
> + );
> + if (Status == EFI_VOLUME_FULL) {
> + //
> + // Volume full here means the image is hashed and its result is extended to PCR.
> + // But the event log can't be saved since log area is full.
> + // Just return EFI_SUCCESS in order not to block the image load.
> + //
> + Status = EFI_SUCCESS;
> + }
> + DEBUG ((DEBUG_INFO, "DxeTpm2MeasureBootHandler - Td MeasurePeImage - %r\n", Status));
> }
>
> Finish:
> - FreePool (Tcg2Event);
> + if (Tcg2Event != NULL) {
> + FreePool (Tcg2Event);
> + }
> +
> + if (TdEvent != NULL) {
> + FreePool (TdEvent);
> + }
>
> return Status;
> }
>
> +/**
> + Get the measure boot protocols.
> +
> + There are 2 measure boot, TCG2 protocol based and Td protocol based.
> +
> + @param MeasureBootProtocols Pointer to the located measure boot protocol instances.
> +
> + @retval EFI_SUCCESS Sucessfully locate the measure boot protocol instances (at least one instance).
> + @retval EFI_UNSUPPORTED Measure boot is not supported.
> +**/
> +EFI_STATUS
> +EFIAPI
> +GetMeasureBootProtocols (
> + MEASURE_BOOT_PROTOCOLS *MeasureBootProtocols
> + )
> +{
> + EFI_STATUS Status;
> + EFI_TCG2_PROTOCOL *Tcg2Protocol;
> + EFI_TD_PROTOCOL *TdProtocol;
> + EFI_TCG2_BOOT_SERVICE_CAPABILITY Tcg2ProtocolCapability;
> + EFI_TD_BOOT_SERVICE_CAPABILITY TdProtocolCapability;
> +
> + TdProtocol = NULL;
> + Status = gBS->LocateProtocol (&gEfiTdProtocolGuid, NULL, (VOID **) &TdProtocol);
> + if (EFI_ERROR (Status)) {
> + //
> + // TdTcg2 protocol is not installed.
> + //
> + DEBUG ((DEBUG_VERBOSE, "TdProtocol is not installed. - %r\n", Status));
> + } else {
> + TdProtocolCapability.Size = sizeof (TdProtocolCapability);
> + Status = TdProtocol->GetCapability (TdProtocol, &TdProtocolCapability);
> + if (EFI_ERROR (Status) || !TdProtocolCapability.TdPresentFlag) {
> + DEBUG ((DEBUG_ERROR, "TdPresentFlag=FALSE. %r\n", Status));
> + TdProtocol = NULL;
> + }
> + }
> +
> + Tcg2Protocol = NULL;
> + Status = gBS->LocateProtocol (&gEfiTcg2ProtocolGuid, NULL, (VOID **) &Tcg2Protocol);
> + if (EFI_ERROR (Status)) {
> + //
> + // Tcg2 protocol is not installed. So, TPM2 is not present.
> + //
> + DEBUG ((DEBUG_VERBOSE, "Tcg2Protocol is not installed. - %r\n", Status));
> + } else {
> + Tcg2ProtocolCapability.Size = (UINT8) sizeof (Tcg2ProtocolCapability);
> + Status = Tcg2Protocol->GetCapability (Tcg2Protocol, &Tcg2ProtocolCapability);
> + if (EFI_ERROR (Status) || (!Tcg2ProtocolCapability.TPMPresentFlag)) {
> + //
> + // TPM device doesn't work or activate.
> + //
> + DEBUG ((DEBUG_ERROR, "TPMPresentFlag=FALSE %r\n", Status));
> + Tcg2Protocol = NULL;
> + }
> + }
> +
> + MeasureBootProtocols->Tcg2Protocol = Tcg2Protocol;
> + MeasureBootProtocols->TdProtocol = TdProtocol;
> +
> + return (Tcg2Protocol == NULL && TdProtocol == NULL) ? EFI_UNSUPPORTED: EFI_SUCCESS;
> +}
> +
> /**
> The security handler is used to abstract platform-specific policy
> from the DXE core response to an attempt to use a file that returns a
> @@ -422,9 +643,8 @@ DxeTpm2MeasureBootHandler (
> IN BOOLEAN BootPolicy
> )
> {
> - EFI_TCG2_PROTOCOL *Tcg2Protocol;
> + MEASURE_BOOT_PROTOCOLS MeasureBootProtocols;
> EFI_STATUS Status;
> - EFI_TCG2_BOOT_SERVICE_CAPABILITY ProtocolCapability;
> EFI_DEVICE_PATH_PROTOCOL *DevicePathNode;
> EFI_DEVICE_PATH_PROTOCOL *OrigDevicePathNode;
> EFI_HANDLE Handle;
> @@ -435,28 +655,19 @@ DxeTpm2MeasureBootHandler (
> EFI_PHYSICAL_ADDRESS FvAddress;
> UINT32 Index;
>
> - Status = gBS->LocateProtocol (&gEfiTcg2ProtocolGuid, NULL, (VOID **) &Tcg2Protocol);
> + MeasureBootProtocols.Tcg2Protocol = NULL;
> + MeasureBootProtocols.TdProtocol = NULL;
> +
> + Status = GetMeasureBootProtocols(&MeasureBootProtocols);
> +
> if (EFI_ERROR (Status)) {
> - //
> - // Tcg2 protocol is not installed. So, TPM2 is not present.
> - // Don't do any measurement, and directly return EFI_SUCCESS.
> - //
[SAMI] It may be helpful to retain the oirginal comment with slight
rewording.
> - DEBUG ((EFI_D_VERBOSE, "DxeTpm2MeasureBootHandler - Tcg2 - %r\n", Status));
> + DEBUG ((DEBUG_INFO, "None of Tcg2Protocol/TdProtocol is installed.\n"));
> return EFI_SUCCESS;
> }
>
> - ProtocolCapability.Size = (UINT8) sizeof (ProtocolCapability);
> - Status = Tcg2Protocol->GetCapability (
> - Tcg2Protocol,
> - &ProtocolCapability
> - );
> - if (EFI_ERROR (Status) || (!ProtocolCapability.TPMPresentFlag)) {
> - //
> - // TPM device doesn't work or activate.
> - //
> - DEBUG ((EFI_D_ERROR, "DxeTpm2MeasureBootHandler (%r) - TPMPresentFlag - %x\n", Status, ProtocolCapability.TPMPresentFlag));
> - return EFI_SUCCESS;
> - }
> + DEBUG ((DEBUG_INFO, "Tcg2Protocol = %p, TdProtocol = %p\n",
> + MeasureBootProtocols.Tcg2Protocol,
> + MeasureBootProtocols.TdProtocol));
>
> //
> // Copy File Device Path
> @@ -502,8 +713,8 @@ DxeTpm2MeasureBootHandler (
> //
> // Measure GPT disk.
> //
> - Status = Tcg2MeasureGptTable (Tcg2Protocol, Handle);
> - DEBUG ((EFI_D_INFO, "DxeTpm2MeasureBootHandler - Tcg2MeasureGptTable - %r\n", Status));
> + Status = Tcg2MeasureGptTable (&MeasureBootProtocols, Handle);
> +
> if (!EFI_ERROR (Status)) {
> //
> // GPT disk check done.
> @@ -647,14 +858,13 @@ DxeTpm2MeasureBootHandler (
> // Measure PE image into TPM log.
> //
> Status = Tcg2MeasurePeImage (
> - Tcg2Protocol,
> + &MeasureBootProtocols,
> (EFI_PHYSICAL_ADDRESS) (UINTN) FileBuffer,
> FileSize,
> (UINTN) ImageContext.ImageAddress,
> ImageContext.ImageType,
> DevicePathNode
> );
> - DEBUG ((EFI_D_INFO, "DxeTpm2MeasureBootHandler - Tcg2MeasurePeImage - %r\n", Status));
> }
>
> //
> @@ -665,7 +875,7 @@ Finish:
> FreePool (OrigDevicePathNode);
> }
>
> - DEBUG ((EFI_D_INFO, "DxeTpm2MeasureBootHandler - %r\n", Status));
> + DEBUG ((DEBUG_INFO, "DxeTpm2MeasureBootHandler - %r\n", Status));
>
> return Status;
> }
> diff --git a/SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.inf b/SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.inf
> index 2506abbe7c8b..29b62c3ba8fa 100644
> --- a/SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.inf
> +++ b/SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.inf
> @@ -61,6 +61,7 @@
>
> [Protocols]
> gEfiTcg2ProtocolGuid ## SOMETIMES_CONSUMES
> + gEfiTdProtocolGuid
> gEfiFirmwareVolumeBlockProtocolGuid ## SOMETIMES_CONSUMES
> gEfiBlockIoProtocolGuid ## SOMETIMES_CONSUMES
> gEfiDiskIoProtocolGuid ## SOMETIMES_CONSUMES
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#82354): https://edk2.groups.io/g/devel/message/82354
Mute This Topic: https://groups.io/mt/86163959/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On October 19, 2021 9:23 PM, Sami Mujawar wrote:
> > //
> > // Read the EFI Partition Table Header
> > //
> > @@ -156,6 +224,15 @@ Tcg2MeasureGptTable (
> > if (PrimaryHeader == NULL) {
> > return EFI_OUT_OF_RESOURCES;
> > }
> > +
> > + //
> > + // PrimaryHeader->SizeOfPartitionEntry should not be zero // if
> > + (PrimaryHeader->SizeOfPartitionEntry == 0) {
> > + DEBUG ((DEBUG_ERROR, "SizeOfPartitionEntry should not be zero!\n"));
> > + return EFI_BAD_BUFFER_SIZE;
> > + }
> [SAMI] I think this check is at an incorrect location. Should this be after the
> ReadDisk() below? Also, PrimaryHeader would need to be freed in the error
> scenario above.
Good capture! It will be fixed in the next version.
> >
> > + if (TdProtocol != NULL) {
> > + TdEvent = CreateTdEventFromTcg2Event (TdProtocol, Tcg2Event,
> EventSize);
> > + if (TdEvent == NULL) {
> > + goto Exit;
> [SAMI] I think Status should be set to reflect an appropriate error code here.
I am thinking if TCG2_PROTOCOL and TEE_PROTOCOL will be installed in the same time?
1) If these 2 protocols are NOT installed in the same time, then the returned status reflect the actual operation result of the protocol.
2) If these 2 protocols can be installed in the same time, then it will be a problem that the how to reflect the operation result of the protocols by the status?
I prefer 1) that these 2 protocols are NOT installed in the same time. Because it doesn't make sense to measure the boot in 2 times.
What's your suggestion?
BTW, CreateTdEventFromTcg2Event will be updated to return a status to indicate the operation result. So that the status can reflect an appropriate error code.
> Also would it be possible to create this event just before calling
> TdProtocol->HashLogExtendEvent at line 351?
> I am trying to understand why is this done differently in
> Tcg2MeasurePeImage() i.e. The TdEvent is created and extended in the same
> if (TdProtocol != NULL) block.
You're right. TdEvent should be created and extended in the same if block. It will be fixed in the next version.
> [/SAMI]
> > +
> > + if (TdProtocol != NULL) {
> > + TdEvent = CreateTdEventFromTcg2Event (TdProtocol, Tcg2Event,
> EventSize);
> > + if (TdEvent == NULL) {
> > + goto Finish;
> [SAMI] I think Status should be set to reflect an appropriate error code here.
It will be fixed in the next version.
> > **) &Tcg2Protocol);
> > + MeasureBootProtocols.Tcg2Protocol = NULL;
> > + MeasureBootProtocols.TdProtocol = NULL;
> > +
> > + Status = GetMeasureBootProtocols(&MeasureBootProtocols);
> > +
> > if (EFI_ERROR (Status)) {
> > - //
> > - // Tcg2 protocol is not installed. So, TPM2 is not present.
> > - // Don't do any measurement, and directly return EFI_SUCCESS.
> > - //
> [SAMI] It may be helpful to retain the oirginal comment with slight
> rewording.
Sure. It will be added and reworded in the next version. Thanks for reminder.
Thanks
Min
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#82729): https://edk2.groups.io/g/devel/message/82729
Mute This Topic: https://groups.io/mt/86163959/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Hi Min,
Please find my response inline marked [SAMI].
Regards,
Sami Mujawar
On Tue, Oct 26, 2021 at 10:19 PM, Min Xu wrote:
>
>
>>
>>> + if (TdProtocol != NULL) {
>>> + TdEvent = CreateTdEventFromTcg2Event (TdProtocol, Tcg2Event,
>>
>> EventSize);
>>
>>> + if (TdEvent == NULL) {
>>> + goto Exit;
>>
>> [SAMI] I think Status should be set to reflect an appropriate error code
>> here.
>
> I am thinking if TCG2_PROTOCOL and TEE_PROTOCOL will be installed in the
> same time?
> 1) If these 2 protocols are NOT installed in the same time, then the
> returned status reflect the actual operation result of the protocol.
> 2) If these 2 protocols can be installed in the same time, then it will be
> a problem that the how to reflect the operation result of the protocols by
> the status?
> I prefer 1) that these 2 protocols are NOT installed in the same time.
> Because it doesn't make sense to measure the boot in 2 times.
> What's your suggestion?
[SAMI] I don't know if there is a use-case for both the protocols to be installed at the same time. But, I would agree it would not make sense to measure twice.
>
> BTW, CreateTdEventFromTcg2Event will be updated to return a status to
> indicate the operation result. So that the status can reflect an
> appropriate error code.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#82998): https://edk2.groups.io/g/devel/message/82998
Mute This Topic: https://groups.io/mt/86163959/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2026 Red Hat, Inc.