Thanks Kiper's review and comment, sorry for later response since vacation. Please see my answer in below, and will submit new patch soon.
> -----Original Message----- > From: Daniel Kiper <[email protected]> > Sent: Friday, February 25, 2022 2:47 AM > To: Lu, Ken <[email protected]> > Cc: [email protected]; Xu, Min M <[email protected]> > Subject: Re: [PATCH] Enable TDX measurement to RTMR register via > EFI_CC_MEASUREMENT_PROTOCOL > > On Tue, Dec 28, 2021 at 10:03:13PM -0500, Lu Ken wrote: > > Intel Trust Domain Extensions(Intel TDX) refers to an Intel technology > > that extends Virtual Machine Extensions(VMX) and Multi-Key Total > > Memory > > Encryption(MK-TME) with a new kind of virtual machine guest called a > > Trust Domain(TD)[1]. A TD runs in a CPU mode that protects the > > confidentiality of its memory contents and its CPU state from any > > other software, including the hosting Virtual Machine Monitor (VMM). > > > > Trust Domain Virtual Firmware (TDVF)[2] is required to provide TD > > services to the TD guest OS. Its reference code is available at > https://github.com/tianocore/edk2-staging/tree/TDVF. > > > > To support TDX attestation, below 4 RTMR registers are used like TPM/TPM2: > > - RTMR[0] is for TDVF configuration > > - RTMR[1] is for the TD OS loader and kernel > > - RTMR[2] is for the OS application > > - RTMR[3] is reserved for special usage only > > > > This patch adds TDX measurement via EFI_CC_MEASUREMENT_PROTOCOL[3] > > along with TPM/TPM2 TCG protocol. > > > > References: > > [1] > > https://software.intel.com/content/dam/develop/external/us/en/document > > s/tdx-whitepaper-v4.pdf [2] > > https://software.intel.com/content/dam/develop/external/us/en/document > > s/tdx-virtual-firmware-design-guide-rev-1.pdf > > [3] > > https://github.com/tianocore/edk2/blob/master/MdePkg/Include/Protocol/ > > CcMeasurement.h > > > > Signed-off-by: Lu Ken <[email protected]> > > --- > > grub-core/commands/efi/tpm.c | 44 ++++++++++ > > include/grub/efi/cc.h | 156 > +++++++++++++++++++++++++++++++++++ > > 2 files changed, 200 insertions(+) > > create mode 100644 include/grub/efi/cc.h > > > > diff --git a/grub-core/commands/efi/tpm.c > > b/grub-core/commands/efi/tpm.c index a97d85368..33b70e791 100644 > > --- a/grub-core/commands/efi/tpm.c > > +++ b/grub-core/commands/efi/tpm.c > > This is Intel specific thing (is not it?) and should not be build into > generic TPM > module. Please create separate TDX module, e.g., grub- > core/commands/i386/efi/tdx.c. [Lu, Ken] EFI_CC_MEASUREMENT_PROTOCOL is not Intel specific, but agreement between AMD/Intel and ARM, please see https://www.mail-archive.com/[email protected]/msg38720.html > > > @@ -22,6 +22,7 @@ > > #include <grub/i18n.h> > > #include <grub/efi/api.h> > > #include <grub/efi/efi.h> > > +#include <grub/efi/cc.h> > > #include <grub/efi/tpm.h> > > #include <grub/mm.h> > > #include <grub/tpm.h> > > @@ -31,6 +32,8 @@ typedef TCG_PCR_EVENT grub_tpm_event_t; > > > > static grub_efi_guid_t tpm_guid = EFI_TPM_GUID; static > > grub_efi_guid_t tpm2_guid = EFI_TPM2_GUID; > > +static grub_efi_guid_t tdx_guid = EFI_TPM2_GUID; static > > +grub_efi_guid_t cc_measurement_guid = > > +EFI_CC_MEASUREMENT_PROTOCOL_GUID; > > > > static grub_efi_handle_t *grub_tpm_handle; static grub_uint8_t > > grub_tpm_version; @@ -221,6 +224,45 @@ grub_tpm2_log_event > > (grub_efi_handle_t tpm_handle, unsigned char *buf, > > return grub_efi_log_event_status (status); } > > > > +static > > +grub_err_t > > +grub_cc_log_event (unsigned char *buf, grub_size_t size, grub_uint8_t pcr, > > + const char *description) { > > + EFI_CC_EVENT *event; > > + grub_efi_status_t status; > > + grub_efi_cc_protocol_t *cc; > > + EFI_CC_MR_INDEX mr; > > + > > + cc = grub_efi_locate_protocol (&cc_measurement_guid, NULL); > > + > > + if (!cc) > > Please use NULL explicitly, i.e., if (cc == NULL) [Lu, Ken] Thanks for your comment, will fix in next version. > > > + return 0; > > + > > + status = efi_call_3 (cc->map_pcr_to_mr_index, cc, pcr, &mr); if > > + (status != GRUB_EFI_SUCCESS) > > + return grub_efi_log_event_status (status); > > + > > + event = grub_zalloc (sizeof (EFI_CC_EVENT) + grub_strlen > > + (description) + 1); if (!event) > > Ditto. [Lu, Ken] Thanks for your comment, will fix in next version. > > > + return grub_error (GRUB_ERR_OUT_OF_MEMORY, > > + N_ ("cannot allocate CC event buffer")); > > + > > + event->Header.HeaderSize = sizeof (EFI_CC_EVENT_HEADER); > > + event->Header.HeaderVersion = EFI_CC_EVENT_HEADER_VERSION; > > + event->Header.MrIndex = mr; event->Header.EventType = EV_IPL; > > + event->Size = sizeof (*event) - sizeof (event->Event) > > + + grub_strlen (description) + 1; grub_memcpy > > + (event->Event, description, grub_strlen (description) + 1); > > + > > + status = efi_call_5 (cc->hash_log_extend_event, cc, 0, (unsigned > > + long)buf, > > What does 0 mean? Could use a constant here? [Lu, Ken] It is defined at https://github.com/tianocore/edk2/blob/62fa37fe7b9df3c54a2d9d90aed9ff0e817ee0c6/MdePkg/Include/Protocol/CcMeasurement.h#L197 from the TCG specification, https://trustedcomputinggroup.org/wp-content/uploads/EFI-Protocol-Specification-rev13-160330final.pdf page 33, the flag value could be 0 or below: #define EFI_TCG2_EXTEND_ONLY 0x0000000000000001 #define PE_COFF_IMAGE 0x0000000000000010 Similar code for TPM at https://git.savannah.gnu.org/cgit/grub.git/tree/grub-core/commands/efi/tpm.c?h=grub-2.06#n217 > > Additionally, "(unsigned long)buf" cast looks incorrect. I think it should be > "grub_efi_physical_address_t (buf)". [Lu, Ken] Thanks for your comment, yes, grub_efi_physical_address_t is much better. > > > + (grub_uint64_t)size, event); > > "(grub_efi_uint64_t) size" instead of "(grub_uint64_t)size" [Lu, Ken] Thanks for your comment, will do > > > + grub_free (event); > > + > > + return grub_efi_log_event_status (status); } > > + > > grub_err_t > > grub_tpm_measure (unsigned char *buf, grub_size_t size, grub_uint8_t pcr, > > const char *description) > > @@ -228,6 +270,8 @@ grub_tpm_measure (unsigned char *buf, grub_size_t > size, grub_uint8_t pcr, > > grub_efi_handle_t tpm_handle; > > grub_efi_uint8_t protocol_version; > > > > + grub_cc_log_event(buf, size, pcr, description); > > + > > if (!grub_tpm_handle_find (&tpm_handle, &protocol_version)) > > return 0; > > > > diff --git a/include/grub/efi/cc.h b/include/grub/efi/cc.h new file > > mode 100644 index 000000000..e49cdf5d9 > > --- /dev/null > > +++ b/include/grub/efi/cc.h > > @@ -0,0 +1,156 @@ > > +/* > > + * GRUB -- GRand Unified Bootloader > > + * Copyright (C) 2015 Free Software Foundation, Inc. > > s/2015/2022/ [Lu, Ken] Thanks for your comment, will fix. > > > + * > > + * GRUB is free software: you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License as published > > + by > > + * the Free Software Foundation, either version 3 of the License, or > > + * (at your option) any later version. > > + * > > + * GRUB is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + * > > + * You should have received a copy of the GNU General Public License > > + * along with GRUB. If not, see <http://www.gnu.org/licenses/>. > > + */ > > + > > +#ifndef GRUB_EFI_CC_HEADER > > +#define GRUB_EFI_CC_HEADER 1 > > Include all headers here which define all types, structures, etc. used below. [Lu, Ken] Thanks for your comment, will fix. > > > +#define EFI_CC_MEASUREMENT_PROTOCOL_GUID \ > > s/EFI_CC_MEASUREMENT_PROTOCOL_GUID/GRUB_EFI_CC_MEASUREMENT_PR > OTOCOL_GUID/ > > > + { 0x96751a3d, 0x72f4, 0x41a6, \ > > + { 0xa7, 0x94, 0xed, 0x5d, 0x0e, 0x67, 0xae, 0x6b } \ }; > > + > > +typedef struct > > +{ > > + grub_efi_uint8_t Major; > > + grub_efi_uint8_t Minor; > > +} EFI_CC_VERSION; > > Are you entirely sure this structure should not be packed? And I can see > similar > thing in the EDK2 implementation. [Lu, Ken] Confirmed with EDK2 owner @Xu, Min M, this structure is not required for pack. Both EFI_TCG2_VERSION and EFI_CC_VERSION are not packed. > > And in general the definition should look more or less like this: > > struct grub_efi_cc_version > { > grub_efi_uint8_t major; > grub_efi_uint8_t minor; > }; > typedef struct grub_efi_cc_version_t; > > > + > > +/* > > + * EFI_CC Type/SubType definition > > + */ > > +#define EFI_CC_TYPE_NONE 0 > > +#define EFI_CC_TYPE_SEV 1 > > +#define EFI_CC_TYPE_TDX 2 > > Please add "GRUB_" prefix to this definitions and align values in one column > with > tabs. [Lu, Ken] Thanks for your comment, will fix. > > > +typedef struct > > +{ > > + grub_efi_uint8_t Type; > > + grub_efi_uint8_t SubType; > > +} EFI_CC_TYPE; > > Same questions and suggestions like in case of EFI_CC_VERSION struct. [Lu, Ken] Like EFI_CC_VERSION, it is not required to pack after confirmed with edk2 owner. > > > +typedef grub_efi_uint32_t EFI_CC_EVENT_LOG_BITMAP; > > typedef grub_efi_uint32_t grub_efi_cc_event_log_bitmap; > > And please update all typedefs below accordingly. [Lu, Ken] Thanks for your comment, will fix. > > > +typedef grub_efi_uint32_t EFI_CC_EVENT_LOG_FORMAT; typedef > > +grub_efi_uint32_t EFI_CC_EVENT_ALGORITHM_BITMAP; typedef > > +grub_efi_uint32_t EFI_CC_MR_INDEX; > > + > > +/* > > + * Intel TDX measure register index > > + */ > > +#define TDX_MR_INDEX_MRTD 0 > > +#define TDX_MR_INDEX_RTMR0 1 > > +#define TDX_MR_INDEX_RTMR1 2 > > +#define TDX_MR_INDEX_RTMR2 3 > > +#define TDX_MR_INDEX_RTMR3 4 > > + > > +#define EFI_CC_EVENT_LOG_FORMAT_TCG_2 0x00000002 #define > > +EFI_CC_BOOT_HASH_ALG_SHA384 0x00000004 #define > > +EFI_CC_EVENT_HEADER_VERSION 1 > > Please add to all defines above "GRUB_" prefix. [Lu, Ken] Thanks for your comment, will fix. > > > +typedef struct tdEFI_CC_EVENT_HEADER > > +{ > > + /* > > + * Size of the event header itself (sizeof(EFI_TD_EVENT_HEADER)) > > + */ > > + grub_efi_uint32_t HeaderSize; > > + > > + /* > > + * Header version. For this version of this specification, > > + * the value shall be 1. > > + */ > > + grub_efi_uint16_t HeaderVersion; > > + > > + /* > > + * Index of the MR that shall be extended > > + */ > > + EFI_CC_MR_INDEX MrIndex; > > + > > + /* > > + * Type of the event that shall be extended (and optionally logged) > > + */ > > + grub_efi_uint32_t EventType; > > +} GRUB_PACKED EFI_CC_EVENT_HEADER; > > Please define this type as I showed above. [Lu, Ken] Thanks for your comment, will fix. > > > +typedef struct tdEFI_CC_EVENT > > +{ > > + /* > > + * Total size of the event including the Size component, the header and > > the > > + * Event data. > > + */ > > + grub_efi_uint32_t Size; > > + EFI_CC_EVENT_HEADER Header; > > + grub_efi_uint8_t Event[1]; > > +} GRUB_PACKED EFI_CC_EVENT; > > Ditto. Additionally, please align member names in one column. > > > +typedef struct > > +{ > > + /* Allocated size of the structure */ > > + grub_efi_uint8_t Size; > > + > > + /* > > + * Version of the EFI_CC_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_CC_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_CC_VERSION ProtocolVersion; > > + > > + /* > > + * Supported hash algorithms > > + */ > > + EFI_CC_EVENT_ALGORITHM_BITMAP HashAlgorithmBitmap; > > + > > + /* > > + * Bitmap of supported event log formats > > + */ > > + EFI_CC_EVENT_LOG_BITMAP SupportedEventLogs; > > + > > + /* > > + * Indicates the CC type > > + */ > > + EFI_CC_TYPE CcType; > > +} EFI_CC_BOOT_SERVICE_CAPABILITY; > > Same comments as above. > > > +struct grub_efi_cc_protocol > > +{ > > + grub_efi_status_t (*get_capability) ( > > + struct grub_efi_cc_protocol *this, > > + EFI_CC_BOOT_SERVICE_CAPABILITY *ProtocolCapability); > > + grub_efi_status_t (*get_event_log) ( > > + struct grub_efi_cc_protocol *this, > > + EFI_CC_EVENT_LOG_FORMAT EventLogFormat, > > + grub_efi_physical_address_t *EventLogLocation, > > + grub_efi_physical_address_t *EventLogLastEntry, > > + grub_efi_boolean_t *EventLogTruncated); > > + grub_efi_status_t (*hash_log_extend_event) ( > > + struct grub_efi_cc_protocol *this, grub_efi_uint64_t Flags, > > + grub_efi_physical_address_t DataToHash, grub_efi_uint64_t > > +DataToHashLen, > > Could you put one argument in one line like, e.g., for get_event_log function? [Lu, Ken] Thanks for your comment, will fix. > > > + EFI_CC_EVENT *EfiCcEvent); > > + grub_efi_status_t (*map_pcr_to_mr_index) (struct grub_efi_cc_protocol > *this, > > + grub_efi_uint32_t PcrIndex, > > + EFI_CC_MR_INDEX > > +*MrIndex); }; > > + > > Please drop this empty line. [Lu, Ken] Thanks for your comment, will fix. > > > +typedef struct grub_efi_cc_protocol grub_efi_cc_protocol_t; > > Daniel _______________________________________________ Grub-devel mailing list [email protected] https://lists.gnu.org/mailman/listinfo/grub-devel
