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 <[email protected]> On Behalf > Of Daniel Kiper > Sent: Wednesday, April 27, 2022 10:37 PM > To: Lu, Ken <[email protected]> > Cc: [email protected]; Xu, Min M <[email protected]> > 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 <[email protected]> > > --- > > > > 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 > [email protected] > https://lists.gnu.org/mailman/listinfo/grub-devel _______________________________________________ Grub-devel mailing list [email protected] https://lists.gnu.org/mailman/listinfo/grub-devel
