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

[Lu, Ken]

Hi Kiper, 

Thanks for your detail review! Appreciate!!

I will continue fix all your suggestions except changing Event[1] => Event[0] 
for grub_efi_cc_event_t. Actually, I think your suggestion is very nice C 
programming tip, it will be more concise and readable. 
But I am thinking how about align with the definition in UEFI now 
https://github.com/tianocore/edk2/blob/d372ab585a2cdc5348af5f701c56c631235fe698/MdePkg/Include/Protocol/CcMeasurement.h#L100.
 I will talk with UEFI owner to make same changes for Event[0] both in EFI and 
grub/shim together in future. Do you agree?

Thanks
Ken

> -----Original Message-----
> From: Grub-devel <grub-devel-bounces+ken.lu=intel.com@gnu.org> On Behalf
> Of Daniel Kiper
> Sent: Wednesday, April 27, 2022 10:37 PM
> To: Lu, Ken <ken.lu@intel.com>
> Cc: grub-devel@gnu.org; Xu, Min M <min.m.xu@intel.com>
> Subject: Re: [PATCH V2] Enable TDX measurement to RTMR register
> 
> 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/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
> >
> > 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...
[Lu, Ken] Sure, I will move it.

> 
> > + */
> > +
> > +#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.
[Lu, Ken] Thanks for reminder, I will fix it

> 
> > +grub_efi_log_event_status (grub_efi_status_t status) {
> > +  switch (status)
> > +    {
> > +    case GRUB_EFI_SUCCESS:
> > +      return 0;
> 
> return GRUB_ERR_NONE;
[Lu, Ken] Thanks for reminder, I will fix it

> 
> > +    case GRUB_EFI_DEVICE_ERROR:
> > +      return grub_error (GRUB_ERR_IO, N_("Command failed"));
> 
> s/Command failed/command failed/
[Lu, Ken] Thanks for this, I will fix it

> 
> 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/
[Lu, Ken] Thanks for this comment, I will fix it

> 
> > +    case GRUB_EFI_BUFFER_TOO_SMALL:
> > +      return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("Output buffer too
> > + small"));
> 
> Ditto.
[Lu, Ken] Thanks for this comment, I will fix it

> 
> > +    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/
[Lu, Ken] Thanks for this comment, I will fix it

> 
> > +    }
> > +}
> > +
> > +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.
[Lu, Ken] I think this is very nice suggestion. For TPM1/TPM2/CC, they all need 
call EFI to get handle, then call EFI to open protocol.
So we should only do once for both getting handle + open protocol. So I will 
optimize code for TPM1/TPM2 and CC according to your suggestion.

> 
> > +  if (cc == NULL)
> > +    return 0;
> 
> return GRUB_ERR_NONE;
[Lu, Ken] Thanks for this comment, I will fix it

> 
> > +  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.
[Lu, Ken] Thanks for this reminder! I will return void, since the error of 
grub_cc_log_event() should not block other TPM log event, because they can 
coexist.

> 
> > +  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...
[Lu, Ken] Thanks for this, I will fix it.

> 
> > +  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.
[Lu, Ken] Very nice C programming tip, it will be more concise and readable. 
Could we keep this Event[1] now firstly aligning with UEFI's definition at 
https://github.com/tianocore/edk2/blob/d372ab585a2cdc5348af5f701c56c631235fe698/MdePkg/Include/Protocol/CcMeasurement.h#L100,
 I will talk with UEFI owner to make same changes in future.

> 
> > +           + 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.
[Lu, Ken] Thanks for this comment, will fix it.

> 
> > +  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
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel