[PATCH 0/2] PE/COFF measurement support

[PATCH 0/2] PE/COFF measurement support

[Masahisa Kojima]

This patch series add the PE/COFF measurement support.
Extending PCR and Event Log is tested with fTPM
running as a OP-TEE TA.

Masahisa Kojima (2):
  efi_loader: expose efi_image_parse() even if UEFI Secure Boot is
    disabled
  efi_loader: add PE/COFF image measurement

 include/efi_loader.h              |   4 +
 include/efi_tcg2.h                |  10 ++
 include/tpm-v2.h                  |   1 +
 lib/efi_loader/efi_image_loader.c |   9 +-
 lib/efi_loader/efi_signature.c    |   2 +
 lib/efi_loader/efi_tcg2.c         | 187 ++++++++++++++++++++++++++++--
 6 files changed, 202 insertions(+), 11 deletions(-)

-- 
2.17.1

[PATCH 1/2] efi_loader: expose efi_image_parse() even if UEFI Secure Boot is disabled

[Masahisa Kojima]

This is preparation for PE/COFF measurement support.
PE/COFF image hash calculation is same in both
UEFI Secure Boot image verification and measurement in
measured boot. This commit exposes the hash calculation
functions even if UEFI Secure Boot is not enabled.

Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
---
 lib/efi_loader/efi_image_loader.c | 2 +-
 lib/efi_loader/efi_signature.c    | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
index f53ef367ec..2c35cb5651 100644
--- a/lib/efi_loader/efi_image_loader.c
+++ b/lib/efi_loader/efi_image_loader.c
@@ -213,7 +213,6 @@ static void efi_set_code_and_data_type(
 	}
 }
 
-#ifdef CONFIG_EFI_SECURE_BOOT
 /**
  * cmp_pe_section() - compare virtual addresses of two PE image sections
  * @arg1:	pointer to pointer to first section header
@@ -422,6 +421,7 @@ err:
 	return false;
 }
 
+#ifdef CONFIG_EFI_SECURE_BOOT
 /**
  * efi_image_unsigned_authenticate() - authenticate unsigned image with
  * SHA256 hash
diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c
index c7ec275414..1dd9d0e130 100644
--- a/lib/efi_loader/efi_signature.c
+++ b/lib/efi_loader/efi_signature.c
@@ -538,6 +538,7 @@ out:
 	EFI_PRINT("%s: Exit, revoked: %d\n", __func__, revoked);
 	return !revoked;
 }
+#endif
 
 /**
  * efi_image_region_add() - add an entry of region
@@ -601,6 +602,7 @@ efi_status_t efi_image_region_add(struct efi_image_regions *regs,
 	return EFI_SUCCESS;
 }
 
+#if defined(CONFIG_EFI_SECURE_BOOT) || defined(CONFIG_EFI_CAPSULE_AUTHENTICATE)
 /**
  * efi_sigstore_free - free signature store
  * @sigstore:	Pointer to signature store structure
-- 
2.17.1

[PATCH 1/2] efi_loader: expose efi_image_parse() even if UEFI Secure Boot is disabled

[Heinrich Schuchardt]

On 15.04.21 15:30, Masahisa Kojima wrote:
> This is preparation for PE/COFF measurement support.
> PE/COFF image hash calculation is same in both
> UEFI Secure Boot image verification and measurement in
> measured boot. This commit exposes the hash calculation
> functions even if UEFI Secure Boot is not enabled.
>
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> ---
>  lib/efi_loader/efi_image_loader.c | 2 +-
>  lib/efi_loader/efi_signature.c    | 2 ++
>  2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
> index f53ef367ec..2c35cb5651 100644
> --- a/lib/efi_loader/efi_image_loader.c
> +++ b/lib/efi_loader/efi_image_loader.c
> @@ -213,7 +213,6 @@ static void efi_set_code_and_data_type(
>  	}
>  }
>
> -#ifdef CONFIG_EFI_SECURE_BOOT
>  /**
>   * cmp_pe_section() - compare virtual addresses of two PE image sections
>   * @arg1:	pointer to pointer to first section header
> @@ -422,6 +421,7 @@ err:
>  	return false;
>  }
>
> +#ifdef CONFIG_EFI_SECURE_BOOT

Please, remove the #ifdef and replace it by

   if (IS_ENABLED(CONFIG_EFI_SECURE_BOOT)) {

in function efi_image_authenticate().

The advantage of using 'if' instead of '#ifdef' is that all branches are
parsed by GCC. This allows us to catch code problems more easily.

>  /**
>   * efi_image_unsigned_authenticate() - authenticate unsigned image with
>   * SHA256 hash
> diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c
> index c7ec275414..1dd9d0e130 100644
> --- a/lib/efi_loader/efi_signature.c
> +++ b/lib/efi_loader/efi_signature.c
> @@ -538,6 +538,7 @@ out:
>  	EFI_PRINT("%s: Exit, revoked: %d\n", __func__, revoked);
>  	return !revoked;
>  }
> +#endif

The #if is superfluous. Just add the missing header

#include <u-boot/rsa-checksum.h>

efi_guid_t efi_guid_image_security_database should be moved to
efi_var_common.c. This allows us to decide if
lib/efi_loader/efi_signature.c shall be compiled in the Makefile.

Best regards

Heinrich

>
>  /**
>   * efi_image_region_add() - add an entry of region
> @@ -601,6 +602,7 @@ efi_status_t efi_image_region_add(struct efi_image_regions *regs,
>  	return EFI_SUCCESS;
>  }
>
> +#if defined(CONFIG_EFI_SECURE_BOOT) || defined(CONFIG_EFI_CAPSULE_AUTHENTICATE)
>  /**
>   * efi_sigstore_free - free signature store
>   * @sigstore:	Pointer to signature store structure
>

[PATCH 2/2] efi_loader: add PE/COFF image measurement

[Masahisa Kojima]

"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>
+#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, &regs, &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;
-- 
2.17.1

[PATCH 2/2] efi_loader: add PE/COFF image measurement

[Heinrich Schuchardt]

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, &regs, &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;
>

[PATCH 2/2] efi_loader: add PE/COFF image measurement

[Ilias Apalodimas]

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, &regs, &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;
> >
> 

[PATCH 2/2] efi_loader: add PE/COFF image measurement

[Heinrich Schuchardt]

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

[PATCH 2/2] efi_loader: add PE/COFF image measurement

[Heinrich Schuchardt]

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, &regs, &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;
>

[PATCH 2/2] efi_loader: add PE/COFF image measurement

[Masahisa Kojima]

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, &regs, &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;
> >
>

[PATCH 2/2] efi_loader: add PE/COFF image measurement

[Ilias Apalodimas]

> > > +             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, &regs, &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;
> > >
> >

[PATCH 2/2] efi_loader: add PE/COFF image measurement

[Heinrich Schuchardt]

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, &regs, &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;
>>>>
>>>

[PATCH 2/2] efi_loader: add PE/COFF image measurement

[Masahisa Kojima]

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, &regs, &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;
> >>>>
> >>>
>