Re: [edk2-devel] [PATCH V2 1/3] MdePkg: Introduce TdProtocol for TD-Guest firmware

["Sami Mujawar" <sami.mujawar@arm.com>]

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.
>>>      #
>>
>>
>>
>>
>
>
> 
>
>