Re: [PATCH V3] Enable TDX measurement to RTMR register

[Daniel Kiper]

On Fri, May 20, 2022 at 09:41:28PM +0800, 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) is required to provide TD services to
> the TD guest OS.[2] Its reference code is available at 
> https://github.com/tianocore/edk2-staging/tree/TDVF.
>
> To support TD measurement/attestation, TDs provide 4 RTMR registers like
> TPM/TPM2 PCR as below:
> - 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 TD Measurement protocol support along with TPM/TPM2 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
>
> Signed-off-by: Lu Ken <ken.lu@intel.com>
> ---
>
> Notes:
>     v3 changes:
>       - Change the grub_efi_cc_event->Event[1] to grub_efi_cc_event->Event[0]
>       - Merge grub-core/commands/efi/cc.c to grub-core/commands/efi/tpm.c
>       - Uses grub_strcpy() instead of grub_memcpy()
>       - Change local variable tpm and cc to static, judge whether was 
> initialized for each log_event call
>       - Other minor updates for coding style
>     v2 changes:
>       - Separate the CC_MEASUREMENT code to standalone file 
> grub-core/commands/efi/cc.c
>       - Use NULL explicity for the judgement like "if (cc != NULL)"
>       - Include all headers for all types, structure used in cc.h
>       - Add the prefix "GRUB_"for all macro definition and the prefix "grub_" 
> for type or structure's field
>       - Align one argument in one line
>
>  grub-core/commands/efi/tpm.c |  78 ++++++++++++++---
>  include/grub/efi/cc.h        | 164 +++++++++++++++++++++++++++++++++++
>  2 files changed, 229 insertions(+), 13 deletions(-)
>  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..9cb5e9ac2 100644
> --- a/grub-core/commands/efi/tpm.c
> +++ b/grub-core/commands/efi/tpm.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,7 @@ 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 cc_measurement_guid = 
> GRUB_EFI_CC_MEASUREMENT_PROTOCOL_GUID;
>
>  static grub_efi_handle_t *grub_tpm_handle;
>  static grub_uint8_t grub_tpm_version;
> @@ -135,17 +137,17 @@ grub_efi_log_event_status (grub_efi_status_t status)
>    switch (status)
>      {
>      case GRUB_EFI_SUCCESS:
> -      return 0;
> +      return GRUB_ERR_NONE;
>      case GRUB_EFI_DEVICE_ERROR:
> -      return grub_error (GRUB_ERR_IO, N_("Command failed"));
> +      return grub_error (GRUB_ERR_IO, N_("command failed"));
>      case GRUB_EFI_INVALID_PARAMETER:
> -      return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("Invalid parameter"));
> +      return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("invalid parameter"));
>      case GRUB_EFI_BUFFER_TOO_SMALL:
> -      return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("Output buffer too 
> small"));
> +      return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("output buffer too 
> small"));
>      case GRUB_EFI_NOT_FOUND:
>        return grub_error (GRUB_ERR_UNKNOWN_DEVICE, N_("TPM unavailable"));
>      default:
> -      return grub_error (GRUB_ERR_UNKNOWN_DEVICE, N_("Unknown TPM error"));
> +      return grub_error (GRUB_ERR_UNKNOWN_DEVICE, N_("unknown TPM error"));

I like these grub_efi_log_event_status() fixes but they should be done
in separate preparatory patch.

>      }
>  }
>
> @@ -156,13 +158,14 @@ grub_tpm1_log_event (grub_efi_handle_t tpm_handle, 
> unsigned char *buf,
>  {
>    grub_tpm_event_t *event;
>    grub_efi_status_t status;
> -  grub_efi_tpm_protocol_t *tpm;
> +  static grub_efi_tpm_protocol_t *tpm = NULL;
>    grub_efi_physical_address_t lastevent;
>    grub_uint32_t algorithm;
>    grub_uint32_t eventnum = 0;
>
> -  tpm = grub_efi_open_protocol (tpm_handle, &tpm_guid,
> -                             GRUB_EFI_OPEN_PROTOCOL_GET_PROTOCOL);
> +  if (tpm == NULL)
> +    tpm = grub_efi_open_protocol (tpm_handle, &tpm_guid,
> +                               GRUB_EFI_OPEN_PROTOCOL_GET_PROTOCOL);

This is not fully correct. Think what will happen when
grub_efi_open_protocol() fails.

I think we have two options here:
  - leave code as is,
  - fix it properly.

The latter would require quite significant overhaul of existing code.
So, I prefer to leave this code as is.

>    if (!grub_tpm1_present (tpm))
>      return 0;
> @@ -175,7 +178,7 @@ grub_tpm1_log_event (grub_efi_handle_t tpm_handle, 
> unsigned char *buf,
>    event->PCRIndex = pcr;
>    event->EventType = EV_IPL;
>    event->EventSize = grub_strlen (description) + 1;
> -  grub_memcpy (event->Event, description, event->EventSize);
> +  grub_strcpy (event->Event, description);

Nice but this begs for another separate patch.

>    algorithm = TCG_ALG_SHA;
>    status = efi_call_7 (tpm->log_extend_event, tpm, (grub_addr_t) buf, 
> (grub_uint64_t) size,
> @@ -192,10 +195,11 @@ grub_tpm2_log_event (grub_efi_handle_t tpm_handle, 
> unsigned char *buf,
>  {
>    EFI_TCG2_EVENT *event;
>    grub_efi_status_t status;
> -  grub_efi_tpm2_protocol_t *tpm;
> +  static grub_efi_tpm2_protocol_t *tpm = NULL;
>
> -  tpm = grub_efi_open_protocol (tpm_handle, &tpm2_guid,
> -                             GRUB_EFI_OPEN_PROTOCOL_GET_PROTOCOL);
> +  if (tpm == NULL)
> +    tpm = grub_efi_open_protocol (tpm_handle, &tpm2_guid,
> +                               GRUB_EFI_OPEN_PROTOCOL_GET_PROTOCOL);

The same comment as to the earlier grub_efi_open_protocol() call.

>    if (!grub_tpm2_present (tpm))
>      return 0;
> @@ -212,7 +216,7 @@ grub_tpm2_log_event (grub_efi_handle_t tpm_handle, 
> unsigned char *buf,
>    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);
> +  grub_strcpy (event->Event, description);
>
>    status = efi_call_5 (tpm->hash_log_extend_event, tpm, 0, (grub_addr_t) buf,
>                      (grub_uint64_t) size, event);
> @@ -221,6 +225,52 @@ grub_tpm2_log_event (grub_efi_handle_t tpm_handle, 
> unsigned char *buf,
>    return grub_efi_log_event_status (status);
>  }
>
> +static
> +void
> +grub_cc_log_event (unsigned char *buf, grub_size_t size, grub_uint8_t pcr,
> +                const char *description)
> +{
> +  grub_efi_cc_event_t *event;
> +  grub_efi_status_t status;
> +  static grub_efi_cc_protocol_t *cc = NULL;
> +  grub_efi_cc_mr_index_t mr;
> +
> +  if (cc == NULL)
> +    if ((cc = grub_efi_locate_protocol (&cc_measurement_guid, NULL)) == NULL)
> +      return;

Ditto. And even we have to add much more code here. Additionally, I have
chatted with some UEFI folks and they told me frequent 
grub_efi_locate_protocol()
calls should not have huge impact on performance. So, I think we should
not optimize the code at this point if gains in comparison to required
changes are potentially insignificant. Though if we see some performance
issues here we can revisit this later.

> +  status = efi_call_3 (cc->map_pcr_to_mr_index, cc, pcr, &mr)
> +  if (status != GRUB_EFI_SUCCESS)
> +    {
> +      grub_efi_log_event_status (status);
> +      return;
> +    }
> +
> +  event = grub_zalloc (sizeof (grub_efi_cc_event_t)
> +                    + grub_strlen (description) + 1);
> +  if (event == NULL)
> +    {
> +      grub_error (GRUB_ERR_OUT_OF_MEMORY,
> +               N_("cannot allocate CC event buffer"));
> +      return;
> +    }
> +
> +  event->Header.HeaderSize = sizeof (grub_efi_cc_event_header_t);
> +  event->Header.HeaderVersion = GRUB_EFI_CC_EVENT_HEADER_VERSION;
> +  event->Header.MrIndex = mr;
> +  event->Header.EventType = EV_IPL;
> +  event->Size = sizeof (*event) + grub_strlen (description) + 1;
> +  grub_strcpy (event->Event, description);
> +
> +  status = efi_call_5 (cc->hash_log_extend_event, cc, 0,
> +                    (grub_efi_physical_address_t) buf,
> +                    (grub_efi_uint64_t) size, event);
> +  grub_free (event);
> +
> +  if (status != GRUB_EFI_SUCCESS)
> +    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 +278,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);
> +

Hmmm... Should not this be behind the "if" below?

>    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..07b3bc755
> --- /dev/null
> +++ b/include/grub/efi/cc.h
> @@ -0,0 +1,164 @@
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2022  Free Software Foundation, Inc.
> + *
> + *  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 <grub/efi/api.h>
> +#include <grub/efi/efi.h>
> +#include <grub/err.h>
> +
> +#define GRUB_EFI_CC_MEASUREMENT_PROTOCOL_GUID \
> +  { 0x96751a3d, 0x72f4, 0x41a6, \
> +    { 0xa7, 0x94, 0xed, 0x5d, 0x0e, 0x67, 0xae, 0x6b } \
> +  };
> +
> +struct grub_efi_cc_version
> +{
> +  grub_efi_uint8_t Major;
> +  grub_efi_uint8_t Minor;
> +};
> +typedef struct grub_efi_cc_version grub_efi_cc_version_t;
> +
> +/*
> + * EFI_CC Type/SubType definition
> + */
> +#define GRUB_EFI_CC_TYPE_NONE        0
> +#define GRUB_EFI_CC_TYPE_SEV 1
> +#define GRUB_EFI_CC_TYPE_TDX 2
> +
> +struct grub_efi_cc_type
> +{
> +  grub_efi_uint8_t Type;
> +  grub_efi_uint8_t SubType;
> +};
> +typedef struct grub_efi_cc_type grub_efi_cc_type_t;
> +
> +typedef grub_efi_uint32_t grub_efi_cc_event_log_bitmap_t;
> +typedef grub_efi_uint32_t grub_efi_cc_event_log_format_t;
> +typedef grub_efi_uint32_t grub_efi_cc_event_algorithm_bitmap_t;
> +typedef grub_efi_uint32_t grub_efi_cc_mr_index_t;
> +
> +/*
> + * Intel TDX measure register index
> + */
> +#define GRUB_TDX_MR_INDEX_MRTD       0
> +#define GRUB_TDX_MR_INDEX_RTMR0      1
> +#define GRUB_TDX_MR_INDEX_RTMR1      2
> +#define GRUB_TDX_MR_INDEX_RTMR2      3
> +#define GRUB_TDX_MR_INDEX_RTMR3      4
> +
> +#define GRUB_EFI_CC_EVENT_LOG_FORMAT_TCG_2   0x00000002
> +#define GRUB_EFI_CC_BOOT_HASH_ALG_SHA384     0x00000004
> +#define GRUB_EFI_CC_EVENT_HEADER_VERSION     1
> +
> +struct grub_efi_cc_event_header
> +{
> +  /*
> +   * Size of the event header itself (sizeof(EFI_TD_EVENT_HEADER))
> +   */

Please format one line comments in this way:

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

Ditto.

> +  grub_efi_cc_mr_index_t MrIndex;
> +
> +  /*
> +   * Type of the event that shall be extended (and optionally logged)
> +   */

Ditto.

> +  grub_efi_uint32_t      EventType;
> +} GRUB_PACKED;
> +typedef struct grub_efi_cc_event_header grub_efi_cc_event_header_t;
> +
> +struct grub_efi_cc_event
> +{
> +  /*
> +   * Total size of the event including the Size component, the header and the
> +   * Event data.
> +   */
> +  grub_efi_uint32_t          Size;
> +  grub_efi_cc_event_header_t Header;
> +  grub_efi_uint8_t           Event[0];
> +} GRUB_PACKED;
> +typedef struct grub_efi_cc_event grub_efi_cc_event_t;
> +
> +struct grub_efi_cc_boot_service_capability
> +{
> +  /* Allocated size of the structure */

... like this one...

> +  grub_efi_uint8_t                     Size;
> +
> +  /*
> +   * Version of the grub_efi_cc_boot_service_capability_t 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.
> +   */
> +  grub_efi_cc_version_t                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.
> +   */
> +  grub_efi_cc_version_t                ProtocolVersion;
> +
> +  /* Supported hash algorithms */
> +  grub_efi_cc_event_algorithm_bitmap_t HashAlgorithmBitmap;
> +
> +  /* Bitmap of supported event log formats */
> +  grub_efi_cc_event_log_bitmap_t       SupportedEventLogs;
> +
> +  /* Indicates the CC type */
> +  grub_efi_cc_type_t CcType;
> +};
> +typedef struct grub_efi_cc_boot_service_capability 
> grub_efi_cc_boot_service_capability_t;
> +
> +struct grub_efi_cc_protocol
> +{
> +  grub_efi_status_t (*get_capability) (
> +       struct grub_efi_cc_protocol *this,
> +       grub_efi_cc_boot_service_capability_t *ProtocolCapability);

grub_efi_status_t (*get_capability) (struct grub_efi_cc_protocol *this,
                                     grub_efi_cc_boot_service_capability_t 
*ProtocolCapability);

> +  grub_efi_status_t (*get_event_log) (
> +       struct grub_efi_cc_protocol *this,
> +       grub_efi_cc_event_log_format_t EventLogFormat,
> +       grub_efi_physical_address_t *EventLogLocation,
> +       grub_efi_physical_address_t *EventLogLastEntry,
> +       grub_efi_boolean_t *EventLogTruncated);

grub_efi_status_t (*get_event_log) (struct grub_efi_cc_protocol *this,
                                    grub_efi_cc_event_log_format_t 
EventLogFormat,
                                    grub_efi_physical_address_t 
*EventLogLocation,
                                    grub_efi_physical_address_t 
*EventLogLastEntry,
                                    grub_efi_boolean_t *EventLogTruncated);

... and below please...

Good examples are in include/grub/efi/api.h too...

> +  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,
> +       grub_efi_cc_event_t *EfiCcEvent);
> +  grub_efi_status_t (*map_pcr_to_mr_index) (
> +       struct grub_efi_cc_protocol *this,
> +       grub_efi_uint32_t PcrIndex,
> +       grub_efi_cc_mr_index_t *MrIndex);
> +};
> +typedef struct grub_efi_cc_protocol grub_efi_cc_protocol_t;
> +
> +grub_err_t grub_cc_log_event (unsigned char *buf, grub_size_t size,
> +                           grub_uint8_t pcr, const char *description);

Daniel