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...@intel.com>
Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Liming Gao
<gaolim...@byosoft.com.cn>; Liu, Zhiguang <zhiguang....@intel.com>; Yao,
Jiewen <jiewen....@intel.com>; Wang, Jian J <jian.j.w...@intel.com>; Lu, Ken
<ken...@intel.com>; nd <n...@arm.com>; Joey Gouly <joey.go...@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.kin...@intel.com>
Cc: Liming Gao <gaolim...@byosoft.com.cn>
Cc: Zhiguang Liu <zhiguang....@intel.com>
Cc: Jiewen Yao <jiewen....@intel.com>
Cc: Jian J Wang <jian.j.w...@intel.com>
Cc: Ken Lu <ken...@intel.com>
Reviewed-by: Jiewen Yao <jiewen....@intel.com>
Signed-off-by: Min Xu <min.m...@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.
#
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#82416): https://edk2.groups.io/g/devel/message/82416
Mute This Topic: https://groups.io/mt/86163958/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
