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

[Daniel Kiper]

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/documents/tdx-whitepaper-v4.pdf
> [2] 
> https://software.intel.com/content/dam/develop/external/us/en/documents/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.

> @@ -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)

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

> +    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?

Additionally, "(unsigned long)buf" cast looks incorrect. I think it should
be "grub_efi_physical_address_t (buf)".

> +                       (grub_uint64_t)size, event);

"(grub_efi_uint64_t) size" instead of "(grub_uint64_t)size"

> +  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/

> + *
> + *  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.

> +#define EFI_CC_MEASUREMENT_PROTOCOL_GUID \

s/EFI_CC_MEASUREMENT_PROTOCOL_GUID/GRUB_EFI_CC_MEASUREMENT_PROTOCOL_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.

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.

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

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

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

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

> +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?

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

> +typedef struct grub_efi_cc_protocol grub_efi_cc_protocol_t;

Daniel