* [PATCH V2 0/3] Introduce TdProtocol into EDK2
@ 2021-10-08 5:21 Min Xu
2021-10-08 5:21 ` [PATCH V2 1/3] MdePkg: Introduce TdProtocol for TD-Guest firmware Min Xu
` (3 more replies)
0 siblings, 4 replies; 20+ messages in thread
From: Min Xu @ 2021-10-08 5:21 UTC (permalink / raw)
To: devel
Cc: Min Xu, Michael D Kinney, Liming Gao, Zhiguang Liu, Jiewen Yao,
Jian J Wang, Ken Lu
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3625
If TD-Guest firmware supports measurement and an event is created,
TD-Guest firmware is designed to report the event log with the same data
structure in TCG-Platform-Firmware-Profile specification with
EFI_TCG2_EVENT_LOG_FORMAT_TCG_2 format.
The TD-Guest firmware supports measurement, the TD Guest Firmware is
designed to produce EFI_TD_PROTOCOL with new GUID EFI_TD_PROTOCOL_GUID
to report event log and provides hash capability.
https://software.intel.com/content/dam/develop/external/us/en/documents/
intel-tdx-guest-hypervisor-communication-interface-1.0-344426-002.pdf
Section 4.3.2 includes the EFI_TD_PROTOCOL.
Patch #1:
Introduce the TD Protocol definition into MdePkg
Patch #2:
Update DxeTpm2MeasureBootLib to support TD based measure boot.
Patch #3:
Update DxeTpmMeasurementLib to support TD based measurement.
Code is at https://github.com/mxu9/edk2/tree/td_protocol.v2
v2 changes:
- TD based measure boot is implemented in DxeTpm2MeasureBootLib.
This minimize the code changes.
- TD based measurement is added. It is implemented in
DxeTpmMeasurementLib.
- Fix the typo in comments.
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>
Cc: Ken Lu <ken.lu@intel.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
Min Xu (3):
MdePkg: Introduce TdProtocol for TD-Guest firmware
SecurityPkg: Support TdProtocol in DxeTpm2MeasureBootLib
SecurityPkg: Support TdProtocol in DxeTpmMeasurementLib
MdePkg/Include/Protocol/TdProtocol.h | 305 +++++++++++++++
MdePkg/MdePkg.dec | 3 +
.../DxeTpm2MeasureBootLib.c | 346 ++++++++++++++----
.../DxeTpm2MeasureBootLib.inf | 1 +
.../DxeTpmMeasurementLib.c | 87 ++++-
.../DxeTpmMeasurementLib.inf | 1 +
6 files changed, 672 insertions(+), 71 deletions(-)
create mode 100644 MdePkg/Include/Protocol/TdProtocol.h
--
2.29.2.windows.2
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH V2 1/3] MdePkg: Introduce TdProtocol for TD-Guest firmware
2021-10-08 5:21 [PATCH V2 0/3] Introduce TdProtocol into EDK2 Min Xu
@ 2021-10-08 5:21 ` Min Xu
2021-10-11 1:37 ` 回复: " gaoliming
2021-10-19 13:21 ` [edk2-devel] " Sami Mujawar
2021-10-08 5:21 ` [PATCH V2 2/3] SecurityPkg: Support TdProtocol in DxeTpm2MeasureBootLib Min Xu
` (2 subsequent siblings)
3 siblings, 2 replies; 20+ messages in thread
From: Min Xu @ 2021-10-08 5:21 UTC (permalink / raw)
To: devel
Cc: Min Xu, Michael D Kinney, Liming Gao, Zhiguang Liu, Jiewen Yao,
Jian J Wang, Ken Lu
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3625
If TD-Guest firmware supports measurement and an event is created,
TD-Guest firmware is designed to report the event log with the same data
structure in TCG-Platform-Firmware-Profile specification with
EFI_TCG2_EVENT_LOG_FORMAT_TCG_2 format.
The TD-Guest firmware supports measurement, the TD Guest Firmware is
designed to produce EFI_TD_PROTOCOL with new GUID EFI_TD_PROTOCOL_GUID
to report event log and provides hash capability.
https://software.intel.com/content/dam/develop/external/us/en/documents/
intel-tdx-guest-hypervisor-communication-interface-1.0-344426-002.pdf
Section 4.3.2 includes the EFI_TD_PROTOCOL.
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>
Cc: Ken Lu <ken.lu@intel.com>
Reviewed-by: Jiewen Yao <jiewen.yao@intel.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
---
MdePkg/Include/Protocol/TdProtocol.h | 305 +++++++++++++++++++++++++++
MdePkg/MdePkg.dec | 3 +
2 files changed, 308 insertions(+)
create mode 100644 MdePkg/Include/Protocol/TdProtocol.h
diff --git a/MdePkg/Include/Protocol/TdProtocol.h b/MdePkg/Include/Protocol/TdProtocol.h
new file mode 100644
index 000000000000..89b09928d33a
--- /dev/null
+++ b/MdePkg/Include/Protocol/TdProtocol.h
@@ -0,0 +1,305 @@
+/** @file
+ If TD-Guest firmware supports measurement and an event is created, TD-Guest
+ firmware is designed to report the event log with the same data structure
+ in TCG-Platform-Firmware-Profile specification with
+ EFI_TCG2_EVENT_LOG_FORMAT_TCG_2 format.
+
+ The TD-Guest firmware supports measurement, the TD Guest Firmware is designed
+ to produce EFI_TD_PROTOCOL with new GUID EFI_TD_PROTOCOL_GUID to report
+ event log and provides hash capability.
+
+Copyright (c) 2020 - 2021, Intel Corporation. All rights reserved.<BR>
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+
+#ifndef TD_PROTOCOL_H_
+#define TD_PROTOCOL_H_
+
+#include <Uefi/UefiBaseType.h>
+#include <IndustryStandard/UefiTcgPlatform.h>
+#include <IndustryStandard/Tpm20.h>
+
+
+#define EFI_TD_PROTOCOL_GUID \
+ { 0x96751a3d, 0x72f4, 0x41a6, { 0xa7, 0x94, 0xed, 0x5d, 0x0e, 0x67, 0xae, 0x6b }}
+extern EFI_GUID gEfiTdProtocolGuid;
+
+typedef struct _EFI_TD_PROTOCOL EFI_TD_PROTOCOL;
+
+typedef struct {
+ UINT8 Major;
+ UINT8 Minor;
+} EFI_TD_VERSION;
+
+typedef UINT32 EFI_TD_EVENT_LOG_BITMAP;
+typedef UINT32 EFI_TD_EVENT_LOG_FORMAT;
+typedef UINT32 EFI_TD_EVENT_ALGORITHM_BITMAP;
+typedef UINT32 EFI_TD_MR_INDEX;
+
+#define EFI_TD_EVENT_LOG_FORMAT_TCG_2 0x00000002
+#define EFI_TD_BOOT_HASH_ALG_SHA384 0x00000004
+
+//
+// This bit is shall be set when an event shall be extended but not logged.
+//
+#define EFI_TD_FLAG_EXTEND_ONLY 0x0000000000000001
+//
+// This bit shall be set when the intent is to measure a PE/COFF image.
+//
+#define EFI_TD_FLAG_PE_COFF_IMAGE 0x0000000000000010
+
+#define MR_INDEX_MRTD 0
+#define MR_INDEX_RTMR0 1
+#define MR_INDEX_RTMR1 2
+#define MR_INDEX_RTMR2 3
+#define MR_INDEX_RTMR3 4
+
+//
+// This bit shall be set when the intent is to measure a PE/COFF image.
+//
+#define PE_COFF_IMAGE 0x0000000000000010
+
+#pragma pack (1)
+
+#define EFI_TD_EVENT_HEADER_VERSION 1
+
+typedef struct {
+ //
+ // Size of the event header itself (sizeof(EFI_TD_EVENT_HEADER)).
+ //
+ UINT32 HeaderSize;
+ //
+ // Header version. For this version of this specification, the value shall be 1.
+ //
+ UINT16 HeaderVersion;
+ //
+ // Index of the MR that shall be extended.
+ //
+ EFI_TD_MR_INDEX MrIndex;
+ //
+ // Type of the event that shall be extended (and optionally logged).
+ //
+ UINT32 EventType;
+} EFI_TD_EVENT_HEADER;
+
+typedef struct {
+ //
+ // Total size of the event including the Size component, the header and the Event data.
+ //
+ UINT32 Size;
+ EFI_TD_EVENT_HEADER Header;
+ UINT8 Event[1];
+} EFI_TD_EVENT;
+
+#pragma pack()
+
+
+typedef struct {
+ //
+ // Allocated size of the structure
+ //
+ UINT8 Size;
+ //
+ // Version of the EFI_TD_BOOT_SERVICE_CAPABILITY structure itself.
+ // For this version of the protocol, the Major version shall be set to 1
+ // and the Minor version shall be set to 1.
+ //
+ EFI_TD_VERSION StructureVersion;
+ //
+ // Version of the EFI TD protocol.
+ // For this version of the protocol, the Major version shall be set to 1
+ // and the Minor version shall be set to 1.
+ //
+ EFI_TD_VERSION ProtocolVersion;
+ //
+ // Supported hash algorithms
+ //
+ EFI_TD_EVENT_ALGORITHM_BITMAP HashAlgorithmBitmap;
+ //
+ // Bitmap of supported event log formats
+ //
+ EFI_TD_EVENT_LOG_BITMAP SupportedEventLogs;
+
+ //
+ // False = TD not present
+ //
+ BOOLEAN TdPresentFlag;
+} EFI_TD_BOOT_SERVICE_CAPABILITY;
+
+/**
+ The EFI_TD_PROTOCOL GetCapability function call provides protocol
+ capability information and state information.
+
+ @param[in] This Indicates the calling context
+ @param[in, out] ProtocolCapability The caller allocates memory for a EFI_TD_BOOT_SERVICE_CAPABILITY
+ structure and sets the size field to the size of the structure allocated.
+ The callee fills in the fields with the EFI protocol capability information
+ and the current EFI TD state information up to the number of fields which
+ fit within the size of the structure passed in.
+
+ @retval EFI_SUCCESS Operation completed successfully.
+ @retval EFI_DEVICE_ERROR The command was unsuccessful.
+ The ProtocolCapability variable will not be populated.
+ @retval EFI_INVALID_PARAMETER One or more of the parameters are incorrect.
+ The ProtocolCapability variable will not be populated.
+ @retval EFI_BUFFER_TOO_SMALL The ProtocolCapability variable is too small to hold the full response.
+ It will be partially populated (required Size field will be set).
+**/
+typedef
+EFI_STATUS
+(EFIAPI *EFI_TD_GET_CAPABILITY) (
+ IN EFI_TD_PROTOCOL *This,
+ IN OUT EFI_TD_BOOT_SERVICE_CAPABILITY *ProtocolCapability
+ );
+
+/**
+ The EFI_TD_PROTOCOL Get Event Log function call allows a caller to
+ retrieve the address of a given event log and its last entry.
+
+ @param[in] This Indicates the calling context
+ @param[in] EventLogFormat The type of the event log for which the information is requested.
+ @param[out] EventLogLocation A pointer to the memory address of the event log.
+ @param[out] EventLogLastEntry If the Event Log contains more than one entry, this is a pointer to the
+ address of the start of the last entry in the event log in memory.
+ @param[out] EventLogTruncated If the Event Log is missing at least one entry because an event would
+ have exceeded the area allocated for events, this value is set to TRUE.
+ Otherwise, the value will be FALSE and the Event Log will be complete.
+
+ @retval EFI_SUCCESS Operation completed successfully.
+ @retval EFI_INVALID_PARAMETER One or more of the parameters are incorrect
+ (e.g. asking for an event log whose format is not supported).
+**/
+typedef
+EFI_STATUS
+(EFIAPI *EFI_TD_GET_EVENT_LOG) (
+ IN EFI_TD_PROTOCOL *This,
+ IN EFI_TD_EVENT_LOG_FORMAT EventLogFormat,
+ OUT EFI_PHYSICAL_ADDRESS *EventLogLocation,
+ OUT EFI_PHYSICAL_ADDRESS *EventLogLastEntry,
+ OUT BOOLEAN *EventLogTruncated
+ );
+
+/**
+ The EFI_TD_PROTOCOL HashLogExtendEvent function call provides callers with
+ an opportunity to extend and optionally log events without requiring
+ knowledge of actual TD commands.
+ The extend operation will occur even if this function cannot create an event
+ log entry (e.g. due to the event log being full).
+
+ @param[in] This Indicates the calling context
+ @param[in] Flags Bitmap providing additional information.
+ @param[in] DataToHash Physical address of the start of the data buffer to be hashed.
+ @param[in] DataToHashLen The length in bytes of the buffer referenced by DataToHash.
+ @param[in] EfiTdEvent Pointer to data buffer containing information about the event.
+
+ @retval EFI_SUCCESS Operation completed successfully.
+ @retval EFI_DEVICE_ERROR The command was unsuccessful.
+ @retval EFI_VOLUME_FULL The extend operation occurred, but the event could not be written to one or more event logs.
+ @retval EFI_INVALID_PARAMETER One or more of the parameters are incorrect.
+ @retval EFI_UNSUPPORTED The PE/COFF image type is not supported.
+**/
+typedef
+EFI_STATUS
+(EFIAPI * EFI_TD_HASH_LOG_EXTEND_EVENT) (
+ IN EFI_TD_PROTOCOL *This,
+ IN UINT64 Flags,
+ IN EFI_PHYSICAL_ADDRESS DataToHash,
+ IN UINT64 DataToHashLen,
+ IN EFI_TD_EVENT *EfiTdEvent
+ );
+
+/**
+ The EFI_TD_PROTOCOL MapPcrToMrIndex function call provides callers
+ the info on TPM PCR<-> measurement register mapping information.
+
+ In current version, we use below mapping:
+ PCR0 -> MRTD (Index 0)
+ PCR1 -> RTMR0 (Index 1)
+ PCR2~6 -> RTMR1 (Index 2)
+ PCR7 -> RTMR0 (Index 1)
+ PCR8~15 -> RTMR2 (Index 3)
+
+ @param[in] This Indicates the calling context
+ @param[in] PcrIndex TPM PCR index.
+ @param[out] MrIndex Measurement register index.
+
+ @retval EFI_SUCCESS The MR index is returned.
+ @retval EFI_INVALID_PARAMETER The MrIndex is NULL.
+ @retval EFI_UNSUPPORTED The PcrIndex is invalid.
+**/
+typedef
+EFI_STATUS
+(EFIAPI * EFI_TD_MAP_PCR_TO_MR_INDEX) (
+ IN EFI_TD_PROTOCOL *This,
+ IN TCG_PCRINDEX PcrIndex,
+ OUT EFI_TD_MR_INDEX *MrIndex
+ );
+
+struct _EFI_TD_PROTOCOL {
+ EFI_TD_GET_CAPABILITY GetCapability;
+ EFI_TD_GET_EVENT_LOG GetEventLog;
+ EFI_TD_HASH_LOG_EXTEND_EVENT HashLogExtendEvent;
+ EFI_TD_MAP_PCR_TO_MR_INDEX MapPcrToMrIndex;
+};
+
+
+//
+// TD event log
+//
+
+#pragma pack(1)
+
+//
+// Crypto Agile Log Entry Format.
+// It is similar with TCG_PCR_EVENT2 except the field of MrIndex and PCRIndex.
+//
+typedef struct {
+ EFI_TD_MR_INDEX MrIndex;
+ UINT32 EventType;
+ TPML_DIGEST_VALUES Digests;
+ UINT32 EventSize;
+ UINT8 Event[1];
+} TD_EVENT;
+
+//
+// EFI TD Event Header
+// It is similar with TCG_PCR_EVENT2_HDR except the field of MrIndex and PCRIndex
+//
+typedef struct {
+ EFI_TD_MR_INDEX MrIndex;
+ UINT32 EventType;
+ TPML_DIGEST_VALUES Digests;
+ UINT32 EventSize;
+} TD_EVENT_HDR;
+
+#pragma pack()
+
+//
+// Log entries after Get Event Log service
+//
+
+
+typedef struct {
+ //
+ // The version of this structure. It shall be set ot 1.
+ //
+ UINT64 Version;
+ //
+ // Number of events recorded after invocation of GetEventLog API
+ //
+ UINT64 NumberOfEvents;
+ //
+ // List of events of type TD_EVENT.
+ //
+ //TD_EVENT Event[1];
+} EFI_TD_FINAL_EVENTS_TABLE;
+
+
+#define EFI_TD_FINAL_EVENTS_TABLE_GUID \
+ {0xdd4a4648, 0x2de7, 0x4665, {0x96, 0x4d, 0x21, 0xd9, 0xef, 0x5f, 0xb4, 0x46}}
+
+extern EFI_GUID gEfiTdFinalEventsTableGuid;
+
+#endif
diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
index 9cdc915ebae9..a31c44d3a689 100644
--- a/MdePkg/MdePkg.dec
+++ b/MdePkg/MdePkg.dec
@@ -1011,6 +1011,9 @@
## Include/Protocol/PcdInfo.h
gGetPcdInfoProtocolGuid = { 0x5be40f57, 0xfa68, 0x4610, { 0xbb, 0xbf, 0xe9, 0xc5, 0xfc, 0xda, 0xd3, 0x65 } }
+ ## Include/Protocol/TdProtocol.h
+ gEfiTdProtocolGuid = { 0x96751a3d, 0x72f4, 0x41a6, { 0xa7, 0x94, 0xed, 0x5d, 0x0e, 0x67, 0xae, 0x6b }}
+
#
# Protocols defined in PI1.0.
#
--
2.29.2.windows.2
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH V2 2/3] SecurityPkg: Support TdProtocol in DxeTpm2MeasureBootLib
2021-10-08 5:21 [PATCH V2 0/3] Introduce TdProtocol into EDK2 Min Xu
2021-10-08 5:21 ` [PATCH V2 1/3] MdePkg: Introduce TdProtocol for TD-Guest firmware Min Xu
@ 2021-10-08 5:21 ` Min Xu
2021-10-19 13:22 ` [edk2-devel] " Sami Mujawar
2021-10-08 5:21 ` [PATCH V2 3/3] SecurityPkg: Support TdProtocol in DxeTpmMeasurementLib Min Xu
2021-10-12 15:26 ` [edk2-devel] [PATCH V2 0/3] Introduce TdProtocol into EDK2 Sami Mujawar
3 siblings, 1 reply; 20+ messages in thread
From: Min Xu @ 2021-10-08 5:21 UTC (permalink / raw)
To: devel
Cc: Min Xu, Michael D Kinney, Liming Gao, Zhiguang Liu, Jiewen Yao,
Jian J Wang
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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH V2 3/3] SecurityPkg: Support TdProtocol in DxeTpmMeasurementLib
2021-10-08 5:21 [PATCH V2 0/3] Introduce TdProtocol into EDK2 Min Xu
2021-10-08 5:21 ` [PATCH V2 1/3] MdePkg: Introduce TdProtocol for TD-Guest firmware Min Xu
2021-10-08 5:21 ` [PATCH V2 2/3] SecurityPkg: Support TdProtocol in DxeTpm2MeasureBootLib Min Xu
@ 2021-10-08 5:21 ` Min Xu
2021-10-19 13:24 ` [edk2-devel] " Sami Mujawar
2021-10-12 15:26 ` [edk2-devel] [PATCH V2 0/3] Introduce TdProtocol into EDK2 Sami Mujawar
3 siblings, 1 reply; 20+ messages in thread
From: Min Xu @ 2021-10-08 5:21 UTC (permalink / raw)
To: devel
Cc: Min Xu, Michael D Kinney, Liming Gao, Zhiguang Liu, Jiewen Yao,
Jian J Wang
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3625
DxeTpmMeasurementLib supports TPM based measurement in DXE phase.
After Td protocol is introduced, TD based measurement needs to be
supported in DxeTpmMeasurementLib as well.
In TpmMeasureAndLogData, TD based measurement will be first called.
If it failed, TPM based measurement will be called sequentially.
Currently there is an assumption that TD based measurement and
TPM based measurement won't be exist at the same time.If the
assumption is not true in the future, we will revisit here then.
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>
---
.../DxeTpmMeasurementLib.c | 87 ++++++++++++++++++-
.../DxeTpmMeasurementLib.inf | 1 +
2 files changed, 85 insertions(+), 3 deletions(-)
diff --git a/SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.c b/SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.c
index 061136ee7860..f8cd289ba62c 100644
--- a/SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.c
+++ b/SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.c
@@ -19,7 +19,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#include <Guid/Acpi.h>
#include <IndustryStandard/Acpi.h>
-
+#include <Protocol/TdProtocol.h>
/**
@@ -149,6 +149,73 @@ Tpm20MeasureAndLogData (
return Status;
}
+/**
+ Tdx measure and log data, and extend the measurement result into a
+ specific TDX RTMR.
+
+ @param[in] PcrIndex PCR Index.
+ @param[in] EventType Event type.
+ @param[in] EventLog Measurement event log.
+ @param[in] LogLen Event log length in bytes.
+ @param[in] HashData The start of the data buffer to be hashed, extended.
+ @param[in] HashDataLen The length, in bytes, of the buffer referenced by HashData
+
+ @retval EFI_SUCCESS Operation completed successfully.
+ @retval EFI_UNSUPPORTED Tdx device not available.
+ @retval EFI_OUT_OF_RESOURCES Out of memory.
+ @retval EFI_DEVICE_ERROR The operation was unsuccessful.
+**/
+EFI_STATUS
+EFIAPI
+TdxMeasureAndLogData (
+ IN UINT32 PcrIndex,
+ IN UINT32 EventType,
+ IN VOID *EventLog,
+ IN UINT32 LogLen,
+ IN VOID *HashData,
+ IN UINT64 HashDataLen
+ )
+{
+ EFI_STATUS Status;
+ EFI_TD_PROTOCOL *TdProtocol;
+ EFI_TD_EVENT *TdEvent;
+ UINT32 MrIndex;
+
+ Status = gBS->LocateProtocol (&gEfiTdProtocolGuid, NULL, (VOID **) &TdProtocol);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ Status = TdProtocol->MapPcrToMrIndex (TdProtocol, PcrIndex, &MrIndex);
+ if (EFI_ERROR (Status)) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ TdEvent = (EFI_TD_EVENT *) AllocateZeroPool (LogLen + sizeof (EFI_TD_EVENT));
+ if(TdEvent == NULL) {
+ return EFI_OUT_OF_RESOURCES;
+ }
+
+ TdEvent->Size = (UINT32) LogLen + sizeof (EFI_TD_EVENT) - sizeof (TdEvent->Event);
+ TdEvent->Header.HeaderSize = sizeof (EFI_TD_EVENT_HEADER);
+ TdEvent->Header.HeaderVersion = EFI_TD_EVENT_HEADER_VERSION;
+ TdEvent->Header.MrIndex = MrIndex;
+ TdEvent->Header.EventType = EventType;
+ CopyMem (&TdEvent->Event[0], EventLog, LogLen);
+
+ Status = TdProtocol->HashLogExtendEvent (
+ TdProtocol,
+ 0,
+ (EFI_PHYSICAL_ADDRESS) (UINTN) HashData,
+ HashDataLen,
+ TdEvent
+ );
+ FreePool (TdEvent);
+
+ return Status;
+}
+
+
/**
Tpm measure and log data, and extend the measurement result into a specific PCR.
@@ -178,9 +245,9 @@ TpmMeasureAndLogData (
EFI_STATUS Status;
//
- // Try to measure using Tpm20 protocol
+ // Try to measure using Td protocol
//
- Status = Tpm20MeasureAndLogData(
+ Status = TdxMeasureAndLogData (
PcrIndex,
EventType,
EventLog,
@@ -189,6 +256,20 @@ TpmMeasureAndLogData (
HashDataLen
);
+ if (EFI_ERROR (Status)) {
+ //
+ // Try to measure using Tpm20 protocol
+ //
+ Status = Tpm20MeasureAndLogData(
+ PcrIndex,
+ EventType,
+ EventLog,
+ LogLen,
+ HashData,
+ HashDataLen
+ );
+ }
+
if (EFI_ERROR (Status)) {
//
// Try to measure using Tpm1.2 protocol
diff --git a/SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.inf b/SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.inf
index 7d41bc41f95d..b919771d5a9e 100644
--- a/SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.inf
+++ b/SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.inf
@@ -42,3 +42,4 @@
[Protocols]
gEfiTcgProtocolGuid ## SOMETIMES_CONSUMES
gEfiTcg2ProtocolGuid ## SOMETIMES_CONSUMES
+ gEfiTdProtocolGuid ## SOMETIMES_CONSUMES
--
2.29.2.windows.2
^ permalink raw reply related [flat|nested] 20+ messages in thread
* 回复: [PATCH V2 1/3] MdePkg: Introduce TdProtocol for TD-Guest firmware
2021-10-08 5:21 ` [PATCH V2 1/3] MdePkg: Introduce TdProtocol for TD-Guest firmware Min Xu
@ 2021-10-11 1:37 ` gaoliming
2021-10-19 13:21 ` [edk2-devel] " Sami Mujawar
1 sibling, 0 replies; 20+ messages in thread
From: gaoliming @ 2021-10-11 1:37 UTC (permalink / raw)
To: 'Min Xu', devel
Cc: 'Michael D Kinney', 'Zhiguang Liu',
'Jiewen Yao', 'Jian J Wang', 'Ken Lu'
Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>
> -----邮件原件-----
> 发件人: Min Xu <min.m.xu@intel.com>
> 发送时间: 2021年10月8日 13:21
> 收件人: devel@edk2.groups.io
> 抄送: Min Xu <min.m.xu@intel.com>; Michael D Kinney
> <michael.d.kinney@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>;
> Zhiguang Liu <zhiguang.liu@intel.com>; Jiewen Yao <jiewen.yao@intel.com>;
> Jian J Wang <jian.j.wang@intel.com>; Ken Lu <ken.lu@intel.com>
> 主题: [PATCH V2 1/3] MdePkg: Introduce TdProtocol for TD-Guest firmware
>
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3625
>
> If TD-Guest firmware supports measurement and an event is created,
> TD-Guest firmware is designed to report the event log with the same data
> structure in TCG-Platform-Firmware-Profile specification with
> EFI_TCG2_EVENT_LOG_FORMAT_TCG_2 format.
>
> The TD-Guest firmware supports measurement, the TD Guest Firmware is
> designed to produce EFI_TD_PROTOCOL with new GUID
> EFI_TD_PROTOCOL_GUID
> to report event log and provides hash capability.
>
> https://software.intel.com/content/dam/develop/external/us/en/documents
> /
> intel-tdx-guest-hypervisor-communication-interface-1.0-344426-002.pdf
> Section 4.3.2 includes the EFI_TD_PROTOCOL.
>
> 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>
> Cc: Ken Lu <ken.lu@intel.com>
> Reviewed-by: Jiewen Yao <jiewen.yao@intel.com>
> Signed-off-by: Min Xu <min.m.xu@intel.com>
> ---
> MdePkg/Include/Protocol/TdProtocol.h | 305
> +++++++++++++++++++++++++++
> MdePkg/MdePkg.dec | 3 +
> 2 files changed, 308 insertions(+)
> create mode 100644 MdePkg/Include/Protocol/TdProtocol.h
>
> diff --git a/MdePkg/Include/Protocol/TdProtocol.h
> b/MdePkg/Include/Protocol/TdProtocol.h
> new file mode 100644
> index 000000000000..89b09928d33a
> --- /dev/null
> +++ b/MdePkg/Include/Protocol/TdProtocol.h
> @@ -0,0 +1,305 @@
> +/** @file
> + If TD-Guest firmware supports measurement and an event is created,
> TD-Guest
> + firmware is designed to report the event log with the same data
structure
> + in TCG-Platform-Firmware-Profile specification with
> + EFI_TCG2_EVENT_LOG_FORMAT_TCG_2 format.
> +
> + The TD-Guest firmware supports measurement, the TD Guest Firmware is
> designed
> + to produce EFI_TD_PROTOCOL with new GUID EFI_TD_PROTOCOL_GUID
> to report
> + event log and provides hash capability.
> +
> +Copyright (c) 2020 - 2021, Intel Corporation. All rights reserved.<BR>
> +SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +
> +#ifndef TD_PROTOCOL_H_
> +#define TD_PROTOCOL_H_
> +
> +#include <Uefi/UefiBaseType.h>
> +#include <IndustryStandard/UefiTcgPlatform.h>
> +#include <IndustryStandard/Tpm20.h>
> +
> +
> +#define EFI_TD_PROTOCOL_GUID \
> + { 0x96751a3d, 0x72f4, 0x41a6, { 0xa7, 0x94, 0xed, 0x5d, 0x0e, 0x67,
0xae,
> 0x6b }}
> +extern EFI_GUID gEfiTdProtocolGuid;
> +
> +typedef struct _EFI_TD_PROTOCOL EFI_TD_PROTOCOL;
> +
> +typedef struct {
> + UINT8 Major;
> + UINT8 Minor;
> +} EFI_TD_VERSION;
> +
> +typedef UINT32 EFI_TD_EVENT_LOG_BITMAP;
> +typedef UINT32 EFI_TD_EVENT_LOG_FORMAT;
> +typedef UINT32
> EFI_TD_EVENT_ALGORITHM_BITMAP;
> +typedef UINT32 EFI_TD_MR_INDEX;
> +
> +#define EFI_TD_EVENT_LOG_FORMAT_TCG_2 0x00000002
> +#define EFI_TD_BOOT_HASH_ALG_SHA384 0x00000004
> +
> +//
> +// This bit is shall be set when an event shall be extended but not
logged.
> +//
> +#define EFI_TD_FLAG_EXTEND_ONLY 0x0000000000000001
> +//
> +// This bit shall be set when the intent is to measure a PE/COFF image.
> +//
> +#define EFI_TD_FLAG_PE_COFF_IMAGE 0x0000000000000010
> +
> +#define MR_INDEX_MRTD 0
> +#define MR_INDEX_RTMR0 1
> +#define MR_INDEX_RTMR1 2
> +#define MR_INDEX_RTMR2 3
> +#define MR_INDEX_RTMR3 4
> +
> +//
> +// This bit shall be set when the intent is to measure a PE/COFF image.
> +//
> +#define PE_COFF_IMAGE 0x0000000000000010
> +
> +#pragma pack (1)
> +
> +#define EFI_TD_EVENT_HEADER_VERSION 1
> +
> +typedef struct {
> + //
> + // Size of the event header itself (sizeof(EFI_TD_EVENT_HEADER)).
> + //
> + UINT32 HeaderSize;
> + //
> + // Header version. For this version of this specification, the value
shall be
> 1.
> + //
> + UINT16 HeaderVersion;
> + //
> + // Index of the MR that shall be extended.
> + //
> + EFI_TD_MR_INDEX MrIndex;
> + //
> + // Type of the event that shall be extended (and optionally logged).
> + //
> + UINT32 EventType;
> +} EFI_TD_EVENT_HEADER;
> +
> +typedef struct {
> + //
> + // Total size of the event including the Size component, the header and
the
> Event data.
> + //
> + UINT32 Size;
> + EFI_TD_EVENT_HEADER Header;
> + UINT8 Event[1];
> +} EFI_TD_EVENT;
> +
> +#pragma pack()
> +
> +
> +typedef struct {
> + //
> + // Allocated size of the structure
> + //
> + UINT8 Size;
> + //
> + // Version of the EFI_TD_BOOT_SERVICE_CAPABILITY structure itself.
> + // For this version of the protocol, the Major version shall be set to
1
> + // and the Minor version shall be set to 1.
> + //
> + EFI_TD_VERSION StructureVersion;
> + //
> + // Version of the EFI TD protocol.
> + // For this version of the protocol, the Major version shall be set to
1
> + // and the Minor version shall be set to 1.
> + //
> + EFI_TD_VERSION ProtocolVersion;
> + //
> + // Supported hash algorithms
> + //
> + EFI_TD_EVENT_ALGORITHM_BITMAP HashAlgorithmBitmap;
> + //
> + // Bitmap of supported event log formats
> + //
> + EFI_TD_EVENT_LOG_BITMAP SupportedEventLogs;
> +
> + //
> + // False = TD not present
> + //
> + BOOLEAN TdPresentFlag;
> +} EFI_TD_BOOT_SERVICE_CAPABILITY;
> +
> +/**
> + The EFI_TD_PROTOCOL GetCapability function call provides protocol
> + capability information and state information.
> +
> + @param[in] This Indicates the calling context
> + @param[in, out] ProtocolCapability The caller allocates memory for a
> EFI_TD_BOOT_SERVICE_CAPABILITY
> + structure and sets the size
> field to the size of the structure allocated.
> + The callee fills in the fields
> with the EFI protocol capability information
> + and the current EFI TD state
> information up to the number of fields which
> + fit within the size of the
> structure passed in.
> +
> + @retval EFI_SUCCESS Operation completed successfully.
> + @retval EFI_DEVICE_ERROR The command was unsuccessful.
> + The ProtocolCapability variable will
> not be populated.
> + @retval EFI_INVALID_PARAMETER One or more of the parameters are
> incorrect.
> + The ProtocolCapability variable will
> not be populated.
> + @retval EFI_BUFFER_TOO_SMALL The ProtocolCapability variable is
> too small to hold the full response.
> + It will be partially populated
> (required Size field will be set).
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI *EFI_TD_GET_CAPABILITY) (
> + IN EFI_TD_PROTOCOL *This,
> + IN OUT EFI_TD_BOOT_SERVICE_CAPABILITY *ProtocolCapability
> + );
> +
> +/**
> + The EFI_TD_PROTOCOL Get Event Log function call allows a caller to
> + retrieve the address of a given event log and its last entry.
> +
> + @param[in] This Indicates the calling context
> + @param[in] EventLogFormat The type of the event log for which
> the information is requested.
> + @param[out] EventLogLocation A pointer to the memory address of
> the event log.
> + @param[out] EventLogLastEntry If the Event Log contains more than
> one entry, this is a pointer to the
> + address of the start of the last
> entry in the event log in memory.
> + @param[out] EventLogTruncated If the Event Log is missing at least one
> entry because an event would
> + have exceeded the area allocated
> for events, this value is set to TRUE.
> + Otherwise, the value will be FALSE
> and the Event Log will be complete.
> +
> + @retval EFI_SUCCESS Operation completed successfully.
> + @retval EFI_INVALID_PARAMETER One or more of the parameters are
> incorrect
> + (e.g. asking for an event log whose
> format is not supported).
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI *EFI_TD_GET_EVENT_LOG) (
> + IN EFI_TD_PROTOCOL *This,
> + IN EFI_TD_EVENT_LOG_FORMAT EventLogFormat,
> + OUT EFI_PHYSICAL_ADDRESS *EventLogLocation,
> + OUT EFI_PHYSICAL_ADDRESS *EventLogLastEntry,
> + OUT BOOLEAN *EventLogTruncated
> + );
> +
> +/**
> + The EFI_TD_PROTOCOL HashLogExtendEvent function call provides callers
> with
> + an opportunity to extend and optionally log events without requiring
> + knowledge of actual TD commands.
> + The extend operation will occur even if this function cannot create an
> event
> + log entry (e.g. due to the event log being full).
> +
> + @param[in] This Indicates the calling context
> + @param[in] Flags Bitmap providing additional
> information.
> + @param[in] DataToHash Physical address of the start of the
> data buffer to be hashed.
> + @param[in] DataToHashLen The length in bytes of the buffer
> referenced by DataToHash.
> + @param[in] EfiTdEvent Pointer to data buffer containing
> information about the event.
> +
> + @retval EFI_SUCCESS Operation completed successfully.
> + @retval EFI_DEVICE_ERROR The command was unsuccessful.
> + @retval EFI_VOLUME_FULL The extend operation occurred, but
> the event could not be written to one or more event logs.
> + @retval EFI_INVALID_PARAMETER One or more of the parameters are
> incorrect.
> + @retval EFI_UNSUPPORTED The PE/COFF image type is not
> supported.
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI * EFI_TD_HASH_LOG_EXTEND_EVENT) (
> + IN EFI_TD_PROTOCOL *This,
> + IN UINT64 Flags,
> + IN EFI_PHYSICAL_ADDRESS DataToHash,
> + IN UINT64 DataToHashLen,
> + IN EFI_TD_EVENT *EfiTdEvent
> + );
> +
> +/**
> + The EFI_TD_PROTOCOL MapPcrToMrIndex function call provides callers
> + the info on TPM PCR<-> measurement register mapping information.
> +
> + In current version, we use below mapping:
> + PCR0 -> MRTD (Index 0)
> + PCR1 -> RTMR0 (Index 1)
> + PCR2~6 -> RTMR1 (Index 2)
> + PCR7 -> RTMR0 (Index 1)
> + PCR8~15 -> RTMR2 (Index 3)
> +
> + @param[in] This Indicates the calling context
> + @param[in] PcrIndex TPM PCR index.
> + @param[out] MrIndex Measurement register index.
> +
> + @retval EFI_SUCCESS The MR index is returned.
> + @retval EFI_INVALID_PARAMETER The MrIndex is NULL.
> + @retval EFI_UNSUPPORTED The PcrIndex is invalid.
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI * EFI_TD_MAP_PCR_TO_MR_INDEX) (
> + IN EFI_TD_PROTOCOL *This,
> + IN TCG_PCRINDEX PcrIndex,
> + OUT EFI_TD_MR_INDEX *MrIndex
> + );
> +
> +struct _EFI_TD_PROTOCOL {
> + EFI_TD_GET_CAPABILITY GetCapability;
> + EFI_TD_GET_EVENT_LOG GetEventLog;
> + EFI_TD_HASH_LOG_EXTEND_EVENT
> HashLogExtendEvent;
> + EFI_TD_MAP_PCR_TO_MR_INDEX MapPcrToMrIndex;
> +};
> +
> +
> +//
> +// TD event log
> +//
> +
> +#pragma pack(1)
> +
> +//
> +// Crypto Agile Log Entry Format.
> +// It is similar with TCG_PCR_EVENT2 except the field of MrIndex and
> PCRIndex.
> +//
> +typedef struct {
> + EFI_TD_MR_INDEX MrIndex;
> + UINT32 EventType;
> + TPML_DIGEST_VALUES Digests;
> + UINT32 EventSize;
> + UINT8 Event[1];
> +} TD_EVENT;
> +
> +//
> +// EFI TD Event Header
> +// It is similar with TCG_PCR_EVENT2_HDR except the field of MrIndex and
> PCRIndex
> +//
> +typedef struct {
> + EFI_TD_MR_INDEX MrIndex;
> + UINT32 EventType;
> + TPML_DIGEST_VALUES Digests;
> + UINT32 EventSize;
> +} TD_EVENT_HDR;
> +
> +#pragma pack()
> +
> +//
> +// Log entries after Get Event Log service
> +//
> +
> +
> +typedef struct {
> + //
> + // The version of this structure. It shall be set ot 1.
> + //
> + UINT64 Version;
> + //
> + // Number of events recorded after invocation of GetEventLog API
> + //
> + UINT64 NumberOfEvents;
> + //
> + // List of events of type TD_EVENT.
> + //
> + //TD_EVENT Event[1];
> +} EFI_TD_FINAL_EVENTS_TABLE;
> +
> +
> +#define EFI_TD_FINAL_EVENTS_TABLE_GUID \
> + {0xdd4a4648, 0x2de7, 0x4665, {0x96, 0x4d, 0x21, 0xd9, 0xef, 0x5f, 0xb4,
> 0x46}}
> +
> +extern EFI_GUID gEfiTdFinalEventsTableGuid;
> +
> +#endif
> diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
> index 9cdc915ebae9..a31c44d3a689 100644
> --- a/MdePkg/MdePkg.dec
> +++ b/MdePkg/MdePkg.dec
> @@ -1011,6 +1011,9 @@
> ## Include/Protocol/PcdInfo.h
> gGetPcdInfoProtocolGuid = { 0x5be40f57, 0xfa68, 0x4610, { 0xbb,
> 0xbf, 0xe9, 0xc5, 0xfc, 0xda, 0xd3, 0x65 } }
>
> + ## Include/Protocol/TdProtocol.h
> + gEfiTdProtocolGuid = { 0x96751a3d, 0x72f4, 0x41a6, { 0xa7,
> 0x94, 0xed, 0x5d, 0x0e, 0x67, 0xae, 0x6b }}
> +
> #
> # Protocols defined in PI1.0.
> #
> --
> 2.29.2.windows.2
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [edk2-devel] [PATCH V2 0/3] Introduce TdProtocol into EDK2
2021-10-08 5:21 [PATCH V2 0/3] Introduce TdProtocol into EDK2 Min Xu
` (2 preceding siblings ...)
2021-10-08 5:21 ` [PATCH V2 3/3] SecurityPkg: Support TdProtocol in DxeTpmMeasurementLib Min Xu
@ 2021-10-12 15:26 ` Sami Mujawar
2021-10-14 5:41 ` Min Xu
3 siblings, 1 reply; 20+ messages in thread
From: Sami Mujawar @ 2021-10-12 15:26 UTC (permalink / raw)
To: devel@edk2.groups.io, min.m.xu@intel.com
Cc: Michael D Kinney, Liming Gao, Zhiguang Liu, Jiewen Yao,
Jian J Wang, Ken Lu, nd
Hi Min,
Thank you for this patch.
I think it would greatly help if the EFI_TD_PROTOCOL is changed to something more architecture neutral. As I understand, this patch series is removing the dependency on TPM for measurement and is instead providing a lightweight interface for extending measurements for Confidential Compute Architecture (CCA) guests.
Considering this, it would be good to generalise EFI_TD_PROTOCOL as a Confidential Compute Architecture Measurement (CCAM) protocol.
In fact, your v2 series demonstrates this need with the introduction of MEASURE_BOOT_PROTOCOLS in "[PATCH V2 2/3] SecurityPkg: Support TdProtocol in DxeTpm2MeasureBootLib [https://edk2.groups.io/g/devel/message/81651]".
As it stands, I feel most of the code can be reused/common. Some interfaces may need to use an architecture specific library, and some configuration options would need to be defined using PCDs.
Kindly let me know your thoughts.
Regards,
Sami Mujawar
On 08/10/2021, 06:24, "devel@edk2.groups.io on behalf of Min Xu via groups.io" <devel@edk2.groups.io on behalf of min.m.xu=intel.com@groups.io> wrote:
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3625
If TD-Guest firmware supports measurement and an event is created,
TD-Guest firmware is designed to report the event log with the same data
structure in TCG-Platform-Firmware-Profile specification with
EFI_TCG2_EVENT_LOG_FORMAT_TCG_2 format.
The TD-Guest firmware supports measurement, the TD Guest Firmware is
designed to produce EFI_TD_PROTOCOL with new GUID EFI_TD_PROTOCOL_GUID
to report event log and provides hash capability.
https://software.intel.com/content/dam/develop/external/us/en/documents/
intel-tdx-guest-hypervisor-communication-interface-1.0-344426-002.pdf
Section 4.3.2 includes the EFI_TD_PROTOCOL.
Patch #1:
Introduce the TD Protocol definition into MdePkg
Patch #2:
Update DxeTpm2MeasureBootLib to support TD based measure boot.
Patch #3:
Update DxeTpmMeasurementLib to support TD based measurement.
Code is at https://github.com/mxu9/edk2/tree/td_protocol.v2
v2 changes:
- TD based measure boot is implemented in DxeTpm2MeasureBootLib.
This minimize the code changes.
- TD based measurement is added. It is implemented in
DxeTpmMeasurementLib.
- Fix the typo in comments.
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>
Cc: Ken Lu <ken.lu@intel.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
Min Xu (3):
MdePkg: Introduce TdProtocol for TD-Guest firmware
SecurityPkg: Support TdProtocol in DxeTpm2MeasureBootLib
SecurityPkg: Support TdProtocol in DxeTpmMeasurementLib
MdePkg/Include/Protocol/TdProtocol.h | 305 +++++++++++++++
MdePkg/MdePkg.dec | 3 +
.../DxeTpm2MeasureBootLib.c | 346 ++++++++++++++----
.../DxeTpm2MeasureBootLib.inf | 1 +
.../DxeTpmMeasurementLib.c | 87 ++++-
.../DxeTpmMeasurementLib.inf | 1 +
6 files changed, 672 insertions(+), 71 deletions(-)
create mode 100644 MdePkg/Include/Protocol/TdProtocol.h
--
2.29.2.windows.2
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [edk2-devel] [PATCH V2 0/3] Introduce TdProtocol into EDK2
2021-10-12 15:26 ` [edk2-devel] [PATCH V2 0/3] Introduce TdProtocol into EDK2 Sami Mujawar
@ 2021-10-14 5:41 ` Min Xu
2021-10-14 11:59 ` Yao, Jiewen
[not found] ` <16ADE3D948B3147A.7007@groups.io>
0 siblings, 2 replies; 20+ messages in thread
From: Min Xu @ 2021-10-14 5:41 UTC (permalink / raw)
To: Sami Mujawar, devel@edk2.groups.io, Yao, Jiewen
Cc: Kinney, Michael D, Liming Gao, Liu, Zhiguang, Wang, Jian J,
Lu, Ken, nd
On October 12, 2021 11:27 PM, Sami Mujawar wrote:
> Hi Min,
>
> Thank you for this patch.
>
> I think it would greatly help if the EFI_TD_PROTOCOL is changed to something
> more architecture neutral. As I understand, this patch series is removing the
> dependency on TPM for measurement and is instead providing a lightweight
> interface for extending measurements for Confidential Compute Architecture
> (CCA) guests.
>
> Considering this, it would be good to generalise EFI_TD_PROTOCOL as a
> Confidential Compute Architecture Measurement (CCAM) protocol.
> In fact, your v2 series demonstrates this need with the introduction of
> MEASURE_BOOT_PROTOCOLS in "[PATCH V2 2/3] SecurityPkg: Support
> TdProtocol in DxeTpm2MeasureBootLib
> [https://edk2.groups.io/g/devel/message/81651]".
>
> As it stands, I feel most of the code can be reused/common. Some interfaces
> may need to use an architecture specific library, and some configuration
> options would need to be defined using PCDs.
>
> Kindly let me know your thoughts.
>
Thanks for your comments. Let me first discuss your feedback with our architecture. We will reply to your proposal a bit later.
Thanks.
Min
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [edk2-devel] [PATCH V2 0/3] Introduce TdProtocol into EDK2
2021-10-14 5:41 ` Min Xu
@ 2021-10-14 11:59 ` Yao, Jiewen
[not found] ` <16ADE3D948B3147A.7007@groups.io>
1 sibling, 0 replies; 20+ messages in thread
From: Yao, Jiewen @ 2021-10-14 11:59 UTC (permalink / raw)
To: Xu, Min M, Sami Mujawar, devel@edk2.groups.io
Cc: Kinney, Michael D, Liming Gao, Liu, Zhiguang, Wang, Jian J,
Lu, Ken, nd
Hi Sami
I am not sure if I can understand your comment -
"Some interfaces may need to use an architecture specific library, and some configuration options would need to be defined using PCDs."
Would you please be more specific?
Thank you
Yao Jiewen
> -----Original Message-----
> From: Xu, Min M <min.m.xu@intel.com>
> Sent: Thursday, October 14, 2021 1:41 PM
> To: Sami Mujawar <Sami.Mujawar@arm.com>; devel@edk2.groups.io; Yao,
> Jiewen <jiewen.yao@intel.com>
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Liming Gao
> <gaoliming@byosoft.com.cn>; Liu, Zhiguang <zhiguang.liu@intel.com>; Wang,
> Jian J <jian.j.wang@intel.com>; Lu, Ken <ken.lu@intel.com>; nd <nd@arm.com>
> Subject: RE: [edk2-devel] [PATCH V2 0/3] Introduce TdProtocol into EDK2
>
> On October 12, 2021 11:27 PM, Sami Mujawar wrote:
> > Hi Min,
> >
> > Thank you for this patch.
> >
> > I think it would greatly help if the EFI_TD_PROTOCOL is changed to something
> > more architecture neutral. As I understand, this patch series is removing the
> > dependency on TPM for measurement and is instead providing a lightweight
> > interface for extending measurements for Confidential Compute Architecture
> > (CCA) guests.
> >
> > Considering this, it would be good to generalise EFI_TD_PROTOCOL as a
> > Confidential Compute Architecture Measurement (CCAM) protocol.
> > In fact, your v2 series demonstrates this need with the introduction of
> > MEASURE_BOOT_PROTOCOLS in "[PATCH V2 2/3] SecurityPkg: Support
> > TdProtocol in DxeTpm2MeasureBootLib
> > [https://edk2.groups.io/g/devel/message/81651]".
> >
> > As it stands, I feel most of the code can be reused/common. Some interfaces
> > may need to use an architecture specific library, and some configuration
> > options would need to be defined using PCDs.
> >
> > Kindly let me know your thoughts.
> >
> Thanks for your comments. Let me first discuss your feedback with our
> architecture. We will reply to your proposal a bit later.
>
> Thanks.
> Min
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [edk2-devel] [PATCH V2 0/3] Introduce TdProtocol into EDK2
[not found] ` <16ADE3D948B3147A.7007@groups.io>
@ 2021-10-14 13:43 ` Yao, Jiewen
2021-10-18 12:59 ` Sami Mujawar
0 siblings, 1 reply; 20+ messages in thread
From: Yao, Jiewen @ 2021-10-14 13:43 UTC (permalink / raw)
To: devel@edk2.groups.io, Yao, Jiewen, Xu, Min M, Sami Mujawar
Cc: Kinney, Michael D, Liming Gao, Liu, Zhiguang, Wang, Jian J,
Lu, Ken, nd
Hi Sami
To clarify my description:
I am OK to define it in an architecture neutral protocol, such as EFI_TEE_MEASUREMENT_PROTOCOL, or EFI_CCAM_PROTOCOL. I am happy to do that.
However, at current point of time, I am not sure how other arch supports those feature, such as
AMD SEV (https://www.amd.com/system/files/TechDocs/56860.pdf), or ARM Realm (https://developer.arm.com/documentation/ddi0615/latest/). I did not find runtime measurement there.
I hope SEV/Realm people to propose what interface change is required. I am happy to discuss the solution here.
Thank you
Yao Jiewen
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Yao, Jiewen
> Sent: Thursday, October 14, 2021 7:59 PM
> To: Xu, Min M <min.m.xu@intel.com>; Sami Mujawar
> <Sami.Mujawar@arm.com>; devel@edk2.groups.io
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Liming Gao
> <gaoliming@byosoft.com.cn>; Liu, Zhiguang <zhiguang.liu@intel.com>; Wang,
> Jian J <jian.j.wang@intel.com>; Lu, Ken <ken.lu@intel.com>; nd <nd@arm.com>
> Subject: Re: [edk2-devel] [PATCH V2 0/3] Introduce TdProtocol into EDK2
>
> Hi Sami
> I am not sure if I can understand your comment -
> "Some interfaces may need to use an architecture specific library, and some
> configuration options would need to be defined using PCDs."
>
> Would you please be more specific?
>
> Thank you
> Yao Jiewen
>
>
> > -----Original Message-----
> > From: Xu, Min M <min.m.xu@intel.com>
> > Sent: Thursday, October 14, 2021 1:41 PM
> > To: Sami Mujawar <Sami.Mujawar@arm.com>; devel@edk2.groups.io; Yao,
> > Jiewen <jiewen.yao@intel.com>
> > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Liming Gao
> > <gaoliming@byosoft.com.cn>; Liu, Zhiguang <zhiguang.liu@intel.com>; Wang,
> > Jian J <jian.j.wang@intel.com>; Lu, Ken <ken.lu@intel.com>; nd
> <nd@arm.com>
> > Subject: RE: [edk2-devel] [PATCH V2 0/3] Introduce TdProtocol into EDK2
> >
> > On October 12, 2021 11:27 PM, Sami Mujawar wrote:
> > > Hi Min,
> > >
> > > Thank you for this patch.
> > >
> > > I think it would greatly help if the EFI_TD_PROTOCOL is changed to
> something
> > > more architecture neutral. As I understand, this patch series is removing the
> > > dependency on TPM for measurement and is instead providing a lightweight
> > > interface for extending measurements for Confidential Compute
> Architecture
> > > (CCA) guests.
> > >
> > > Considering this, it would be good to generalise EFI_TD_PROTOCOL as a
> > > Confidential Compute Architecture Measurement (CCAM) protocol.
> > > In fact, your v2 series demonstrates this need with the introduction of
> > > MEASURE_BOOT_PROTOCOLS in "[PATCH V2 2/3] SecurityPkg: Support
> > > TdProtocol in DxeTpm2MeasureBootLib
> > > [https://edk2.groups.io/g/devel/message/81651]".
> > >
> > > As it stands, I feel most of the code can be reused/common. Some
> interfaces
> > > may need to use an architecture specific library, and some configuration
> > > options would need to be defined using PCDs.
> > >
> > > Kindly let me know your thoughts.
> > >
> > Thanks for your comments. Let me first discuss your feedback with our
> > architecture. We will reply to your proposal a bit later.
> >
> > Thanks.
> > Min
>
>
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [edk2-devel] [PATCH V2 0/3] Introduce TdProtocol into EDK2
2021-10-14 13:43 ` Yao, Jiewen
@ 2021-10-18 12:59 ` Sami Mujawar
2021-10-18 13:06 ` Yao, Jiewen
0 siblings, 1 reply; 20+ messages in thread
From: Sami Mujawar @ 2021-10-18 12:59 UTC (permalink / raw)
To: Yao, Jiewen, devel@edk2.groups.io, Xu, Min M
Cc: Kinney, Michael D, Liming Gao, Liu, Zhiguang, Wang, Jian J,
Lu, Ken, nd
Hi Jiewen,
We don't have publicly available documentation to share in this area, however we strongly prefer an architecture agnostic solution, and my initial suggestions takes us in that direction. I am happy to work on mailing list with you to make these changes and review necessary code changes. Does that work for you?
Regards,
Sami Mujawar
On 14/10/2021, 17:01, "Yao, Jiewen" <jiewen.yao@intel.com> wrote:
Hi Sami
To clarify my description:
I am OK to define it in an architecture neutral protocol, such as EFI_TEE_MEASUREMENT_PROTOCOL, or EFI_CCAM_PROTOCOL. I am happy to do that.
However, at current point of time, I am not sure how other arch supports those feature, such as
AMD SEV (https://www.amd.com/system/files/TechDocs/56860.pdf), or ARM Realm (https://developer.arm.com/documentation/ddi0615/latest/). I did not find runtime measurement there.
I hope SEV/Realm people to propose what interface change is required. I am happy to discuss the solution here.
Thank you
Yao Jiewen
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Yao, Jiewen
> Sent: Thursday, October 14, 2021 7:59 PM
> To: Xu, Min M <min.m.xu@intel.com>; Sami Mujawar
> <Sami.Mujawar@arm.com>; devel@edk2.groups.io
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Liming Gao
> <gaoliming@byosoft.com.cn>; Liu, Zhiguang <zhiguang.liu@intel.com>; Wang,
> Jian J <jian.j.wang@intel.com>; Lu, Ken <ken.lu@intel.com>; nd <nd@arm.com>
> Subject: Re: [edk2-devel] [PATCH V2 0/3] Introduce TdProtocol into EDK2
>
> Hi Sami
> I am not sure if I can understand your comment -
> "Some interfaces may need to use an architecture specific library, and some
> configuration options would need to be defined using PCDs."
>
> Would you please be more specific?
>
> Thank you
> Yao Jiewen
>
>
> > -----Original Message-----
> > From: Xu, Min M <min.m.xu@intel.com>
> > Sent: Thursday, October 14, 2021 1:41 PM
> > To: Sami Mujawar <Sami.Mujawar@arm.com>; devel@edk2.groups.io; Yao,
> > Jiewen <jiewen.yao@intel.com>
> > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Liming Gao
> > <gaoliming@byosoft.com.cn>; Liu, Zhiguang <zhiguang.liu@intel.com>; Wang,
> > Jian J <jian.j.wang@intel.com>; Lu, Ken <ken.lu@intel.com>; nd
> <nd@arm.com>
> > Subject: RE: [edk2-devel] [PATCH V2 0/3] Introduce TdProtocol into EDK2
> >
> > On October 12, 2021 11:27 PM, Sami Mujawar wrote:
> > > Hi Min,
> > >
> > > Thank you for this patch.
> > >
> > > I think it would greatly help if the EFI_TD_PROTOCOL is changed to
> something
> > > more architecture neutral. As I understand, this patch series is removing the
> > > dependency on TPM for measurement and is instead providing a lightweight
> > > interface for extending measurements for Confidential Compute
> Architecture
> > > (CCA) guests.
> > >
> > > Considering this, it would be good to generalise EFI_TD_PROTOCOL as a
> > > Confidential Compute Architecture Measurement (CCAM) protocol.
> > > In fact, your v2 series demonstrates this need with the introduction of
> > > MEASURE_BOOT_PROTOCOLS in "[PATCH V2 2/3] SecurityPkg: Support
> > > TdProtocol in DxeTpm2MeasureBootLib
> > > [https://edk2.groups.io/g/devel/message/81651]".
> > >
> > > As it stands, I feel most of the code can be reused/common. Some
> interfaces
> > > may need to use an architecture specific library, and some configuration
> > > options would need to be defined using PCDs.
> > >
> > > Kindly let me know your thoughts.
> > >
> > Thanks for your comments. Let me first discuss your feedback with our
> > architecture. We will reply to your proposal a bit later.
> >
> > Thanks.
> > Min
>
>
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [edk2-devel] [PATCH V2 0/3] Introduce TdProtocol into EDK2
2021-10-18 12:59 ` Sami Mujawar
@ 2021-10-18 13:06 ` Yao, Jiewen
2021-10-19 9:51 ` Sami Mujawar
0 siblings, 1 reply; 20+ messages in thread
From: Yao, Jiewen @ 2021-10-18 13:06 UTC (permalink / raw)
To: Sami Mujawar, devel@edk2.groups.io, Xu, Min M
Cc: Kinney, Michael D, Liming Gao, Liu, Zhiguang, Wang, Jian J,
Lu, Ken, nd
Sure. It works fine.
May I know *when* I can get your initial feedback ?
I hope in days or 1 or 2 weeks.
I don’t want to wait for several months, as we have features depend upon it.
Thank you
Yao Jiewen
> -----Original Message-----
> From: Sami Mujawar <Sami.Mujawar@arm.com>
> Sent: Monday, October 18, 2021 9:00 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io; Xu, Min M
> <min.m.xu@intel.com>
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Liming Gao
> <gaoliming@byosoft.com.cn>; Liu, Zhiguang <zhiguang.liu@intel.com>; Wang,
> Jian J <jian.j.wang@intel.com>; Lu, Ken <ken.lu@intel.com>; nd <nd@arm.com>
> Subject: Re: [edk2-devel] [PATCH V2 0/3] Introduce TdProtocol into EDK2
>
> Hi Jiewen,
>
> We don't have publicly available documentation to share in this area, however
> we strongly prefer an architecture agnostic solution, and my initial suggestions
> takes us in that direction. I am happy to work on mailing list with you to make
> these changes and review necessary code changes. Does that work for you?
>
> Regards,
>
> Sami Mujawar
>
> On 14/10/2021, 17:01, "Yao, Jiewen" <jiewen.yao@intel.com> wrote:
>
> Hi Sami
> To clarify my description:
> I am OK to define it in an architecture neutral protocol, such as
> EFI_TEE_MEASUREMENT_PROTOCOL, or EFI_CCAM_PROTOCOL. I am happy to
> do that.
>
> However, at current point of time, I am not sure how other arch supports
> those feature, such as
> AMD SEV (https://www.amd.com/system/files/TechDocs/56860.pdf), or ARM
> Realm (https://developer.arm.com/documentation/ddi0615/latest/). I did not
> find runtime measurement there.
>
> I hope SEV/Realm people to propose what interface change is required. I am
> happy to discuss the solution here.
>
> Thank you
> Yao Jiewen
>
>
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Yao,
> Jiewen
> > Sent: Thursday, October 14, 2021 7:59 PM
> > To: Xu, Min M <min.m.xu@intel.com>; Sami Mujawar
> > <Sami.Mujawar@arm.com>; devel@edk2.groups.io
> > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Liming Gao
> > <gaoliming@byosoft.com.cn>; Liu, Zhiguang <zhiguang.liu@intel.com>;
> Wang,
> > Jian J <jian.j.wang@intel.com>; Lu, Ken <ken.lu@intel.com>; nd
> <nd@arm.com>
> > Subject: Re: [edk2-devel] [PATCH V2 0/3] Introduce TdProtocol into EDK2
> >
> > Hi Sami
> > I am not sure if I can understand your comment -
> > "Some interfaces may need to use an architecture specific library, and some
> > configuration options would need to be defined using PCDs."
> >
> > Would you please be more specific?
> >
> > Thank you
> > Yao Jiewen
> >
> >
> > > -----Original Message-----
> > > From: Xu, Min M <min.m.xu@intel.com>
> > > Sent: Thursday, October 14, 2021 1:41 PM
> > > To: Sami Mujawar <Sami.Mujawar@arm.com>; devel@edk2.groups.io;
> Yao,
> > > Jiewen <jiewen.yao@intel.com>
> > > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Liming Gao
> > > <gaoliming@byosoft.com.cn>; Liu, Zhiguang <zhiguang.liu@intel.com>;
> Wang,
> > > Jian J <jian.j.wang@intel.com>; Lu, Ken <ken.lu@intel.com>; nd
> > <nd@arm.com>
> > > Subject: RE: [edk2-devel] [PATCH V2 0/3] Introduce TdProtocol into EDK2
> > >
> > > On October 12, 2021 11:27 PM, Sami Mujawar wrote:
> > > > Hi Min,
> > > >
> > > > Thank you for this patch.
> > > >
> > > > I think it would greatly help if the EFI_TD_PROTOCOL is changed to
> > something
> > > > more architecture neutral. As I understand, this patch series is removing
> the
> > > > dependency on TPM for measurement and is instead providing a
> lightweight
> > > > interface for extending measurements for Confidential Compute
> > Architecture
> > > > (CCA) guests.
> > > >
> > > > Considering this, it would be good to generalise EFI_TD_PROTOCOL as a
> > > > Confidential Compute Architecture Measurement (CCAM) protocol.
> > > > In fact, your v2 series demonstrates this need with the introduction of
> > > > MEASURE_BOOT_PROTOCOLS in "[PATCH V2 2/3] SecurityPkg: Support
> > > > TdProtocol in DxeTpm2MeasureBootLib
> > > > [https://edk2.groups.io/g/devel/message/81651]".
> > > >
> > > > As it stands, I feel most of the code can be reused/common. Some
> > interfaces
> > > > may need to use an architecture specific library, and some configuration
> > > > options would need to be defined using PCDs.
> > > >
> > > > Kindly let me know your thoughts.
> > > >
> > > Thanks for your comments. Let me first discuss your feedback with our
> > > architecture. We will reply to your proposal a bit later.
> > >
> > > Thanks.
> > > Min
> >
> >
> >
> >
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [edk2-devel] [PATCH V2 0/3] Introduce TdProtocol into EDK2
2021-10-18 13:06 ` Yao, Jiewen
@ 2021-10-19 9:51 ` Sami Mujawar
2021-10-19 13:06 ` Min Xu
0 siblings, 1 reply; 20+ messages in thread
From: Sami Mujawar @ 2021-10-19 9:51 UTC (permalink / raw)
To: Yao, Jiewen, devel@edk2.groups.io, Xu, Min M
Cc: Kinney, Michael D, Liming Gao, Liu, Zhiguang, Wang, Jian J,
Lu, Ken, nd
Hi Jiewen,
I will start providing the feedback for this series starting today.
I may need some help to understand the sequence of the various patch series that enable this feature and would be grateful if you could point me to a Github branch that I can refer.
Regards,
Sami Mujawar
On 18/10/2021, 14:06, "Yao, Jiewen" <jiewen.yao@intel.com> wrote:
Sure. It works fine.
May I know *when* I can get your initial feedback ?
I hope in days or 1 or 2 weeks.
I don’t want to wait for several months, as we have features depend upon it.
Thank you
Yao Jiewen
> -----Original Message-----
> From: Sami Mujawar <Sami.Mujawar@arm.com>
> Sent: Monday, October 18, 2021 9:00 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io; Xu, Min M
> <min.m.xu@intel.com>
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Liming Gao
> <gaoliming@byosoft.com.cn>; Liu, Zhiguang <zhiguang.liu@intel.com>; Wang,
> Jian J <jian.j.wang@intel.com>; Lu, Ken <ken.lu@intel.com>; nd <nd@arm.com>
> Subject: Re: [edk2-devel] [PATCH V2 0/3] Introduce TdProtocol into EDK2
>
> Hi Jiewen,
>
> We don't have publicly available documentation to share in this area, however
> we strongly prefer an architecture agnostic solution, and my initial suggestions
> takes us in that direction. I am happy to work on mailing list with you to make
> these changes and review necessary code changes. Does that work for you?
>
> Regards,
>
> Sami Mujawar
>
> On 14/10/2021, 17:01, "Yao, Jiewen" <jiewen.yao@intel.com> wrote:
>
> Hi Sami
> To clarify my description:
> I am OK to define it in an architecture neutral protocol, such as
> EFI_TEE_MEASUREMENT_PROTOCOL, or EFI_CCAM_PROTOCOL. I am happy to
> do that.
>
> However, at current point of time, I am not sure how other arch supports
> those feature, such as
> AMD SEV (https://www.amd.com/system/files/TechDocs/56860.pdf), or ARM
> Realm (https://developer.arm.com/documentation/ddi0615/latest/). I did not
> find runtime measurement there.
>
> I hope SEV/Realm people to propose what interface change is required. I am
> happy to discuss the solution here.
>
> Thank you
> Yao Jiewen
>
>
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Yao,
> Jiewen
> > Sent: Thursday, October 14, 2021 7:59 PM
> > To: Xu, Min M <min.m.xu@intel.com>; Sami Mujawar
> > <Sami.Mujawar@arm.com>; devel@edk2.groups.io
> > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Liming Gao
> > <gaoliming@byosoft.com.cn>; Liu, Zhiguang <zhiguang.liu@intel.com>;
> Wang,
> > Jian J <jian.j.wang@intel.com>; Lu, Ken <ken.lu@intel.com>; nd
> <nd@arm.com>
> > Subject: Re: [edk2-devel] [PATCH V2 0/3] Introduce TdProtocol into EDK2
> >
> > Hi Sami
> > I am not sure if I can understand your comment -
> > "Some interfaces may need to use an architecture specific library, and some
> > configuration options would need to be defined using PCDs."
> >
> > Would you please be more specific?
> >
> > Thank you
> > Yao Jiewen
> >
> >
> > > -----Original Message-----
> > > From: Xu, Min M <min.m.xu@intel.com>
> > > Sent: Thursday, October 14, 2021 1:41 PM
> > > To: Sami Mujawar <Sami.Mujawar@arm.com>; devel@edk2.groups.io;
> Yao,
> > > Jiewen <jiewen.yao@intel.com>
> > > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Liming Gao
> > > <gaoliming@byosoft.com.cn>; Liu, Zhiguang <zhiguang.liu@intel.com>;
> Wang,
> > > Jian J <jian.j.wang@intel.com>; Lu, Ken <ken.lu@intel.com>; nd
> > <nd@arm.com>
> > > Subject: RE: [edk2-devel] [PATCH V2 0/3] Introduce TdProtocol into EDK2
> > >
> > > On October 12, 2021 11:27 PM, Sami Mujawar wrote:
> > > > Hi Min,
> > > >
> > > > Thank you for this patch.
> > > >
> > > > I think it would greatly help if the EFI_TD_PROTOCOL is changed to
> > something
> > > > more architecture neutral. As I understand, this patch series is removing
> the
> > > > dependency on TPM for measurement and is instead providing a
> lightweight
> > > > interface for extending measurements for Confidential Compute
> > Architecture
> > > > (CCA) guests.
> > > >
> > > > Considering this, it would be good to generalise EFI_TD_PROTOCOL as a
> > > > Confidential Compute Architecture Measurement (CCAM) protocol.
> > > > In fact, your v2 series demonstrates this need with the introduction of
> > > > MEASURE_BOOT_PROTOCOLS in "[PATCH V2 2/3] SecurityPkg: Support
> > > > TdProtocol in DxeTpm2MeasureBootLib
> > > > [https://edk2.groups.io/g/devel/message/81651]".
> > > >
> > > > As it stands, I feel most of the code can be reused/common. Some
> > interfaces
> > > > may need to use an architecture specific library, and some configuration
> > > > options would need to be defined using PCDs.
> > > >
> > > > Kindly let me know your thoughts.
> > > >
> > > Thanks for your comments. Let me first discuss your feedback with our
> > > architecture. We will reply to your proposal a bit later.
> > >
> > > Thanks.
> > > Min
> >
> >
> >
> >
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [edk2-devel] [PATCH V2 0/3] Introduce TdProtocol into EDK2
2021-10-19 9:51 ` Sami Mujawar
@ 2021-10-19 13:06 ` Min Xu
0 siblings, 0 replies; 20+ messages in thread
From: Min Xu @ 2021-10-19 13:06 UTC (permalink / raw)
To: Sami Mujawar, Yao, Jiewen, devel@edk2.groups.io
Cc: Kinney, Michael D, Liming Gao, Liu, Zhiguang, Wang, Jian J,
Lu, Ken, nd
On October 19, 2021 5:52 PM, Sami Mujawar wrote:
> I will start providing the feedback for this series starting today.
> I may need some help to understand the sequence of the various patch
> series that enable this feature and would be grateful if you could point me to
> a Github branch that I can refer.
Hi, Sami
The code of this patch-set is at https://github.com/mxu9/edk2/tree/td_protocol.v2
https://github.com/tianocore/edk2-staging/tree/TDVF is the complete code of TDVF.
Please be noted, TDVF is in the process of upstreaming, so the code at https://github.com/tianocore/edk2-staging/tree/TDVF may be different from the patch-sets we submit to the community.
Any comment is welcomed.
Thanks.
Min
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [edk2-devel] [PATCH V2 1/3] MdePkg: Introduce TdProtocol for TD-Guest firmware
2021-10-08 5:21 ` [PATCH V2 1/3] MdePkg: Introduce TdProtocol for TD-Guest firmware Min Xu
2021-10-11 1:37 ` 回复: " gaoliming
@ 2021-10-19 13:21 ` Sami Mujawar
2021-10-19 14:40 ` Yao, Jiewen
1 sibling, 1 reply; 20+ messages in thread
From: Sami Mujawar @ 2021-10-19 13:21 UTC (permalink / raw)
To: devel, min.m.xu
Cc: Michael D Kinney, Liming Gao, Zhiguang Liu, Jiewen Yao,
Jian J Wang, Ken Lu, nd, Joey Gouly
Hi Min, Jiewen,
Thank you for this patch.
I think the protocol definition can be made architecturally neutral with
a few modifications marked inline as [SAMI].
I am fine with renaming the protocol to either
EFI_TEE_MEASUREMENT_PROTOCOL or EFI_CCAM_PROTOCOL. Similarly, some of
the data structures, variables, etc. would need renaming as well.
Please let me know if you have any queries.
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
>
> If TD-Guest firmware supports measurement and an event is created,
> TD-Guest firmware is designed to report the event log with the same data
> structure in TCG-Platform-Firmware-Profile specification with
> EFI_TCG2_EVENT_LOG_FORMAT_TCG_2 format.
>
> The TD-Guest firmware supports measurement, the TD Guest Firmware is
> designed to produce EFI_TD_PROTOCOL with new GUID EFI_TD_PROTOCOL_GUID
> to report event log and provides hash capability.
>
> https://software.intel.com/content/dam/develop/external/us/en/documents/
> intel-tdx-guest-hypervisor-communication-interface-1.0-344426-002.pdf
> Section 4.3.2 includes the EFI_TD_PROTOCOL.
>
> 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>
> Cc: Ken Lu <ken.lu@intel.com>
> Reviewed-by: Jiewen Yao <jiewen.yao@intel.com>
> Signed-off-by: Min Xu <min.m.xu@intel.com>
> ---
> MdePkg/Include/Protocol/TdProtocol.h | 305 +++++++++++++++++++++++++++
> MdePkg/MdePkg.dec | 3 +
> 2 files changed, 308 insertions(+)
> create mode 100644 MdePkg/Include/Protocol/TdProtocol.h
>
> diff --git a/MdePkg/Include/Protocol/TdProtocol.h b/MdePkg/Include/Protocol/TdProtocol.h
> new file mode 100644
> index 000000000000..89b09928d33a
> --- /dev/null
> +++ b/MdePkg/Include/Protocol/TdProtocol.h
> @@ -0,0 +1,305 @@
> +/** @file
> + If TD-Guest firmware supports measurement and an event is created, TD-Guest
> + firmware is designed to report the event log with the same data structure
> + in TCG-Platform-Firmware-Profile specification with
> + EFI_TCG2_EVENT_LOG_FORMAT_TCG_2 format.
> +
> + The TD-Guest firmware supports measurement, the TD Guest Firmware is designed
> + to produce EFI_TD_PROTOCOL with new GUID EFI_TD_PROTOCOL_GUID to report
> + event log and provides hash capability.
> +
> +Copyright (c) 2020 - 2021, Intel Corporation. All rights reserved.<BR>
> +SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +
> +#ifndef TD_PROTOCOL_H_
> +#define TD_PROTOCOL_H_
> +
> +#include <Uefi/UefiBaseType.h>
> +#include <IndustryStandard/UefiTcgPlatform.h>
> +#include <IndustryStandard/Tpm20.h>
[SAMI] Maybe the Tpm20.h include is not required here. Can you check,
please?
> +
> +
> +#define EFI_TD_PROTOCOL_GUID \
> + { 0x96751a3d, 0x72f4, 0x41a6, { 0xa7, 0x94, 0xed, 0x5d, 0x0e, 0x67, 0xae, 0x6b }}
> +extern EFI_GUID gEfiTdProtocolGuid;
> +
> +typedef struct _EFI_TD_PROTOCOL EFI_TD_PROTOCOL;
[SAMI] I think this could be renamed to either
EFI_TEE_MEASUREMENT_PROTOCOL or EFI_CCAM_PROTOCOL. Similarly, the usage
of _TD_ would need to be replaced accordingly.
> +
> +typedef struct {
> + UINT8 Major;
> + UINT8 Minor;
> +} EFI_TD_VERSION;
> +
> +typedef UINT32 EFI_TD_EVENT_LOG_BITMAP;
> +typedef UINT32 EFI_TD_EVENT_LOG_FORMAT;
> +typedef UINT32 EFI_TD_EVENT_ALGORITHM_BITMAP;
> +typedef UINT32 EFI_TD_MR_INDEX;
> +
> +#define EFI_TD_EVENT_LOG_FORMAT_TCG_2 0x00000002
> +#define EFI_TD_BOOT_HASH_ALG_SHA384 0x00000004
[SAMI] It is good that the values for these macros match that of TCG2. I
believe it should be possible to extend these to add macros for other
algorithms in the future.
> +
> +//
> +// This bit is shall be set when an event shall be extended but not logged.
> +//
> +#define EFI_TD_FLAG_EXTEND_ONLY 0x0000000000000001
> +//
> +// This bit shall be set when the intent is to measure a PE/COFF image.
> +//
> +#define EFI_TD_FLAG_PE_COFF_IMAGE 0x0000000000000010
> +
> +#define MR_INDEX_MRTD 0
> +#define MR_INDEX_RTMR0 1
> +#define MR_INDEX_RTMR1 2
> +#define MR_INDEX_RTMR2 3
> +#define MR_INDEX_RTMR3 4
> +
[SAMI] I think these indexes could go to a TD specific include file Or
the indexes could be defined generically. May be it would be good to
introduce a PcdMaxMrIndex that is configurable for different
architectures. This may be useful should any asserts/checks are needed
in the code.
> +//
> +// This bit shall be set when the intent is to measure a PE/COFF image.
> +//
> +#define PE_COFF_IMAGE 0x0000000000000010
> +
[SAMI] I think this macro is not needed.
> +#pragma pack (1)
> +
> +#define EFI_TD_EVENT_HEADER_VERSION 1
> +
> +typedef struct {
> + //
> + // Size of the event header itself (sizeof(EFI_TD_EVENT_HEADER)).
> + //
> + UINT32 HeaderSize;
> + //
> + // Header version. For this version of this specification, the value shall be 1.
> + //
> + UINT16 HeaderVersion;
> + //
> + // Index of the MR that shall be extended.
> + //
> + EFI_TD_MR_INDEX MrIndex;
> + //
> + // Type of the event that shall be extended (and optionally logged).
> + //
> + UINT32 EventType;
> +} EFI_TD_EVENT_HEADER;
> +
> +typedef struct {
> + //
> + // Total size of the event including the Size component, the header and the Event data.
> + //
> + UINT32 Size;
> + EFI_TD_EVENT_HEADER Header;
> + UINT8 Event[1];
> +} EFI_TD_EVENT;
> +
> +#pragma pack()
> +
> +
> +typedef struct {
> + //
> + // Allocated size of the structure
> + //
> + UINT8 Size;
> + //
> + // Version of the EFI_TD_BOOT_SERVICE_CAPABILITY structure itself.
> + // For this version of the protocol, the Major version shall be set to 1
> + // and the Minor version shall be set to 1.
> + //
> + EFI_TD_VERSION StructureVersion;
> + //
> + // Version of the EFI TD protocol.
> + // For this version of the protocol, the Major version shall be set to 1
> + // and the Minor version shall be set to 1.
> + //
> + EFI_TD_VERSION ProtocolVersion;
[SAMI] Should the protocol version be 1.0 (Major.Minor), as this is the
first introduction. Same for the StructureVersion field above.
> + //
> + // Supported hash algorithms
> + //
> + EFI_TD_EVENT_ALGORITHM_BITMAP HashAlgorithmBitmap;
> + //
> + // Bitmap of supported event log formats
> + //
> + EFI_TD_EVENT_LOG_BITMAP SupportedEventLogs;
> +
> + //
> + // False = TD not present
> + //
> + BOOLEAN TdPresentFlag;
[SAMI] I believe this would need to be renamed to something like
TeePresentFlag or CcaPresentFlag or a suitable alternative.
> +} EFI_TD_BOOT_SERVICE_CAPABILITY;
> +
> +/**
> + The EFI_TD_PROTOCOL GetCapability function call provides protocol
> + capability information and state information.
> +
> + @param[in] This Indicates the calling context
> + @param[in, out] ProtocolCapability The caller allocates memory for a EFI_TD_BOOT_SERVICE_CAPABILITY
> + structure and sets the size field to the size of the structure allocated.
> + The callee fills in the fields with the EFI protocol capability information
> + and the current EFI TD state information up to the number of fields which
> + fit within the size of the structure passed in.
> +
> + @retval EFI_SUCCESS Operation completed successfully.
> + @retval EFI_DEVICE_ERROR The command was unsuccessful.
> + The ProtocolCapability variable will not be populated.
> + @retval EFI_INVALID_PARAMETER One or more of the parameters are incorrect.
> + The ProtocolCapability variable will not be populated.
> + @retval EFI_BUFFER_TOO_SMALL The ProtocolCapability variable is too small to hold the full response.
> + It will be partially populated (required Size field will be set).
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI *EFI_TD_GET_CAPABILITY) (
> + IN EFI_TD_PROTOCOL *This,
> + IN OUT EFI_TD_BOOT_SERVICE_CAPABILITY *ProtocolCapability
> + );
> +
> +/**
> + The EFI_TD_PROTOCOL Get Event Log function call allows a caller to
> + retrieve the address of a given event log and its last entry.
> +
> + @param[in] This Indicates the calling context
> + @param[in] EventLogFormat The type of the event log for which the information is requested.
> + @param[out] EventLogLocation A pointer to the memory address of the event log.
> + @param[out] EventLogLastEntry If the Event Log contains more than one entry, this is a pointer to the
> + address of the start of the last entry in the event log in memory.
> + @param[out] EventLogTruncated If the Event Log is missing at least one entry because an event would
> + have exceeded the area allocated for events, this value is set to TRUE.
> + Otherwise, the value will be FALSE and the Event Log will be complete.
> +
> + @retval EFI_SUCCESS Operation completed successfully.
> + @retval EFI_INVALID_PARAMETER One or more of the parameters are incorrect
> + (e.g. asking for an event log whose format is not supported).
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI *EFI_TD_GET_EVENT_LOG) (
> + IN EFI_TD_PROTOCOL *This,
> + IN EFI_TD_EVENT_LOG_FORMAT EventLogFormat,
> + OUT EFI_PHYSICAL_ADDRESS *EventLogLocation,
> + OUT EFI_PHYSICAL_ADDRESS *EventLogLastEntry,
> + OUT BOOLEAN *EventLogTruncated
> + );
> +
> +/**
> + The EFI_TD_PROTOCOL HashLogExtendEvent function call provides callers with
> + an opportunity to extend and optionally log events without requiring
> + knowledge of actual TD commands.
> + The extend operation will occur even if this function cannot create an event
> + log entry (e.g. due to the event log being full).
> +
> + @param[in] This Indicates the calling context
> + @param[in] Flags Bitmap providing additional information.
> + @param[in] DataToHash Physical address of the start of the data buffer to be hashed.
> + @param[in] DataToHashLen The length in bytes of the buffer referenced by DataToHash.
> + @param[in] EfiTdEvent Pointer to data buffer containing information about the event.
> +
> + @retval EFI_SUCCESS Operation completed successfully.
> + @retval EFI_DEVICE_ERROR The command was unsuccessful.
> + @retval EFI_VOLUME_FULL The extend operation occurred, but the event could not be written to one or more event logs.
> + @retval EFI_INVALID_PARAMETER One or more of the parameters are incorrect.
> + @retval EFI_UNSUPPORTED The PE/COFF image type is not supported.
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI * EFI_TD_HASH_LOG_EXTEND_EVENT) (
> + IN EFI_TD_PROTOCOL *This,
> + IN UINT64 Flags,
> + IN EFI_PHYSICAL_ADDRESS DataToHash,
> + IN UINT64 DataToHashLen,
> + IN EFI_TD_EVENT *EfiTdEvent
> + );
> +
> +/**
> + The EFI_TD_PROTOCOL MapPcrToMrIndex function call provides callers
> + the info on TPM PCR<-> measurement register mapping information.
> +
> + In current version, we use below mapping:
> + PCR0 -> MRTD (Index 0)
> + PCR1 -> RTMR0 (Index 1)
> + PCR2~6 -> RTMR1 (Index 2)
> + PCR7 -> RTMR0 (Index 1)
> + PCR8~15 -> RTMR2 (Index 3)
> +
[SAMI] I think different architecures may map the PCRs differently. I
think the comment could be reworded to a more generic representation of
the mapping.
Also, I need to check the mailing list if there is a patch that adds the
TD protocol implementaiton, and if it could be made generic as well.
Maybe the protocol implementation would need to use an architecture
specific library that provides the mapping function.
[/SAMI]
> + @param[in] This Indicates the calling context
> + @param[in] PcrIndex TPM PCR index.
> + @param[out] MrIndex Measurement register index.
> +
> + @retval EFI_SUCCESS The MR index is returned.
> + @retval EFI_INVALID_PARAMETER The MrIndex is NULL.
> + @retval EFI_UNSUPPORTED The PcrIndex is invalid.
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI * EFI_TD_MAP_PCR_TO_MR_INDEX) (
> + IN EFI_TD_PROTOCOL *This,
> + IN TCG_PCRINDEX PcrIndex,
> + OUT EFI_TD_MR_INDEX *MrIndex
> + );
> +
> +struct _EFI_TD_PROTOCOL {
> + EFI_TD_GET_CAPABILITY GetCapability;
> + EFI_TD_GET_EVENT_LOG GetEventLog;
> + EFI_TD_HASH_LOG_EXTEND_EVENT HashLogExtendEvent;
> + EFI_TD_MAP_PCR_TO_MR_INDEX MapPcrToMrIndex;
> +};
> +
> +
> +//
> +// TD event log
> +//
> +
> +#pragma pack(1)
> +
> +//
> +// Crypto Agile Log Entry Format.
> +// It is similar with TCG_PCR_EVENT2 except the field of MrIndex and PCRIndex.
> +//
> +typedef struct {
> + EFI_TD_MR_INDEX MrIndex;
> + UINT32 EventType;
> + TPML_DIGEST_VALUES Digests;
> + UINT32 EventSize;
> + UINT8 Event[1];
> +} TD_EVENT;
> +
> +//
> +// EFI TD Event Header
> +// It is similar with TCG_PCR_EVENT2_HDR except the field of MrIndex and PCRIndex
> +//
> +typedef struct {
> + EFI_TD_MR_INDEX MrIndex;
> + UINT32 EventType;
> + TPML_DIGEST_VALUES Digests;
> + UINT32 EventSize;
> +} TD_EVENT_HDR;
> +
> +#pragma pack()
> +
> +//
> +// Log entries after Get Event Log service
> +//
> +
> +
> +typedef struct {
> + //
> + // The version of this structure. It shall be set ot 1.
[SAMI] It may be good to define a macro for the events table version,
similar to EFI_TCG2_FINAL_EVENTS_TABLE_VERSION.
> + //
> + UINT64 Version;
> + //
> + // Number of events recorded after invocation of GetEventLog API
> + //
> + UINT64 NumberOfEvents;
> + //
> + // List of events of type TD_EVENT.
> + //
> + //TD_EVENT Event[1];
> +} EFI_TD_FINAL_EVENTS_TABLE;
> +
> +
> +#define EFI_TD_FINAL_EVENTS_TABLE_GUID \
> + {0xdd4a4648, 0x2de7, 0x4665, {0x96, 0x4d, 0x21, 0xd9, 0xef, 0x5f, 0xb4, 0x46}}
> +
> +extern EFI_GUID gEfiTdFinalEventsTableGuid;
> +
> +#endif
> diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
> index 9cdc915ebae9..a31c44d3a689 100644
> --- a/MdePkg/MdePkg.dec
> +++ b/MdePkg/MdePkg.dec
> @@ -1011,6 +1011,9 @@
> ## Include/Protocol/PcdInfo.h
> gGetPcdInfoProtocolGuid = { 0x5be40f57, 0xfa68, 0x4610, { 0xbb, 0xbf, 0xe9, 0xc5, 0xfc, 0xda, 0xd3, 0x65 } }
>
> + ## Include/Protocol/TdProtocol.h
> + gEfiTdProtocolGuid = { 0x96751a3d, 0x72f4, 0x41a6, { 0xa7, 0x94, 0xed, 0x5d, 0x0e, 0x67, 0xae, 0x6b }}
> +
> #
> # Protocols defined in PI1.0.
> #
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [edk2-devel] [PATCH V2 2/3] SecurityPkg: Support TdProtocol in DxeTpm2MeasureBootLib
2021-10-08 5:21 ` [PATCH V2 2/3] SecurityPkg: Support TdProtocol in DxeTpm2MeasureBootLib Min Xu
@ 2021-10-19 13:22 ` Sami Mujawar
2021-10-27 5:19 ` Min Xu
0 siblings, 1 reply; 20+ messages in thread
From: Sami Mujawar @ 2021-10-19 13:22 UTC (permalink / raw)
To: devel, min.m.xu
Cc: Michael D Kinney, Liming Gao, Zhiguang Liu, Jiewen Yao,
Jian J Wang, nd, Joey Gouly
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [edk2-devel] [PATCH V2 3/3] SecurityPkg: Support TdProtocol in DxeTpmMeasurementLib
2021-10-08 5:21 ` [PATCH V2 3/3] SecurityPkg: Support TdProtocol in DxeTpmMeasurementLib Min Xu
@ 2021-10-19 13:24 ` Sami Mujawar
0 siblings, 0 replies; 20+ messages in thread
From: Sami Mujawar @ 2021-10-19 13:24 UTC (permalink / raw)
To: devel, min.m.xu
Cc: Michael D Kinney, Liming Gao, Zhiguang Liu, Jiewen Yao,
Jian J Wang, nd, Joey Gouly
Hi Min, Jiewen,
I believe this patch would need updating based on the changes done to
patch 1/3 to make the measurment protocol architecture neutral. Other
than that the code changes in this patch look good to me.
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
>
> DxeTpmMeasurementLib supports TPM based measurement in DXE phase.
> After Td protocol is introduced, TD based measurement needs to be
> supported in DxeTpmMeasurementLib as well.
>
> In TpmMeasureAndLogData, TD based measurement will be first called.
> If it failed, TPM based measurement will be called sequentially.
> Currently there is an assumption that TD based measurement and
> TPM based measurement won't be exist at the same time.If the
> assumption is not true in the future, we will revisit here then.
>
> 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>
> ---
> .../DxeTpmMeasurementLib.c | 87 ++++++++++++++++++-
> .../DxeTpmMeasurementLib.inf | 1 +
> 2 files changed, 85 insertions(+), 3 deletions(-)
>
> diff --git a/SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.c b/SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.c
> index 061136ee7860..f8cd289ba62c 100644
> --- a/SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.c
> +++ b/SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.c
> @@ -19,7 +19,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>
> #include <Guid/Acpi.h>
> #include <IndustryStandard/Acpi.h>
> -
> +#include <Protocol/TdProtocol.h>
>
>
> /**
> @@ -149,6 +149,73 @@ Tpm20MeasureAndLogData (
> return Status;
> }
>
> +/**
> + Tdx measure and log data, and extend the measurement result into a
> + specific TDX RTMR.
> +
> + @param[in] PcrIndex PCR Index.
> + @param[in] EventType Event type.
> + @param[in] EventLog Measurement event log.
> + @param[in] LogLen Event log length in bytes.
> + @param[in] HashData The start of the data buffer to be hashed, extended.
> + @param[in] HashDataLen The length, in bytes, of the buffer referenced by HashData
> +
> + @retval EFI_SUCCESS Operation completed successfully.
> + @retval EFI_UNSUPPORTED Tdx device not available.
> + @retval EFI_OUT_OF_RESOURCES Out of memory.
> + @retval EFI_DEVICE_ERROR The operation was unsuccessful.
> +**/
> +EFI_STATUS
> +EFIAPI
> +TdxMeasureAndLogData (
> + IN UINT32 PcrIndex,
> + IN UINT32 EventType,
> + IN VOID *EventLog,
> + IN UINT32 LogLen,
> + IN VOID *HashData,
> + IN UINT64 HashDataLen
> + )
> +{
> + EFI_STATUS Status;
> + EFI_TD_PROTOCOL *TdProtocol;
> + EFI_TD_EVENT *TdEvent;
> + UINT32 MrIndex;
> +
> + Status = gBS->LocateProtocol (&gEfiTdProtocolGuid, NULL, (VOID **) &TdProtocol);
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> +
> + Status = TdProtocol->MapPcrToMrIndex (TdProtocol, PcrIndex, &MrIndex);
> + if (EFI_ERROR (Status)) {
> + return EFI_INVALID_PARAMETER;
> + }
> +
> + TdEvent = (EFI_TD_EVENT *) AllocateZeroPool (LogLen + sizeof (EFI_TD_EVENT));
> + if(TdEvent == NULL) {
> + return EFI_OUT_OF_RESOURCES;
> + }
> +
> + TdEvent->Size = (UINT32) LogLen + sizeof (EFI_TD_EVENT) - sizeof (TdEvent->Event);
> + TdEvent->Header.HeaderSize = sizeof (EFI_TD_EVENT_HEADER);
> + TdEvent->Header.HeaderVersion = EFI_TD_EVENT_HEADER_VERSION;
> + TdEvent->Header.MrIndex = MrIndex;
> + TdEvent->Header.EventType = EventType;
> + CopyMem (&TdEvent->Event[0], EventLog, LogLen);
> +
> + Status = TdProtocol->HashLogExtendEvent (
> + TdProtocol,
> + 0,
> + (EFI_PHYSICAL_ADDRESS) (UINTN) HashData,
> + HashDataLen,
> + TdEvent
> + );
> + FreePool (TdEvent);
> +
> + return Status;
> +}
> +
> +
> /**
> Tpm measure and log data, and extend the measurement result into a specific PCR.
>
> @@ -178,9 +245,9 @@ TpmMeasureAndLogData (
> EFI_STATUS Status;
>
> //
> - // Try to measure using Tpm20 protocol
> + // Try to measure using Td protocol
> //
> - Status = Tpm20MeasureAndLogData(
> + Status = TdxMeasureAndLogData (
> PcrIndex,
> EventType,
> EventLog,
> @@ -189,6 +256,20 @@ TpmMeasureAndLogData (
> HashDataLen
> );
>
> + if (EFI_ERROR (Status)) {
> + //
> + // Try to measure using Tpm20 protocol
> + //
> + Status = Tpm20MeasureAndLogData(
> + PcrIndex,
> + EventType,
> + EventLog,
> + LogLen,
> + HashData,
> + HashDataLen
> + );
> + }
> +
> if (EFI_ERROR (Status)) {
> //
> // Try to measure using Tpm1.2 protocol
> diff --git a/SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.inf b/SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.inf
> index 7d41bc41f95d..b919771d5a9e 100644
> --- a/SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.inf
> +++ b/SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.inf
> @@ -42,3 +42,4 @@
> [Protocols]
> gEfiTcgProtocolGuid ## SOMETIMES_CONSUMES
> gEfiTcg2ProtocolGuid ## SOMETIMES_CONSUMES
> + gEfiTdProtocolGuid ## SOMETIMES_CONSUMES
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [edk2-devel] [PATCH V2 1/3] MdePkg: Introduce TdProtocol for TD-Guest firmware
2021-10-19 13:21 ` [edk2-devel] " Sami Mujawar
@ 2021-10-19 14:40 ` Yao, Jiewen
2021-10-20 9:26 ` Sami Mujawar
0 siblings, 1 reply; 20+ messages in thread
From: Yao, Jiewen @ 2021-10-19 14:40 UTC (permalink / raw)
To: devel@edk2.groups.io, sami.mujawar@arm.com, Xu, Min M
Cc: Kinney, Michael D, Liming Gao, Liu, Zhiguang, Wang, Jian J,
Lu, Ken, nd, Joey Gouly
Good feedback. Thank you very much, Sami.
Response inline.
I proposed some naming change. Please let us know if that is OK.
Thank you
Yao, Jiewen
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Sami
> Mujawar
> Sent: Tuesday, October 19, 2021 9:21 PM
> To: devel@edk2.groups.io; Xu, Min M <min.m.xu@intel.com>
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Liming Gao
> <gaoliming@byosoft.com.cn>; Liu, Zhiguang <zhiguang.liu@intel.com>; Yao,
> Jiewen <jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Lu, Ken
> <ken.lu@intel.com>; nd <nd@arm.com>; Joey Gouly <Joey.Gouly@arm.com>
> Subject: Re: [edk2-devel] [PATCH V2 1/3] MdePkg: Introduce TdProtocol for TD-
> Guest firmware
>
> Hi Min, Jiewen,
>
> Thank you for this patch.
>
> I think the protocol definition can be made architecturally neutral with
> a few modifications marked inline as [SAMI].
>
> I am fine with renaming the protocol to either
> EFI_TEE_MEASUREMENT_PROTOCOL or EFI_CCAM_PROTOCOL. Similarly, some
> of
> the data structures, variables, etc. would need renaming as well.
>
> Please let me know if you have any queries.
>
> 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
> >
> > If TD-Guest firmware supports measurement and an event is created,
> > TD-Guest firmware is designed to report the event log with the same data
> > structure in TCG-Platform-Firmware-Profile specification with
> > EFI_TCG2_EVENT_LOG_FORMAT_TCG_2 format.
> >
> > The TD-Guest firmware supports measurement, the TD Guest Firmware is
> > designed to produce EFI_TD_PROTOCOL with new GUID
> EFI_TD_PROTOCOL_GUID
> > to report event log and provides hash capability.
> >
> > https://software.intel.com/content/dam/develop/external/us/en/documents/
> > intel-tdx-guest-hypervisor-communication-interface-1.0-344426-002.pdf
> > Section 4.3.2 includes the EFI_TD_PROTOCOL.
> >
> > 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>
> > Cc: Ken Lu <ken.lu@intel.com>
> > Reviewed-by: Jiewen Yao <jiewen.yao@intel.com>
> > Signed-off-by: Min Xu <min.m.xu@intel.com>
> > ---
> > MdePkg/Include/Protocol/TdProtocol.h | 305
> +++++++++++++++++++++++++++
> > MdePkg/MdePkg.dec | 3 +
> > 2 files changed, 308 insertions(+)
> > create mode 100644 MdePkg/Include/Protocol/TdProtocol.h
> >
> > diff --git a/MdePkg/Include/Protocol/TdProtocol.h
> b/MdePkg/Include/Protocol/TdProtocol.h
> > new file mode 100644
> > index 000000000000..89b09928d33a
> > --- /dev/null
> > +++ b/MdePkg/Include/Protocol/TdProtocol.h
> > @@ -0,0 +1,305 @@
> > +/** @file
> > + If TD-Guest firmware supports measurement and an event is created, TD-
> Guest
> > + firmware is designed to report the event log with the same data structure
> > + in TCG-Platform-Firmware-Profile specification with
> > + EFI_TCG2_EVENT_LOG_FORMAT_TCG_2 format.
> > +
> > + The TD-Guest firmware supports measurement, the TD Guest Firmware is
> designed
> > + to produce EFI_TD_PROTOCOL with new GUID EFI_TD_PROTOCOL_GUID to
> report
> > + event log and provides hash capability.
> > +
> > +Copyright (c) 2020 - 2021, Intel Corporation. All rights reserved.<BR>
> > +SPDX-License-Identifier: BSD-2-Clause-Patent
> > +
> > +**/
> > +
> > +
> > +#ifndef TD_PROTOCOL_H_
> > +#define TD_PROTOCOL_H_
> > +
> > +#include <Uefi/UefiBaseType.h>
> > +#include <IndustryStandard/UefiTcgPlatform.h>
> > +#include <IndustryStandard/Tpm20.h>
> [SAMI] Maybe the Tpm20.h include is not required here. Can you check,
> please?
[Jiewen] Right. I don’t think we do need it in this definition.
I feel we just copy it from Tcg2Protocol.h.
> > +
> > +
> > +#define EFI_TD_PROTOCOL_GUID \
> > + { 0x96751a3d, 0x72f4, 0x41a6, { 0xa7, 0x94, 0xed, 0x5d, 0x0e, 0x67, 0xae,
> 0x6b }}
> > +extern EFI_GUID gEfiTdProtocolGuid;
> > +
> > +typedef struct _EFI_TD_PROTOCOL EFI_TD_PROTOCOL;
> [SAMI] I think this could be renamed to either
> EFI_TEE_MEASUREMENT_PROTOCOL or EFI_CCAM_PROTOCOL. Similarly, the
> usage
> of _TD_ would need to be replaced accordingly.
[Jiewen] Agree.
I propose "EFI_TD_PROTOCOL"->"EFI_TEE_MEASUREMENT_PROTOCOL"
Also "TD"->"TEE"
> > +
> > +typedef struct {
> > + UINT8 Major;
> > + UINT8 Minor;
> > +} EFI_TD_VERSION;
> > +
> > +typedef UINT32 EFI_TD_EVENT_LOG_BITMAP;
> > +typedef UINT32 EFI_TD_EVENT_LOG_FORMAT;
> > +typedef UINT32 EFI_TD_EVENT_ALGORITHM_BITMAP;
> > +typedef UINT32 EFI_TD_MR_INDEX;
> > +
> > +#define EFI_TD_EVENT_LOG_FORMAT_TCG_2 0x00000002
> > +#define EFI_TD_BOOT_HASH_ALG_SHA384 0x00000004
> [SAMI] It is good that the values for these macros match that of TCG2. I
> believe it should be possible to extend these to add macros for other
> algorithms in the future.
[Jiewen] Agree. If we change "TD"->"TEE", we can add other algo based upon real use case.
E.g. EFI_TEE_EVENT_LOG_..., EFI_TEE_MR_INDEX, EFI_TEE_BOOT_HASH_...
> > +
> > +//
> > +// This bit is shall be set when an event shall be extended but not logged.
> > +//
> > +#define EFI_TD_FLAG_EXTEND_ONLY 0x0000000000000001
> > +//
> > +// This bit shall be set when the intent is to measure a PE/COFF image.
> > +//
> > +#define EFI_TD_FLAG_PE_COFF_IMAGE 0x0000000000000010
> > +
> > +#define MR_INDEX_MRTD 0
> > +#define MR_INDEX_RTMR0 1
> > +#define MR_INDEX_RTMR1 2
> > +#define MR_INDEX_RTMR2 3
> > +#define MR_INDEX_RTMR3 4
> > +
> [SAMI] I think these indexes could go to a TD specific include file Or
> the indexes could be defined generically. May be it would be good to
> introduce a PcdMaxMrIndex that is configurable for different
> architectures. This may be useful should any asserts/checks are needed
> in the code.
[Jiewen] Right. This is TD specific index.
We need rename it to TDX_MR_INDEX_*.
I think we need add new fields in EFI_TD_BOOT_SERVICE_CAPABILITY.
typedef UINT8 EFI_TEE_TYPE; // match https://github.com/tianocore/edk2/blob/master/OvmfPkg/Include/WorkArea.h, NONE = 0, AMD_SEV = 1, INTEL_TDX = 2, we can add more here.
typedef UINT8 EFI_TEE_SUBTYPE; // TEE-type specific subtype.
As such, the caller can know what event log / index it is using.
E.g. If TeeType is TDX, then the INDEX matches the TDX RTMR.
If TeeType is Realm, then the INDEX matches something else.
I am not sure the usage of PcdMaxMrIndex. We SHALL NOT define PCD in a protocol in general.
Would you please share your idea on how to use PcdMaxMrIndex? Then we can have better solution.
> > +//
> > +// This bit shall be set when the intent is to measure a PE/COFF image.
> > +//
> > +#define PE_COFF_IMAGE 0x0000000000000010
> > +
> [SAMI] I think this macro is not needed.
[Jiewen] This is to align with TCG2 protocol. I think we need to measurement UEFI image.
Is there any concern to keep it?
> > +#pragma pack (1)
> > +
> > +#define EFI_TD_EVENT_HEADER_VERSION 1
> > +
> > +typedef struct {
> > + //
> > + // Size of the event header itself (sizeof(EFI_TD_EVENT_HEADER)).
> > + //
> > + UINT32 HeaderSize;
> > + //
> > + // Header version. For this version of this specification, the value shall be 1.
> > + //
> > + UINT16 HeaderVersion;
> > + //
> > + // Index of the MR that shall be extended.
> > + //
> > + EFI_TD_MR_INDEX MrIndex;
> > + //
> > + // Type of the event that shall be extended (and optionally logged).
> > + //
> > + UINT32 EventType;
> > +} EFI_TD_EVENT_HEADER;
> > +
> > +typedef struct {
> > + //
> > + // Total size of the event including the Size component, the header and the
> Event data.
> > + //
> > + UINT32 Size;
> > + EFI_TD_EVENT_HEADER Header;
> > + UINT8 Event[1];
> > +} EFI_TD_EVENT;
> > +
> > +#pragma pack()
> > +
> > +
> > +typedef struct {
> > + //
> > + // Allocated size of the structure
> > + //
> > + UINT8 Size;
> > + //
> > + // Version of the EFI_TD_BOOT_SERVICE_CAPABILITY structure itself.
> > + // For this version of the protocol, the Major version shall be set to 1
> > + // and the Minor version shall be set to 1.
> > + //
> > + EFI_TD_VERSION StructureVersion;
> > + //
> > + // Version of the EFI TD protocol.
> > + // For this version of the protocol, the Major version shall be set to 1
> > + // and the Minor version shall be set to 1.
> > + //
> > + EFI_TD_VERSION ProtocolVersion;
> [SAMI] Should the protocol version be 1.0 (Major.Minor), as this is the
> first introduction. Same for the StructureVersion field above.
[Jiewen] Copy/Past from TCG2.
Sure. We can start from 1.0 (not 1.1).
> > + //
> > + // Supported hash algorithms
> > + //
> > + EFI_TD_EVENT_ALGORITHM_BITMAP HashAlgorithmBitmap;
> > + //
> > + // Bitmap of supported event log formats
> > + //
> > + EFI_TD_EVENT_LOG_BITMAP SupportedEventLogs;
> > +
> > + //
> > + // False = TD not present
> > + //
> > + BOOLEAN TdPresentFlag;
> [SAMI] I believe this would need to be renamed to something like
> TeePresentFlag or CcaPresentFlag or a suitable alternative.
[Jiewen] Agree. I propose to remove TdPresentFlag.
Add "EFI_TEE_TYPE TeeType;" // 0 - None, 1 - SEV, 2 - TDX, ...
> > +} EFI_TD_BOOT_SERVICE_CAPABILITY;
> > +
> > +/**
> > + The EFI_TD_PROTOCOL GetCapability function call provides protocol
> > + capability information and state information.
> > +
> > + @param[in] This Indicates the calling context
> > + @param[in, out] ProtocolCapability The caller allocates memory for a
> EFI_TD_BOOT_SERVICE_CAPABILITY
> > + structure and sets the size field to the size of the
> structure allocated.
> > + The callee fills in the fields with the EFI protocol
> capability information
> > + and the current EFI TD state information up to the
> number of fields which
> > + fit within the size of the structure passed in.
> > +
> > + @retval EFI_SUCCESS Operation completed successfully.
> > + @retval EFI_DEVICE_ERROR The command was unsuccessful.
> > + The ProtocolCapability variable will not be populated.
> > + @retval EFI_INVALID_PARAMETER One or more of the parameters are
> incorrect.
> > + The ProtocolCapability variable will not be populated.
> > + @retval EFI_BUFFER_TOO_SMALL The ProtocolCapability variable is too
> small to hold the full response.
> > + It will be partially populated (required Size field will be set).
> > +**/
> > +typedef
> > +EFI_STATUS
> > +(EFIAPI *EFI_TD_GET_CAPABILITY) (
> > + IN EFI_TD_PROTOCOL *This,
> > + IN OUT EFI_TD_BOOT_SERVICE_CAPABILITY *ProtocolCapability
> > + );
> > +
> > +/**
> > + The EFI_TD_PROTOCOL Get Event Log function call allows a caller to
> > + retrieve the address of a given event log and its last entry.
> > +
> > + @param[in] This Indicates the calling context
> > + @param[in] EventLogFormat The type of the event log for which the
> information is requested.
> > + @param[out] EventLogLocation A pointer to the memory address of the
> event log.
> > + @param[out] EventLogLastEntry If the Event Log contains more than one
> entry, this is a pointer to the
> > + address of the start of the last entry in the event log in
> memory.
> > + @param[out] EventLogTruncated If the Event Log is missing at least one
> entry because an event would
> > + have exceeded the area allocated for events, this value is
> set to TRUE.
> > + Otherwise, the value will be FALSE and the Event Log will
> be complete.
> > +
> > + @retval EFI_SUCCESS Operation completed successfully.
> > + @retval EFI_INVALID_PARAMETER One or more of the parameters are
> incorrect
> > + (e.g. asking for an event log whose format is not
> supported).
> > +**/
> > +typedef
> > +EFI_STATUS
> > +(EFIAPI *EFI_TD_GET_EVENT_LOG) (
> > + IN EFI_TD_PROTOCOL *This,
> > + IN EFI_TD_EVENT_LOG_FORMAT EventLogFormat,
> > + OUT EFI_PHYSICAL_ADDRESS *EventLogLocation,
> > + OUT EFI_PHYSICAL_ADDRESS *EventLogLastEntry,
> > + OUT BOOLEAN *EventLogTruncated
> > + );
> > +
> > +/**
> > + The EFI_TD_PROTOCOL HashLogExtendEvent function call provides callers
> with
> > + an opportunity to extend and optionally log events without requiring
> > + knowledge of actual TD commands.
> > + The extend operation will occur even if this function cannot create an event
> > + log entry (e.g. due to the event log being full).
> > +
> > + @param[in] This Indicates the calling context
> > + @param[in] Flags Bitmap providing additional information.
> > + @param[in] DataToHash Physical address of the start of the data
> buffer to be hashed.
> > + @param[in] DataToHashLen The length in bytes of the buffer referenced
> by DataToHash.
> > + @param[in] EfiTdEvent Pointer to data buffer containing information
> about the event.
> > +
> > + @retval EFI_SUCCESS Operation completed successfully.
> > + @retval EFI_DEVICE_ERROR The command was unsuccessful.
> > + @retval EFI_VOLUME_FULL The extend operation occurred, but the
> event could not be written to one or more event logs.
> > + @retval EFI_INVALID_PARAMETER One or more of the parameters are
> incorrect.
> > + @retval EFI_UNSUPPORTED The PE/COFF image type is not supported.
> > +**/
> > +typedef
> > +EFI_STATUS
> > +(EFIAPI * EFI_TD_HASH_LOG_EXTEND_EVENT) (
> > + IN EFI_TD_PROTOCOL *This,
> > + IN UINT64 Flags,
> > + IN EFI_PHYSICAL_ADDRESS DataToHash,
> > + IN UINT64 DataToHashLen,
> > + IN EFI_TD_EVENT *EfiTdEvent
> > + );
> > +
> > +/**
> > + The EFI_TD_PROTOCOL MapPcrToMrIndex function call provides callers
> > + the info on TPM PCR<-> measurement register mapping information.
> > +
> > + In current version, we use below mapping:
> > + PCR0 -> MRTD (Index 0)
> > + PCR1 -> RTMR0 (Index 1)
> > + PCR2~6 -> RTMR1 (Index 2)
> > + PCR7 -> RTMR0 (Index 1)
> > + PCR8~15 -> RTMR2 (Index 3)
> > +
> [SAMI] I think different architecures may map the PCRs differently. I
> think the comment could be reworded to a more generic representation of
> the mapping.
> Also, I need to check the mailing list if there is a patch that adds the
> TD protocol implementaiton, and if it could be made generic as well.
> Maybe the protocol implementation would need to use an architecture
> specific library that provides the mapping function.
> [/SAMI]
[Jiewen] Agree. We should clear up the comment.
The caller shall be generic enough to use this API to get the mapping.
The caller shall NOT make any assumption.
> > + @param[in] This Indicates the calling context
> > + @param[in] PcrIndex TPM PCR index.
> > + @param[out] MrIndex Measurement register index.
> > +
> > + @retval EFI_SUCCESS The MR index is returned.
> > + @retval EFI_INVALID_PARAMETER The MrIndex is NULL.
> > + @retval EFI_UNSUPPORTED The PcrIndex is invalid.
> > +**/
> > +typedef
> > +EFI_STATUS
> > +(EFIAPI * EFI_TD_MAP_PCR_TO_MR_INDEX) (
> > + IN EFI_TD_PROTOCOL *This,
> > + IN TCG_PCRINDEX PcrIndex,
> > + OUT EFI_TD_MR_INDEX *MrIndex
> > + );
> > +
> > +struct _EFI_TD_PROTOCOL {
> > + EFI_TD_GET_CAPABILITY GetCapability;
> > + EFI_TD_GET_EVENT_LOG GetEventLog;
> > + EFI_TD_HASH_LOG_EXTEND_EVENT HashLogExtendEvent;
> > + EFI_TD_MAP_PCR_TO_MR_INDEX MapPcrToMrIndex;
> > +};
> > +
> > +
> > +//
> > +// TD event log
> > +//
> > +
> > +#pragma pack(1)
> > +
> > +//
> > +// Crypto Agile Log Entry Format.
> > +// It is similar with TCG_PCR_EVENT2 except the field of MrIndex and
> PCRIndex.
> > +//
> > +typedef struct {
> > + EFI_TD_MR_INDEX MrIndex;
> > + UINT32 EventType;
> > + TPML_DIGEST_VALUES Digests;
> > + UINT32 EventSize;
> > + UINT8 Event[1];
> > +} TD_EVENT;
> > +
> > +//
> > +// EFI TD Event Header
> > +// It is similar with TCG_PCR_EVENT2_HDR except the field of MrIndex and
> PCRIndex
> > +//
> > +typedef struct {
> > + EFI_TD_MR_INDEX MrIndex;
> > + UINT32 EventType;
> > + TPML_DIGEST_VALUES Digests;
> > + UINT32 EventSize;
> > +} TD_EVENT_HDR;
> > +
> > +#pragma pack()
> > +
> > +//
> > +// Log entries after Get Event Log service
> > +//
> > +
> > +
> > +typedef struct {
> > + //
> > + // The version of this structure. It shall be set ot 1.
> [SAMI] It may be good to define a macro for the events table version,
> similar to EFI_TCG2_FINAL_EVENTS_TABLE_VERSION.
[Jiewen] Agree.
> > + //
> > + UINT64 Version;
> > + //
> > + // Number of events recorded after invocation of GetEventLog API
> > + //
> > + UINT64 NumberOfEvents;
> > + //
> > + // List of events of type TD_EVENT.
> > + //
> > + //TD_EVENT Event[1];
> > +} EFI_TD_FINAL_EVENTS_TABLE;
> > +
> > +
> > +#define EFI_TD_FINAL_EVENTS_TABLE_GUID \
> > + {0xdd4a4648, 0x2de7, 0x4665, {0x96, 0x4d, 0x21, 0xd9, 0xef, 0x5f, 0xb4,
> 0x46}}
> > +
> > +extern EFI_GUID gEfiTdFinalEventsTableGuid;
> > +
> > +#endif
> > diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
> > index 9cdc915ebae9..a31c44d3a689 100644
> > --- a/MdePkg/MdePkg.dec
> > +++ b/MdePkg/MdePkg.dec
> > @@ -1011,6 +1011,9 @@
> > ## Include/Protocol/PcdInfo.h
> > gGetPcdInfoProtocolGuid = { 0x5be40f57, 0xfa68, 0x4610, { 0xbb, 0xbf,
> 0xe9, 0xc5, 0xfc, 0xda, 0xd3, 0x65 } }
> >
> > + ## Include/Protocol/TdProtocol.h
> > + gEfiTdProtocolGuid = { 0x96751a3d, 0x72f4, 0x41a6, { 0xa7, 0x94,
> 0xed, 0x5d, 0x0e, 0x67, 0xae, 0x6b }}
> > +
> > #
> > # Protocols defined in PI1.0.
> > #
>
>
>
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [edk2-devel] [PATCH V2 1/3] MdePkg: Introduce TdProtocol for TD-Guest firmware
2021-10-19 14:40 ` Yao, Jiewen
@ 2021-10-20 9:26 ` Sami Mujawar
0 siblings, 0 replies; 20+ messages in thread
From: Sami Mujawar @ 2021-10-20 9:26 UTC (permalink / raw)
To: devel, jiewen.yao, Xu, Min M
Cc: Kinney, Michael D, Liming Gao, Liu, Zhiguang, Wang, Jian J,
Lu, Ken, nd, Joey Gouly
Hi Jiewen,
Please find my response inline marked [SAMI].
Regards,
Sami Mujawar
On 19/10/2021 03:40 PM, Yao, Jiewen via groups.io wrote:
> Good feedback. Thank you very much, Sami.
>
> Response inline.
>
> I proposed some naming change. Please let us know if that is OK.
>
> Thank you
> Yao, Jiewen
>
>
>
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Sami
>> Mujawar
>> Sent: Tuesday, October 19, 2021 9:21 PM
>> To: devel@edk2.groups.io; Xu, Min M <min.m.xu@intel.com>
>> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Liming Gao
>> <gaoliming@byosoft.com.cn>; Liu, Zhiguang <zhiguang.liu@intel.com>; Yao,
>> Jiewen <jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Lu, Ken
>> <ken.lu@intel.com>; nd <nd@arm.com>; Joey Gouly <Joey.Gouly@arm.com>
>> Subject: Re: [edk2-devel] [PATCH V2 1/3] MdePkg: Introduce TdProtocol for TD-
>> Guest firmware
>>
>> Hi Min, Jiewen,
>>
>> Thank you for this patch.
>>
>> I think the protocol definition can be made architecturally neutral with
>> a few modifications marked inline as [SAMI].
>>
>> I am fine with renaming the protocol to either
>> EFI_TEE_MEASUREMENT_PROTOCOL or EFI_CCAM_PROTOCOL. Similarly, some
>> of
>> the data structures, variables, etc. would need renaming as well.
>>
>> Please let me know if you have any queries.
>>
>> 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
>>>
>>> If TD-Guest firmware supports measurement and an event is created,
>>> TD-Guest firmware is designed to report the event log with the same data
>>> structure in TCG-Platform-Firmware-Profile specification with
>>> EFI_TCG2_EVENT_LOG_FORMAT_TCG_2 format.
>>>
>>> The TD-Guest firmware supports measurement, the TD Guest Firmware is
>>> designed to produce EFI_TD_PROTOCOL with new GUID
>> EFI_TD_PROTOCOL_GUID
>>> to report event log and provides hash capability.
>>>
>>> https://software.intel.com/content/dam/develop/external/us/en/documents/
>>> intel-tdx-guest-hypervisor-communication-interface-1.0-344426-002.pdf
>>> Section 4.3.2 includes the EFI_TD_PROTOCOL.
>>>
>>> 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>
>>> Cc: Ken Lu <ken.lu@intel.com>
>>> Reviewed-by: Jiewen Yao <jiewen.yao@intel.com>
>>> Signed-off-by: Min Xu <min.m.xu@intel.com>
>>> ---
>>> MdePkg/Include/Protocol/TdProtocol.h | 305
>> +++++++++++++++++++++++++++
>>> MdePkg/MdePkg.dec | 3 +
>>> 2 files changed, 308 insertions(+)
>>> create mode 100644 MdePkg/Include/Protocol/TdProtocol.h
>>>
>>> diff --git a/MdePkg/Include/Protocol/TdProtocol.h
>> b/MdePkg/Include/Protocol/TdProtocol.h
>>> new file mode 100644
>>> index 000000000000..89b09928d33a
>>> --- /dev/null
>>> +++ b/MdePkg/Include/Protocol/TdProtocol.h
>>> @@ -0,0 +1,305 @@
>>> +/** @file
>>> + If TD-Guest firmware supports measurement and an event is created, TD-
>> Guest
>>> + firmware is designed to report the event log with the same data structure
>>> + in TCG-Platform-Firmware-Profile specification with
>>> + EFI_TCG2_EVENT_LOG_FORMAT_TCG_2 format.
>>> +
>>> + The TD-Guest firmware supports measurement, the TD Guest Firmware is
>> designed
>>> + to produce EFI_TD_PROTOCOL with new GUID EFI_TD_PROTOCOL_GUID to
>> report
>>> + event log and provides hash capability.
>>> +
>>> +Copyright (c) 2020 - 2021, Intel Corporation. All rights reserved.<BR>
>>> +SPDX-License-Identifier: BSD-2-Clause-Patent
>>> +
>>> +**/
>>> +
>>> +
>>> +#ifndef TD_PROTOCOL_H_
>>> +#define TD_PROTOCOL_H_
>>> +
>>> +#include <Uefi/UefiBaseType.h>
>>> +#include <IndustryStandard/UefiTcgPlatform.h>
>>> +#include <IndustryStandard/Tpm20.h>
>> [SAMI] Maybe the Tpm20.h include is not required here. Can you check,
>> please?
> [Jiewen] Right. I don’t think we do need it in this definition.
> I feel we just copy it from Tcg2Protocol.h.
>
>>> +
>>> +
>>> +#define EFI_TD_PROTOCOL_GUID \
>>> + { 0x96751a3d, 0x72f4, 0x41a6, { 0xa7, 0x94, 0xed, 0x5d, 0x0e, 0x67, 0xae,
>> 0x6b }}
>>> +extern EFI_GUID gEfiTdProtocolGuid;
>>> +
>>> +typedef struct _EFI_TD_PROTOCOL EFI_TD_PROTOCOL;
>> [SAMI] I think this could be renamed to either
>> EFI_TEE_MEASUREMENT_PROTOCOL or EFI_CCAM_PROTOCOL. Similarly, the
>> usage
>> of _TD_ would need to be replaced accordingly.
> [Jiewen] Agree.
> I propose "EFI_TD_PROTOCOL"->"EFI_TEE_MEASUREMENT_PROTOCOL"
> Also "TD"->"TEE"
[SAMI] Agree.
>>> +
>>> +typedef struct {
>>> + UINT8 Major;
>>> + UINT8 Minor;
>>> +} EFI_TD_VERSION;
>>> +
>>> +typedef UINT32 EFI_TD_EVENT_LOG_BITMAP;
>>> +typedef UINT32 EFI_TD_EVENT_LOG_FORMAT;
>>> +typedef UINT32 EFI_TD_EVENT_ALGORITHM_BITMAP;
>>> +typedef UINT32 EFI_TD_MR_INDEX;
>>> +
>>> +#define EFI_TD_EVENT_LOG_FORMAT_TCG_2 0x00000002
>>> +#define EFI_TD_BOOT_HASH_ALG_SHA384 0x00000004
>> [SAMI] It is good that the values for these macros match that of TCG2. I
>> believe it should be possible to extend these to add macros for other
>> algorithms in the future.
> [Jiewen] Agree. If we change "TD"->"TEE", we can add other algo based upon real use case.
> E.g. EFI_TEE_EVENT_LOG_..., EFI_TEE_MR_INDEX, EFI_TEE_BOOT_HASH_...
[SAMI] Ack.
>>> +
>>> +//
>>> +// This bit is shall be set when an event shall be extended but not logged.
>>> +//
>>> +#define EFI_TD_FLAG_EXTEND_ONLY 0x0000000000000001
>>> +//
>>> +// This bit shall be set when the intent is to measure a PE/COFF image.
>>> +//
>>> +#define EFI_TD_FLAG_PE_COFF_IMAGE 0x0000000000000010
>>> +
>>> +#define MR_INDEX_MRTD 0
>>> +#define MR_INDEX_RTMR0 1
>>> +#define MR_INDEX_RTMR1 2
>>> +#define MR_INDEX_RTMR2 3
>>> +#define MR_INDEX_RTMR3 4
>>> +
>> [SAMI] I think these indexes could go to a TD specific include file Or
>> the indexes could be defined generically. May be it would be good to
>> introduce a PcdMaxMrIndex that is configurable for different
>> architectures. This may be useful should any asserts/checks are needed
>> in the code.
> [Jiewen] Right. This is TD specific index.
> We need rename it to TDX_MR_INDEX_*.
>
> I think we need add new fields in EFI_TD_BOOT_SERVICE_CAPABILITY.
>
> typedef UINT8 EFI_TEE_TYPE; // match https://github.com/tianocore/edk2/blob/master/OvmfPkg/Include/WorkArea.h, NONE = 0, AMD_SEV = 1, INTEL_TDX = 2, we can add more here.
> typedef UINT8 EFI_TEE_SUBTYPE; // TEE-type specific subtype.
[SAMI] This is a good idea. If I understand correctly, there is this
going to be a new enum definition for the TEE architecture (with NONE
=0, AMD_SEV = 1, INTEL_TDX = 2, etc.), and the EFI_TEE_SUBTYPE values
would probably be left to be defined by the respective architectures.
> As such, the caller can know what event log / index it is using.
>
> E.g. If TeeType is TDX, then the INDEX matches the TDX RTMR.
> If TeeType is Realm, then the INDEX matches something else.
[SAMI] Agree.
> I am not sure the usage of PcdMaxMrIndex. We SHALL NOT define PCD in a protocol in general.
> Would you please share your idea on how to use PcdMaxMrIndex? Then we can have better solution.
[SAMI] The TCG2 protocol definition file has MAX_PCR_INDEX. So, I
thought introducing a PCD for MaxMrIndex will provide flexibility for
defining the maximum number of measurement registers provided by
different architectures.
At this point I don't see a usecase other than for validating that the
MaxMrIndex is not exceeded. So, we can drop PcdMaxMrIndex for now.
[/SAMI]
>
>>> +//
>>> +// This bit shall be set when the intent is to measure a PE/COFF image.
>>> +//
>>> +#define PE_COFF_IMAGE 0x0000000000000010
>>> +
>> [SAMI] I think this macro is not needed.
> [Jiewen] This is to align with TCG2 protocol. I think we need to measurement UEFI image.
> Is there any concern to keep it?
[SAMI] I thought EFI_TD_FLAG_PE_COFF_IMAGE above is for the same
purpose. So, thought this was duplicate.
>
>
>>> +#pragma pack (1)
>>> +
>>> +#define EFI_TD_EVENT_HEADER_VERSION 1
>>> +
>>> +typedef struct {
>>> + //
>>> + // Size of the event header itself (sizeof(EFI_TD_EVENT_HEADER)).
>>> + //
>>> + UINT32 HeaderSize;
>>> + //
>>> + // Header version. For this version of this specification, the value shall be 1.
>>> + //
>>> + UINT16 HeaderVersion;
>>> + //
>>> + // Index of the MR that shall be extended.
>>> + //
>>> + EFI_TD_MR_INDEX MrIndex;
>>> + //
>>> + // Type of the event that shall be extended (and optionally logged).
>>> + //
>>> + UINT32 EventType;
>>> +} EFI_TD_EVENT_HEADER;
>>> +
>>> +typedef struct {
>>> + //
>>> + // Total size of the event including the Size component, the header and the
>> Event data.
>>> + //
>>> + UINT32 Size;
>>> + EFI_TD_EVENT_HEADER Header;
>>> + UINT8 Event[1];
>>> +} EFI_TD_EVENT;
>>> +
>>> +#pragma pack()
>>> +
>>> +
>>> +typedef struct {
>>> + //
>>> + // Allocated size of the structure
>>> + //
>>> + UINT8 Size;
>>> + //
>>> + // Version of the EFI_TD_BOOT_SERVICE_CAPABILITY structure itself.
>>> + // For this version of the protocol, the Major version shall be set to 1
>>> + // and the Minor version shall be set to 1.
>>> + //
>>> + EFI_TD_VERSION StructureVersion;
>>> + //
>>> + // Version of the EFI TD protocol.
>>> + // For this version of the protocol, the Major version shall be set to 1
>>> + // and the Minor version shall be set to 1.
>>> + //
>>> + EFI_TD_VERSION ProtocolVersion;
>> [SAMI] Should the protocol version be 1.0 (Major.Minor), as this is the
>> first introduction. Same for the StructureVersion field above.
> [Jiewen] Copy/Past from TCG2.
> Sure. We can start from 1.0 (not 1.1).
>
>>> + //
>>> + // Supported hash algorithms
>>> + //
>>> + EFI_TD_EVENT_ALGORITHM_BITMAP HashAlgorithmBitmap;
>>> + //
>>> + // Bitmap of supported event log formats
>>> + //
>>> + EFI_TD_EVENT_LOG_BITMAP SupportedEventLogs;
>>> +
>>> + //
>>> + // False = TD not present
>>> + //
>>> + BOOLEAN TdPresentFlag;
>> [SAMI] I believe this would need to be renamed to something like
>> TeePresentFlag or CcaPresentFlag or a suitable alternative.
> [Jiewen] Agree. I propose to remove TdPresentFlag.
> Add "EFI_TEE_TYPE TeeType;" // 0 - None, 1 - SEV, 2 - TDX, ...
[SAMI] Ack.
>
>>> +} EFI_TD_BOOT_SERVICE_CAPABILITY;
>>> +
>>> +/**
>>> + The EFI_TD_PROTOCOL GetCapability function call provides protocol
>>> + capability information and state information.
>>> +
>>> + @param[in] This Indicates the calling context
>>> + @param[in, out] ProtocolCapability The caller allocates memory for a
>> EFI_TD_BOOT_SERVICE_CAPABILITY
>>> + structure and sets the size field to the size of the
>> structure allocated.
>>> + The callee fills in the fields with the EFI protocol
>> capability information
>>> + and the current EFI TD state information up to the
>> number of fields which
>>> + fit within the size of the structure passed in.
>>> +
>>> + @retval EFI_SUCCESS Operation completed successfully.
>>> + @retval EFI_DEVICE_ERROR The command was unsuccessful.
>>> + The ProtocolCapability variable will not be populated.
>>> + @retval EFI_INVALID_PARAMETER One or more of the parameters are
>> incorrect.
>>> + The ProtocolCapability variable will not be populated.
>>> + @retval EFI_BUFFER_TOO_SMALL The ProtocolCapability variable is too
>> small to hold the full response.
>>> + It will be partially populated (required Size field will be set).
>>> +**/
>>> +typedef
>>> +EFI_STATUS
>>> +(EFIAPI *EFI_TD_GET_CAPABILITY) (
>>> + IN EFI_TD_PROTOCOL *This,
>>> + IN OUT EFI_TD_BOOT_SERVICE_CAPABILITY *ProtocolCapability
>>> + );
>>> +
>>> +/**
>>> + The EFI_TD_PROTOCOL Get Event Log function call allows a caller to
>>> + retrieve the address of a given event log and its last entry.
>>> +
>>> + @param[in] This Indicates the calling context
>>> + @param[in] EventLogFormat The type of the event log for which the
>> information is requested.
>>> + @param[out] EventLogLocation A pointer to the memory address of the
>> event log.
>>> + @param[out] EventLogLastEntry If the Event Log contains more than one
>> entry, this is a pointer to the
>>> + address of the start of the last entry in the event log in
>> memory.
>>> + @param[out] EventLogTruncated If the Event Log is missing at least one
>> entry because an event would
>>> + have exceeded the area allocated for events, this value is
>> set to TRUE.
>>> + Otherwise, the value will be FALSE and the Event Log will
>> be complete.
>>> +
>>> + @retval EFI_SUCCESS Operation completed successfully.
>>> + @retval EFI_INVALID_PARAMETER One or more of the parameters are
>> incorrect
>>> + (e.g. asking for an event log whose format is not
>> supported).
>>> +**/
>>> +typedef
>>> +EFI_STATUS
>>> +(EFIAPI *EFI_TD_GET_EVENT_LOG) (
>>> + IN EFI_TD_PROTOCOL *This,
>>> + IN EFI_TD_EVENT_LOG_FORMAT EventLogFormat,
>>> + OUT EFI_PHYSICAL_ADDRESS *EventLogLocation,
>>> + OUT EFI_PHYSICAL_ADDRESS *EventLogLastEntry,
>>> + OUT BOOLEAN *EventLogTruncated
>>> + );
>>> +
>>> +/**
>>> + The EFI_TD_PROTOCOL HashLogExtendEvent function call provides callers
>> with
>>> + an opportunity to extend and optionally log events without requiring
>>> + knowledge of actual TD commands.
>>> + The extend operation will occur even if this function cannot create an event
>>> + log entry (e.g. due to the event log being full).
>>> +
>>> + @param[in] This Indicates the calling context
>>> + @param[in] Flags Bitmap providing additional information.
>>> + @param[in] DataToHash Physical address of the start of the data
>> buffer to be hashed.
>>> + @param[in] DataToHashLen The length in bytes of the buffer referenced
>> by DataToHash.
>>> + @param[in] EfiTdEvent Pointer to data buffer containing information
>> about the event.
>>> +
>>> + @retval EFI_SUCCESS Operation completed successfully.
>>> + @retval EFI_DEVICE_ERROR The command was unsuccessful.
>>> + @retval EFI_VOLUME_FULL The extend operation occurred, but the
>> event could not be written to one or more event logs.
>>> + @retval EFI_INVALID_PARAMETER One or more of the parameters are
>> incorrect.
>>> + @retval EFI_UNSUPPORTED The PE/COFF image type is not supported.
>>> +**/
>>> +typedef
>>> +EFI_STATUS
>>> +(EFIAPI * EFI_TD_HASH_LOG_EXTEND_EVENT) (
>>> + IN EFI_TD_PROTOCOL *This,
>>> + IN UINT64 Flags,
>>> + IN EFI_PHYSICAL_ADDRESS DataToHash,
>>> + IN UINT64 DataToHashLen,
>>> + IN EFI_TD_EVENT *EfiTdEvent
>>> + );
>>> +
>>> +/**
>>> + The EFI_TD_PROTOCOL MapPcrToMrIndex function call provides callers
>>> + the info on TPM PCR<-> measurement register mapping information.
>>> +
>>> + In current version, we use below mapping:
>>> + PCR0 -> MRTD (Index 0)
>>> + PCR1 -> RTMR0 (Index 1)
>>> + PCR2~6 -> RTMR1 (Index 2)
>>> + PCR7 -> RTMR0 (Index 1)
>>> + PCR8~15 -> RTMR2 (Index 3)
>>> +
>> [SAMI] I think different architecures may map the PCRs differently. I
>> think the comment could be reworded to a more generic representation of
>> the mapping.
>> Also, I need to check the mailing list if there is a patch that adds the
>> TD protocol implementaiton, and if it could be made generic as well.
>> Maybe the protocol implementation would need to use an architecture
>> specific library that provides the mapping function.
>> [/SAMI]
> [Jiewen] Agree. We should clear up the comment.
> The caller shall be generic enough to use this API to get the mapping.
> The caller shall NOT make any assumption.
[SAMI] Ack.
>
>
>>> + @param[in] This Indicates the calling context
>>> + @param[in] PcrIndex TPM PCR index.
>>> + @param[out] MrIndex Measurement register index.
>>> +
>>> + @retval EFI_SUCCESS The MR index is returned.
>>> + @retval EFI_INVALID_PARAMETER The MrIndex is NULL.
>>> + @retval EFI_UNSUPPORTED The PcrIndex is invalid.
>>> +**/
>>> +typedef
>>> +EFI_STATUS
>>> +(EFIAPI * EFI_TD_MAP_PCR_TO_MR_INDEX) (
>>> + IN EFI_TD_PROTOCOL *This,
>>> + IN TCG_PCRINDEX PcrIndex,
>>> + OUT EFI_TD_MR_INDEX *MrIndex
>>> + );
>>> +
>>> +struct _EFI_TD_PROTOCOL {
>>> + EFI_TD_GET_CAPABILITY GetCapability;
>>> + EFI_TD_GET_EVENT_LOG GetEventLog;
>>> + EFI_TD_HASH_LOG_EXTEND_EVENT HashLogExtendEvent;
>>> + EFI_TD_MAP_PCR_TO_MR_INDEX MapPcrToMrIndex;
>>> +};
>>> +
>>> +
>>> +//
>>> +// TD event log
>>> +//
>>> +
>>> +#pragma pack(1)
>>> +
>>> +//
>>> +// Crypto Agile Log Entry Format.
>>> +// It is similar with TCG_PCR_EVENT2 except the field of MrIndex and
>> PCRIndex.
>>> +//
>>> +typedef struct {
>>> + EFI_TD_MR_INDEX MrIndex;
>>> + UINT32 EventType;
>>> + TPML_DIGEST_VALUES Digests;
>>> + UINT32 EventSize;
>>> + UINT8 Event[1];
>>> +} TD_EVENT;
>>> +
>>> +//
>>> +// EFI TD Event Header
>>> +// It is similar with TCG_PCR_EVENT2_HDR except the field of MrIndex and
>> PCRIndex
>>> +//
>>> +typedef struct {
>>> + EFI_TD_MR_INDEX MrIndex;
>>> + UINT32 EventType;
>>> + TPML_DIGEST_VALUES Digests;
>>> + UINT32 EventSize;
>>> +} TD_EVENT_HDR;
>>> +
>>> +#pragma pack()
>>> +
>>> +//
>>> +// Log entries after Get Event Log service
>>> +//
>>> +
>>> +
>>> +typedef struct {
>>> + //
>>> + // The version of this structure. It shall be set ot 1.
>> [SAMI] It may be good to define a macro for the events table version,
>> similar to EFI_TCG2_FINAL_EVENTS_TABLE_VERSION.
> [Jiewen] Agree.
>
>>> + //
>>> + UINT64 Version;
>>> + //
>>> + // Number of events recorded after invocation of GetEventLog API
>>> + //
>>> + UINT64 NumberOfEvents;
>>> + //
>>> + // List of events of type TD_EVENT.
>>> + //
>>> + //TD_EVENT Event[1];
>>> +} EFI_TD_FINAL_EVENTS_TABLE;
>>> +
>>> +
>>> +#define EFI_TD_FINAL_EVENTS_TABLE_GUID \
>>> + {0xdd4a4648, 0x2de7, 0x4665, {0x96, 0x4d, 0x21, 0xd9, 0xef, 0x5f, 0xb4,
>> 0x46}}
>>> +
>>> +extern EFI_GUID gEfiTdFinalEventsTableGuid;
>>> +
>>> +#endif
>>> diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
>>> index 9cdc915ebae9..a31c44d3a689 100644
>>> --- a/MdePkg/MdePkg.dec
>>> +++ b/MdePkg/MdePkg.dec
>>> @@ -1011,6 +1011,9 @@
>>> ## Include/Protocol/PcdInfo.h
>>> gGetPcdInfoProtocolGuid = { 0x5be40f57, 0xfa68, 0x4610, { 0xbb, 0xbf,
>> 0xe9, 0xc5, 0xfc, 0xda, 0xd3, 0x65 } }
>>> + ## Include/Protocol/TdProtocol.h
>>> + gEfiTdProtocolGuid = { 0x96751a3d, 0x72f4, 0x41a6, { 0xa7, 0x94,
>> 0xed, 0x5d, 0x0e, 0x67, 0xae, 0x6b }}
>>> +
>>> #
>>> # Protocols defined in PI1.0.
>>> #
>>
>>
>>
>>
>
>
>
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [edk2-devel] [PATCH V2 2/3] SecurityPkg: Support TdProtocol in DxeTpm2MeasureBootLib
2021-10-19 13:22 ` [edk2-devel] " Sami Mujawar
@ 2021-10-27 5:19 ` Min Xu
2021-11-01 13:35 ` Sami Mujawar
0 siblings, 1 reply; 20+ messages in thread
From: Min Xu @ 2021-10-27 5:19 UTC (permalink / raw)
To: devel@edk2.groups.io, sami.mujawar@arm.com
Cc: Kinney, Michael D, Liming Gao, Liu, Zhiguang, Yao, Jiewen,
Wang, Jian J, nd, Joey Gouly
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [edk2-devel] [PATCH V2 2/3] SecurityPkg: Support TdProtocol in DxeTpm2MeasureBootLib
2021-10-27 5:19 ` Min Xu
@ 2021-11-01 13:35 ` Sami Mujawar
0 siblings, 0 replies; 20+ messages in thread
From: Sami Mujawar @ 2021-11-01 13:35 UTC (permalink / raw)
To: Min Xu, devel
[-- Attachment #1: Type: text/plain, Size: 1332 bytes --]
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.
[-- Attachment #2: Type: text/html, Size: 1434 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2021-11-01 13:35 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-10-08 5:21 [PATCH V2 0/3] Introduce TdProtocol into EDK2 Min Xu
2021-10-08 5:21 ` [PATCH V2 1/3] MdePkg: Introduce TdProtocol for TD-Guest firmware Min Xu
2021-10-11 1:37 ` 回复: " gaoliming
2021-10-19 13:21 ` [edk2-devel] " Sami Mujawar
2021-10-19 14:40 ` Yao, Jiewen
2021-10-20 9:26 ` Sami Mujawar
2021-10-08 5:21 ` [PATCH V2 2/3] SecurityPkg: Support TdProtocol in DxeTpm2MeasureBootLib Min Xu
2021-10-19 13:22 ` [edk2-devel] " Sami Mujawar
2021-10-27 5:19 ` Min Xu
2021-11-01 13:35 ` Sami Mujawar
2021-10-08 5:21 ` [PATCH V2 3/3] SecurityPkg: Support TdProtocol in DxeTpmMeasurementLib Min Xu
2021-10-19 13:24 ` [edk2-devel] " Sami Mujawar
2021-10-12 15:26 ` [edk2-devel] [PATCH V2 0/3] Introduce TdProtocol into EDK2 Sami Mujawar
2021-10-14 5:41 ` Min Xu
2021-10-14 11:59 ` Yao, Jiewen
[not found] ` <16ADE3D948B3147A.7007@groups.io>
2021-10-14 13:43 ` Yao, Jiewen
2021-10-18 12:59 ` Sami Mujawar
2021-10-18 13:06 ` Yao, Jiewen
2021-10-19 9:51 ` Sami Mujawar
2021-10-19 13:06 ` Min Xu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox