From: Min M Xu <min.m.xu@intel.com>
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4243
TdxHelperLib provides below helper functions for a td-guest.
- TdxHelperProcessTdHob
- TdxHelperMeasureTdHob
- TdxHelperMeasureCfvImage
- TdxHelperBuildGuidHobForTdxMeasurement
This patch implements below helper functions in SecTdxHelperLib:
- TdxHelperMeasureTdHob measure/extend TdHob and store the measurement
value in workarea.
- TdxHelperMeasureCfvImage measure/extend the Configuration FV image and
store the measurement value in workarea.
- TdxHelperBuildGuidHobForTdxMeasurement is only valid in PEI-LESS
startup mode. It builds GuidHob for tdx measurement in SEC phase.
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Michael Roth <michael.roth@amd.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
---
OvmfPkg/IntelTdx/TdxHelperLib/SecTdxHelper.c | 150 +++++++++-
.../IntelTdx/TdxHelperLib/SecTdxHelperLib.inf | 1 +
.../IntelTdx/TdxHelperLib/TdxMeasurementHob.c | 266 ++++++++++++++++++
3 files changed, 414 insertions(+), 3 deletions(-)
create mode 100644 OvmfPkg/IntelTdx/TdxHelperLib/TdxMeasurementHob.c
diff --git a/OvmfPkg/IntelTdx/TdxHelperLib/SecTdxHelper.c b/OvmfPkg/IntelTdx/TdxHelperLib/SecTdxHelper.c
index 4fb2d2a8acad..82242d37d1ed 100644
--- a/OvmfPkg/IntelTdx/TdxHelperLib/SecTdxHelper.c
+++ b/OvmfPkg/IntelTdx/TdxHelperLib/SecTdxHelper.c
@@ -32,6 +32,18 @@
#define AP_STACK_SIZE SIZE_16KB
#define APS_STACK_SIZE(CpusNum) (ALIGN_VALUE(CpusNum*AP_STACK_SIZE, SIZE_2MB))
+/**
+ Build the GuidHob for tdx measurements which were done in SEC phase.
+ The measurement values are stored in WorkArea.
+
+ @retval EFI_SUCCESS The GuidHob is built successfully
+ @retval Others Other errors as indicated
+**/
+EFI_STATUS
+InternalBuildGuidHobForTdxMeasurement (
+ VOID
+ );
+
/**
This function will be called to accept pages. Only BSP accepts pages.
@@ -793,6 +805,67 @@ TdxHelperProcessTdHob (
return Status;
}
+//
+// SHA512_CTX is defined in <openssl/sha.h> and its size is 216 bytes.
+// It can be built successfully with GCC5 compiler but failed with VS2019.
+// The error code showed in VS2019 is that "openssl/sha.h" cannot be found.
+// To overcome this error SHA512_CTX_SIZE is defined.
+//
+#define SHA512_CTX_SIZ 216
+
+/**
+ * Calculate the sha384 of input Data and extend it to RTMR register.
+ *
+ * @param RtmrIndex Index of the RTMR register
+ * @param DataToHash Data to be hashed
+ * @param DataToHashLen Length of the data
+ * @param Digest Hash value of the input data
+ * @param DigestLen Length of the hash value
+ *
+ * @retval EFI_SUCCESS Successfully hash and extend to RTMR
+ * @retval Others Other errors as indicated
+ */
+STATIC
+EFI_STATUS
+HashAndExtendToRtmr (
+ IN UINT32 RtmrIndex,
+ IN VOID *DataToHash,
+ IN UINTN DataToHashLen,
+ OUT UINT8 *Digest,
+ IN UINTN DigestLen
+ )
+{
+ EFI_STATUS Status;
+ UINT8 Sha384Ctx[SHA512_CTX_SIZ];
+
+ if ((DataToHash == NULL) || (DataToHashLen == 0)) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ if ((Digest == NULL) || (DigestLen != SHA384_DIGEST_SIZE)) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ //
+ // Calculate the sha384 of the data
+ //
+ Sha384Init (Sha384Ctx);
+ Sha384Update (Sha384Ctx, DataToHash, DataToHashLen);
+ Sha384Final (Sha384Ctx, Digest);
+
+ //
+ // Extend to RTMR
+ //
+ Status = TdExtendRtmr (
+ (UINT32 *)Digest,
+ SHA384_DIGEST_SIZE,
+ (UINT8)RtmrIndex
+ );
+
+ ASSERT (!EFI_ERROR (Status));
+ return Status;
+}
+
/**
In Tdx guest, TdHob is passed from host VMM to guest firmware and it contains
the information of the memory resource. From the security perspective before
@@ -807,7 +880,47 @@ TdxHelperMeasureTdHob (
VOID
)
{
- return EFI_UNSUPPORTED;
+ EFI_PEI_HOB_POINTERS Hob;
+ EFI_STATUS Status;
+ UINT8 Digest[SHA384_DIGEST_SIZE];
+ OVMF_WORK_AREA *WorkArea;
+ VOID *TdHob;
+
+ TdHob = (VOID *)(UINTN)FixedPcdGet32 (PcdOvmfSecGhcbBase);
+ Hob.Raw = (UINT8 *)TdHob;
+
+ //
+ // Walk thru the TdHob list until end of list.
+ //
+ while (!END_OF_HOB_LIST (Hob)) {
+ Hob.Raw = GET_NEXT_HOB (Hob);
+ }
+
+ Status = HashAndExtendToRtmr (
+ 0,
+ (UINT8 *)TdHob,
+ (UINTN)((UINT8 *)Hob.Raw - (UINT8 *)TdHob),
+ Digest,
+ SHA384_DIGEST_SIZE
+ );
+
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ //
+ // This function is called in SEC phase and at that moment the Hob service
+ // is not available. So the TdHob measurement value is stored in workarea.
+ //
+ WorkArea = (OVMF_WORK_AREA *)FixedPcdGet32 (PcdOvmfWorkAreaBase);
+ if (WorkArea == NULL) {
+ return EFI_DEVICE_ERROR;
+ }
+
+ WorkArea->TdxWorkArea.SecTdxWorkArea.TdxMeasurementsData.MeasurementsBitmap |= TDX_MEASUREMENT_TDHOB_BITMASK;
+ CopyMem (WorkArea->TdxWorkArea.SecTdxWorkArea.TdxMeasurementsData.TdHobHashValue, Digest, SHA384_DIGEST_SIZE);
+
+ return EFI_SUCCESS;
}
/**
@@ -824,10 +937,37 @@ TdxHelperMeasureCfvImage (
VOID
)
{
- return EFI_UNSUPPORTED;
+ EFI_STATUS Status;
+ UINT8 Digest[SHA384_DIGEST_SIZE];
+ OVMF_WORK_AREA *WorkArea;
+
+ Status = HashAndExtendToRtmr (
+ 0,
+ (UINT8 *)(UINTN)PcdGet32 (PcdOvmfFlashNvStorageVariableBase),
+ (UINT64)PcdGet32 (PcdCfvRawDataSize),
+ Digest,
+ SHA384_DIGEST_SIZE
+ );
+
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ //
+ // This function is called in SEC phase and at that moment the Hob service
+ // is not available. So CfvImage measurement value is stored in workarea.
+ //
+ WorkArea = (OVMF_WORK_AREA *)FixedPcdGet32 (PcdOvmfWorkAreaBase);
+ if (WorkArea == NULL) {
+ return EFI_DEVICE_ERROR;
+ }
+
+ WorkArea->TdxWorkArea.SecTdxWorkArea.TdxMeasurementsData.MeasurementsBitmap |= TDX_MEASUREMENT_CFVIMG_BITMASK;
+ CopyMem (WorkArea->TdxWorkArea.SecTdxWorkArea.TdxMeasurementsData.CfvImgHashValue, Digest, SHA384_DIGEST_SIZE);
+
+ return EFI_SUCCESS;
}
-/**
/**
Build the GuidHob for tdx measurements which were done in SEC phase.
The measurement values are stored in WorkArea.
@@ -841,5 +981,9 @@ TdxHelperBuildGuidHobForTdxMeasurement (
VOID
)
{
+ #ifdef TDX_PEI_LESS_BOOT
+ return InternalBuildGuidHobForTdxMeasurement ();
+ #else
return EFI_UNSUPPORTED;
+ #endif
}
diff --git a/OvmfPkg/IntelTdx/TdxHelperLib/SecTdxHelperLib.inf b/OvmfPkg/IntelTdx/TdxHelperLib/SecTdxHelperLib.inf
index 3c6b96f7759a..d17b84c01f20 100644
--- a/OvmfPkg/IntelTdx/TdxHelperLib/SecTdxHelperLib.inf
+++ b/OvmfPkg/IntelTdx/TdxHelperLib/SecTdxHelperLib.inf
@@ -24,6 +24,7 @@
[Sources]
SecTdxHelper.c
+ TdxMeasurementHob.c
[Packages]
CryptoPkg/CryptoPkg.dec
diff --git a/OvmfPkg/IntelTdx/TdxHelperLib/TdxMeasurementHob.c b/OvmfPkg/IntelTdx/TdxHelperLib/TdxMeasurementHob.c
new file mode 100644
index 000000000000..906d4df485b8
--- /dev/null
+++ b/OvmfPkg/IntelTdx/TdxHelperLib/TdxMeasurementHob.c
@@ -0,0 +1,266 @@
+/** @file
+ Build GuidHob for tdx measurement.
+
+ Copyright (c) 2022 - 2023, Intel Corporation. All rights reserved.<BR>
+
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include <Base.h>
+#include <PiPei.h>
+#include <IndustryStandard/UefiTcgPlatform.h>
+#include <IndustryStandard/Tpm20.h>
+#include <Library/BaseLib.h>
+#include <Library/DebugLib.h>
+#include <Library/HobLib.h>
+#include <Library/BaseMemoryLib.h>
+#include <Library/MemoryAllocationLib.h>
+#include <Library/PrintLib.h>
+#include <Library/TpmMeasurementLib.h>
+#include <Pi/PrePiHob.h>
+#include <WorkArea.h>
+#include <ConfidentialComputingGuestAttr.h>
+
+#pragma pack(1)
+
+#define HANDOFF_TABLE_DESC "TdxTable"
+typedef struct {
+ UINT8 TableDescriptionSize;
+ UINT8 TableDescription[sizeof (HANDOFF_TABLE_DESC)];
+ UINT64 NumberOfTables;
+ EFI_CONFIGURATION_TABLE TableEntry[1];
+} TDX_HANDOFF_TABLE_POINTERS2;
+
+#define FV_HANDOFF_TABLE_DESC "Fv(XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX)"
+typedef struct {
+ UINT8 BlobDescriptionSize;
+ UINT8 BlobDescription[sizeof (FV_HANDOFF_TABLE_DESC)];
+ EFI_PHYSICAL_ADDRESS BlobBase;
+ UINT64 BlobLength;
+} FV_HANDOFF_TABLE_POINTERS2;
+
+#pragma pack()
+
+/**
+ * Build GuidHob for Tdx measurement.
+ *
+ * Tdx measurement includes the measurement of TdHob and CFV. They're measured
+ * and extended to RTMR registers in SEC phase. Because at that moment the Hob
+ * service are not available. So the values of the measurement are saved in
+ * workarea and will be built into GuidHob after the Hob service is ready.
+ *
+ * @param RtmrIndex RTMR index
+ * @param EventType Event type
+ * @param EventData Event data
+ * @param EventSize Size of event data
+ * @param HashValue Hash value
+ * @param HashSize Size of hash
+ *
+ * @retval EFI_SUCCESS Successfully build the GuidHobs
+ * @retval Others Other error as indicated
+ */
+STATIC
+EFI_STATUS
+BuildTdxMeasurementGuidHob (
+ UINT32 RtmrIndex,
+ UINT32 EventType,
+ UINT8 *EventData,
+ UINT32 EventSize,
+ UINT8 *HashValue,
+ UINT32 HashSize
+ )
+{
+ VOID *EventHobData;
+ UINT8 *Ptr;
+ TPML_DIGEST_VALUES *TdxDigest;
+
+ if (HashSize != SHA384_DIGEST_SIZE) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ #define TDX_DIGEST_VALUE_LEN (sizeof (UINT32) + sizeof (TPMI_ALG_HASH) + SHA384_DIGEST_SIZE)
+
+ EventHobData = BuildGuidHob (
+ &gCcEventEntryHobGuid,
+ sizeof (TCG_PCRINDEX) + sizeof (TCG_EVENTTYPE) +
+ TDX_DIGEST_VALUE_LEN +
+ sizeof (UINT32) + EventSize
+ );
+
+ if (EventHobData == NULL) {
+ return EFI_OUT_OF_RESOURCES;
+ }
+
+ Ptr = (UINT8 *)EventHobData;
+
+ //
+ // There are 2 types of measurement registers in TDX: MRTD and RTMR[0-3].
+ // According to UEFI Spec 2.10 Section 38.4.1, RTMR[0-3] is mapped to MrIndex[1-4].
+ // So RtmrIndex must be increased by 1 before the event log is created.
+ //
+ RtmrIndex++;
+ CopyMem (Ptr, &RtmrIndex, sizeof (UINT32));
+ Ptr += sizeof (UINT32);
+
+ CopyMem (Ptr, &EventType, sizeof (TCG_EVENTTYPE));
+ Ptr += sizeof (TCG_EVENTTYPE);
+
+ TdxDigest = (TPML_DIGEST_VALUES *)Ptr;
+ TdxDigest->count = 1;
+ TdxDigest->digests[0].hashAlg = TPM_ALG_SHA384;
+ CopyMem (
+ TdxDigest->digests[0].digest.sha384,
+ HashValue,
+ SHA384_DIGEST_SIZE
+ );
+ Ptr += TDX_DIGEST_VALUE_LEN;
+
+ CopyMem (Ptr, &EventSize, sizeof (UINT32));
+ Ptr += sizeof (UINT32);
+
+ CopyMem (Ptr, (VOID *)EventData, EventSize);
+ Ptr += EventSize;
+
+ return EFI_SUCCESS;
+}
+
+/**
+ Get the FvName from the FV header.
+ Causion: The FV is untrusted input.
+ @param[in] FvBase Base address of FV image.
+ @param[in] FvLength Length of FV image.
+ @return FvName pointer
+ @retval NULL FvName is NOT found
+**/
+STATIC
+VOID *
+GetFvName (
+ IN EFI_PHYSICAL_ADDRESS FvBase,
+ IN UINT64 FvLength
+ )
+{
+ EFI_FIRMWARE_VOLUME_HEADER *FvHeader;
+ EFI_FIRMWARE_VOLUME_EXT_HEADER *FvExtHeader;
+
+ if (FvBase >= MAX_ADDRESS) {
+ return NULL;
+ }
+
+ if (FvLength >= MAX_ADDRESS - FvBase) {
+ return NULL;
+ }
+
+ if (FvLength < sizeof (EFI_FIRMWARE_VOLUME_HEADER)) {
+ return NULL;
+ }
+
+ FvHeader = (EFI_FIRMWARE_VOLUME_HEADER *)(UINTN)FvBase;
+ if (FvHeader->ExtHeaderOffset < sizeof (EFI_FIRMWARE_VOLUME_HEADER)) {
+ return NULL;
+ }
+
+ if (FvHeader->ExtHeaderOffset + sizeof (EFI_FIRMWARE_VOLUME_EXT_HEADER) > FvLength) {
+ return NULL;
+ }
+
+ FvExtHeader = (EFI_FIRMWARE_VOLUME_EXT_HEADER *)(UINTN)(FvBase + FvHeader->ExtHeaderOffset);
+
+ return &FvExtHeader->FvName;
+}
+
+/**
+ Build the GuidHob for tdx measurements which were done in SEC phase.
+ The measurement values are stored in WorkArea.
+
+ @retval EFI_SUCCESS The GuidHob is built successfully
+ @retval Others Other errors as indicated
+**/
+EFI_STATUS
+InternalBuildGuidHobForTdxMeasurement (
+ VOID
+ )
+{
+ EFI_STATUS Status;
+ OVMF_WORK_AREA *WorkArea;
+ VOID *TdHobList;
+ TDX_HANDOFF_TABLE_POINTERS2 HandoffTables;
+ VOID *FvName;
+ FV_HANDOFF_TABLE_POINTERS2 FvBlob2;
+ EFI_PHYSICAL_ADDRESS FvBase;
+ UINT64 FvLength;
+ UINT8 *HashValue;
+
+ if (!TdIsEnabled ()) {
+ ASSERT (FALSE);
+ return EFI_UNSUPPORTED;
+ }
+
+ WorkArea = (OVMF_WORK_AREA *)FixedPcdGet32 (PcdOvmfWorkAreaBase);
+ if (WorkArea == NULL) {
+ return EFI_ABORTED;
+ }
+
+ Status = EFI_SUCCESS;
+
+ //
+ // Build the GuidHob for TdHob measurement
+ //
+ TdHobList = (VOID *)(UINTN)FixedPcdGet32 (PcdOvmfSecGhcbBase);
+ if (WorkArea->TdxWorkArea.SecTdxWorkArea.TdxMeasurementsData.MeasurementsBitmap & TDX_MEASUREMENT_TDHOB_BITMASK) {
+ HashValue = WorkArea->TdxWorkArea.SecTdxWorkArea.TdxMeasurementsData.TdHobHashValue;
+ HandoffTables.TableDescriptionSize = sizeof (HandoffTables.TableDescription);
+ CopyMem (HandoffTables.TableDescription, HANDOFF_TABLE_DESC, sizeof (HandoffTables.TableDescription));
+ HandoffTables.NumberOfTables = 1;
+ CopyGuid (&(HandoffTables.TableEntry[0].VendorGuid), &gUefiOvmfPkgTokenSpaceGuid);
+ HandoffTables.TableEntry[0].VendorTable = TdHobList;
+
+ Status = BuildTdxMeasurementGuidHob (
+ 0, // RtmrIndex
+ EV_EFI_HANDOFF_TABLES2, // EventType
+ (UINT8 *)(UINTN)&HandoffTables, // EventData
+ sizeof (HandoffTables), // EventSize
+ HashValue, // HashValue
+ SHA384_DIGEST_SIZE // HashSize
+ );
+ }
+
+ if (EFI_ERROR (Status)) {
+ ASSERT (FALSE);
+ return Status;
+ }
+
+ //
+ // Build the GuidHob for Cfv measurement
+ //
+ if (WorkArea->TdxWorkArea.SecTdxWorkArea.TdxMeasurementsData.MeasurementsBitmap & TDX_MEASUREMENT_CFVIMG_BITMASK) {
+ HashValue = WorkArea->TdxWorkArea.SecTdxWorkArea.TdxMeasurementsData.CfvImgHashValue;
+ FvBase = (UINT64)PcdGet32 (PcdOvmfFlashNvStorageVariableBase);
+ FvLength = (UINT64)PcdGet32 (PcdCfvRawDataSize);
+ FvBlob2.BlobDescriptionSize = sizeof (FvBlob2.BlobDescription);
+ CopyMem (FvBlob2.BlobDescription, FV_HANDOFF_TABLE_DESC, sizeof (FvBlob2.BlobDescription));
+ FvName = GetFvName (FvBase, FvLength);
+ if (FvName != NULL) {
+ AsciiSPrint ((CHAR8 *)FvBlob2.BlobDescription, sizeof (FvBlob2.BlobDescription), "Fv(%g)", FvName);
+ }
+
+ FvBlob2.BlobBase = FvBase;
+ FvBlob2.BlobLength = FvLength;
+
+ Status = BuildTdxMeasurementGuidHob (
+ 0, // RtmrIndex
+ EV_EFI_PLATFORM_FIRMWARE_BLOB2, // EventType
+ (VOID *)&FvBlob2, // EventData
+ sizeof (FvBlob2), // EventSize
+ HashValue, // HashValue
+ SHA384_DIGEST_SIZE // HashSize
+ );
+ }
+
+ if (EFI_ERROR (Status)) {
+ ASSERT (FALSE);
+ return Status;
+ }
+
+ return EFI_SUCCESS;
+}
--
2.29.2.windows.2
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98849): https://edk2.groups.io/g/devel/message/98849
Mute This Topic: https://groups.io/mt/96370898/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-> @@ -807,7 +880,47 @@ TdxHelperMeasureTdHob (
> VOID
> )
> {
> - return EFI_UNSUPPORTED;
> + EFI_PEI_HOB_POINTERS Hob;
> + EFI_STATUS Status;
> + UINT8 Digest[SHA384_DIGEST_SIZE];
> + OVMF_WORK_AREA *WorkArea;
> + VOID *TdHob;
> +
> + TdHob = (VOID *)(UINTN)FixedPcdGet32 (PcdOvmfSecGhcbBase);
> + Hob.Raw = (UINT8 *)TdHob;
> +
> + //
> + // Walk thru the TdHob list until end of list.
> + //
> + while (!END_OF_HOB_LIST (Hob)) {
> + Hob.Raw = GET_NEXT_HOB (Hob);
> + }
Hmm? Isn't there just a single TdHob? Why do you need to walk the list
here?
> +#pragma pack(1)
> +
> +#define HANDOFF_TABLE_DESC "TdxTable"
> +typedef struct {
> + UINT8 TableDescriptionSize;
> + UINT8 TableDescription[sizeof (HANDOFF_TABLE_DESC)];
> + UINT64 NumberOfTables;
> + EFI_CONFIGURATION_TABLE TableEntry[1];
> +} TDX_HANDOFF_TABLE_POINTERS2;
> +
> +#define FV_HANDOFF_TABLE_DESC "Fv(XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX)"
> +typedef struct {
> + UINT8 BlobDescriptionSize;
> + UINT8 BlobDescription[sizeof (FV_HANDOFF_TABLE_DESC)];
> + EFI_PHYSICAL_ADDRESS BlobBase;
> + UINT64 BlobLength;
> +} FV_HANDOFF_TABLE_POINTERS2;
> +
> +#pragma pack()
Why do you need this? For standard event types we should have those
structs already defined somewhere in edk2 I think ...
take care,
Gerd
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98881): https://edk2.groups.io/g/devel/message/98881
Mute This Topic: https://groups.io/mt/96370898/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On January 19, 2023 5:54 PM, Gerd Hoffmann wrote:
>
> > +#pragma pack(1)
> > +
> > +#define HANDOFF_TABLE_DESC "TdxTable"
> > +typedef struct {
> > + UINT8 TableDescriptionSize;
> > + UINT8 TableDescription[sizeof (HANDOFF_TABLE_DESC)];
> > + UINT64 NumberOfTables;
> > + EFI_CONFIGURATION_TABLE TableEntry[1];
> > +} TDX_HANDOFF_TABLE_POINTERS2;
> > +
> > +#define FV_HANDOFF_TABLE_DESC "Fv(XXXXXXXX-XXXX-XXXX-XXXX-
> XXXXXXXXXXXX)"
> > +typedef struct {
> > + UINT8 BlobDescriptionSize;
> > + UINT8 BlobDescription[sizeof (FV_HANDOFF_TABLE_DESC)];
> > + EFI_PHYSICAL_ADDRESS BlobBase;
> > + UINT64 BlobLength;
> > +} FV_HANDOFF_TABLE_POINTERS2;
> > +
> > +#pragma pack()
>
> Why do you need this? For standard event types we should have those
> structs already defined somewhere in edk2 I think ...
>
FV_HANDOFF_TABLE_POINTERS2 is related to standard event type (EV_EFI_PLATFORM_FIRMWARE_BLOB2).
According to comment (https://github.com/tianocore/edk2/blob/master/MdePkg/Include/IndustryStandard/UefiTcgPlatform.h#L145-L156) we can see this event type uses the structure of UEFI_PLATFORM_FIRMWARE_BLOB2. It is not a data struct with fixed size. Instead its size depends on BlobDescriptionSize.
Tcg2Pei measures the FV image with the event type (EV_EFI_PLATFORM_FIRMWARE_BLOB2) and data struct (FV_HANDOFF_TABLE_POINTERS2).
Tdx measurement does the same measurement to the Configuration FV image.
@Yao, Jiewen Can we define FV_HANDOFF_TABLE_POINTERS2 and FV_HANDOFF_TABLE_POINTERS2 in MdePkg/Include/IndustryStandard/UefiTcgPlatform.h?
Thanks
Min
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98909): https://edk2.groups.io/g/devel/message/98909
Mute This Topic: https://groups.io/mt/96370898/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
> Can we define FV_HANDOFF_TABLE_POINTERS2 and FV_HANDOFF_TABLE_POINTERS2 in MdePkg/Include/IndustryStandard/UefiTcgPlatform.h?
[Jiewen] No. We cannot move to MdePkg.
TCG defines the field to be variable length. Something like below:
typedef struct {
UINT8 TableDescriptionSize;
UINT8 TableDescription[TableDescriptionSize];
UINT64 NumberOfTables;
EFI_CONFIGURATION_TABLE TableEntry[NumberOfTables];
} HANDOFF_TABLE_POINTERS2;
typedef struct {
UINT8 BlobDescriptionSize;
UINT8 BlobDescription[BlobDescriptionSize];
EFI_PHYSICAL_ADDRESS BlobBase;
UINT64 BlobLength;
} HANDOFF_TABLE_POINTERS2;
The implementation can choose its own length as they wish.
Thank you
Yao, Jiewen
> -----Original Message-----
> From: Xu, Min M <min.m.xu@intel.com>
> Sent: Friday, January 20, 2023 3:40 PM
> To: Gerd Hoffmann <kraxel@redhat.com>; Yao, Jiewen
> <jiewen.yao@intel.com>
> Cc: devel@edk2.groups.io; Aktas, Erdem <erdemaktas@google.com>; James
> Bottomley <jejb@linux.ibm.com>; Tom Lendacky
> <thomas.lendacky@amd.com>; Michael Roth <michael.roth@amd.com>
> Subject: RE: [PATCH V2 04/10] OvmfPkg/IntelTdx: Implement other helper
> functions in SecTdxHelperLib
>
> On January 19, 2023 5:54 PM, Gerd Hoffmann wrote:
> >
> > > +#pragma pack(1)
> > > +
> > > +#define HANDOFF_TABLE_DESC "TdxTable"
> > > +typedef struct {
> > > + UINT8 TableDescriptionSize;
> > > + UINT8 TableDescription[sizeof (HANDOFF_TABLE_DESC)];
> > > + UINT64 NumberOfTables;
> > > + EFI_CONFIGURATION_TABLE TableEntry[1];
> > > +} TDX_HANDOFF_TABLE_POINTERS2;
> > > +
> > > +#define FV_HANDOFF_TABLE_DESC "Fv(XXXXXXXX-XXXX-XXXX-XXXX-
> > XXXXXXXXXXXX)"
> > > +typedef struct {
> > > + UINT8 BlobDescriptionSize;
> > > + UINT8 BlobDescription[sizeof (FV_HANDOFF_TABLE_DESC)];
> > > + EFI_PHYSICAL_ADDRESS BlobBase;
> > > + UINT64 BlobLength;
> > > +} FV_HANDOFF_TABLE_POINTERS2;
> > > +
> > > +#pragma pack()
> >
> > Why do you need this? For standard event types we should have those
> > structs already defined somewhere in edk2 I think ...
> >
> FV_HANDOFF_TABLE_POINTERS2 is related to standard event type
> (EV_EFI_PLATFORM_FIRMWARE_BLOB2).
> According to comment
> (https://github.com/tianocore/edk2/blob/master/MdePkg/Include/Industry
> Standard/UefiTcgPlatform.h#L145-L156) we can see this event type uses the
> structure of UEFI_PLATFORM_FIRMWARE_BLOB2. It is not a data struct with
> fixed size. Instead its size depends on BlobDescriptionSize.
> Tcg2Pei measures the FV image with the event type
> (EV_EFI_PLATFORM_FIRMWARE_BLOB2) and data struct
> (FV_HANDOFF_TABLE_POINTERS2).
> Tdx measurement does the same measurement to the Configuration FV
> image.
> @Yao, Jiewen Can we define FV_HANDOFF_TABLE_POINTERS2 and
> FV_HANDOFF_TABLE_POINTERS2 in
> MdePkg/Include/IndustryStandard/UefiTcgPlatform.h?
>
> Thanks
> Min
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98910): https://edk2.groups.io/g/devel/message/98910
Mute This Topic: https://groups.io/mt/96370898/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On Fri, Jan 20, 2023 at 08:10:45AM +0000, Yao, Jiewen wrote:
> > Can we define FV_HANDOFF_TABLE_POINTERS2 and FV_HANDOFF_TABLE_POINTERS2 in MdePkg/Include/IndustryStandard/UefiTcgPlatform.h?
>
> [Jiewen] No. We cannot move to MdePkg.
> TCG defines the field to be variable length. Something like below:
>
> typedef struct {
> UINT8 TableDescriptionSize;
> UINT8 TableDescription[TableDescriptionSize];
> UINT64 NumberOfTables;
> EFI_CONFIGURATION_TABLE TableEntry[NumberOfTables];
> } HANDOFF_TABLE_POINTERS2;
>
> typedef struct {
> UINT8 BlobDescriptionSize;
> UINT8 BlobDescription[BlobDescriptionSize];
> EFI_PHYSICAL_ADDRESS BlobBase;
> UINT64 BlobLength;
> } HANDOFF_TABLE_POINTERS2;
>
> The implementation can choose its own length as they wish.
Why doesn't follow TDX standard TCG practices here?
take care,
Gerd
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98918): https://edk2.groups.io/g/devel/message/98918
Mute This Topic: https://groups.io/mt/96370898/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On January 20, 2023 6:18 PM, Gerd Hoffmann wrote:
> On Fri, Jan 20, 2023 at 08:10:45AM +0000, Yao, Jiewen wrote:
> > > Can we define FV_HANDOFF_TABLE_POINTERS2 and
> FV_HANDOFF_TABLE_POINTERS2 in
> MdePkg/Include/IndustryStandard/UefiTcgPlatform.h?
> >
> > [Jiewen] No. We cannot move to MdePkg.
> > TCG defines the field to be variable length. Something like below:
> >
> > typedef struct {
> > UINT8 TableDescriptionSize;
> > UINT8 TableDescription[TableDescriptionSize];
> > UINT64 NumberOfTables;
> > EFI_CONFIGURATION_TABLE TableEntry[NumberOfTables];
> > } HANDOFF_TABLE_POINTERS2;
> >
> > typedef struct {
> > UINT8 BlobDescriptionSize;
> > UINT8 BlobDescription[BlobDescriptionSize];
> > EFI_PHYSICAL_ADDRESS BlobBase;
> > UINT64 BlobLength;
> > } HANDOFF_TABLE_POINTERS2;
> >
> > The implementation can choose its own length as they wish.
>
> Why doesn't follow TDX standard TCG practices here?
>
As Jiewen mentioned TCG defines the field to be variable length. The implementation can choose its own length. Below are some examples.
Tcg2Pei defines its FV_HANDOFF_TABLE_POINTERS2. (https://github.com/tianocore/edk2/blob/master/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c#L126-L136)
SmbiosMeasurementDxe defines its SMBIOS_HANDOFF_TABLE_POINTERS2 (https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Universal/SmbiosMeasurementDxe/SmbiosMeasurementDxe.c#L113-L123)
TcgEventLogRecordLib defines the PLATFORM_FIRMWARE_BLOB2_STRUCT and HANDOFF_TABLE_POINTERS2_STRUCT. https://github.com/tianocore/edk2/blob/master/SecurityPkg/Include/Library/TcgEventLogRecordLib.h#L14-L32
I think TDX follow the same practice above to define its own TDX_HANDOFF_TABLE_POINTERS2 and FV_HANDOFF_TABLE_POINTERS2. (FV_HANDOFF_TABLE_POINTERS2 happens to be same as the one in Tcg2Pei.) To make the definition more clear, TDX can define the name as CFV_HANDOFF_TABLE_POINTERS2.
@Gerd, Hoffmann what's your thought?
Thanks
Min
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98919): https://edk2.groups.io/g/devel/message/98919
Mute This Topic: https://groups.io/mt/96370898/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
> > > [Jiewen] No. We cannot move to MdePkg.
> > > TCG defines the field to be variable length. Something like below:
> > >
> > > typedef struct {
> > > UINT8 TableDescriptionSize;
> > > UINT8 TableDescription[TableDescriptionSize];
> > > UINT64 NumberOfTables;
> > > EFI_CONFIGURATION_TABLE TableEntry[NumberOfTables];
> > > } HANDOFF_TABLE_POINTERS2;
> > >
> > > typedef struct {
> > > UINT8 BlobDescriptionSize;
> > > UINT8 BlobDescription[BlobDescriptionSize];
> > > EFI_PHYSICAL_ADDRESS BlobBase;
> > > UINT64 BlobLength;
> > > } HANDOFF_TABLE_POINTERS2;
> > >
> > > The implementation can choose its own length as they wish.
> >
> > Why doesn't follow TDX standard TCG practices here?
> >
> As Jiewen mentioned TCG defines the field to be variable length. The implementation can choose its own length. Below are some examples.
> Tcg2Pei defines its FV_HANDOFF_TABLE_POINTERS2. (https://github.com/tianocore/edk2/blob/master/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c#L126-L136)
> SmbiosMeasurementDxe defines its SMBIOS_HANDOFF_TABLE_POINTERS2 (https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Universal/SmbiosMeasurementDxe/SmbiosMeasurementDxe.c#L113-L123)
> TcgEventLogRecordLib defines the PLATFORM_FIRMWARE_BLOB2_STRUCT and HANDOFF_TABLE_POINTERS2_STRUCT. https://github.com/tianocore/edk2/blob/master/SecurityPkg/Include/Library/TcgEventLogRecordLib.h#L14-L32
> I think TDX follow the same practice above to define its own
> TDX_HANDOFF_TABLE_POINTERS2 and FV_HANDOFF_TABLE_POINTERS2.
> (FV_HANDOFF_TABLE_POINTERS2 happens to be same as the one in Tcg2Pei.)
Ok, that makes sense. The TdHob is tdx-specific, measuring a firmware
volume is not. I'm still wondering why the structs for standard events
(like the firmware volume) are not in some shared header file ...
take care,
Gerd
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98923): https://edk2.groups.io/g/devel/message/98923
Mute This Topic: https://groups.io/mt/96370898/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On January 20, 2023 9:51 PM, Gerd Hoffmann wrote:
>
> > > > [Jiewen] No. We cannot move to MdePkg.
> > > > TCG defines the field to be variable length. Something like below:
> > > >
> > > > typedef struct {
> > > > UINT8 TableDescriptionSize;
> > > > UINT8 TableDescription[TableDescriptionSize];
> > > > UINT64 NumberOfTables;
> > > > EFI_CONFIGURATION_TABLE TableEntry[NumberOfTables];
> > > > } HANDOFF_TABLE_POINTERS2;
> > > >
> > > > typedef struct {
> > > > UINT8 BlobDescriptionSize;
> > > > UINT8 BlobDescription[BlobDescriptionSize];
> > > > EFI_PHYSICAL_ADDRESS BlobBase;
> > > > UINT64 BlobLength;
> > > > } HANDOFF_TABLE_POINTERS2;
> > > >
> > > > The implementation can choose its own length as they wish.
> > >
> > > Why doesn't follow TDX standard TCG practices here?
> > >
> > As Jiewen mentioned TCG defines the field to be variable length. The
> implementation can choose its own length. Below are some examples.
> > Tcg2Pei defines its FV_HANDOFF_TABLE_POINTERS2.
> > (https://github.com/tianocore/edk2/blob/master/SecurityPkg/Tcg/Tcg2Pei
> > /Tcg2Pei.c#L126-L136) SmbiosMeasurementDxe defines its
> > SMBIOS_HANDOFF_TABLE_POINTERS2
> >
> (https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Universal/
> > SmbiosMeasurementDxe/SmbiosMeasurementDxe.c#L113-L123)
> > TcgEventLogRecordLib defines the PLATFORM_FIRMWARE_BLOB2_STRUCT
> and
> > HANDOFF_TABLE_POINTERS2_STRUCT.
> > https://github.com/tianocore/edk2/blob/master/SecurityPkg/Include/Libr
> > ary/TcgEventLogRecordLib.h#L14-L32
>
> > I think TDX follow the same practice above to define its own
> > TDX_HANDOFF_TABLE_POINTERS2 and FV_HANDOFF_TABLE_POINTERS2.
> > (FV_HANDOFF_TABLE_POINTERS2 happens to be same as the one in
> Tcg2Pei.)
>
> Ok, that makes sense. The TdHob is tdx-specific, measuring a firmware
> volume is not. I'm still wondering why the structs for standard events (like
> the firmware volume) are not in some shared header file ...
>
Hi, Gerd
Actually I tried to find some common header file to define the events (the firmware volume). But it seems there is no such header file. Let me check the code again and see if there is such header file.
Thanks
Min
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98950): https://edk2.groups.io/g/devel/message/98950
Mute This Topic: https://groups.io/mt/96370898/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On January 19, 2023 5:54 PM, Gerd Hoffmann wrote:
> > @@ -807,7 +880,47 @@ TdxHelperMeasureTdHob (
> > VOID
> > )
> > {
> > - return EFI_UNSUPPORTED;
> > + EFI_PEI_HOB_POINTERS Hob;
> > + EFI_STATUS Status;
> > + UINT8 Digest[SHA384_DIGEST_SIZE];
> > + OVMF_WORK_AREA *WorkArea;
> > + VOID *TdHob;
> > +
> > + TdHob = (VOID *)(UINTN)FixedPcdGet32 (PcdOvmfSecGhcbBase);
> > + Hob.Raw = (UINT8 *)TdHob;
> > +
> > + //
> > + // Walk thru the TdHob list until end of list.
> > + //
> > + while (!END_OF_HOB_LIST (Hob)) {
> > + Hob.Raw = GET_NEXT_HOB (Hob);
> > + }
>
> Hmm? Isn't there just a single TdHob? Why do you need to walk the list here?
No, TdHob is a HobList and it contains several Hobs, including the ResourceDescriptorHobs which describe the memory regions. So we have to walk thru the hob list to find out its length.
>
> > +#pragma pack(1)
> > +
> > +#define HANDOFF_TABLE_DESC "TdxTable"
> > +typedef struct {
> > + UINT8 TableDescriptionSize;
> > + UINT8 TableDescription[sizeof (HANDOFF_TABLE_DESC)];
> > + UINT64 NumberOfTables;
> > + EFI_CONFIGURATION_TABLE TableEntry[1];
> > +} TDX_HANDOFF_TABLE_POINTERS2;
> > +
> > +#define FV_HANDOFF_TABLE_DESC "Fv(XXXXXXXX-XXXX-XXXX-XXXX-
> XXXXXXXXXXXX)"
> > +typedef struct {
> > + UINT8 BlobDescriptionSize;
> > + UINT8 BlobDescription[sizeof (FV_HANDOFF_TABLE_DESC)];
> > + EFI_PHYSICAL_ADDRESS BlobBase;
> > + UINT64 BlobLength;
> > +} FV_HANDOFF_TABLE_POINTERS2;
> > +
> > +#pragma pack()
>
> Why do you need this? For standard event types we should have those
> structs already defined somewhere in edk2 I think ...
These structs are defined in SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c. Let me think can they be moved to a common header file. Thanks for reminder.
Thanks
Min
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98907): https://edk2.groups.io/g/devel/message/98907
Mute This Topic: https://groups.io/mt/96370898/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2026 Red Hat, Inc.