Re: [PATCH v8 3/3] efi_loader: add PE/COFF image measurement

On Mon, 24 May 2021 at 21:53, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:

new_efi);

...

bool efi_image_parse(void *efi, size_t len, struct efi_image_regions **regp, WIN_CERTIFICATE **auth, size_t *auth_len);

diff --git a/include/efi_tcg2.h b/include/efi_tcg2.h index 40e241ce31..bcfb98168a 100644 --- a/include/efi_tcg2.h +++ b/include/efi_tcg2.h @@ -9,6 +9,7 @@ #if !defined _EFI_TCG2_PROTOCOL_H_ #define _EFI_TCG2_PROTOCOL_H_

+#include <efi_api.h> #include <tpm-v2.h>

#define EFI_TCG2_PROTOCOL_GUID \ @@ -53,6 +54,14 @@ struct efi_tcg2_event { u8 event[]; } __packed;

+struct uefi_image_load_event {

• efi_physical_addr_t image_location_in_memory;
  

• u64 image_length_in_memory;
  

• u64 image_link_time_address;
  

• u64 length_of_device_path;
  

• struct efi_device_path device_path[];
  

+};

struct efi_tcg2_boot_service_capability { u8 size; struct efi_tcg2_version structure_version; diff --git a/include/tpm-v2.h b/include/tpm-v2.h index 7de7d6a57d..247b386967 100644 --- a/include/tpm-v2.h +++ b/include/tpm-v2.h @@ -70,6 +70,24 @@ struct udevice; #define EV_TABLE_OF_DEVICES ((u32)0x0000000B) #define EV_COMPACT_HASH ((u32)0x0000000C)

+/*

• • event types, cf.
• • "TCG PC Client Platform Firmware Profile Specification", Family "2.0"
• • rev 1.04, June 3, 2019
• */

+#define EV_EFI_EVENT_BASE ((u32)0x80000000) +#define EV_EFI_VARIABLE_DRIVER_CONFIG ((u32)0x80000001) +#define EV_EFI_VARIABLE_BOOT ((u32)0x80000002) +#define EV_EFI_BOOT_SERVICES_APPLICATION ((u32)0x80000003) +#define EV_EFI_BOOT_SERVICES_DRIVER ((u32)0x80000004) +#define EV_EFI_RUNTIME_SERVICES_DRIVER ((u32)0x80000005) +#define EV_EFI_GPT_EVENT ((u32)0x80000006) +#define EV_EFI_ACTION ((u32)0x80000007) +#define EV_EFI_PLATFORM_FIRMWARE_BLOB ((u32)0x80000008) +#define EV_EFI_HANDOFF_TABLES ((u32)0x80000009) +#define EV_EFI_HCRTM_EVENT ((u32)0x80000010) +#define EV_EFI_VARIABLE_AUTHORITY ((u32)0x800000E0)

/* TPMS_TAGGED_PROPERTY Structure */ struct tpms_tagged_property { u32 property; diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index 98845b8ba3..0e6200fa25 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -309,6 +309,7 @@ config EFI_TCG2_PROTOCOL select SHA512_ALGO select SHA384 select SHA512

• select HASH_CALCULATE
  help
    Provide a EFI_TCG2_PROTOCOL implementation using the TPM hardware
    of the platform.
  

diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c index fe1ee198e2..f37a85e56e 100644 --- a/lib/efi_loader/efi_image_loader.c +++ b/lib/efi_loader/efi_image_loader.c @@ -302,6 +302,40 @@ static int cmp_pe_section(const void *arg1, const void *arg2) return 1; }

+/**

• • efi_prepare_aligned_image() - prepare 8-byte aligned image
• • @efi: pointer to the EFI binary
• • @efi_size: size of @efi binary
• • @new_efi: pointer to the newly allocated image
• • If @efi is not 8-byte aligned, this function newly allocates
• • the image buffer and updates @efi_size.
• • Return: valid pointer to a image, return NULL if allocation fails.
• */

+void *efi_prepare_aligned_image(void *efi, u64 *efi_size, void **new_efi) +{

• size_t new_efi_size;
  

• void *p;
  

• /*
  

•  * Size must be 8-byte aligned and the trailing bytes must be
  

•  * zero'ed. Otherwise hash value may be incorrect.
  

•  */
  

• if (!IS_ALIGNED(*efi_size, 8)) {
  

•         new_efi_size = ALIGN(*efi_size, 8);
  

•         p = calloc(new_efi_size, 1);
  

•         if (!p)
  

•                 return NULL;
  

•         memcpy(p, efi, *efi_size);
  

•         *efi_size = new_efi_size;
  

Do we really need new_efi here? I might be missing some context for the original code, but since we return the new pointer, can't we just use that in the caller? If so the whole void **new_efi is not needed?

new_efi is required. The caller uses returned pointer, it will be the pointer to the original efi image or newly allocated buffer. Caller will need new_efi to free the newly allocated buffer.

...

[...]

...

• ret = __get_active_pcr_banks(&active);
  

• if (ret != EFI_SUCCESS) {
  

•         ret = EFI_DEVICE_ERROR;
  

__get_active_pcr_banks is supposed to return the correct efi_status_t code. I don't think we need EFI_DEVICE_ERROR here.

Thank you. I will just use return value of __get_active_pcr_banks().

•   if (!efi_image_parse(efi, efi_size, &regs, &wincerts,
  

•                        &wincerts_len)) {
  

•           log_err("Parsing PE executable image failed\n");
  

•           ret = EFI_INVALID_PARAMETER;
  

•           goto out;
  

I re-checked TCG spec, if PE/COFF image is corrupted or not understood, the function shall return EFI_UNSUPPORTED. I will also update.

Thanks, Masahisa Kojima

Thanks! /Ilias