RE: [PATCH] Enable TDX measurement to RTMR register via EFI_CC_MEASUREMENT_PROTOCOL

[Lu, Ken]

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 <dkiper@net-space.pl>
> Sent: Friday, February 25, 2022 2:47 AM
> To: Lu, Ken <ken.lu@intel.com>
> Cc: grub-devel@gnu.org; Xu, Min M <min.m.xu@intel.com>
> 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 <ken.lu@intel.com>
> > ---
> >  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/devel@edk2.groups.io/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