| Message ID | 20210415133020.29175-3-masahisa.kojima@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | PE/COFF measurement support | expand |
On 15.04.21 15:30, Masahisa Kojima wrote: > "TCG PC Client Platform Firmware Profile Specification" > requires to measure every attempt to load and execute > a OS Loader(a UEFI application) into PCR[4]. > This commit adds the PE/COFF image measurement, extends PCR, > and appends measurement into Event Log. > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> Please, provide a unit test that we can run in Gitlab CI on either the sandbox or QEMU. QEMU allows to use a TPM emulation, cf. https://qemu-project.gitlab.io/qemu/specs/tpm.html#the-qemu-tpm-emulator-device https://github.com/stefanberger/swtpm Best regards Heinrich > --- > include/efi_loader.h | 4 + > include/efi_tcg2.h | 10 ++ > include/tpm-v2.h | 1 + > lib/efi_loader/efi_image_loader.c | 7 ++ > lib/efi_loader/efi_tcg2.c | 187 ++++++++++++++++++++++++++++-- > 5 files changed, 199 insertions(+), 10 deletions(-) > > diff --git a/include/efi_loader.h b/include/efi_loader.h > index de1a496a97..b02bc93c8e 100644 > --- a/include/efi_loader.h > +++ b/include/efi_loader.h > @@ -426,6 +426,10 @@ efi_status_t efi_disk_register(void); > efi_status_t efi_rng_register(void); > /* Called by efi_init_obj_list() to install EFI_TCG2_PROTOCOL */ > efi_status_t efi_tcg2_register(void); > +/* measure the pe-coff image, extend PCR and add Event Log */ > +efi_status_t tcg2_measure_pe_image(void *efi, u64 efi_size, > + struct efi_loaded_image_obj *handle, > + struct efi_loaded_image *loaded_image_info); > /* Create handles and protocols for the partitions of a block device */ > int efi_disk_create_partitions(efi_handle_t parent, struct blk_desc *desc, > const char *if_typename, int diskid, > diff --git a/include/efi_tcg2.h b/include/efi_tcg2.h > index 40e241ce31..f8d46c5fd2 100644 > --- a/include/efi_tcg2.h > +++ b/include/efi_tcg2.h > @@ -9,6 +9,8 @@ > #if !defined _EFI_TCG2_PROTOCOL_H_ > #define _EFI_TCG2_PROTOCOL_H_ > > +#include <efi.h> > +#include <efi_api.h> > #include <tpm-v2.h> > > #define EFI_TCG2_PROTOCOL_GUID \ > @@ -53,6 +55,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[]; > +} __packed; > + > 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 df67a196cf..ab9c04dc0a 100644 > --- a/include/tpm-v2.h > +++ b/include/tpm-v2.h > @@ -61,6 +61,7 @@ struct udevice; > #define EV_S_CRTM_VERSION ((u32)0x00000008) > #define EV_CPU_MICROCODE ((u32)0x00000009) > #define EV_TABLE_OF_DEVICES ((u32)0x0000000B) > +#define EV_EFI_BOOT_SERVICES_APPLICATION ((u32)0x80000003) > > /* TPMS_TAGGED_PROPERTY Structure */ > struct tpms_tagged_property { > diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c > index 2c35cb5651..b032ec5dd8 100644 > --- a/lib/efi_loader/efi_image_loader.c > +++ b/lib/efi_loader/efi_image_loader.c > @@ -829,6 +829,13 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle, > goto err; > } > > +#if CONFIG_IS_ENABLED(EFI_TCG2_PROTOCOL) > + /* Measure an PE/COFF image */ > + if (tcg2_measure_pe_image(efi, efi_size, handle, > + loaded_image_info)) > + log_err("PE image measurement failed\n"); > +#endif > + > /* Copy PE headers */ > memcpy(efi_reloc, efi, > sizeof(*dos) > diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c > index ed86a220fb..9fab07605f 100644 > --- a/lib/efi_loader/efi_tcg2.c > +++ b/lib/efi_loader/efi_tcg2.c > @@ -13,8 +13,10 @@ > #include <efi_loader.h> > #include <efi_tcg2.h> > #include <log.h> > +#include <malloc.h> > #include <version.h> > #include <tpm-v2.h> > +#include <u-boot/rsa.h> > #include <u-boot/sha1.h> > #include <u-boot/sha256.h> > #include <u-boot/sha512.h> > @@ -709,6 +711,172 @@ out: > return EFI_EXIT(ret); > } > > +static efi_status_t tcg2_hash_pe_image(void *efi, u64 efi_size, > + struct tpml_digest_values *digest_list) > +{ > + IMAGE_NT_HEADERS32 *nt; > + WIN_CERTIFICATE *wincerts = NULL; > + size_t wincerts_len; > + struct efi_image_regions *regs = NULL; > + void *new_efi = NULL; > + size_t new_efi_size; > + u8 hash[TPM2_SHA512_DIGEST_SIZE]; > + efi_status_t ret; > + u32 active; > + int i; > + > + ret = efi_check_pe(efi, efi_size, (void **)&nt); > + if (ret != EFI_SUCCESS) { > + log_err("Not a valid PE-COFF file\n"); > + return EFI_INVALID_PARAMETER; > + } > + > + /* > + * 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); > + new_efi = calloc(new_efi_size, 1); > + if (!new_efi) > + return EFI_OUT_OF_RESOURCES; > + memcpy(new_efi, efi, efi_size); > + efi = new_efi; > + efi_size = new_efi_size; > + } > + > + if (!efi_image_parse(efi, efi_size, ®s, &wincerts, > + &wincerts_len)) { > + log_err("Parsing PE executable image failed\n"); > + ret = EFI_INVALID_PARAMETER; > + goto out; > + } > + > + ret = __get_active_pcr_banks(&active); > + if (ret != EFI_SUCCESS) { > + ret = EFI_DEVICE_ERROR; > + goto out; > + } > + > + digest_list->count = 0; > + for (i = 0; i < MAX_HASH_COUNT; i++) { > + u16 hash_alg = hash_algo_list[i].hash_alg; > + > + if (!(active & alg_to_mask(hash_alg))) > + continue; > + switch (hash_alg) { > + case TPM2_ALG_SHA1: > + hash_calculate("sha1", regs->reg, regs->num, hash); > + digest_list->count++; > + break; > + case TPM2_ALG_SHA256: > + hash_calculate("sha256", regs->reg, regs->num, hash); > + digest_list->count++; > + break; > + case TPM2_ALG_SHA384: > + hash_calculate("sha384", regs->reg, regs->num, hash); > + digest_list->count++; > + break; > + case TPM2_ALG_SHA512: > + hash_calculate("sha512", regs->reg, regs->num, hash); > + digest_list->count++; > + break; > + default: > + EFI_PRINT("Unsupported algorithm %x\n", hash_alg); > + return EFI_INVALID_PARAMETER; > + } > + digest_list->digests[i].hash_alg = hash_alg; > + memcpy(&digest_list->digests[i].digest, hash, (u32)alg_to_len(hash_alg)); > + } > + > +out: > + free(new_efi); > + free(regs); > + > + return ret; > +} > + > +efi_status_t tcg2_measure_pe_image(void *efi, u64 efi_size, > + struct efi_loaded_image_obj *handle, > + struct efi_loaded_image *loaded_image) > +{ > + struct tpml_digest_values digest_list; > + efi_status_t ret; > + struct udevice *dev; > + u32 pcr_index, event_type, event_size; > + struct uefi_image_load_event *image_load_event; > + u8 *event; > + struct efi_device_path *device_path; > + u32 device_path_length; > + IMAGE_DOS_HEADER *dos; > + IMAGE_NT_HEADERS32 *nt; > + > + ret = platform_get_tpm2_device(&dev); > + if (ret != EFI_SUCCESS) > + return ret; > + > + if (handle->image_type == IMAGE_SUBSYSTEM_EFI_APPLICATION) { > + pcr_index = 4; > + event_type = EV_EFI_BOOT_SERVICES_APPLICATION; > + } else { > + return EFI_UNSUPPORTED; > + } > + > + ret = tcg2_hash_pe_image(efi, efi_size, &digest_list); > + if (ret != EFI_SUCCESS) > + return ret; > + > + ret = tcg2_pcr_extend(dev, pcr_index, &digest_list); > + if (ret != EFI_SUCCESS) > + return ret; > + > + loaded_image->system_table->boottime->handle_protocol(&handle->header, > + &efi_guid_loaded_image_device_path, > + (void **)&device_path); > + device_path_length = efi_dp_size(device_path); > + if (device_path_length > 0) { > + /* add end node size */ > + device_path_length += sizeof(struct efi_device_path); > + } > + event_size = sizeof(struct uefi_image_load_event) + device_path_length; > + event = malloc(event_size); > + if (!event) > + return EFI_OUT_OF_RESOURCES; > + > + image_load_event = (struct uefi_image_load_event *)event; > + image_load_event->image_location_in_memory = (efi_physical_addr_t)efi; > + image_load_event->image_length_in_memory = efi_size; > + image_load_event->length_of_device_path = device_path_length; > + > + dos = (IMAGE_DOS_HEADER *)efi; > + nt = (IMAGE_NT_HEADERS32 *)(efi + dos->e_lfanew); > + if (nt->OptionalHeader.Magic == IMAGE_NT_OPTIONAL_HDR64_MAGIC) { > + IMAGE_NT_HEADERS64 *nt64 = (IMAGE_NT_HEADERS64 *)nt; > + > + image_load_event->image_link_time_address = > + nt64->OptionalHeader.ImageBase; > + } else if (nt->OptionalHeader.Magic == IMAGE_NT_OPTIONAL_HDR32_MAGIC) { > + image_load_event->image_link_time_address = > + nt->OptionalHeader.ImageBase; > + } else { > + ret = EFI_INVALID_PARAMETER; > + goto out; > + } > + > + if (device_path_length > 0) { > + memcpy(image_load_event->device_path, device_path, > + device_path_length); > + } > + > + ret = tcg2_agile_log_append(pcr_index, event_type, &digest_list, > + event_size, event); > + > +out: > + free(event); > + > + return ret; > +} > + > /** > * efi_tcg2_hash_log_extend_event() - extend and optionally log events > * > @@ -761,24 +929,23 @@ efi_tcg2_hash_log_extend_event(struct efi_tcg2_protocol *this, u64 flags, > /* > * if PE_COFF_IMAGE is set we need to make sure the image is not > * corrupted, verify it and hash the PE/COFF image in accordance with > - * the procedure specified in "Calculating the PE Image Hash" > - * section of the "Windows Authenticode Portable Executable Signature > + * the procedure specified in "Calculating the PE Image Hash" > + * section of the "Windows Authenticode Portable Executable Signature > * Format" > - * Not supported for now > */ > if (flags & PE_COFF_IMAGE) { > - ret = EFI_UNSUPPORTED; > - goto out; > + ret = tcg2_hash_pe_image((void *)data_to_hash, data_to_hash_len, > + &digest_list); > + } else { > + ret = tcg2_create_digest((u8 *)data_to_hash, data_to_hash_len, > + &digest_list); > } > + if (ret != EFI_SUCCESS) > + goto out; > > pcr_index = efi_tcg_event->header.pcr_index; > event_type = efi_tcg_event->header.event_type; > > - ret = tcg2_create_digest((u8 *)data_to_hash, data_to_hash_len, > - &digest_list); > - if (ret != EFI_SUCCESS) > - goto out; > - > ret = tcg2_pcr_extend(dev, pcr_index, &digest_list); > if (ret != EFI_SUCCESS) > goto out; >
Hi Heinrich, On Thu, Apr 15, 2021 at 04:08:55PM +0200, Heinrich Schuchardt wrote: > On 15.04.21 15:30, Masahisa Kojima wrote: > > "TCG PC Client Platform Firmware Profile Specification" > > requires to measure every attempt to load and execute > > a OS Loader(a UEFI application) into PCR[4]. > > This commit adds the PE/COFF image measurement, extends PCR, > > and appends measurement into Event Log. > > > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> > > Please, provide a unit test that we can run in Gitlab CI on either the > sandbox or QEMU. The additions to the EFI TCG2 fall under the same category as the initial patchset and unfortunately suffer from the same problems wrt to using the sandbox TPM2. The sandbox capabilities are limited for testing this, starting from the fact that that we can't even get the tpm2 capabilities we need to start the protocol correctly. ./drivers/tpm/tpm2_tis_sandbox.c only supports TPM_CAP_TPM_PROPERTIES which is limited compared to what the TCG code in EFI expects. Similar functionality is missing from extending and checking PCRs properly etc. > > QEMU allows to use a TPM emulation, cf. > https://qemu-project.gitlab.io/qemu/specs/tpm.html#the-qemu-tpm-emulator-device > https://github.com/stefanberger/swtpm Kojima and I will have a look since this is the only viable option in order to get useful selftests. Imho we should review and maybe accept this patch in parallel though, since it's adding more bits of the TCG PC client specification. Thanks! /Ilias > > Best regards > > Heinrich > > > --- > > include/efi_loader.h | 4 + > > include/efi_tcg2.h | 10 ++ > > include/tpm-v2.h | 1 + > > lib/efi_loader/efi_image_loader.c | 7 ++ > > lib/efi_loader/efi_tcg2.c | 187 ++++++++++++++++++++++++++++-- > > 5 files changed, 199 insertions(+), 10 deletions(-) > > > > diff --git a/include/efi_loader.h b/include/efi_loader.h > > index de1a496a97..b02bc93c8e 100644 > > --- a/include/efi_loader.h > > +++ b/include/efi_loader.h > > @@ -426,6 +426,10 @@ efi_status_t efi_disk_register(void); > > efi_status_t efi_rng_register(void); > > /* Called by efi_init_obj_list() to install EFI_TCG2_PROTOCOL */ > > efi_status_t efi_tcg2_register(void); > > +/* measure the pe-coff image, extend PCR and add Event Log */ > > +efi_status_t tcg2_measure_pe_image(void *efi, u64 efi_size, > > + struct efi_loaded_image_obj *handle, > > + struct efi_loaded_image *loaded_image_info); > > /* Create handles and protocols for the partitions of a block device */ > > int efi_disk_create_partitions(efi_handle_t parent, struct blk_desc *desc, > > const char *if_typename, int diskid, > > diff --git a/include/efi_tcg2.h b/include/efi_tcg2.h > > index 40e241ce31..f8d46c5fd2 100644 > > --- a/include/efi_tcg2.h > > +++ b/include/efi_tcg2.h > > @@ -9,6 +9,8 @@ > > #if !defined _EFI_TCG2_PROTOCOL_H_ > > #define _EFI_TCG2_PROTOCOL_H_ > > > > +#include <efi.h> > > +#include <efi_api.h> > > #include <tpm-v2.h> > > > > #define EFI_TCG2_PROTOCOL_GUID \ > > @@ -53,6 +55,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[]; > > +} __packed; > > + > > 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 df67a196cf..ab9c04dc0a 100644 > > --- a/include/tpm-v2.h > > +++ b/include/tpm-v2.h > > @@ -61,6 +61,7 @@ struct udevice; > > #define EV_S_CRTM_VERSION ((u32)0x00000008) > > #define EV_CPU_MICROCODE ((u32)0x00000009) > > #define EV_TABLE_OF_DEVICES ((u32)0x0000000B) > > +#define EV_EFI_BOOT_SERVICES_APPLICATION ((u32)0x80000003) > > > > /* TPMS_TAGGED_PROPERTY Structure */ > > struct tpms_tagged_property { > > diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c > > index 2c35cb5651..b032ec5dd8 100644 > > --- a/lib/efi_loader/efi_image_loader.c > > +++ b/lib/efi_loader/efi_image_loader.c > > @@ -829,6 +829,13 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle, > > goto err; > > } > > > > +#if CONFIG_IS_ENABLED(EFI_TCG2_PROTOCOL) > > + /* Measure an PE/COFF image */ > > + if (tcg2_measure_pe_image(efi, efi_size, handle, > > + loaded_image_info)) > > + log_err("PE image measurement failed\n"); > > +#endif > > + > > /* Copy PE headers */ > > memcpy(efi_reloc, efi, > > sizeof(*dos) > > diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c > > index ed86a220fb..9fab07605f 100644 > > --- a/lib/efi_loader/efi_tcg2.c > > +++ b/lib/efi_loader/efi_tcg2.c > > @@ -13,8 +13,10 @@ > > #include <efi_loader.h> > > #include <efi_tcg2.h> > > #include <log.h> > > +#include <malloc.h> > > #include <version.h> > > #include <tpm-v2.h> > > +#include <u-boot/rsa.h> > > #include <u-boot/sha1.h> > > #include <u-boot/sha256.h> > > #include <u-boot/sha512.h> > > @@ -709,6 +711,172 @@ out: > > return EFI_EXIT(ret); > > } > > > > +static efi_status_t tcg2_hash_pe_image(void *efi, u64 efi_size, > > + struct tpml_digest_values *digest_list) > > +{ > > + IMAGE_NT_HEADERS32 *nt; > > + WIN_CERTIFICATE *wincerts = NULL; > > + size_t wincerts_len; > > + struct efi_image_regions *regs = NULL; > > + void *new_efi = NULL; > > + size_t new_efi_size; > > + u8 hash[TPM2_SHA512_DIGEST_SIZE]; > > + efi_status_t ret; > > + u32 active; > > + int i; > > + > > + ret = efi_check_pe(efi, efi_size, (void **)&nt); > > + if (ret != EFI_SUCCESS) { > > + log_err("Not a valid PE-COFF file\n"); > > + return EFI_INVALID_PARAMETER; > > + } > > + > > + /* > > + * 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); > > + new_efi = calloc(new_efi_size, 1); > > + if (!new_efi) > > + return EFI_OUT_OF_RESOURCES; > > + memcpy(new_efi, efi, efi_size); > > + efi = new_efi; > > + efi_size = new_efi_size; > > + } > > + > > + if (!efi_image_parse(efi, efi_size, ®s, &wincerts, > > + &wincerts_len)) { > > + log_err("Parsing PE executable image failed\n"); > > + ret = EFI_INVALID_PARAMETER; > > + goto out; > > + } > > + > > + ret = __get_active_pcr_banks(&active); > > + if (ret != EFI_SUCCESS) { > > + ret = EFI_DEVICE_ERROR; > > + goto out; > > + } > > + > > + digest_list->count = 0; > > + for (i = 0; i < MAX_HASH_COUNT; i++) { > > + u16 hash_alg = hash_algo_list[i].hash_alg; > > + > > + if (!(active & alg_to_mask(hash_alg))) > > + continue; > > + switch (hash_alg) { > > + case TPM2_ALG_SHA1: > > + hash_calculate("sha1", regs->reg, regs->num, hash); > > + digest_list->count++; > > + break; > > + case TPM2_ALG_SHA256: > > + hash_calculate("sha256", regs->reg, regs->num, hash); > > + digest_list->count++; > > + break; > > + case TPM2_ALG_SHA384: > > + hash_calculate("sha384", regs->reg, regs->num, hash); > > + digest_list->count++; > > + break; > > + case TPM2_ALG_SHA512: > > + hash_calculate("sha512", regs->reg, regs->num, hash); > > + digest_list->count++; > > + break; > > + default: > > + EFI_PRINT("Unsupported algorithm %x\n", hash_alg); > > + return EFI_INVALID_PARAMETER; > > + } > > + digest_list->digests[i].hash_alg = hash_alg; > > + memcpy(&digest_list->digests[i].digest, hash, (u32)alg_to_len(hash_alg)); > > + } > > + > > +out: > > + free(new_efi); > > + free(regs); > > + > > + return ret; > > +} > > + > > +efi_status_t tcg2_measure_pe_image(void *efi, u64 efi_size, > > + struct efi_loaded_image_obj *handle, > > + struct efi_loaded_image *loaded_image) > > +{ > > + struct tpml_digest_values digest_list; > > + efi_status_t ret; > > + struct udevice *dev; > > + u32 pcr_index, event_type, event_size; > > + struct uefi_image_load_event *image_load_event; > > + u8 *event; > > + struct efi_device_path *device_path; > > + u32 device_path_length; > > + IMAGE_DOS_HEADER *dos; > > + IMAGE_NT_HEADERS32 *nt; > > + > > + ret = platform_get_tpm2_device(&dev); > > + if (ret != EFI_SUCCESS) > > + return ret; > > + > > + if (handle->image_type == IMAGE_SUBSYSTEM_EFI_APPLICATION) { > > + pcr_index = 4; > > + event_type = EV_EFI_BOOT_SERVICES_APPLICATION; > > + } else { > > + return EFI_UNSUPPORTED; > > + } > > + > > + ret = tcg2_hash_pe_image(efi, efi_size, &digest_list); > > + if (ret != EFI_SUCCESS) > > + return ret; > > + > > + ret = tcg2_pcr_extend(dev, pcr_index, &digest_list); > > + if (ret != EFI_SUCCESS) > > + return ret; > > + > > + loaded_image->system_table->boottime->handle_protocol(&handle->header, > > + &efi_guid_loaded_image_device_path, > > + (void **)&device_path); > > + device_path_length = efi_dp_size(device_path); > > + if (device_path_length > 0) { > > + /* add end node size */ > > + device_path_length += sizeof(struct efi_device_path); > > + } > > + event_size = sizeof(struct uefi_image_load_event) + device_path_length; > > + event = malloc(event_size); > > + if (!event) > > + return EFI_OUT_OF_RESOURCES; > > + > > + image_load_event = (struct uefi_image_load_event *)event; > > + image_load_event->image_location_in_memory = (efi_physical_addr_t)efi; > > + image_load_event->image_length_in_memory = efi_size; > > + image_load_event->length_of_device_path = device_path_length; > > + > > + dos = (IMAGE_DOS_HEADER *)efi; > > + nt = (IMAGE_NT_HEADERS32 *)(efi + dos->e_lfanew); > > + if (nt->OptionalHeader.Magic == IMAGE_NT_OPTIONAL_HDR64_MAGIC) { > > + IMAGE_NT_HEADERS64 *nt64 = (IMAGE_NT_HEADERS64 *)nt; > > + > > + image_load_event->image_link_time_address = > > + nt64->OptionalHeader.ImageBase; > > + } else if (nt->OptionalHeader.Magic == IMAGE_NT_OPTIONAL_HDR32_MAGIC) { > > + image_load_event->image_link_time_address = > > + nt->OptionalHeader.ImageBase; > > + } else { > > + ret = EFI_INVALID_PARAMETER; > > + goto out; > > + } > > + > > + if (device_path_length > 0) { > > + memcpy(image_load_event->device_path, device_path, > > + device_path_length); > > + } > > + > > + ret = tcg2_agile_log_append(pcr_index, event_type, &digest_list, > > + event_size, event); > > + > > +out: > > + free(event); > > + > > + return ret; > > +} > > + > > /** > > * efi_tcg2_hash_log_extend_event() - extend and optionally log events > > * > > @@ -761,24 +929,23 @@ efi_tcg2_hash_log_extend_event(struct efi_tcg2_protocol *this, u64 flags, > > /* > > * if PE_COFF_IMAGE is set we need to make sure the image is not > > * corrupted, verify it and hash the PE/COFF image in accordance with > > - * the procedure specified in "Calculating the PE Image Hash" > > - * section of the "Windows Authenticode Portable Executable Signature > > + * the procedure specified in "Calculating the PE Image Hash" > > + * section of the "Windows Authenticode Portable Executable Signature > > * Format" > > - * Not supported for now > > */ > > if (flags & PE_COFF_IMAGE) { > > - ret = EFI_UNSUPPORTED; > > - goto out; > > + ret = tcg2_hash_pe_image((void *)data_to_hash, data_to_hash_len, > > + &digest_list); > > + } else { > > + ret = tcg2_create_digest((u8 *)data_to_hash, data_to_hash_len, > > + &digest_list); > > } > > + if (ret != EFI_SUCCESS) > > + goto out; > > > > pcr_index = efi_tcg_event->header.pcr_index; > > event_type = efi_tcg_event->header.event_type; > > > > - ret = tcg2_create_digest((u8 *)data_to_hash, data_to_hash_len, > > - &digest_list); > > - if (ret != EFI_SUCCESS) > > - goto out; > > - > > ret = tcg2_pcr_extend(dev, pcr_index, &digest_list); > > if (ret != EFI_SUCCESS) > > goto out; > > >
On 4/15/21 3:30 PM, Masahisa Kojima wrote: > "TCG PC Client Platform Firmware Profile Specification" > requires to measure every attempt to load and execute > a OS Loader(a UEFI application) into PCR[4]. > This commit adds the PE/COFF image measurement, extends PCR, > and appends measurement into Event Log. > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> > --- > include/efi_loader.h | 4 + > include/efi_tcg2.h | 10 ++ > include/tpm-v2.h | 1 + > lib/efi_loader/efi_image_loader.c | 7 ++ > lib/efi_loader/efi_tcg2.c | 187 ++++++++++++++++++++++++++++-- > 5 files changed, 199 insertions(+), 10 deletions(-) > > diff --git a/include/efi_loader.h b/include/efi_loader.h > index de1a496a97..b02bc93c8e 100644 > --- a/include/efi_loader.h > +++ b/include/efi_loader.h > @@ -426,6 +426,10 @@ efi_status_t efi_disk_register(void); > efi_status_t efi_rng_register(void); > /* Called by efi_init_obj_list() to install EFI_TCG2_PROTOCOL */ > efi_status_t efi_tcg2_register(void); > +/* measure the pe-coff image, extend PCR and add Event Log */ > +efi_status_t tcg2_measure_pe_image(void *efi, u64 efi_size, > + struct efi_loaded_image_obj *handle, > + struct efi_loaded_image *loaded_image_info); > /* Create handles and protocols for the partitions of a block device */ > int efi_disk_create_partitions(efi_handle_t parent, struct blk_desc *desc, > const char *if_typename, int diskid, > diff --git a/include/efi_tcg2.h b/include/efi_tcg2.h > index 40e241ce31..f8d46c5fd2 100644 > --- a/include/efi_tcg2.h > +++ b/include/efi_tcg2.h > @@ -9,6 +9,8 @@ > #if !defined _EFI_TCG2_PROTOCOL_H_ > #define _EFI_TCG2_PROTOCOL_H_ > > +#include <efi.h> This include is already included in efi_api.h. > +#include <efi_api.h> > #include <tpm-v2.h> > > #define EFI_TCG2_PROTOCOL_GUID \ > @@ -53,6 +55,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[]; A device path is not an array of struct efi_device_path. But the first element is of this type. So ok. > +} __packed; Why should this be __packed? You don't use arrays of this structure and it is naturally packed. > + > 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 df67a196cf..ab9c04dc0a 100644 > --- a/include/tpm-v2.h > +++ b/include/tpm-v2.h > @@ -61,6 +61,7 @@ struct udevice; > #define EV_S_CRTM_VERSION ((u32)0x00000008) > #define EV_CPU_MICROCODE ((u32)0x00000009) > #define EV_TABLE_OF_DEVICES ((u32)0x0000000B) Please, add a comment here that the following values are defined in the "TCG EFI Platform Specification". > +#define EV_EFI_BOOT_SERVICES_APPLICATION ((u32)0x80000003) Please, add all EV_EFI_* constants. > > /* TPMS_TAGGED_PROPERTY Structure */ > struct tpms_tagged_property { > diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c > index 2c35cb5651..b032ec5dd8 100644 > --- a/lib/efi_loader/efi_image_loader.c > +++ b/lib/efi_loader/efi_image_loader.c > @@ -829,6 +829,13 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle, > goto err; > } > > +#if CONFIG_IS_ENABLED(EFI_TCG2_PROTOCOL) > + /* Measure an PE/COFF image */ > + if (tcg2_measure_pe_image(efi, efi_size, handle, > + loaded_image_info)) > + log_err("PE image measurement failed\n"); > +#endif > + > /* Copy PE headers */ > memcpy(efi_reloc, efi, > sizeof(*dos) > diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c > index ed86a220fb..9fab07605f 100644 > --- a/lib/efi_loader/efi_tcg2.c > +++ b/lib/efi_loader/efi_tcg2.c > @@ -13,8 +13,10 @@ > #include <efi_loader.h> > #include <efi_tcg2.h> > #include <log.h> > +#include <malloc.h> > #include <version.h> > #include <tpm-v2.h> > +#include <u-boot/rsa.h> > #include <u-boot/sha1.h> > #include <u-boot/sha256.h> > #include <u-boot/sha512.h> > @@ -709,6 +711,172 @@ out: > return EFI_EXIT(ret); > } > > +static efi_status_t tcg2_hash_pe_image(void *efi, u64 efi_size, > + struct tpml_digest_values *digest_list) > +{ > + IMAGE_NT_HEADERS32 *nt; > + WIN_CERTIFICATE *wincerts = NULL; > + size_t wincerts_len; > + struct efi_image_regions *regs = NULL; > + void *new_efi = NULL; > + size_t new_efi_size; > + u8 hash[TPM2_SHA512_DIGEST_SIZE]; > + efi_status_t ret; > + u32 active; > + int i; > + > + ret = efi_check_pe(efi, efi_size, (void **)&nt); Why are you calling this function? It is already called in efi_load_pe(). > + if (ret != EFI_SUCCESS) { > + log_err("Not a valid PE-COFF file\n"); > + return EFI_INVALID_PARAMETER; > + } > + > + /* > + * 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); > + new_efi = calloc(new_efi_size, 1); > + if (!new_efi) > + return EFI_OUT_OF_RESOURCES; > + memcpy(new_efi, efi, efi_size); > + efi = new_efi; > + efi_size = new_efi_size; > + } > + > + if (!efi_image_parse(efi, efi_size, ®s, &wincerts, > + &wincerts_len)) { > + log_err("Parsing PE executable image failed\n"); > + ret = EFI_INVALID_PARAMETER; > + goto out; > + } Please, don't duplicate code from efi_image_authenticate(). Extract a common function instead. > + > + ret = __get_active_pcr_banks(&active); > + if (ret != EFI_SUCCESS) { > + ret = EFI_DEVICE_ERROR; > + goto out; > + } > + > + digest_list->count = 0; > + for (i = 0; i < MAX_HASH_COUNT; i++) { > + u16 hash_alg = hash_algo_list[i].hash_alg; > + > + if (!(active & alg_to_mask(hash_alg))) > + continue; > + switch (hash_alg) { > + case TPM2_ALG_SHA1: SHA1 is known to be unsafe. Why would we support it? > + hash_calculate("sha1", regs->reg, regs->num, hash); > + digest_list->count++; Why do you repeat this line in every case of the switch statement? Please, put it below. > + break; > + case TPM2_ALG_SHA256: > + hash_calculate("sha256", regs->reg, regs->num, hash); > + digest_list->count++; > + break; > + case TPM2_ALG_SHA384: > + hash_calculate("sha384", regs->reg, regs->num, hash); > + digest_list->count++; > + break; > + case TPM2_ALG_SHA512: > + hash_calculate("sha512", regs->reg, regs->num, hash); > + digest_list->count++; > + break; > + default: > + EFI_PRINT("Unsupported algorithm %x\n", hash_alg); > + return EFI_INVALID_PARAMETER; > + } > + digest_list->digests[i].hash_alg = hash_alg; > + memcpy(&digest_list->digests[i].digest, hash, (u32)alg_to_len(hash_alg)); > + } > + > +out: > + free(new_efi); > + free(regs); > + > + return ret; > +} > + > +efi_status_t tcg2_measure_pe_image(void *efi, u64 efi_size, > + struct efi_loaded_image_obj *handle, > + struct efi_loaded_image *loaded_image) > +{ > + struct tpml_digest_values digest_list; > + efi_status_t ret; > + struct udevice *dev; > + u32 pcr_index, event_type, event_size; > + struct uefi_image_load_event *image_load_event; > + u8 *event; The variable event is not needed. You can use image_load_event directly. > + struct efi_device_path *device_path; > + u32 device_path_length; > + IMAGE_DOS_HEADER *dos; > + IMAGE_NT_HEADERS32 *nt; > + > + ret = platform_get_tpm2_device(&dev); > + if (ret != EFI_SUCCESS) > + return ret; > + > + if (handle->image_type == IMAGE_SUBSYSTEM_EFI_APPLICATION) { We should measure drivers too. Cf. EV_EFI_BOOT_SERVICES_DRIVER, EV_EFI_RUNTIME_SERVICES_DRIVER. > + pcr_index = 4; > + event_type = EV_EFI_BOOT_SERVICES_APPLICATION; > + } else { > + return EFI_UNSUPPORTED; > + } > + > + ret = tcg2_hash_pe_image(efi, efi_size, &digest_list); > + if (ret != EFI_SUCCESS) > + return ret; > + > + ret = tcg2_pcr_extend(dev, pcr_index, &digest_list); > + if (ret != EFI_SUCCESS) > + return ret; > + > + loaded_image->system_table->boottime->handle_protocol(&handle->header, > + &efi_guid_loaded_image_device_path, > + (void **)&device_path); You would have to use EFI_CALL() here. Please, use efi_search_protocol() instead of all this indirection. Best regards Heinrich > + device_path_length = efi_dp_size(device_path); > + if (device_path_length > 0) { > + /* add end node size */ > + device_path_length += sizeof(struct efi_device_path); > + } > + event_size = sizeof(struct uefi_image_load_event) + device_path_length; > + event = malloc(event_size); > + if (!event) > + return EFI_OUT_OF_RESOURCES; > + > + image_load_event = (struct uefi_image_load_event *)event; > + image_load_event->image_location_in_memory = (efi_physical_addr_t)efi; > + image_load_event->image_length_in_memory = efi_size; > + image_load_event->length_of_device_path = device_path_length; > + > + dos = (IMAGE_DOS_HEADER *)efi; > + nt = (IMAGE_NT_HEADERS32 *)(efi + dos->e_lfanew); > + if (nt->OptionalHeader.Magic == IMAGE_NT_OPTIONAL_HDR64_MAGIC) { > + IMAGE_NT_HEADERS64 *nt64 = (IMAGE_NT_HEADERS64 *)nt; > + > + image_load_event->image_link_time_address = > + nt64->OptionalHeader.ImageBase; > + } else if (nt->OptionalHeader.Magic == IMAGE_NT_OPTIONAL_HDR32_MAGIC) { > + image_load_event->image_link_time_address = > + nt->OptionalHeader.ImageBase; > + } else { > + ret = EFI_INVALID_PARAMETER; > + goto out; > + } > + > + if (device_path_length > 0) { > + memcpy(image_load_event->device_path, device_path, > + device_path_length); > + } > + > + ret = tcg2_agile_log_append(pcr_index, event_type, &digest_list, > + event_size, event); > + > +out: > + free(event); > + > + return ret; > +} > + > /** > * efi_tcg2_hash_log_extend_event() - extend and optionally log events > * > @@ -761,24 +929,23 @@ efi_tcg2_hash_log_extend_event(struct efi_tcg2_protocol *this, u64 flags, > /* > * if PE_COFF_IMAGE is set we need to make sure the image is not > * corrupted, verify it and hash the PE/COFF image in accordance with > - * the procedure specified in "Calculating the PE Image Hash" > - * section of the "Windows Authenticode Portable Executable Signature > + * the procedure specified in "Calculating the PE Image Hash" > + * section of the "Windows Authenticode Portable Executable Signature > * Format" > - * Not supported for now > */ > if (flags & PE_COFF_IMAGE) { > - ret = EFI_UNSUPPORTED; > - goto out; > + ret = tcg2_hash_pe_image((void *)data_to_hash, data_to_hash_len, > + &digest_list); > + } else { > + ret = tcg2_create_digest((u8 *)data_to_hash, data_to_hash_len, > + &digest_list); > } > + if (ret != EFI_SUCCESS) > + goto out; > > pcr_index = efi_tcg_event->header.pcr_index; > event_type = efi_tcg_event->header.event_type; > > - ret = tcg2_create_digest((u8 *)data_to_hash, data_to_hash_len, > - &digest_list); > - if (ret != EFI_SUCCESS) > - goto out; > - > ret = tcg2_pcr_extend(dev, pcr_index, &digest_list); > if (ret != EFI_SUCCESS) > goto out; >
On 4/16/21 10:42 PM, Ilias Apalodimas wrote: > Hi Heinrich, > > On Thu, Apr 15, 2021 at 04:08:55PM +0200, Heinrich Schuchardt wrote: >> On 15.04.21 15:30, Masahisa Kojima wrote: >>> "TCG PC Client Platform Firmware Profile Specification" >>> requires to measure every attempt to load and execute >>> a OS Loader(a UEFI application) into PCR[4]. >>> This commit adds the PE/COFF image measurement, extends PCR, >>> and appends measurement into Event Log. >>> >>> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> >> >> Please, provide a unit test that we can run in Gitlab CI on either the >> sandbox or QEMU. > > The additions to the EFI TCG2 fall under the same category as the initial > patchset and unfortunately suffer from the same problems wrt to using the > sandbox TPM2. > The sandbox capabilities are limited for testing this, starting from the fact > that that we can't even get the tpm2 capabilities we need to start the > protocol correctly. > ./drivers/tpm/tpm2_tis_sandbox.c only supports TPM_CAP_TPM_PROPERTIES which is > limited compared to what the TCG code in EFI expects. Similar functionality > is missing from extending and checking PCRs properly etc. > >> >> QEMU allows to use a TPM emulation, cf. >> https://qemu-project.gitlab.io/qemu/specs/tpm.html#the-qemu-tpm-emulator-device >> https://github.com/stefanberger/swtpm > > Kojima and I will have a look since this is the only viable option in order to > get useful selftests. > Imho we should review and maybe accept this patch in parallel though, since > it's adding more bits of the TCG PC client specification. > > Thanks! > /Ilias Hello Masahisa, I am done with my review of the series and waiting for your v2. Ilias suggested to implement tests in a separate series. I am fine with this if Ilias supplies a tested-by sign-off for this series. Best regards Heinrich
Hi Heinrich, Thank you very much for your review comments. > > + ret = efi_check_pe(efi, efi_size, (void **)&nt); > Why are you calling this function? It is already called in efi_load_pe(). This is for the case that tcg2_hash_pe_image() is called from efi_tcg2_hash_log_extend_event(). Anyway, I will move efi_check_pe() call inside of efi_tcg2_hash_log_extend_event(). > > + if (!(active & alg_to_mask(hash_alg))) > > + continue; > > + switch (hash_alg) { > > + case TPM2_ALG_SHA1: > > SHA1 is known to be unsafe. Why would we support it? Basically I agree with removing SHA1 support. This efi_tcg2.c implementation aims to support TCG v2, so there is no reason to keep SHA1. Anyway, SHA1 is supported in tcg2_create_digest() for the measurement other than PE/COFF image. Do we also remove SHA1 from tcg2_create_digest()? For other comments, I will modify the code and send v2 patch. Thanks, Masahisa On Wed, 21 Apr 2021 at 19:57, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 4/15/21 3:30 PM, Masahisa Kojima wrote: > > "TCG PC Client Platform Firmware Profile Specification" > > requires to measure every attempt to load and execute > > a OS Loader(a UEFI application) into PCR[4]. > > This commit adds the PE/COFF image measurement, extends PCR, > > and appends measurement into Event Log. > > > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> > > --- > > include/efi_loader.h | 4 + > > include/efi_tcg2.h | 10 ++ > > include/tpm-v2.h | 1 + > > lib/efi_loader/efi_image_loader.c | 7 ++ > > lib/efi_loader/efi_tcg2.c | 187 ++++++++++++++++++++++++++++-- > > 5 files changed, 199 insertions(+), 10 deletions(-) > > > > diff --git a/include/efi_loader.h b/include/efi_loader.h > > index de1a496a97..b02bc93c8e 100644 > > --- a/include/efi_loader.h > > +++ b/include/efi_loader.h > > @@ -426,6 +426,10 @@ efi_status_t efi_disk_register(void); > > efi_status_t efi_rng_register(void); > > /* Called by efi_init_obj_list() to install EFI_TCG2_PROTOCOL */ > > efi_status_t efi_tcg2_register(void); > > +/* measure the pe-coff image, extend PCR and add Event Log */ > > +efi_status_t tcg2_measure_pe_image(void *efi, u64 efi_size, > > + struct efi_loaded_image_obj *handle, > > + struct efi_loaded_image *loaded_image_info); > > /* Create handles and protocols for the partitions of a block device */ > > int efi_disk_create_partitions(efi_handle_t parent, struct blk_desc *desc, > > const char *if_typename, int diskid, > > diff --git a/include/efi_tcg2.h b/include/efi_tcg2.h > > index 40e241ce31..f8d46c5fd2 100644 > > --- a/include/efi_tcg2.h > > +++ b/include/efi_tcg2.h > > @@ -9,6 +9,8 @@ > > #if !defined _EFI_TCG2_PROTOCOL_H_ > > #define _EFI_TCG2_PROTOCOL_H_ > > > > +#include <efi.h> > > This include is already included in efi_api.h. > > > +#include <efi_api.h> > > #include <tpm-v2.h> > > > > #define EFI_TCG2_PROTOCOL_GUID \ > > @@ -53,6 +55,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[]; > > A device path is not an array of struct efi_device_path. But the first > element is of this type. So ok. > > > +} __packed; > > Why should this be __packed? You don't use arrays of this structure and > it is naturally packed. > > > + > > 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 df67a196cf..ab9c04dc0a 100644 > > --- a/include/tpm-v2.h > > +++ b/include/tpm-v2.h > > @@ -61,6 +61,7 @@ struct udevice; > > #define EV_S_CRTM_VERSION ((u32)0x00000008) > > #define EV_CPU_MICROCODE ((u32)0x00000009) > > #define EV_TABLE_OF_DEVICES ((u32)0x0000000B) > > Please, add a comment here that the following values are defined in the > "TCG EFI Platform Specification". > > > +#define EV_EFI_BOOT_SERVICES_APPLICATION ((u32)0x80000003) > > Please, add all EV_EFI_* constants. > > > > > /* TPMS_TAGGED_PROPERTY Structure */ > > struct tpms_tagged_property { > > diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c > > index 2c35cb5651..b032ec5dd8 100644 > > --- a/lib/efi_loader/efi_image_loader.c > > +++ b/lib/efi_loader/efi_image_loader.c > > @@ -829,6 +829,13 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle, > > goto err; > > } > > > > +#if CONFIG_IS_ENABLED(EFI_TCG2_PROTOCOL) > > + /* Measure an PE/COFF image */ > > + if (tcg2_measure_pe_image(efi, efi_size, handle, > > + loaded_image_info)) > > + log_err("PE image measurement failed\n"); > > +#endif > > + > > /* Copy PE headers */ > > memcpy(efi_reloc, efi, > > sizeof(*dos) > > diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c > > index ed86a220fb..9fab07605f 100644 > > --- a/lib/efi_loader/efi_tcg2.c > > +++ b/lib/efi_loader/efi_tcg2.c > > @@ -13,8 +13,10 @@ > > #include <efi_loader.h> > > #include <efi_tcg2.h> > > #include <log.h> > > +#include <malloc.h> > > #include <version.h> > > #include <tpm-v2.h> > > +#include <u-boot/rsa.h> > > #include <u-boot/sha1.h> > > #include <u-boot/sha256.h> > > #include <u-boot/sha512.h> > > @@ -709,6 +711,172 @@ out: > > return EFI_EXIT(ret); > > } > > > > +static efi_status_t tcg2_hash_pe_image(void *efi, u64 efi_size, > > + struct tpml_digest_values *digest_list) > > +{ > > + IMAGE_NT_HEADERS32 *nt; > > + WIN_CERTIFICATE *wincerts = NULL; > > + size_t wincerts_len; > > + struct efi_image_regions *regs = NULL; > > + void *new_efi = NULL; > > + size_t new_efi_size; > > + u8 hash[TPM2_SHA512_DIGEST_SIZE]; > > + efi_status_t ret; > > + u32 active; > > + int i; > > + > > + ret = efi_check_pe(efi, efi_size, (void **)&nt); > Why are you calling this function? It is already called in efi_load_pe(). > > > + if (ret != EFI_SUCCESS) { > > + log_err("Not a valid PE-COFF file\n"); > > + return EFI_INVALID_PARAMETER; > > + } > > + > > + /* > > + * 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); > > + new_efi = calloc(new_efi_size, 1); > > + if (!new_efi) > > + return EFI_OUT_OF_RESOURCES; > > + memcpy(new_efi, efi, efi_size); > > + efi = new_efi; > > + efi_size = new_efi_size; > > + } > > + > > + if (!efi_image_parse(efi, efi_size, ®s, &wincerts, > > + &wincerts_len)) { > > + log_err("Parsing PE executable image failed\n"); > > + ret = EFI_INVALID_PARAMETER; > > + goto out; > > + } > > Please, don't duplicate code from efi_image_authenticate(). Extract a > common function instead. > > > + > > + ret = __get_active_pcr_banks(&active); > > + if (ret != EFI_SUCCESS) { > > + ret = EFI_DEVICE_ERROR; > > + goto out; > > + } > > + > > + digest_list->count = 0; > > + for (i = 0; i < MAX_HASH_COUNT; i++) { > > + u16 hash_alg = hash_algo_list[i].hash_alg; > > + > > + if (!(active & alg_to_mask(hash_alg))) > > + continue; > > + switch (hash_alg) { > > + case TPM2_ALG_SHA1: > > SHA1 is known to be unsafe. Why would we support it? > > > + hash_calculate("sha1", regs->reg, regs->num, hash); > > + digest_list->count++; > > Why do you repeat this line in every case of the switch statement? > Please, put it below. > > > + break; > > + case TPM2_ALG_SHA256: > > + hash_calculate("sha256", regs->reg, regs->num, hash); > > + digest_list->count++; > > + break; > > + case TPM2_ALG_SHA384: > > + hash_calculate("sha384", regs->reg, regs->num, hash); > > + digest_list->count++; > > + break; > > + case TPM2_ALG_SHA512: > > + hash_calculate("sha512", regs->reg, regs->num, hash); > > + digest_list->count++; > > + break; > > + default: > > + EFI_PRINT("Unsupported algorithm %x\n", hash_alg); > > + return EFI_INVALID_PARAMETER; > > + } > > + digest_list->digests[i].hash_alg = hash_alg; > > + memcpy(&digest_list->digests[i].digest, hash, (u32)alg_to_len(hash_alg)); > > + } > > + > > +out: > > + free(new_efi); > > + free(regs); > > + > > + return ret; > > +} > > + > > +efi_status_t tcg2_measure_pe_image(void *efi, u64 efi_size, > > + struct efi_loaded_image_obj *handle, > > + struct efi_loaded_image *loaded_image) > > +{ > > + struct tpml_digest_values digest_list; > > + efi_status_t ret; > > + struct udevice *dev; > > + u32 pcr_index, event_type, event_size; > > + struct uefi_image_load_event *image_load_event; > > + u8 *event; > > The variable event is not needed. You can use image_load_event directly. > > > + struct efi_device_path *device_path; > > + u32 device_path_length; > > + IMAGE_DOS_HEADER *dos; > > + IMAGE_NT_HEADERS32 *nt; > > + > > + ret = platform_get_tpm2_device(&dev); > > + if (ret != EFI_SUCCESS) > > + return ret; > > + > > + if (handle->image_type == IMAGE_SUBSYSTEM_EFI_APPLICATION) { > > We should measure drivers too. Cf. > EV_EFI_BOOT_SERVICES_DRIVER, EV_EFI_RUNTIME_SERVICES_DRIVER. > > > + pcr_index = 4; > > + event_type = EV_EFI_BOOT_SERVICES_APPLICATION; > > + } else { > > + return EFI_UNSUPPORTED; > > + } > > + > > + ret = tcg2_hash_pe_image(efi, efi_size, &digest_list); > > + if (ret != EFI_SUCCESS) > > + return ret; > > + > > + ret = tcg2_pcr_extend(dev, pcr_index, &digest_list); > > + if (ret != EFI_SUCCESS) > > + return ret; > > + > > + loaded_image->system_table->boottime->handle_protocol(&handle->header, > > + &efi_guid_loaded_image_device_path, > > + (void **)&device_path); > > You would have to use EFI_CALL() here. > > Please, use efi_search_protocol() instead of all this indirection. > > Best regards > > Heinrich > > > + device_path_length = efi_dp_size(device_path); > > + if (device_path_length > 0) { > > + /* add end node size */ > > + device_path_length += sizeof(struct efi_device_path); > > + } > > + event_size = sizeof(struct uefi_image_load_event) + device_path_length; > > + event = malloc(event_size); > > + if (!event) > > + return EFI_OUT_OF_RESOURCES; > > + > > + image_load_event = (struct uefi_image_load_event *)event; > > + image_load_event->image_location_in_memory = (efi_physical_addr_t)efi; > > + image_load_event->image_length_in_memory = efi_size; > > + image_load_event->length_of_device_path = device_path_length; > > + > > + dos = (IMAGE_DOS_HEADER *)efi; > > + nt = (IMAGE_NT_HEADERS32 *)(efi + dos->e_lfanew); > > + if (nt->OptionalHeader.Magic == IMAGE_NT_OPTIONAL_HDR64_MAGIC) { > > + IMAGE_NT_HEADERS64 *nt64 = (IMAGE_NT_HEADERS64 *)nt; > > + > > + image_load_event->image_link_time_address = > > + nt64->OptionalHeader.ImageBase; > > + } else if (nt->OptionalHeader.Magic == IMAGE_NT_OPTIONAL_HDR32_MAGIC) { > > + image_load_event->image_link_time_address = > > + nt->OptionalHeader.ImageBase; > > + } else { > > + ret = EFI_INVALID_PARAMETER; > > + goto out; > > + } > > + > > + if (device_path_length > 0) { > > + memcpy(image_load_event->device_path, device_path, > > + device_path_length); > > + } > > + > > + ret = tcg2_agile_log_append(pcr_index, event_type, &digest_list, > > + event_size, event); > > + > > +out: > > + free(event); > > + > > + return ret; > > +} > > + > > /** > > * efi_tcg2_hash_log_extend_event() - extend and optionally log events > > * > > @@ -761,24 +929,23 @@ efi_tcg2_hash_log_extend_event(struct efi_tcg2_protocol *this, u64 flags, > > /* > > * if PE_COFF_IMAGE is set we need to make sure the image is not > > * corrupted, verify it and hash the PE/COFF image in accordance with > > - * the procedure specified in "Calculating the PE Image Hash" > > - * section of the "Windows Authenticode Portable Executable Signature > > + * the procedure specified in "Calculating the PE Image Hash" > > + * section of the "Windows Authenticode Portable Executable Signature > > * Format" > > - * Not supported for now > > */ > > if (flags & PE_COFF_IMAGE) { > > - ret = EFI_UNSUPPORTED; > > - goto out; > > + ret = tcg2_hash_pe_image((void *)data_to_hash, data_to_hash_len, > > + &digest_list); > > + } else { > > + ret = tcg2_create_digest((u8 *)data_to_hash, data_to_hash_len, > > + &digest_list); > > } > > + if (ret != EFI_SUCCESS) > > + goto out; > > > > pcr_index = efi_tcg_event->header.pcr_index; > > event_type = efi_tcg_event->header.event_type; > > > > - ret = tcg2_create_digest((u8 *)data_to_hash, data_to_hash_len, > > - &digest_list); > > - if (ret != EFI_SUCCESS) > > - goto out; > > - > > ret = tcg2_pcr_extend(dev, pcr_index, &digest_list); > > if (ret != EFI_SUCCESS) > > goto out; > > >
> > > + if (!(active & alg_to_mask(hash_alg))) > > > + continue; > > > + switch (hash_alg) { > > > + case TPM2_ALG_SHA1: > > > > SHA1 is known to be unsafe. Why would we support it? > > Basically I agree with removing SHA1 support. > This efi_tcg2.c implementation aims to support TCG v2, so there is no > reason to keep SHA1. > Anyway, SHA1 is supported in tcg2_create_digest() for the measurement > other than PE/COFF image. Do we also remove SHA1 from > tcg2_create_digest()? > The hardware dictates what kind of SHAxxx you are supposed to add in the EventLog and the PCRs. Why would we remove the functionality? If someone considers SHA1 unsafe, he can just disable it from his hardware and remove it from the active algorithms. Cheers /Ilias > For other comments, I will modify the code and send v2 patch. > > Thanks, > Masahisa > > > On Wed, 21 Apr 2021 at 19:57, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > On 4/15/21 3:30 PM, Masahisa Kojima wrote: > > > "TCG PC Client Platform Firmware Profile Specification" > > > requires to measure every attempt to load and execute > > > a OS Loader(a UEFI application) into PCR[4]. > > > This commit adds the PE/COFF image measurement, extends PCR, > > > and appends measurement into Event Log. > > > > > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> > > > --- > > > include/efi_loader.h | 4 + > > > include/efi_tcg2.h | 10 ++ > > > include/tpm-v2.h | 1 + > > > lib/efi_loader/efi_image_loader.c | 7 ++ > > > lib/efi_loader/efi_tcg2.c | 187 ++++++++++++++++++++++++++++-- > > > 5 files changed, 199 insertions(+), 10 deletions(-) > > > > > > diff --git a/include/efi_loader.h b/include/efi_loader.h > > > index de1a496a97..b02bc93c8e 100644 > > > --- a/include/efi_loader.h > > > +++ b/include/efi_loader.h > > > @@ -426,6 +426,10 @@ efi_status_t efi_disk_register(void); > > > efi_status_t efi_rng_register(void); > > > /* Called by efi_init_obj_list() to install EFI_TCG2_PROTOCOL */ > > > efi_status_t efi_tcg2_register(void); > > > +/* measure the pe-coff image, extend PCR and add Event Log */ > > > +efi_status_t tcg2_measure_pe_image(void *efi, u64 efi_size, > > > + struct efi_loaded_image_obj *handle, > > > + struct efi_loaded_image *loaded_image_info); > > > /* Create handles and protocols for the partitions of a block device */ > > > int efi_disk_create_partitions(efi_handle_t parent, struct blk_desc *desc, > > > const char *if_typename, int diskid, > > > diff --git a/include/efi_tcg2.h b/include/efi_tcg2.h > > > index 40e241ce31..f8d46c5fd2 100644 > > > --- a/include/efi_tcg2.h > > > +++ b/include/efi_tcg2.h > > > @@ -9,6 +9,8 @@ > > > #if !defined _EFI_TCG2_PROTOCOL_H_ > > > #define _EFI_TCG2_PROTOCOL_H_ > > > > > > +#include <efi.h> > > > > This include is already included in efi_api.h. > > > > > +#include <efi_api.h> > > > #include <tpm-v2.h> > > > > > > #define EFI_TCG2_PROTOCOL_GUID \ > > > @@ -53,6 +55,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[]; > > > > A device path is not an array of struct efi_device_path. But the first > > element is of this type. So ok. > > > > > +} __packed; > > > > Why should this be __packed? You don't use arrays of this structure and > > it is naturally packed. > > > > > + > > > 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 df67a196cf..ab9c04dc0a 100644 > > > --- a/include/tpm-v2.h > > > +++ b/include/tpm-v2.h > > > @@ -61,6 +61,7 @@ struct udevice; > > > #define EV_S_CRTM_VERSION ((u32)0x00000008) > > > #define EV_CPU_MICROCODE ((u32)0x00000009) > > > #define EV_TABLE_OF_DEVICES ((u32)0x0000000B) > > > > Please, add a comment here that the following values are defined in the > > "TCG EFI Platform Specification". > > > > > +#define EV_EFI_BOOT_SERVICES_APPLICATION ((u32)0x80000003) > > > > Please, add all EV_EFI_* constants. > > > > > > > > /* TPMS_TAGGED_PROPERTY Structure */ > > > struct tpms_tagged_property { > > > diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c > > > index 2c35cb5651..b032ec5dd8 100644 > > > --- a/lib/efi_loader/efi_image_loader.c > > > +++ b/lib/efi_loader/efi_image_loader.c > > > @@ -829,6 +829,13 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle, > > > goto err; > > > } > > > > > > +#if CONFIG_IS_ENABLED(EFI_TCG2_PROTOCOL) > > > + /* Measure an PE/COFF image */ > > > + if (tcg2_measure_pe_image(efi, efi_size, handle, > > > + loaded_image_info)) > > > + log_err("PE image measurement failed\n"); > > > +#endif > > > + > > > /* Copy PE headers */ > > > memcpy(efi_reloc, efi, > > > sizeof(*dos) > > > diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c > > > index ed86a220fb..9fab07605f 100644 > > > --- a/lib/efi_loader/efi_tcg2.c > > > +++ b/lib/efi_loader/efi_tcg2.c > > > @@ -13,8 +13,10 @@ > > > #include <efi_loader.h> > > > #include <efi_tcg2.h> > > > #include <log.h> > > > +#include <malloc.h> > > > #include <version.h> > > > #include <tpm-v2.h> > > > +#include <u-boot/rsa.h> > > > #include <u-boot/sha1.h> > > > #include <u-boot/sha256.h> > > > #include <u-boot/sha512.h> > > > @@ -709,6 +711,172 @@ out: > > > return EFI_EXIT(ret); > > > } > > > > > > +static efi_status_t tcg2_hash_pe_image(void *efi, u64 efi_size, > > > + struct tpml_digest_values *digest_list) > > > +{ > > > + IMAGE_NT_HEADERS32 *nt; > > > + WIN_CERTIFICATE *wincerts = NULL; > > > + size_t wincerts_len; > > > + struct efi_image_regions *regs = NULL; > > > + void *new_efi = NULL; > > > + size_t new_efi_size; > > > + u8 hash[TPM2_SHA512_DIGEST_SIZE]; > > > + efi_status_t ret; > > > + u32 active; > > > + int i; > > > + > > > + ret = efi_check_pe(efi, efi_size, (void **)&nt); > > Why are you calling this function? It is already called in efi_load_pe(). > > > > > + if (ret != EFI_SUCCESS) { > > > + log_err("Not a valid PE-COFF file\n"); > > > + return EFI_INVALID_PARAMETER; > > > + } > > > + > > > + /* > > > + * 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); > > > + new_efi = calloc(new_efi_size, 1); > > > + if (!new_efi) > > > + return EFI_OUT_OF_RESOURCES; > > > + memcpy(new_efi, efi, efi_size); > > > + efi = new_efi; > > > + efi_size = new_efi_size; > > > + } > > > + > > > + if (!efi_image_parse(efi, efi_size, ®s, &wincerts, > > > + &wincerts_len)) { > > > + log_err("Parsing PE executable image failed\n"); > > > + ret = EFI_INVALID_PARAMETER; > > > + goto out; > > > + } > > > > Please, don't duplicate code from efi_image_authenticate(). Extract a > > common function instead. > > > > > + > > > + ret = __get_active_pcr_banks(&active); > > > + if (ret != EFI_SUCCESS) { > > > + ret = EFI_DEVICE_ERROR; > > > + goto out; > > > + } > > > + > > > + digest_list->count = 0; > > > + for (i = 0; i < MAX_HASH_COUNT; i++) { > > > + u16 hash_alg = hash_algo_list[i].hash_alg; > > > + > > > + if (!(active & alg_to_mask(hash_alg))) > > > + continue; > > > + switch (hash_alg) { > > > + case TPM2_ALG_SHA1: > > > > SHA1 is known to be unsafe. Why would we support it? > > > > > + hash_calculate("sha1", regs->reg, regs->num, hash); > > > + digest_list->count++; > > > > Why do you repeat this line in every case of the switch statement? > > Please, put it below. > > > > > + break; > > > + case TPM2_ALG_SHA256: > > > + hash_calculate("sha256", regs->reg, regs->num, hash); > > > + digest_list->count++; > > > + break; > > > + case TPM2_ALG_SHA384: > > > + hash_calculate("sha384", regs->reg, regs->num, hash); > > > + digest_list->count++; > > > + break; > > > + case TPM2_ALG_SHA512: > > > + hash_calculate("sha512", regs->reg, regs->num, hash); > > > + digest_list->count++; > > > + break; > > > + default: > > > + EFI_PRINT("Unsupported algorithm %x\n", hash_alg); > > > + return EFI_INVALID_PARAMETER; > > > + } > > > + digest_list->digests[i].hash_alg = hash_alg; > > > + memcpy(&digest_list->digests[i].digest, hash, (u32)alg_to_len(hash_alg)); > > > + } > > > + > > > +out: > > > + free(new_efi); > > > + free(regs); > > > + > > > + return ret; > > > +} > > > + > > > +efi_status_t tcg2_measure_pe_image(void *efi, u64 efi_size, > > > + struct efi_loaded_image_obj *handle, > > > + struct efi_loaded_image *loaded_image) > > > +{ > > > + struct tpml_digest_values digest_list; > > > + efi_status_t ret; > > > + struct udevice *dev; > > > + u32 pcr_index, event_type, event_size; > > > + struct uefi_image_load_event *image_load_event; > > > + u8 *event; > > > > The variable event is not needed. You can use image_load_event directly. > > > > > + struct efi_device_path *device_path; > > > + u32 device_path_length; > > > + IMAGE_DOS_HEADER *dos; > > > + IMAGE_NT_HEADERS32 *nt; > > > + > > > + ret = platform_get_tpm2_device(&dev); > > > + if (ret != EFI_SUCCESS) > > > + return ret; > > > + > > > + if (handle->image_type == IMAGE_SUBSYSTEM_EFI_APPLICATION) { > > > > We should measure drivers too. Cf. > > EV_EFI_BOOT_SERVICES_DRIVER, EV_EFI_RUNTIME_SERVICES_DRIVER. > > > > > + pcr_index = 4; > > > + event_type = EV_EFI_BOOT_SERVICES_APPLICATION; > > > + } else { > > > + return EFI_UNSUPPORTED; > > > + } > > > + > > > + ret = tcg2_hash_pe_image(efi, efi_size, &digest_list); > > > + if (ret != EFI_SUCCESS) > > > + return ret; > > > + > > > + ret = tcg2_pcr_extend(dev, pcr_index, &digest_list); > > > + if (ret != EFI_SUCCESS) > > > + return ret; > > > + > > > + loaded_image->system_table->boottime->handle_protocol(&handle->header, > > > + &efi_guid_loaded_image_device_path, > > > + (void **)&device_path); > > > > You would have to use EFI_CALL() here. > > > > Please, use efi_search_protocol() instead of all this indirection. > > > > Best regards > > > > Heinrich > > > > > + device_path_length = efi_dp_size(device_path); > > > + if (device_path_length > 0) { > > > + /* add end node size */ > > > + device_path_length += sizeof(struct efi_device_path); > > > + } > > > + event_size = sizeof(struct uefi_image_load_event) + device_path_length; > > > + event = malloc(event_size); > > > + if (!event) > > > + return EFI_OUT_OF_RESOURCES; > > > + > > > + image_load_event = (struct uefi_image_load_event *)event; > > > + image_load_event->image_location_in_memory = (efi_physical_addr_t)efi; > > > + image_load_event->image_length_in_memory = efi_size; > > > + image_load_event->length_of_device_path = device_path_length; > > > + > > > + dos = (IMAGE_DOS_HEADER *)efi; > > > + nt = (IMAGE_NT_HEADERS32 *)(efi + dos->e_lfanew); > > > + if (nt->OptionalHeader.Magic == IMAGE_NT_OPTIONAL_HDR64_MAGIC) { > > > + IMAGE_NT_HEADERS64 *nt64 = (IMAGE_NT_HEADERS64 *)nt; > > > + > > > + image_load_event->image_link_time_address = > > > + nt64->OptionalHeader.ImageBase; > > > + } else if (nt->OptionalHeader.Magic == IMAGE_NT_OPTIONAL_HDR32_MAGIC) { > > > + image_load_event->image_link_time_address = > > > + nt->OptionalHeader.ImageBase; > > > + } else { > > > + ret = EFI_INVALID_PARAMETER; > > > + goto out; > > > + } > > > + > > > + if (device_path_length > 0) { > > > + memcpy(image_load_event->device_path, device_path, > > > + device_path_length); > > > + } > > > + > > > + ret = tcg2_agile_log_append(pcr_index, event_type, &digest_list, > > > + event_size, event); > > > + > > > +out: > > > + free(event); > > > + > > > + return ret; > > > +} > > > + > > > /** > > > * efi_tcg2_hash_log_extend_event() - extend and optionally log events > > > * > > > @@ -761,24 +929,23 @@ efi_tcg2_hash_log_extend_event(struct efi_tcg2_protocol *this, u64 flags, > > > /* > > > * if PE_COFF_IMAGE is set we need to make sure the image is not > > > * corrupted, verify it and hash the PE/COFF image in accordance with > > > - * the procedure specified in "Calculating the PE Image Hash" > > > - * section of the "Windows Authenticode Portable Executable Signature > > > + * the procedure specified in "Calculating the PE Image Hash" > > > + * section of the "Windows Authenticode Portable Executable Signature > > > * Format" > > > - * Not supported for now > > > */ > > > if (flags & PE_COFF_IMAGE) { > > > - ret = EFI_UNSUPPORTED; > > > - goto out; > > > + ret = tcg2_hash_pe_image((void *)data_to_hash, data_to_hash_len, > > > + &digest_list); > > > + } else { > > > + ret = tcg2_create_digest((u8 *)data_to_hash, data_to_hash_len, > > > + &digest_list); > > > } > > > + if (ret != EFI_SUCCESS) > > > + goto out; > > > > > > pcr_index = efi_tcg_event->header.pcr_index; > > > event_type = efi_tcg_event->header.event_type; > > > > > > - ret = tcg2_create_digest((u8 *)data_to_hash, data_to_hash_len, > > > - &digest_list); > > > - if (ret != EFI_SUCCESS) > > > - goto out; > > > - > > > ret = tcg2_pcr_extend(dev, pcr_index, &digest_list); > > > if (ret != EFI_SUCCESS) > > > goto out; > > > > >
On 22.04.21 10:09, Ilias Apalodimas wrote: >>>> + if (!(active & alg_to_mask(hash_alg))) >>>> + continue; >>>> + switch (hash_alg) { >>>> + case TPM2_ALG_SHA1: >>> >>> SHA1 is known to be unsafe. Why would we support it? >> >> Basically I agree with removing SHA1 support. >> This efi_tcg2.c implementation aims to support TCG v2, so there is no >> reason to keep SHA1. >> Anyway, SHA1 is supported in tcg2_create_digest() for the measurement >> other than PE/COFF image. Do we also remove SHA1 from >> tcg2_create_digest()? >> > > The hardware dictates what kind of SHAxxx you are supposed to add in the > EventLog and the PCRs. Why would we remove the functionality? If someone > considers SHA1 unsafe, he can just disable it from his hardware and remove it > from the active algorithms. > > > Cheers > /Ilias The TCG EFI ProtocolSpecification explicitely enumerates the four hashing algorithms of Masahisa's patch, see chapter 6.4.3, "Related Definitions". So let's support them. Best regards Heinrich > >> For other comments, I will modify the code and send v2 patch. >> >> Thanks, >> Masahisa >> >> >> On Wed, 21 Apr 2021 at 19:57, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >>> >>> On 4/15/21 3:30 PM, Masahisa Kojima wrote: >>>> "TCG PC Client Platform Firmware Profile Specification" >>>> requires to measure every attempt to load and execute >>>> a OS Loader(a UEFI application) into PCR[4]. >>>> This commit adds the PE/COFF image measurement, extends PCR, >>>> and appends measurement into Event Log. >>>> >>>> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> >>>> --- >>>> include/efi_loader.h | 4 + >>>> include/efi_tcg2.h | 10 ++ >>>> include/tpm-v2.h | 1 + >>>> lib/efi_loader/efi_image_loader.c | 7 ++ >>>> lib/efi_loader/efi_tcg2.c | 187 ++++++++++++++++++++++++++++-- >>>> 5 files changed, 199 insertions(+), 10 deletions(-) >>>> >>>> diff --git a/include/efi_loader.h b/include/efi_loader.h >>>> index de1a496a97..b02bc93c8e 100644 >>>> --- a/include/efi_loader.h >>>> +++ b/include/efi_loader.h >>>> @@ -426,6 +426,10 @@ efi_status_t efi_disk_register(void); >>>> efi_status_t efi_rng_register(void); >>>> /* Called by efi_init_obj_list() to install EFI_TCG2_PROTOCOL */ >>>> efi_status_t efi_tcg2_register(void); >>>> +/* measure the pe-coff image, extend PCR and add Event Log */ >>>> +efi_status_t tcg2_measure_pe_image(void *efi, u64 efi_size, >>>> + struct efi_loaded_image_obj *handle, >>>> + struct efi_loaded_image *loaded_image_info); >>>> /* Create handles and protocols for the partitions of a block device */ >>>> int efi_disk_create_partitions(efi_handle_t parent, struct blk_desc *desc, >>>> const char *if_typename, int diskid, >>>> diff --git a/include/efi_tcg2.h b/include/efi_tcg2.h >>>> index 40e241ce31..f8d46c5fd2 100644 >>>> --- a/include/efi_tcg2.h >>>> +++ b/include/efi_tcg2.h >>>> @@ -9,6 +9,8 @@ >>>> #if !defined _EFI_TCG2_PROTOCOL_H_ >>>> #define _EFI_TCG2_PROTOCOL_H_ >>>> >>>> +#include <efi.h> >>> >>> This include is already included in efi_api.h. >>> >>>> +#include <efi_api.h> >>>> #include <tpm-v2.h> >>>> >>>> #define EFI_TCG2_PROTOCOL_GUID \ >>>> @@ -53,6 +55,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[]; >>> >>> A device path is not an array of struct efi_device_path. But the first >>> element is of this type. So ok. >>> >>>> +} __packed; >>> >>> Why should this be __packed? You don't use arrays of this structure and >>> it is naturally packed. >>> >>>> + >>>> 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 df67a196cf..ab9c04dc0a 100644 >>>> --- a/include/tpm-v2.h >>>> +++ b/include/tpm-v2.h >>>> @@ -61,6 +61,7 @@ struct udevice; >>>> #define EV_S_CRTM_VERSION ((u32)0x00000008) >>>> #define EV_CPU_MICROCODE ((u32)0x00000009) >>>> #define EV_TABLE_OF_DEVICES ((u32)0x0000000B) >>> >>> Please, add a comment here that the following values are defined in the >>> "TCG EFI Platform Specification". >>> >>>> +#define EV_EFI_BOOT_SERVICES_APPLICATION ((u32)0x80000003) >>> >>> Please, add all EV_EFI_* constants. >>> >>>> >>>> /* TPMS_TAGGED_PROPERTY Structure */ >>>> struct tpms_tagged_property { >>>> diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c >>>> index 2c35cb5651..b032ec5dd8 100644 >>>> --- a/lib/efi_loader/efi_image_loader.c >>>> +++ b/lib/efi_loader/efi_image_loader.c >>>> @@ -829,6 +829,13 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle, >>>> goto err; >>>> } >>>> >>>> +#if CONFIG_IS_ENABLED(EFI_TCG2_PROTOCOL) >>>> + /* Measure an PE/COFF image */ >>>> + if (tcg2_measure_pe_image(efi, efi_size, handle, >>>> + loaded_image_info)) >>>> + log_err("PE image measurement failed\n"); >>>> +#endif >>>> + >>>> /* Copy PE headers */ >>>> memcpy(efi_reloc, efi, >>>> sizeof(*dos) >>>> diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c >>>> index ed86a220fb..9fab07605f 100644 >>>> --- a/lib/efi_loader/efi_tcg2.c >>>> +++ b/lib/efi_loader/efi_tcg2.c >>>> @@ -13,8 +13,10 @@ >>>> #include <efi_loader.h> >>>> #include <efi_tcg2.h> >>>> #include <log.h> >>>> +#include <malloc.h> >>>> #include <version.h> >>>> #include <tpm-v2.h> >>>> +#include <u-boot/rsa.h> >>>> #include <u-boot/sha1.h> >>>> #include <u-boot/sha256.h> >>>> #include <u-boot/sha512.h> >>>> @@ -709,6 +711,172 @@ out: >>>> return EFI_EXIT(ret); >>>> } >>>> >>>> +static efi_status_t tcg2_hash_pe_image(void *efi, u64 efi_size, >>>> + struct tpml_digest_values *digest_list) >>>> +{ >>>> + IMAGE_NT_HEADERS32 *nt; >>>> + WIN_CERTIFICATE *wincerts = NULL; >>>> + size_t wincerts_len; >>>> + struct efi_image_regions *regs = NULL; >>>> + void *new_efi = NULL; >>>> + size_t new_efi_size; >>>> + u8 hash[TPM2_SHA512_DIGEST_SIZE]; >>>> + efi_status_t ret; >>>> + u32 active; >>>> + int i; >>>> + >>>> + ret = efi_check_pe(efi, efi_size, (void **)&nt); >>> Why are you calling this function? It is already called in efi_load_pe(). >>> >>>> + if (ret != EFI_SUCCESS) { >>>> + log_err("Not a valid PE-COFF file\n"); >>>> + return EFI_INVALID_PARAMETER; >>>> + } >>>> + >>>> + /* >>>> + * 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); >>>> + new_efi = calloc(new_efi_size, 1); >>>> + if (!new_efi) >>>> + return EFI_OUT_OF_RESOURCES; >>>> + memcpy(new_efi, efi, efi_size); >>>> + efi = new_efi; >>>> + efi_size = new_efi_size; >>>> + } >>>> + >>>> + if (!efi_image_parse(efi, efi_size, ®s, &wincerts, >>>> + &wincerts_len)) { >>>> + log_err("Parsing PE executable image failed\n"); >>>> + ret = EFI_INVALID_PARAMETER; >>>> + goto out; >>>> + } >>> >>> Please, don't duplicate code from efi_image_authenticate(). Extract a >>> common function instead. >>> >>>> + >>>> + ret = __get_active_pcr_banks(&active); >>>> + if (ret != EFI_SUCCESS) { >>>> + ret = EFI_DEVICE_ERROR; >>>> + goto out; >>>> + } >>>> + >>>> + digest_list->count = 0; >>>> + for (i = 0; i < MAX_HASH_COUNT; i++) { >>>> + u16 hash_alg = hash_algo_list[i].hash_alg; >>>> + >>>> + if (!(active & alg_to_mask(hash_alg))) >>>> + continue; >>>> + switch (hash_alg) { >>>> + case TPM2_ALG_SHA1: >>> >>> SHA1 is known to be unsafe. Why would we support it? >>> >>>> + hash_calculate("sha1", regs->reg, regs->num, hash); >>>> + digest_list->count++; >>> >>> Why do you repeat this line in every case of the switch statement? >>> Please, put it below. >>> >>>> + break; >>>> + case TPM2_ALG_SHA256: >>>> + hash_calculate("sha256", regs->reg, regs->num, hash); >>>> + digest_list->count++; >>>> + break; >>>> + case TPM2_ALG_SHA384: >>>> + hash_calculate("sha384", regs->reg, regs->num, hash); >>>> + digest_list->count++; >>>> + break; >>>> + case TPM2_ALG_SHA512: >>>> + hash_calculate("sha512", regs->reg, regs->num, hash); >>>> + digest_list->count++; >>>> + break; >>>> + default: >>>> + EFI_PRINT("Unsupported algorithm %x\n", hash_alg); >>>> + return EFI_INVALID_PARAMETER; >>>> + } >>>> + digest_list->digests[i].hash_alg = hash_alg; >>>> + memcpy(&digest_list->digests[i].digest, hash, (u32)alg_to_len(hash_alg)); >>>> + } >>>> + >>>> +out: >>>> + free(new_efi); >>>> + free(regs); >>>> + >>>> + return ret; >>>> +} >>>> + >>>> +efi_status_t tcg2_measure_pe_image(void *efi, u64 efi_size, >>>> + struct efi_loaded_image_obj *handle, >>>> + struct efi_loaded_image *loaded_image) >>>> +{ >>>> + struct tpml_digest_values digest_list; >>>> + efi_status_t ret; >>>> + struct udevice *dev; >>>> + u32 pcr_index, event_type, event_size; >>>> + struct uefi_image_load_event *image_load_event; >>>> + u8 *event; >>> >>> The variable event is not needed. You can use image_load_event directly. >>> >>>> + struct efi_device_path *device_path; >>>> + u32 device_path_length; >>>> + IMAGE_DOS_HEADER *dos; >>>> + IMAGE_NT_HEADERS32 *nt; >>>> + >>>> + ret = platform_get_tpm2_device(&dev); >>>> + if (ret != EFI_SUCCESS) >>>> + return ret; >>>> + >>>> + if (handle->image_type == IMAGE_SUBSYSTEM_EFI_APPLICATION) { >>> >>> We should measure drivers too. Cf. >>> EV_EFI_BOOT_SERVICES_DRIVER, EV_EFI_RUNTIME_SERVICES_DRIVER. >>> >>>> + pcr_index = 4; >>>> + event_type = EV_EFI_BOOT_SERVICES_APPLICATION; >>>> + } else { >>>> + return EFI_UNSUPPORTED; >>>> + } >>>> + >>>> + ret = tcg2_hash_pe_image(efi, efi_size, &digest_list); >>>> + if (ret != EFI_SUCCESS) >>>> + return ret; >>>> + >>>> + ret = tcg2_pcr_extend(dev, pcr_index, &digest_list); >>>> + if (ret != EFI_SUCCESS) >>>> + return ret; >>>> + >>>> + loaded_image->system_table->boottime->handle_protocol(&handle->header, >>>> + &efi_guid_loaded_image_device_path, >>>> + (void **)&device_path); >>> >>> You would have to use EFI_CALL() here. >>> >>> Please, use efi_search_protocol() instead of all this indirection. >>> >>> Best regards >>> >>> Heinrich >>> >>>> + device_path_length = efi_dp_size(device_path); >>>> + if (device_path_length > 0) { >>>> + /* add end node size */ >>>> + device_path_length += sizeof(struct efi_device_path); >>>> + } >>>> + event_size = sizeof(struct uefi_image_load_event) + device_path_length; >>>> + event = malloc(event_size); >>>> + if (!event) >>>> + return EFI_OUT_OF_RESOURCES; >>>> + >>>> + image_load_event = (struct uefi_image_load_event *)event; >>>> + image_load_event->image_location_in_memory = (efi_physical_addr_t)efi; >>>> + image_load_event->image_length_in_memory = efi_size; >>>> + image_load_event->length_of_device_path = device_path_length; >>>> + >>>> + dos = (IMAGE_DOS_HEADER *)efi; >>>> + nt = (IMAGE_NT_HEADERS32 *)(efi + dos->e_lfanew); >>>> + if (nt->OptionalHeader.Magic == IMAGE_NT_OPTIONAL_HDR64_MAGIC) { >>>> + IMAGE_NT_HEADERS64 *nt64 = (IMAGE_NT_HEADERS64 *)nt; >>>> + >>>> + image_load_event->image_link_time_address = >>>> + nt64->OptionalHeader.ImageBase; >>>> + } else if (nt->OptionalHeader.Magic == IMAGE_NT_OPTIONAL_HDR32_MAGIC) { >>>> + image_load_event->image_link_time_address = >>>> + nt->OptionalHeader.ImageBase; >>>> + } else { >>>> + ret = EFI_INVALID_PARAMETER; >>>> + goto out; >>>> + } >>>> + >>>> + if (device_path_length > 0) { >>>> + memcpy(image_load_event->device_path, device_path, >>>> + device_path_length); >>>> + } >>>> + >>>> + ret = tcg2_agile_log_append(pcr_index, event_type, &digest_list, >>>> + event_size, event); >>>> + >>>> +out: >>>> + free(event); >>>> + >>>> + return ret; >>>> +} >>>> + >>>> /** >>>> * efi_tcg2_hash_log_extend_event() - extend and optionally log events >>>> * >>>> @@ -761,24 +929,23 @@ efi_tcg2_hash_log_extend_event(struct efi_tcg2_protocol *this, u64 flags, >>>> /* >>>> * if PE_COFF_IMAGE is set we need to make sure the image is not >>>> * corrupted, verify it and hash the PE/COFF image in accordance with >>>> - * the procedure specified in "Calculating the PE Image Hash" >>>> - * section of the "Windows Authenticode Portable Executable Signature >>>> + * the procedure specified in "Calculating the PE Image Hash" >>>> + * section of the "Windows Authenticode Portable Executable Signature >>>> * Format" >>>> - * Not supported for now >>>> */ >>>> if (flags & PE_COFF_IMAGE) { >>>> - ret = EFI_UNSUPPORTED; >>>> - goto out; >>>> + ret = tcg2_hash_pe_image((void *)data_to_hash, data_to_hash_len, >>>> + &digest_list); >>>> + } else { >>>> + ret = tcg2_create_digest((u8 *)data_to_hash, data_to_hash_len, >>>> + &digest_list); >>>> } >>>> + if (ret != EFI_SUCCESS) >>>> + goto out; >>>> >>>> pcr_index = efi_tcg_event->header.pcr_index; >>>> event_type = efi_tcg_event->header.event_type; >>>> >>>> - ret = tcg2_create_digest((u8 *)data_to_hash, data_to_hash_len, >>>> - &digest_list); >>>> - if (ret != EFI_SUCCESS) >>>> - goto out; >>>> - >>>> ret = tcg2_pcr_extend(dev, pcr_index, &digest_list); >>>> if (ret != EFI_SUCCESS) >>>> goto out; >>>> >>>
Hi Heinrich, > > +++ b/include/tpm-v2.h > > @@ -61,6 +61,7 @@ struct udevice; > > #define EV_S_CRTM_VERSION ((u32)0x00000008) > > #define EV_CPU_MICROCODE ((u32)0x00000009) > > #define EV_TABLE_OF_DEVICES ((u32)0x0000000B) > > Please, add a comment here that the following values are defined in the > "TCG EFI Platform Specification". > > > +#define EV_EFI_BOOT_SERVICES_APPLICATION ((u32)0x80000003) > > Please, add all EV_EFI_* constants. I have sent v2 patch and added EV_EFI_* constants. Anyway, EV_EFI_* are defined in the "TCG PC Client Platform Firmware Profile Specification" and it is better to add remaining events described in "Table 9 Event" as a separate patch. Best Regards, Masahisa On Thu, 22 Apr 2021 at 17:23, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 22.04.21 10:09, Ilias Apalodimas wrote: > >>>> + if (!(active & alg_to_mask(hash_alg))) > >>>> + continue; > >>>> + switch (hash_alg) { > >>>> + case TPM2_ALG_SHA1: > >>> > >>> SHA1 is known to be unsafe. Why would we support it? > >> > >> Basically I agree with removing SHA1 support. > >> This efi_tcg2.c implementation aims to support TCG v2, so there is no > >> reason to keep SHA1. > >> Anyway, SHA1 is supported in tcg2_create_digest() for the measurement > >> other than PE/COFF image. Do we also remove SHA1 from > >> tcg2_create_digest()? > >> > > > > The hardware dictates what kind of SHAxxx you are supposed to add in the > > EventLog and the PCRs. Why would we remove the functionality? If someone > > considers SHA1 unsafe, he can just disable it from his hardware and remove it > > from the active algorithms. > > > > > > Cheers > > /Ilias > > > The TCG EFI ProtocolSpecification explicitely enumerates the four > hashing algorithms of Masahisa's patch, see chapter 6.4.3, "Related > Definitions". > > So let's support them. > > Best regards > > Heinrich > > > > >> For other comments, I will modify the code and send v2 patch. > >> > >> Thanks, > >> Masahisa > >> > >> > >> On Wed, 21 Apr 2021 at 19:57, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > >>> > >>> On 4/15/21 3:30 PM, Masahisa Kojima wrote: > >>>> "TCG PC Client Platform Firmware Profile Specification" > >>>> requires to measure every attempt to load and execute > >>>> a OS Loader(a UEFI application) into PCR[4]. > >>>> This commit adds the PE/COFF image measurement, extends PCR, > >>>> and appends measurement into Event Log. > >>>> > >>>> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> > >>>> --- > >>>> include/efi_loader.h | 4 + > >>>> include/efi_tcg2.h | 10 ++ > >>>> include/tpm-v2.h | 1 + > >>>> lib/efi_loader/efi_image_loader.c | 7 ++ > >>>> lib/efi_loader/efi_tcg2.c | 187 ++++++++++++++++++++++++++++-- > >>>> 5 files changed, 199 insertions(+), 10 deletions(-) > >>>> > >>>> diff --git a/include/efi_loader.h b/include/efi_loader.h > >>>> index de1a496a97..b02bc93c8e 100644 > >>>> --- a/include/efi_loader.h > >>>> +++ b/include/efi_loader.h > >>>> @@ -426,6 +426,10 @@ efi_status_t efi_disk_register(void); > >>>> efi_status_t efi_rng_register(void); > >>>> /* Called by efi_init_obj_list() to install EFI_TCG2_PROTOCOL */ > >>>> efi_status_t efi_tcg2_register(void); > >>>> +/* measure the pe-coff image, extend PCR and add Event Log */ > >>>> +efi_status_t tcg2_measure_pe_image(void *efi, u64 efi_size, > >>>> + struct efi_loaded_image_obj *handle, > >>>> + struct efi_loaded_image *loaded_image_info); > >>>> /* Create handles and protocols for the partitions of a block device */ > >>>> int efi_disk_create_partitions(efi_handle_t parent, struct blk_desc *desc, > >>>> const char *if_typename, int diskid, > >>>> diff --git a/include/efi_tcg2.h b/include/efi_tcg2.h > >>>> index 40e241ce31..f8d46c5fd2 100644 > >>>> --- a/include/efi_tcg2.h > >>>> +++ b/include/efi_tcg2.h > >>>> @@ -9,6 +9,8 @@ > >>>> #if !defined _EFI_TCG2_PROTOCOL_H_ > >>>> #define _EFI_TCG2_PROTOCOL_H_ > >>>> > >>>> +#include <efi.h> > >>> > >>> This include is already included in efi_api.h. > >>> > >>>> +#include <efi_api.h> > >>>> #include <tpm-v2.h> > >>>> > >>>> #define EFI_TCG2_PROTOCOL_GUID \ > >>>> @@ -53,6 +55,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[]; > >>> > >>> A device path is not an array of struct efi_device_path. But the first > >>> element is of this type. So ok. > >>> > >>>> +} __packed; > >>> > >>> Why should this be __packed? You don't use arrays of this structure and > >>> it is naturally packed. > >>> > >>>> + > >>>> 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 df67a196cf..ab9c04dc0a 100644 > >>>> --- a/include/tpm-v2.h > >>>> +++ b/include/tpm-v2.h > >>>> @@ -61,6 +61,7 @@ struct udevice; > >>>> #define EV_S_CRTM_VERSION ((u32)0x00000008) > >>>> #define EV_CPU_MICROCODE ((u32)0x00000009) > >>>> #define EV_TABLE_OF_DEVICES ((u32)0x0000000B) > >>> > >>> Please, add a comment here that the following values are defined in the > >>> "TCG EFI Platform Specification". > >>> > >>>> +#define EV_EFI_BOOT_SERVICES_APPLICATION ((u32)0x80000003) > >>> > >>> Please, add all EV_EFI_* constants. > >>> > >>>> > >>>> /* TPMS_TAGGED_PROPERTY Structure */ > >>>> struct tpms_tagged_property { > >>>> diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c > >>>> index 2c35cb5651..b032ec5dd8 100644 > >>>> --- a/lib/efi_loader/efi_image_loader.c > >>>> +++ b/lib/efi_loader/efi_image_loader.c > >>>> @@ -829,6 +829,13 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle, > >>>> goto err; > >>>> } > >>>> > >>>> +#if CONFIG_IS_ENABLED(EFI_TCG2_PROTOCOL) > >>>> + /* Measure an PE/COFF image */ > >>>> + if (tcg2_measure_pe_image(efi, efi_size, handle, > >>>> + loaded_image_info)) > >>>> + log_err("PE image measurement failed\n"); > >>>> +#endif > >>>> + > >>>> /* Copy PE headers */ > >>>> memcpy(efi_reloc, efi, > >>>> sizeof(*dos) > >>>> diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c > >>>> index ed86a220fb..9fab07605f 100644 > >>>> --- a/lib/efi_loader/efi_tcg2.c > >>>> +++ b/lib/efi_loader/efi_tcg2.c > >>>> @@ -13,8 +13,10 @@ > >>>> #include <efi_loader.h> > >>>> #include <efi_tcg2.h> > >>>> #include <log.h> > >>>> +#include <malloc.h> > >>>> #include <version.h> > >>>> #include <tpm-v2.h> > >>>> +#include <u-boot/rsa.h> > >>>> #include <u-boot/sha1.h> > >>>> #include <u-boot/sha256.h> > >>>> #include <u-boot/sha512.h> > >>>> @@ -709,6 +711,172 @@ out: > >>>> return EFI_EXIT(ret); > >>>> } > >>>> > >>>> +static efi_status_t tcg2_hash_pe_image(void *efi, u64 efi_size, > >>>> + struct tpml_digest_values *digest_list) > >>>> +{ > >>>> + IMAGE_NT_HEADERS32 *nt; > >>>> + WIN_CERTIFICATE *wincerts = NULL; > >>>> + size_t wincerts_len; > >>>> + struct efi_image_regions *regs = NULL; > >>>> + void *new_efi = NULL; > >>>> + size_t new_efi_size; > >>>> + u8 hash[TPM2_SHA512_DIGEST_SIZE]; > >>>> + efi_status_t ret; > >>>> + u32 active; > >>>> + int i; > >>>> + > >>>> + ret = efi_check_pe(efi, efi_size, (void **)&nt); > >>> Why are you calling this function? It is already called in efi_load_pe(). > >>> > >>>> + if (ret != EFI_SUCCESS) { > >>>> + log_err("Not a valid PE-COFF file\n"); > >>>> + return EFI_INVALID_PARAMETER; > >>>> + } > >>>> + > >>>> + /* > >>>> + * 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); > >>>> + new_efi = calloc(new_efi_size, 1); > >>>> + if (!new_efi) > >>>> + return EFI_OUT_OF_RESOURCES; > >>>> + memcpy(new_efi, efi, efi_size); > >>>> + efi = new_efi; > >>>> + efi_size = new_efi_size; > >>>> + } > >>>> + > >>>> + if (!efi_image_parse(efi, efi_size, ®s, &wincerts, > >>>> + &wincerts_len)) { > >>>> + log_err("Parsing PE executable image failed\n"); > >>>> + ret = EFI_INVALID_PARAMETER; > >>>> + goto out; > >>>> + } > >>> > >>> Please, don't duplicate code from efi_image_authenticate(). Extract a > >>> common function instead. > >>> > >>>> + > >>>> + ret = __get_active_pcr_banks(&active); > >>>> + if (ret != EFI_SUCCESS) { > >>>> + ret = EFI_DEVICE_ERROR; > >>>> + goto out; > >>>> + } > >>>> + > >>>> + digest_list->count = 0; > >>>> + for (i = 0; i < MAX_HASH_COUNT; i++) { > >>>> + u16 hash_alg = hash_algo_list[i].hash_alg; > >>>> + > >>>> + if (!(active & alg_to_mask(hash_alg))) > >>>> + continue; > >>>> + switch (hash_alg) { > >>>> + case TPM2_ALG_SHA1: > >>> > >>> SHA1 is known to be unsafe. Why would we support it? > >>> > >>>> + hash_calculate("sha1", regs->reg, regs->num, hash); > >>>> + digest_list->count++; > >>> > >>> Why do you repeat this line in every case of the switch statement? > >>> Please, put it below. > >>> > >>>> + break; > >>>> + case TPM2_ALG_SHA256: > >>>> + hash_calculate("sha256", regs->reg, regs->num, hash); > >>>> + digest_list->count++; > >>>> + break; > >>>> + case TPM2_ALG_SHA384: > >>>> + hash_calculate("sha384", regs->reg, regs->num, hash); > >>>> + digest_list->count++; > >>>> + break; > >>>> + case TPM2_ALG_SHA512: > >>>> + hash_calculate("sha512", regs->reg, regs->num, hash); > >>>> + digest_list->count++; > >>>> + break; > >>>> + default: > >>>> + EFI_PRINT("Unsupported algorithm %x\n", hash_alg); > >>>> + return EFI_INVALID_PARAMETER; > >>>> + } > >>>> + digest_list->digests[i].hash_alg = hash_alg; > >>>> + memcpy(&digest_list->digests[i].digest, hash, (u32)alg_to_len(hash_alg)); > >>>> + } > >>>> + > >>>> +out: > >>>> + free(new_efi); > >>>> + free(regs); > >>>> + > >>>> + return ret; > >>>> +} > >>>> + > >>>> +efi_status_t tcg2_measure_pe_image(void *efi, u64 efi_size, > >>>> + struct efi_loaded_image_obj *handle, > >>>> + struct efi_loaded_image *loaded_image) > >>>> +{ > >>>> + struct tpml_digest_values digest_list; > >>>> + efi_status_t ret; > >>>> + struct udevice *dev; > >>>> + u32 pcr_index, event_type, event_size; > >>>> + struct uefi_image_load_event *image_load_event; > >>>> + u8 *event; > >>> > >>> The variable event is not needed. You can use image_load_event directly. > >>> > >>>> + struct efi_device_path *device_path; > >>>> + u32 device_path_length; > >>>> + IMAGE_DOS_HEADER *dos; > >>>> + IMAGE_NT_HEADERS32 *nt; > >>>> + > >>>> + ret = platform_get_tpm2_device(&dev); > >>>> + if (ret != EFI_SUCCESS) > >>>> + return ret; > >>>> + > >>>> + if (handle->image_type == IMAGE_SUBSYSTEM_EFI_APPLICATION) { > >>> > >>> We should measure drivers too. Cf. > >>> EV_EFI_BOOT_SERVICES_DRIVER, EV_EFI_RUNTIME_SERVICES_DRIVER. > >>> > >>>> + pcr_index = 4; > >>>> + event_type = EV_EFI_BOOT_SERVICES_APPLICATION; > >>>> + } else { > >>>> + return EFI_UNSUPPORTED; > >>>> + } > >>>> + > >>>> + ret = tcg2_hash_pe_image(efi, efi_size, &digest_list); > >>>> + if (ret != EFI_SUCCESS) > >>>> + return ret; > >>>> + > >>>> + ret = tcg2_pcr_extend(dev, pcr_index, &digest_list); > >>>> + if (ret != EFI_SUCCESS) > >>>> + return ret; > >>>> + > >>>> + loaded_image->system_table->boottime->handle_protocol(&handle->header, > >>>> + &efi_guid_loaded_image_device_path, > >>>> + (void **)&device_path); > >>> > >>> You would have to use EFI_CALL() here. > >>> > >>> Please, use efi_search_protocol() instead of all this indirection. > >>> > >>> Best regards > >>> > >>> Heinrich > >>> > >>>> + device_path_length = efi_dp_size(device_path); > >>>> + if (device_path_length > 0) { > >>>> + /* add end node size */ > >>>> + device_path_length += sizeof(struct efi_device_path); > >>>> + } > >>>> + event_size = sizeof(struct uefi_image_load_event) + device_path_length; > >>>> + event = malloc(event_size); > >>>> + if (!event) > >>>> + return EFI_OUT_OF_RESOURCES; > >>>> + > >>>> + image_load_event = (struct uefi_image_load_event *)event; > >>>> + image_load_event->image_location_in_memory = (efi_physical_addr_t)efi; > >>>> + image_load_event->image_length_in_memory = efi_size; > >>>> + image_load_event->length_of_device_path = device_path_length; > >>>> + > >>>> + dos = (IMAGE_DOS_HEADER *)efi; > >>>> + nt = (IMAGE_NT_HEADERS32 *)(efi + dos->e_lfanew); > >>>> + if (nt->OptionalHeader.Magic == IMAGE_NT_OPTIONAL_HDR64_MAGIC) { > >>>> + IMAGE_NT_HEADERS64 *nt64 = (IMAGE_NT_HEADERS64 *)nt; > >>>> + > >>>> + image_load_event->image_link_time_address = > >>>> + nt64->OptionalHeader.ImageBase; > >>>> + } else if (nt->OptionalHeader.Magic == IMAGE_NT_OPTIONAL_HDR32_MAGIC) { > >>>> + image_load_event->image_link_time_address = > >>>> + nt->OptionalHeader.ImageBase; > >>>> + } else { > >>>> + ret = EFI_INVALID_PARAMETER; > >>>> + goto out; > >>>> + } > >>>> + > >>>> + if (device_path_length > 0) { > >>>> + memcpy(image_load_event->device_path, device_path, > >>>> + device_path_length); > >>>> + } > >>>> + > >>>> + ret = tcg2_agile_log_append(pcr_index, event_type, &digest_list, > >>>> + event_size, event); > >>>> + > >>>> +out: > >>>> + free(event); > >>>> + > >>>> + return ret; > >>>> +} > >>>> + > >>>> /** > >>>> * efi_tcg2_hash_log_extend_event() - extend and optionally log events > >>>> * > >>>> @@ -761,24 +929,23 @@ efi_tcg2_hash_log_extend_event(struct efi_tcg2_protocol *this, u64 flags, > >>>> /* > >>>> * if PE_COFF_IMAGE is set we need to make sure the image is not > >>>> * corrupted, verify it and hash the PE/COFF image in accordance with > >>>> - * the procedure specified in "Calculating the PE Image Hash" > >>>> - * section of the "Windows Authenticode Portable Executable Signature > >>>> + * the procedure specified in "Calculating the PE Image Hash" > >>>> + * section of the "Windows Authenticode Portable Executable Signature > >>>> * Format" > >>>> - * Not supported for now > >>>> */ > >>>> if (flags & PE_COFF_IMAGE) { > >>>> - ret = EFI_UNSUPPORTED; > >>>> - goto out; > >>>> + ret = tcg2_hash_pe_image((void *)data_to_hash, data_to_hash_len, > >>>> + &digest_list); > >>>> + } else { > >>>> + ret = tcg2_create_digest((u8 *)data_to_hash, data_to_hash_len, > >>>> + &digest_list); > >>>> } > >>>> + if (ret != EFI_SUCCESS) > >>>> + goto out; > >>>> > >>>> pcr_index = efi_tcg_event->header.pcr_index; > >>>> event_type = efi_tcg_event->header.event_type; > >>>> > >>>> - ret = tcg2_create_digest((u8 *)data_to_hash, data_to_hash_len, > >>>> - &digest_list); > >>>> - if (ret != EFI_SUCCESS) > >>>> - goto out; > >>>> - > >>>> ret = tcg2_pcr_extend(dev, pcr_index, &digest_list); > >>>> if (ret != EFI_SUCCESS) > >>>> goto out; > >>>> > >>> >
diff --git a/include/efi_loader.h b/include/efi_loader.h index de1a496a97..b02bc93c8e 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -426,6 +426,10 @@ efi_status_t efi_disk_register(void); efi_status_t efi_rng_register(void); /* Called by efi_init_obj_list() to install EFI_TCG2_PROTOCOL */ efi_status_t efi_tcg2_register(void); +/* measure the pe-coff image, extend PCR and add Event Log */ +efi_status_t tcg2_measure_pe_image(void *efi, u64 efi_size, + struct efi_loaded_image_obj *handle, + struct efi_loaded_image *loaded_image_info); /* Create handles and protocols for the partitions of a block device */ int efi_disk_create_partitions(efi_handle_t parent, struct blk_desc *desc, const char *if_typename, int diskid, diff --git a/include/efi_tcg2.h b/include/efi_tcg2.h index 40e241ce31..f8d46c5fd2 100644 --- a/include/efi_tcg2.h +++ b/include/efi_tcg2.h @@ -9,6 +9,8 @@ #if !defined _EFI_TCG2_PROTOCOL_H_ #define _EFI_TCG2_PROTOCOL_H_ +#include <efi.h> +#include <efi_api.h> #include <tpm-v2.h> #define EFI_TCG2_PROTOCOL_GUID \ @@ -53,6 +55,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[]; +} __packed; + 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 df67a196cf..ab9c04dc0a 100644 --- a/include/tpm-v2.h +++ b/include/tpm-v2.h @@ -61,6 +61,7 @@ struct udevice; #define EV_S_CRTM_VERSION ((u32)0x00000008) #define EV_CPU_MICROCODE ((u32)0x00000009) #define EV_TABLE_OF_DEVICES ((u32)0x0000000B) +#define EV_EFI_BOOT_SERVICES_APPLICATION ((u32)0x80000003) /* TPMS_TAGGED_PROPERTY Structure */ struct tpms_tagged_property { diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c index 2c35cb5651..b032ec5dd8 100644 --- a/lib/efi_loader/efi_image_loader.c +++ b/lib/efi_loader/efi_image_loader.c @@ -829,6 +829,13 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle, goto err; } +#if CONFIG_IS_ENABLED(EFI_TCG2_PROTOCOL) + /* Measure an PE/COFF image */ + if (tcg2_measure_pe_image(efi, efi_size, handle, + loaded_image_info)) + log_err("PE image measurement failed\n"); +#endif + /* Copy PE headers */ memcpy(efi_reloc, efi, sizeof(*dos) diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c index ed86a220fb..9fab07605f 100644 --- a/lib/efi_loader/efi_tcg2.c +++ b/lib/efi_loader/efi_tcg2.c @@ -13,8 +13,10 @@ #include <efi_loader.h> #include <efi_tcg2.h> #include <log.h> +#include <malloc.h> #include <version.h> #include <tpm-v2.h> +#include <u-boot/rsa.h> #include <u-boot/sha1.h> #include <u-boot/sha256.h> #include <u-boot/sha512.h> @@ -709,6 +711,172 @@ out: return EFI_EXIT(ret); } +static efi_status_t tcg2_hash_pe_image(void *efi, u64 efi_size, + struct tpml_digest_values *digest_list) +{ + IMAGE_NT_HEADERS32 *nt; + WIN_CERTIFICATE *wincerts = NULL; + size_t wincerts_len; + struct efi_image_regions *regs = NULL; + void *new_efi = NULL; + size_t new_efi_size; + u8 hash[TPM2_SHA512_DIGEST_SIZE]; + efi_status_t ret; + u32 active; + int i; + + ret = efi_check_pe(efi, efi_size, (void **)&nt); + if (ret != EFI_SUCCESS) { + log_err("Not a valid PE-COFF file\n"); + return EFI_INVALID_PARAMETER; + } + + /* + * 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); + new_efi = calloc(new_efi_size, 1); + if (!new_efi) + return EFI_OUT_OF_RESOURCES; + memcpy(new_efi, efi, efi_size); + efi = new_efi; + efi_size = new_efi_size; + } + + if (!efi_image_parse(efi, efi_size, ®s, &wincerts, + &wincerts_len)) { + log_err("Parsing PE executable image failed\n"); + ret = EFI_INVALID_PARAMETER; + goto out; + } + + ret = __get_active_pcr_banks(&active); + if (ret != EFI_SUCCESS) { + ret = EFI_DEVICE_ERROR; + goto out; + } + + digest_list->count = 0; + for (i = 0; i < MAX_HASH_COUNT; i++) { + u16 hash_alg = hash_algo_list[i].hash_alg; + + if (!(active & alg_to_mask(hash_alg))) + continue; + switch (hash_alg) { + case TPM2_ALG_SHA1: + hash_calculate("sha1", regs->reg, regs->num, hash); + digest_list->count++; + break; + case TPM2_ALG_SHA256: + hash_calculate("sha256", regs->reg, regs->num, hash); + digest_list->count++; + break; + case TPM2_ALG_SHA384: + hash_calculate("sha384", regs->reg, regs->num, hash); + digest_list->count++; + break; + case TPM2_ALG_SHA512: + hash_calculate("sha512", regs->reg, regs->num, hash); + digest_list->count++; + break; + default: + EFI_PRINT("Unsupported algorithm %x\n", hash_alg); + return EFI_INVALID_PARAMETER; + } + digest_list->digests[i].hash_alg = hash_alg; + memcpy(&digest_list->digests[i].digest, hash, (u32)alg_to_len(hash_alg)); + } + +out: + free(new_efi); + free(regs); + + return ret; +} + +efi_status_t tcg2_measure_pe_image(void *efi, u64 efi_size, + struct efi_loaded_image_obj *handle, + struct efi_loaded_image *loaded_image) +{ + struct tpml_digest_values digest_list; + efi_status_t ret; + struct udevice *dev; + u32 pcr_index, event_type, event_size; + struct uefi_image_load_event *image_load_event; + u8 *event; + struct efi_device_path *device_path; + u32 device_path_length; + IMAGE_DOS_HEADER *dos; + IMAGE_NT_HEADERS32 *nt; + + ret = platform_get_tpm2_device(&dev); + if (ret != EFI_SUCCESS) + return ret; + + if (handle->image_type == IMAGE_SUBSYSTEM_EFI_APPLICATION) { + pcr_index = 4; + event_type = EV_EFI_BOOT_SERVICES_APPLICATION; + } else { + return EFI_UNSUPPORTED; + } + + ret = tcg2_hash_pe_image(efi, efi_size, &digest_list); + if (ret != EFI_SUCCESS) + return ret; + + ret = tcg2_pcr_extend(dev, pcr_index, &digest_list); + if (ret != EFI_SUCCESS) + return ret; + + loaded_image->system_table->boottime->handle_protocol(&handle->header, + &efi_guid_loaded_image_device_path, + (void **)&device_path); + device_path_length = efi_dp_size(device_path); + if (device_path_length > 0) { + /* add end node size */ + device_path_length += sizeof(struct efi_device_path); + } + event_size = sizeof(struct uefi_image_load_event) + device_path_length; + event = malloc(event_size); + if (!event) + return EFI_OUT_OF_RESOURCES; + + image_load_event = (struct uefi_image_load_event *)event; + image_load_event->image_location_in_memory = (efi_physical_addr_t)efi; + image_load_event->image_length_in_memory = efi_size; + image_load_event->length_of_device_path = device_path_length; + + dos = (IMAGE_DOS_HEADER *)efi; + nt = (IMAGE_NT_HEADERS32 *)(efi + dos->e_lfanew); + if (nt->OptionalHeader.Magic == IMAGE_NT_OPTIONAL_HDR64_MAGIC) { + IMAGE_NT_HEADERS64 *nt64 = (IMAGE_NT_HEADERS64 *)nt; + + image_load_event->image_link_time_address = + nt64->OptionalHeader.ImageBase; + } else if (nt->OptionalHeader.Magic == IMAGE_NT_OPTIONAL_HDR32_MAGIC) { + image_load_event->image_link_time_address = + nt->OptionalHeader.ImageBase; + } else { + ret = EFI_INVALID_PARAMETER; + goto out; + } + + if (device_path_length > 0) { + memcpy(image_load_event->device_path, device_path, + device_path_length); + } + + ret = tcg2_agile_log_append(pcr_index, event_type, &digest_list, + event_size, event); + +out: + free(event); + + return ret; +} + /** * efi_tcg2_hash_log_extend_event() - extend and optionally log events * @@ -761,24 +929,23 @@ efi_tcg2_hash_log_extend_event(struct efi_tcg2_protocol *this, u64 flags, /* * if PE_COFF_IMAGE is set we need to make sure the image is not * corrupted, verify it and hash the PE/COFF image in accordance with - * the procedure specified in "Calculating the PE Image Hash" - * section of the "Windows Authenticode Portable Executable Signature + * the procedure specified in "Calculating the PE Image Hash" + * section of the "Windows Authenticode Portable Executable Signature * Format" - * Not supported for now */ if (flags & PE_COFF_IMAGE) { - ret = EFI_UNSUPPORTED; - goto out; + ret = tcg2_hash_pe_image((void *)data_to_hash, data_to_hash_len, + &digest_list); + } else { + ret = tcg2_create_digest((u8 *)data_to_hash, data_to_hash_len, + &digest_list); } + if (ret != EFI_SUCCESS) + goto out; pcr_index = efi_tcg_event->header.pcr_index; event_type = efi_tcg_event->header.event_type; - ret = tcg2_create_digest((u8 *)data_to_hash, data_to_hash_len, - &digest_list); - if (ret != EFI_SUCCESS) - goto out; - ret = tcg2_pcr_extend(dev, pcr_index, &digest_list); if (ret != EFI_SUCCESS) goto out;
"TCG PC Client Platform Firmware Profile Specification" requires to measure every attempt to load and execute a OS Loader(a UEFI application) into PCR[4]. This commit adds the PE/COFF image measurement, extends PCR, and appends measurement into Event Log. Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> --- include/efi_loader.h | 4 + include/efi_tcg2.h | 10 ++ include/tpm-v2.h | 1 + lib/efi_loader/efi_image_loader.c | 7 ++ lib/efi_loader/efi_tcg2.c | 187 ++++++++++++++++++++++++++++-- 5 files changed, 199 insertions(+), 10 deletions(-) -- 2.17.1