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

[Daniel Kiper]

First of all, sorry for late reply but I am busy...

On Mon, Mar 14, 2022 at 02:52:22PM +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:
>     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/Makefile.core.def  |   1 +
>  grub-core/commands/efi/cc.c  |  88 +++++++++++++++++++
>  grub-core/commands/efi/tpm.c |   3 +
>  include/grub/efi/cc.h        | 164 +++++++++++++++++++++++++++++++++++
>  4 files changed, 256 insertions(+)
>  create mode 100644 grub-core/commands/efi/cc.c
>  create mode 100644 include/grub/efi/cc.h
>
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index 6b00eb555..dbae796a7 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -2561,6 +2561,7 @@ module = {
>    name = tpm;
>    common = commands/tpm.c;
>    efi = commands/efi/tpm.c;
> +  efi = commands/efi/cc.c;
>    enable = efi;
>  };
>
> diff --git a/grub-core/commands/efi/cc.c b/grub-core/commands/efi/cc.c
> new file mode 100644
> index 000000000..0e6b417c4
> --- /dev/null
> +++ b/grub-core/commands/efi/cc.c
> @@ -0,0 +1,88 @@
> +/*
> + *  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/>.
> + *
> + *  EFI CC measurement support code.

You told me this feature is not Intel specific. In this case I would
move back this code to the grub-core/commands/efi/tpm.c. Sorry about
that...

> + */
> +
> +#include <grub/err.h>
> +#include <grub/i18n.h>
> +#include <grub/efi/api.h>
> +#include <grub/efi/efi.h>
> +#include <grub/efi/cc.h>
> +#include <grub/tpm.h>
> +#include <grub/mm.h>
> +
> +static grub_efi_guid_t cc_measurement_guid = 
> GRUB_EFI_CC_MEASUREMENT_PROTOCOL_GUID;
> +
> +static inline grub_err_t

Please drop inline from here. Compiler will know how to optimize this
function properly.

> +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"));

s/Command failed/command failed/

Most error messages should start with lowercase.

> +    case GRUB_EFI_INVALID_PARAMETER:
> +      return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("Invalid parameter"));

s/Invalid parameter/invalid parameter/

> +    case GRUB_EFI_BUFFER_TOO_SMALL:
> +      return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("Output buffer too 
> small"));

Ditto.

> +    case GRUB_EFI_NOT_FOUND:
> +      return grub_error (GRUB_ERR_UNKNOWN_DEVICE, N_("TPM unavailable"));

However, this one is OK.

> +    default:
> +      return grub_error (GRUB_ERR_UNKNOWN_DEVICE, N_("Unknown TPM error"));

s/Unknown TPM error/unknown TPM error/

> +    }
> +}
> +
> +grub_err_t
> +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;
> +  grub_efi_cc_protocol_t *cc;
> +  grub_efi_cc_mr_index_t mr;
> +
> +  cc = grub_efi_locate_protocol (&cc_measurement_guid, NULL);

I am not convinced calling into firmware on every event is good
idea. I think we should avoid that especially if at tpm module init
protocol was not available. On the other hand if protocol was available
during tpm module init is it safe to assume it is still available during
an tpm event? If not we can call grub_efi_locate_protocol() on every
event until return value is not NULL.

> +  if (cc == NULL)
> +    return 0;

return GRUB_ERR_NONE;

> +  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);

I do not fully understand that. You return an error and later the
return value from grub_cc_log_event() call is completely ignored.
I think you should decide how to cope with errors first and ignore
them in this function. e.g. return void, or process errors from
grub_cc_log_event() call in the caller accordingly.

> +  event = grub_zalloc (sizeof (grub_efi_cc_event_t)
> +                    + grub_strlen (description) + 1);
> +  if (event == NULL)
> +    return grub_error (GRUB_ERR_OUT_OF_MEMORY,
> +                    N_ ("cannot allocate CC event buffer"));

You do not need space after N_. Yeah, I know it is confusing but it is
what it is. Sorry...

> +  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) - sizeof (event->Event)

You will not need '- sizeof (event->Event)' if you define Event[0],
yes, 0 size, member in the grub_efi_cc_event_t.

> +             + grub_strlen (description) + 1;
> +  grub_memcpy (event->Event, description, grub_strlen (description) + 1);

I think grub_strcpy() would be more natural here. And you do not need
copy '\0' because grub_zalloc() did work for you earlier.

> +  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);
> +
> +  return grub_efi_log_event_status (status);
> +}
> diff --git a/grub-core/commands/efi/tpm.c b/grub-core/commands/efi/tpm.c
> index a97d85368..c40093fb4 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>
> @@ -228,6 +229,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);

... and here you are ignoring return value...

Daniel