[PATCH 0/5] add measurement support

[PATCH 0/5] add measurement support

[Masahisa Kojima]

This patch series add the support of measurement
descibed in TCG PC Client PFP spec(Version 1.05 Revision 23).

Eventlog generated with this patch series are tested on
the aarch64 based machine(Socionext Developerbox) and fTPM
running on OP-TEE.
The eventlog result is almost same result as the one
generated by edk2 running on the Devloperbox and Secure96.

This patch series does not cover all measurement requirements
described in TCG spec, the remaing items will be supported
in the future.
Major missing items in TCG PC Client PFP spec:
 1) If the secure boot variables are updated after they are
   initially measured in PCR[7] and before ExitBootServices()
   has completed, the platform MAY be restarted OR the variables
   MUST be remeasured into PCR[7].
 2) SMBIOS structure measurement
 3) "DeployedMode" and "AuditMode" measurement
 4) EV_EFI_GPT_EVENT event
 5) Measurement of U-boot itself. I assume U-boot measurement will be done
    by the former firmware such as trusted firmware.

Masahisa Kojima (5):
  efi_loader: increase eventlog buffer size
  efi_loader: add secure boot variable measurement
  efi_loader: add boot variable measurement
  efi_loader: add ExitBootServices() measurement
  efi_loader: refactor efi_append_scrtm_version()

 include/efi_loader.h          |   5 +
 include/efi_tcg2.h            |  20 +++
 include/tpm-v2.h              |  18 +-
 lib/efi_loader/Kconfig        |   2 +-
 lib/efi_loader/efi_boottime.c |  25 +++
 lib/efi_loader/efi_tcg2.c     | 328 +++++++++++++++++++++++++++++++++-
 6 files changed, 390 insertions(+), 8 deletions(-)

-- 
2.17.1

[PATCH 1/5] efi_loader: increase eventlog buffer size

[Masahisa Kojima]

This is a preperation to add eventlog support
described in TCG PC Client PFP spec.

Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
---
 lib/efi_loader/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
index b2ab48a048..a87bf3cc98 100644
--- a/lib/efi_loader/Kconfig
+++ b/lib/efi_loader/Kconfig
@@ -327,7 +327,7 @@ config EFI_TCG2_PROTOCOL
 config EFI_TCG2_PROTOCOL_EVENTLOG_SIZE
 	int "EFI_TCG2_PROTOCOL EventLog size"
 	depends on EFI_TCG2_PROTOCOL
-	default 4096
+	default 16384
 	help
 		Define the size of the EventLog for EFI_TCG2_PROTOCOL. Note that
 		this is going to be allocated twice. One for the eventlog it self
-- 
2.17.1

[PATCH 2/5] efi_loader: add secure boot variable measurement

[Masahisa Kojima]

TCG PC Client PFP spec requires to measure the secure
boot policy before validating the UEFI image.
This commit adds the secure boot variable measurement
of "SecureBoot", "PK", "KEK", "db" and "dbx".

Note that this implementation assumes that secure boot
variables are pre-configured and not be set/updated in runtime.

Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
---
 include/efi_tcg2.h        |  20 ++++++
 lib/efi_loader/efi_tcg2.c | 135 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 155 insertions(+)

diff --git a/include/efi_tcg2.h b/include/efi_tcg2.h
index bcfb98168a..8d7b77c087 100644
--- a/include/efi_tcg2.h
+++ b/include/efi_tcg2.h
@@ -142,6 +142,26 @@ struct efi_tcg2_final_events_table {
 	struct tcg_pcr_event2 event[];
 };
 
+/**
+ * struct tdUEFI_VARIABLE_DATA
+ * @variable_name:		The vendorGUID parameter in the
+ *				GetVariable() API.
+ * @unicode_name_length:	The length in CHAR16 of the Unicode name of
+ *				the variable.
+ * @variable_data_length:	The size of the variable data.
+ * @unicode_name:		The CHAR16 unicode name of the variable
+ *				without NULL-terminator.
+ * @variable_data:		The data parameter of the efi variable
+ *				in the GetVariable() API.
+ */
+struct efi_tcg2_uefi_variable_data {
+	efi_guid_t variable_name;
+	u64 unicode_name_length;
+	u64 variable_data_length;
+	u16 unicode_name[1];
+	u8 variable_data[1];
+};
+
 struct efi_tcg2_protocol {
 	efi_status_t (EFIAPI * get_capability)(struct efi_tcg2_protocol *this,
 					       struct efi_tcg2_boot_service_capability *capability);
diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
index 1319a8b378..2a248bd62a 100644
--- a/lib/efi_loader/efi_tcg2.c
+++ b/lib/efi_loader/efi_tcg2.c
@@ -78,6 +78,19 @@ static const struct digest_info hash_algo_list[] = {
 	},
 };
 
+struct variable_info {
+	u16		*name;
+	const efi_guid_t	*guid;
+};
+
+static struct variable_info secure_variables[] = {
+	{L"SecureBoot", &efi_global_variable_guid},
+	{L"PK", &efi_global_variable_guid},
+	{L"KEK", &efi_global_variable_guid},
+	{L"db", &efi_guid_image_security_database},
+	{L"dbx", &efi_guid_image_security_database},
+};
+
 #define MAX_HASH_COUNT ARRAY_SIZE(hash_algo_list)
 
 /**
@@ -1264,6 +1277,39 @@ free_pool:
 	return ret;
 }
 
+/**
+ * tcg2_measure_event() - common function to add event log and extend PCR
+ *
+ * @dev:		TPM device
+ * @pcr_index:		PCR index
+ * @event_type:		type of event added
+ * @size:		event size
+ * @event:		event data
+ *
+ * Return:	status code
+ */
+static efi_status_t EFIAPI
+tcg2_measure_event(struct udevice *dev, u32 pcr_index, u32 event_type,
+		   u32 size, u8 event[])
+{
+	struct tpml_digest_values digest_list;
+	efi_status_t ret;
+
+	ret = tcg2_create_digest(event, size, &digest_list);
+	if (ret != EFI_SUCCESS)
+		goto out;
+
+	ret = tcg2_pcr_extend(dev, pcr_index, &digest_list);
+	if (ret != EFI_SUCCESS)
+		goto out;
+
+	ret = tcg2_agile_log_append(pcr_index, event_type, &digest_list,
+				    size, event);
+
+out:
+	return ret;
+}
+
 /**
  * efi_append_scrtm_version - Append an S-CRTM EV_S_CRTM_VERSION event on the
  *			      eventlog and extend the PCRs
@@ -1294,6 +1340,88 @@ out:
 	return ret;
 }
 
+/**
+ * tcg2_measure_variable() - add variable event log and extend PCR
+ *
+ * @dev:		TPM device
+ * @pcr_index:		PCR index
+ * @event_type:		type of event added
+ * @var_name:		variable name
+ * @guid:		guid
+ * @data_size:		variable data size
+ * @data:		variable data
+ *
+ * Return:	status code
+ */
+static efi_status_t tcg2_measure_variable(struct udevice *dev, u32 pcr_index,
+					  u32 event_type, u16 *var_name,
+					  const efi_guid_t *guid,
+					  efi_uintn_t data_size, u8 *data)
+{
+	u32 event_size;
+	efi_status_t ret;
+	struct efi_tcg2_uefi_variable_data *event;
+
+	event_size = sizeof(event->variable_name) +
+		     sizeof(event->unicode_name_length) +
+		     sizeof(event->variable_data_length) +
+		     (u16_strlen(var_name) * sizeof(*var_name)) + data_size;
+	event = malloc(event_size);
+	if (!event)
+		return EFI_OUT_OF_RESOURCES;
+
+	guidcpy(&event->variable_name, guid);
+	event->unicode_name_length = u16_strlen(var_name);
+	event->variable_data_length = data_size;
+	memcpy(event->unicode_name, var_name,
+	       (event->unicode_name_length * sizeof(*event->unicode_name)));
+	memcpy((u16 *)event->unicode_name + event->unicode_name_length,
+	       (u8 *)data, data_size);
+	ret = tcg2_measure_event(dev, pcr_index, event_type, event_size,
+				 (u8 *)event);
+	free(event);
+	return ret;
+}
+
+/**
+ * tcg2_measure_secure_boot_variable() - measure secure boot variables
+ *
+ * @dev:	TPM device
+ *
+ * Return:	status code
+ */
+static efi_status_t tcg2_measure_secure_boot_variable(struct udevice *dev)
+{
+	u8 *data;
+	efi_uintn_t data_size;
+	u32 count, i;
+	efi_status_t ret;
+
+	count = ARRAY_SIZE(secure_variables);
+	for (i = 0; i < count; i++) {
+		data = efi_get_var(secure_variables[i].name,
+				   secure_variables[i].guid,
+				   &data_size);
+
+		ret = tcg2_measure_variable(dev, 7,
+					    EV_EFI_VARIABLE_DRIVER_CONFIG,
+					    secure_variables[i].name,
+					    secure_variables[i].guid,
+					    data_size, (u8 *)data);
+		free(data);
+		if (ret != EFI_SUCCESS)
+			goto error;
+	}
+
+	/*
+	 * TODO: add DBT and DBR measurement support when u-boot supports
+	 * these variables.
+	 */
+
+error:
+	return ret;
+}
+
 /**
  * efi_tcg2_register() - register EFI_TCG2_PROTOCOL
  *
@@ -1328,6 +1456,13 @@ efi_status_t efi_tcg2_register(void)
 		tcg2_uninit();
 		goto fail;
 	}
+
+	ret = tcg2_measure_secure_boot_variable(dev);
+	if (ret != EFI_SUCCESS) {
+		tcg2_uninit();
+		goto fail;
+	}
+
 	return ret;
 
 fail:
-- 
2.17.1

[PATCH 3/5] efi_loader: add boot variable measurement

[Masahisa Kojima]

TCG PC Client PFP spec requires to measure "Boot####"
and "BootOrder" variables, EV_SEPARATOR event prior
to the Ready to Boot invocation.
Since u-boot does not implement Ready to Boot event,
these measurements are performed when efi_start_image() is called.

TCG spec also requires to measure "Calling EFI Application from
Boot Option" for each boot attempt, and "Returning from EFI
Application from Boot Option" if a boot device returns control
back to the Boot Manager.

Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
---
 include/efi_loader.h          |   4 ++
 include/tpm-v2.h              |  18 ++++-
 lib/efi_loader/efi_boottime.c |  20 ++++++
 lib/efi_loader/efi_tcg2.c     | 123 ++++++++++++++++++++++++++++++++++
 4 files changed, 164 insertions(+), 1 deletion(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index 0a9c82a257..281ffff30f 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -407,6 +407,10 @@ efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size);
 efi_status_t efi_init_variables(void);
 /* Notify ExitBootServices() is called */
 void efi_variables_boot_exit_notify(void);
+/* Measure efi application invocation */
+efi_status_t EFIAPI efi_tcg2_measure_efi_app_invocation(void);
+/* Measure efi application exit */
+efi_status_t EFIAPI efi_tcg2_measure_efi_app_exit(void);
 /* Called by bootefi to initialize root node */
 efi_status_t efi_root_node_register(void);
 /* Called by bootefi to initialize runtime */
diff --git a/include/tpm-v2.h b/include/tpm-v2.h
index 3e48e35861..8a7b7f1874 100644
--- a/include/tpm-v2.h
+++ b/include/tpm-v2.h
@@ -73,7 +73,7 @@ struct udevice;
 /*
  * event types, cf.
  * "TCG PC Client Platform Firmware Profile Specification", Family "2.0"
- * rev 1.04, June 3, 2019
+ * Level 00 Version 1.05 Revision 23, May 7, 2021
  */
 #define EV_EFI_EVENT_BASE			((u32)0x80000000)
 #define EV_EFI_VARIABLE_DRIVER_CONFIG		((u32)0x80000001)
@@ -85,8 +85,24 @@ struct udevice;
 #define EV_EFI_ACTION				((u32)0x80000007)
 #define EV_EFI_PLATFORM_FIRMWARE_BLOB		((u32)0x80000008)
 #define EV_EFI_HANDOFF_TABLES			((u32)0x80000009)
+#define EV_EFI_PLATFORM_FIRMWARE_BLOB2		((u32)0x8000000A)
+#define EV_EFI_HANDOFF_TABLES2			((u32)0x8000000B)
+#define EV_EFI_VARIABLE_BOOT2			((u32)0x8000000C)
 #define EV_EFI_HCRTM_EVENT			((u32)0x80000010)
 #define EV_EFI_VARIABLE_AUTHORITY		((u32)0x800000E0)
+#define EV_EFI_SPDM_FIRMWARE_BLOB		((u32)0x800000E1)
+#define EV_EFI_SPDM_FIRMWARE_CONFIG		((u32)0x800000E2)
+
+#define EFI_CALLING_EFI_APPLICATION         \
+	"Calling EFI Application from Boot Option"
+#define EFI_RETURNING_FROM_EFI_APPLICATION  \
+	"Returning from EFI Application from Boot Option"
+#define EFI_EXIT_BOOT_SERVICES_INVOCATION   \
+	"Exit Boot Services Invocation"
+#define EFI_EXIT_BOOT_SERVICES_FAILED       \
+	"Exit Boot Services Returned with Failure"
+#define EFI_EXIT_BOOT_SERVICES_SUCCEEDED    \
+	"Exit Boot Services Returned with Success"
 
 /* TPMS_TAGGED_PROPERTY Structure */
 struct tpms_tagged_property {
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index f6d5ba05e3..2914800c56 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -2993,6 +2993,16 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
 	image_obj->exit_status = &exit_status;
 	image_obj->exit_jmp = &exit_jmp;
 
+	if (IS_ENABLED(CONFIG_EFI_TCG2_PROTOCOL)) {
+		if (image_obj->image_type == IMAGE_SUBSYSTEM_EFI_APPLICATION) {
+			ret = efi_tcg2_measure_efi_app_invocation();
+			if (ret != EFI_SUCCESS) {
+				EFI_PRINT("tcg2 measurement fails(0x%lx)\n",
+					  ret);
+			}
+		}
+	}
+
 	/* call the image! */
 	if (setjmp(&exit_jmp)) {
 		/*
@@ -3251,6 +3261,16 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
 	    exit_status != EFI_SUCCESS)
 		efi_delete_image(image_obj, loaded_image_protocol);
 
+	if (IS_ENABLED(CONFIG_EFI_TCG2_PROTOCOL)) {
+		if (image_obj->image_type == IMAGE_SUBSYSTEM_EFI_APPLICATION) {
+			ret = efi_tcg2_measure_efi_app_exit();
+			if (ret != EFI_SUCCESS) {
+				EFI_PRINT("tcg2 measurement fails(0x%lx)\n",
+					  ret);
+			}
+		}
+	}
+
 	/* Make sure entry/exit counts for EFI world cross-overs match */
 	EFI_EXIT(exit_status);
 
diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
index 2a248bd62a..6e903e3cb3 100644
--- a/lib/efi_loader/efi_tcg2.c
+++ b/lib/efi_loader/efi_tcg2.c
@@ -35,6 +35,7 @@ struct event_log_buffer {
 };
 
 static struct event_log_buffer event_log;
+static bool tcg2_efi_app_invoked;
 /*
  * When requesting TPM2_CAP_TPM_PROPERTIES the value is on a standard offset.
  * Since the current tpm2_get_capability() response buffers starts at
@@ -1383,6 +1384,128 @@ static efi_status_t tcg2_measure_variable(struct udevice *dev, u32 pcr_index,
 	return ret;
 }
 
+/**
+ * tcg2_measure_boot_variable() - measure boot variables
+ *
+ * @dev:	TPM device
+ *
+ * Return:	status code
+ */
+static efi_status_t tcg2_measure_boot_variable(struct udevice *dev)
+{
+	u16 *boot_order;
+	u16 var_name[] = L"BootOrder";
+	u16 boot_name[] = L"Boot0000";
+	u16 hexmap[] = L"0123456789ABCDEF";
+	u8 *bootvar;
+	efi_uintn_t var_data_size;
+	u32 count, i;
+	efi_status_t ret;
+
+	boot_order = efi_get_var(var_name, &efi_global_variable_guid,
+				 &var_data_size);
+	if (!boot_order) {
+		log_info("BootOrder not defined\n");
+		ret = EFI_NOT_FOUND;
+		goto error;
+	}
+
+	ret = tcg2_measure_variable(dev, 1, EV_EFI_VARIABLE_BOOT2, var_name,
+				    &efi_global_variable_guid, var_data_size,
+				    (u8 *)boot_order);
+	if (ret != EFI_SUCCESS)
+		goto error;
+
+	count = var_data_size / sizeof(*boot_order);
+	for (i = 0; i < count; i++) {
+		boot_name[4] = hexmap[(boot_order[i] & 0xf000) >> 12];
+		boot_name[5] = hexmap[(boot_order[i] & 0x0f00) >> 8];
+		boot_name[6] = hexmap[(boot_order[i] & 0x00f0) >> 4];
+		boot_name[7] = hexmap[(boot_order[i] & 0x000f)];
+
+		bootvar = efi_get_var(boot_name, &efi_global_variable_guid,
+				      &var_data_size);
+
+		if (!bootvar) {
+			log_info("%ls not found\n", boot_name);
+			continue;
+		}
+
+		ret = tcg2_measure_variable(dev, 1, EV_EFI_VARIABLE_BOOT2,
+					    boot_name,
+					    &efi_global_variable_guid,
+					    var_data_size, bootvar);
+		free(bootvar);
+		if (ret != EFI_SUCCESS)
+			goto error;
+	}
+
+error:
+	free(boot_order);
+	return ret;
+}
+
+/**
+ * efi_tcg2_measure_efi_app_invocation() - measure efi app invocation
+ *
+ * Return:	status code
+ */
+efi_status_t EFIAPI efi_tcg2_measure_efi_app_invocation(void)
+{
+	efi_status_t ret;
+	u32 pcr_index;
+	struct udevice *dev;
+	u32 event = 0;
+
+	if (tcg2_efi_app_invoked)
+		return EFI_SUCCESS;
+
+	ret = platform_get_tpm2_device(&dev);
+	if (ret != EFI_SUCCESS)
+		return ret;
+
+	ret = tcg2_measure_boot_variable(dev);
+	if (ret != EFI_SUCCESS)
+		goto out;
+
+	ret = tcg2_measure_event(dev, 4, EV_EFI_ACTION,
+				 strlen(EFI_CALLING_EFI_APPLICATION),
+				 (u8 *)EFI_CALLING_EFI_APPLICATION);
+	if (ret != EFI_SUCCESS)
+		goto out;
+
+	for (pcr_index = 0; pcr_index <= 7; pcr_index++) {
+		ret = tcg2_measure_event(dev, pcr_index, EV_SEPARATOR,
+					 sizeof(event), (u8 *)&event);
+		if (ret != EFI_SUCCESS)
+			goto out;
+	}
+
+	tcg2_efi_app_invoked = true;
+out:
+	return ret;
+}
+
+/**
+ * efi_tcg2_measure_efi_app_exit() - measure efi app exit
+ *
+ * Return:	status code
+ */
+efi_status_t EFIAPI efi_tcg2_measure_efi_app_exit(void)
+{
+	efi_status_t ret;
+	struct udevice *dev;
+
+	ret = platform_get_tpm2_device(&dev);
+	if (ret != EFI_SUCCESS)
+		return ret;
+
+	ret = tcg2_measure_event(dev, 4, EV_EFI_ACTION,
+				 strlen(EFI_RETURNING_FROM_EFI_APPLICATION),
+				 (u8 *)EFI_RETURNING_FROM_EFI_APPLICATION);
+	return ret;
+}
+
 /**
  * tcg2_measure_secure_boot_variable() - measure secure boot variables
  *
-- 
2.17.1

[PATCH 4/5] efi_loader: add ExitBootServices() measurement

[Masahisa Kojima]

TCG PC Client PFP spec requires to measure
"Exit Boot Services Invocation" if ExitBootServices() is invoked.
Depending upon the return code from the ExitBootServices() call,
"Exit Boot Services Returned with Success" or "Exit Boot Services
Returned with Failure" is also measured.

Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
---
 include/efi_loader.h          |  1 +
 lib/efi_loader/efi_boottime.c |  5 +++
 lib/efi_loader/efi_tcg2.c     | 70 +++++++++++++++++++++++++++++++++++
 3 files changed, 76 insertions(+)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index 281ffff30f..e9bd1aac08 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -407,6 +407,7 @@ efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size);
 efi_status_t efi_init_variables(void);
 /* Notify ExitBootServices() is called */
 void efi_variables_boot_exit_notify(void);
+efi_status_t efi_tcg2_notify_exit_boot_services_failed(void);
 /* Measure efi application invocation */
 efi_status_t EFIAPI efi_tcg2_measure_efi_app_invocation(void);
 /* Measure efi application exit */
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 2914800c56..6e07ef65bc 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -2181,6 +2181,11 @@ static efi_status_t EFIAPI efi_exit_boot_services(efi_handle_t image_handle,
 	efi_set_watchdog(0);
 	WATCHDOG_RESET();
 out:
+	if (ret != EFI_SUCCESS) {
+		if (IS_ENABLED(CONFIG_EFI_TCG2_PROTOCOL))
+			efi_tcg2_notify_exit_boot_services_failed();
+	}
+
 	return EFI_EXIT(ret);
 }
 
diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
index 6e903e3cb3..823abd8217 100644
--- a/lib/efi_loader/efi_tcg2.c
+++ b/lib/efi_loader/efi_tcg2.c
@@ -1506,6 +1506,67 @@ efi_status_t EFIAPI efi_tcg2_measure_efi_app_exit(void)
 	return ret;
 }
 
+/**
+ * efi_tcg2_notify_exit_boot_services() - ExitBootService callback
+ *
+ * @event:	callback event
+ * @context:	callback context
+ */
+static void EFIAPI
+efi_tcg2_notify_exit_boot_services(struct efi_event *event, void *context)
+{
+	efi_status_t ret;
+	struct udevice *dev;
+
+	EFI_ENTRY("%p, %p", event, context);
+
+	ret = platform_get_tpm2_device(&dev);
+	if (ret != EFI_SUCCESS)
+		goto out;
+
+	ret = tcg2_measure_event(dev, 5, EV_EFI_ACTION,
+				 strlen(EFI_EXIT_BOOT_SERVICES_INVOCATION),
+				 (u8 *)EFI_EXIT_BOOT_SERVICES_INVOCATION);
+	if (ret != EFI_SUCCESS)
+		goto out;
+
+	ret = tcg2_measure_event(dev, 5, EV_EFI_ACTION,
+				 strlen(EFI_EXIT_BOOT_SERVICES_SUCCEEDED),
+				 (u8 *)EFI_EXIT_BOOT_SERVICES_SUCCEEDED);
+
+out:
+	EFI_EXIT(ret);
+}
+
+/**
+ * efi_tcg2_notify_exit_boot_services_failed()
+ *  - notify ExitBootServices() is failed
+ *
+ * Return:	status code
+ */
+efi_status_t efi_tcg2_notify_exit_boot_services_failed(void)
+{
+	struct udevice *dev;
+	efi_status_t ret;
+
+	ret = platform_get_tpm2_device(&dev);
+	if (ret != EFI_SUCCESS)
+		goto out;
+
+	ret = tcg2_measure_event(dev, 5, EV_EFI_ACTION,
+				 sizeof(EFI_EXIT_BOOT_SERVICES_INVOCATION),
+				 (u8 *)EFI_EXIT_BOOT_SERVICES_INVOCATION);
+	if (ret != EFI_SUCCESS)
+		goto out;
+
+	ret = tcg2_measure_event(dev, 5, EV_EFI_ACTION,
+				 sizeof(EFI_EXIT_BOOT_SERVICES_FAILED),
+				 (u8 *)EFI_EXIT_BOOT_SERVICES_FAILED);
+
+out:
+	return ret;
+}
+
 /**
  * tcg2_measure_secure_boot_variable() - measure secure boot variables
  *
@@ -1556,6 +1617,7 @@ efi_status_t efi_tcg2_register(void)
 {
 	efi_status_t ret = EFI_SUCCESS;
 	struct udevice *dev;
+	struct efi_event *event;
 
 	ret = platform_get_tpm2_device(&dev);
 	if (ret != EFI_SUCCESS) {
@@ -1580,6 +1642,14 @@ efi_status_t efi_tcg2_register(void)
 		goto fail;
 	}
 
+	ret = efi_create_event(EVT_SIGNAL_EXIT_BOOT_SERVICES, TPL_CALLBACK,
+			       efi_tcg2_notify_exit_boot_services, NULL,
+			       NULL, &event);
+	if (ret != EFI_SUCCESS) {
+		tcg2_uninit();
+		goto fail;
+	}
+
 	ret = tcg2_measure_secure_boot_variable(dev);
 	if (ret != EFI_SUCCESS) {
 		tcg2_uninit();
-- 
2.17.1

[PATCH 5/5] efi_loader: refactor efi_append_scrtm_version()

[Masahisa Kojima]

Refactor efi_append_scrtm_version() to use common
function for adding eventlog and extending PCR.

Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
---
 lib/efi_loader/efi_tcg2.c | 14 +-------------
 1 file changed, 1 insertion(+), 13 deletions(-)

diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
index 823abd8217..00e442cea5 100644
--- a/lib/efi_loader/efi_tcg2.c
+++ b/lib/efi_loader/efi_tcg2.c
@@ -1321,23 +1321,11 @@ out:
  */
 static efi_status_t efi_append_scrtm_version(struct udevice *dev)
 {
-	struct tpml_digest_values digest_list;
 	u8 ver[] = U_BOOT_VERSION_STRING;
-	const int pcr_index = 0;
 	efi_status_t ret;
 
-	ret = tcg2_create_digest(ver, sizeof(ver), &digest_list);
-	if (ret != EFI_SUCCESS)
-		goto out;
+	ret = tcg2_measure_event(dev, 0, EV_S_CRTM_VERSION, sizeof(ver), ver);
 
-	ret = tcg2_pcr_extend(dev, pcr_index, &digest_list);
-	if (ret != EFI_SUCCESS)
-		goto out;
-
-	ret = tcg2_agile_log_append(pcr_index, EV_S_CRTM_VERSION, &digest_list,
-				    sizeof(ver), ver);
-
-out:
 	return ret;
 }
 
-- 
2.17.1

Re: [PATCH 1/5] efi_loader: increase eventlog buffer size

[Heinrich Schuchardt]

On 7/7/21 3:36 PM, Masahisa Kojima wrote:
> This is a preperation to add eventlog support
> described in TCG PC Client PFP spec.
>
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> ---
>   lib/efi_loader/Kconfig | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> index b2ab48a048..a87bf3cc98 100644
> --- a/lib/efi_loader/Kconfig
> +++ b/lib/efi_loader/Kconfig
> @@ -327,7 +327,7 @@ config EFI_TCG2_PROTOCOL
>   config EFI_TCG2_PROTOCOL_EVENTLOG_SIZE
>   	int "EFI_TCG2_PROTOCOL EventLog size"
>   	depends on EFI_TCG2_PROTOCOL
> -	default 4096
> +	default 16384

I found this text in EDK II:

Minimum length(in bytes) of the system preboot TCG event log area(LAML)
-----------------------------------------------------------------------

For PC Client Implementation spec up to and including 1.2 the minimum
log size is 64KB. (SecurityPkg/SecurityPkg.dec)

Why should ours be smaller?

Best regards

Heinrich

>   	help
>   		Define the size of the EventLog for EFI_TCG2_PROTOCOL. Note that
>   		this is going to be allocated twice. One for the eventlog it self
>

Re: [PATCH 2/5] efi_loader: add secure boot variable measurement

[Simon Glass]

Hi Masahisa,

On Wed, 7 Jul 2021 at 07:36, Masahisa Kojima <masahisa.kojima@linaro.org> wrote:
>
> TCG PC Client PFP spec requires to measure the secure
> boot policy before validating the UEFI image.
> This commit adds the secure boot variable measurement
> of "SecureBoot", "PK", "KEK", "db" and "dbx".
>
> Note that this implementation assumes that secure boot
> variables are pre-configured and not be set/updated in runtime.
>
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> ---
>  include/efi_tcg2.h        |  20 ++++++
>  lib/efi_loader/efi_tcg2.c | 135 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 155 insertions(+)

Where are the tests for this code, please?

Regards,
Simon

Re: [PATCH 2/5] efi_loader: add secure boot variable measurement

[Ilias Apalodimas]

Hi Simon, 

On Wed, Jul 07, 2021 at 11:37:01AM -0600, Simon Glass wrote:
> Hi Masahisa,
> 
> On Wed, 7 Jul 2021 at 07:36, Masahisa Kojima <masahisa.kojima@linaro.org> wrote:
> >
> > TCG PC Client PFP spec requires to measure the secure
> > boot policy before validating the UEFI image.
> > This commit adds the secure boot variable measurement
> > of "SecureBoot", "PK", "KEK", "db" and "dbx".
> >
> > Note that this implementation assumes that secure boot
> > variables are pre-configured and not be set/updated in runtime.
> >
> > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > ---
> >  include/efi_tcg2.h        |  20 ++++++
> >  lib/efi_loader/efi_tcg2.c | 135 ++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 155 insertions(+)
> 
> Where are the tests for this code, please?

As we discussed in the past, the EFI TCG code can't be tested with the
asndbox as-is.  I'll have a look on your sandbox patches in case we can now
use those, but in any case, I've sent a TPM mmio based driver.  Even if the
sandbox is still not enough we can add tests once the mmio TPM driver gets
merged

Cheers
/Ilias
> 
> Regards,
> Simon

Re: [PATCH 2/5] efi_loader: add secure boot variable measurement

[Simon Glass]

Hi Ilias,

On Wed, 7 Jul 2021 at 11:40, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Simon,
>
> On Wed, Jul 07, 2021 at 11:37:01AM -0600, Simon Glass wrote:
> > Hi Masahisa,
> >
> > On Wed, 7 Jul 2021 at 07:36, Masahisa Kojima <masahisa.kojima@linaro.org> wrote:
> > >
> > > TCG PC Client PFP spec requires to measure the secure
> > > boot policy before validating the UEFI image.
> > > This commit adds the secure boot variable measurement
> > > of "SecureBoot", "PK", "KEK", "db" and "dbx".
> > >
> > > Note that this implementation assumes that secure boot
> > > variables are pre-configured and not be set/updated in runtime.
> > >
> > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > > ---
> > >  include/efi_tcg2.h        |  20 ++++++
> > >  lib/efi_loader/efi_tcg2.c | 135 ++++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 155 insertions(+)
> >
> > Where are the tests for this code, please?
>
> As we discussed in the past, the EFI TCG code can't be tested with the
> asndbox as-is.  I'll have a look on your sandbox patches in case we can now
> use those, but in any case, I've sent a TPM mmio based driver.  Even if the
> sandbox is still not enough we can add tests once the mmio TPM driver gets
> merged

Can you add features to the sandbox driver? I just sent a series that
added nvdata, for example.

Regards,
Simon

Re: [PATCH 2/5] efi_loader: add secure boot variable measurement

[Ilias Apalodimas]

On Wed, Jul 07, 2021 at 11:49:33AM -0600, Simon Glass wrote:
> Hi Ilias,
> 
> On Wed, 7 Jul 2021 at 11:40, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > Hi Simon,
> >
> > On Wed, Jul 07, 2021 at 11:37:01AM -0600, Simon Glass wrote:
> > > Hi Masahisa,
> > >
> > > On Wed, 7 Jul 2021 at 07:36, Masahisa Kojima <masahisa.kojima@linaro.org> wrote:
> > > >
> > > > TCG PC Client PFP spec requires to measure the secure
> > > > boot policy before validating the UEFI image.
> > > > This commit adds the secure boot variable measurement
> > > > of "SecureBoot", "PK", "KEK", "db" and "dbx".
> > > >
> > > > Note that this implementation assumes that secure boot
> > > > variables are pre-configured and not be set/updated in runtime.
> > > >
> > > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > > > ---
> > > >  include/efi_tcg2.h        |  20 ++++++
> > > >  lib/efi_loader/efi_tcg2.c | 135 ++++++++++++++++++++++++++++++++++++++
> > > >  2 files changed, 155 insertions(+)
> > >
> > > Where are the tests for this code, please?
> >
> > As we discussed in the past, the EFI TCG code can't be tested with the
> > asndbox as-is.  I'll have a look on your sandbox patches in case we can now
> > use those, but in any case, I've sent a TPM mmio based driver.  Even if the
> > sandbox is still not enough we can add tests once the mmio TPM driver gets
> > merged
> 
> Can you add features to the sandbox driver? I just sent a series that
> added nvdata, for example.

Yea I've seen that, I was going to have a look.  I'll try but my schedule
is pretty tight atm.

Thanks
/Ilias
> 
> Regards,
> Simon

Re: [PATCH 3/5] efi_loader: add boot variable measurement

[Ilias Apalodimas]

Hi Kojima-san,
> +{

[...]

> +	u16 *boot_order;
> +	u16 var_name[] = L"BootOrder";
> +	u16 boot_name[] = L"Boot0000";
> +	u16 hexmap[] = L"0123456789ABCDEF";
> +	u8 *bootvar;
> +	efi_uintn_t var_data_size;
> +	u32 count, i;
> +	efi_status_t ret;
> +
> +	boot_order = efi_get_var(var_name, &efi_global_variable_guid,
> +				 &var_data_size);
> +	if (!boot_order) {
> +		log_info("BootOrder not defined\n");
> +		ret = EFI_NOT_FOUND;
> +		goto error;
> +	}
> +
> +	ret = tcg2_measure_variable(dev, 1, EV_EFI_VARIABLE_BOOT2, var_name,
> +				    &efi_global_variable_guid, var_data_size,
> +				    (u8 *)boot_order);
> +	if (ret != EFI_SUCCESS)
> +		goto error;
> +
> +	count = var_data_size / sizeof(*boot_order);
> +	for (i = 0; i < count; i++) {
> +		boot_name[4] = hexmap[(boot_order[i] & 0xf000) >> 12];
> +		boot_name[5] = hexmap[(boot_order[i] & 0x0f00) >> 8];
> +		boot_name[6] = hexmap[(boot_order[i] & 0x00f0) >> 4];
> +		boot_name[7] = hexmap[(boot_order[i] & 0x000f)];

Can you use efi_create_indexed_name() instead?

[...]
> +	for (pcr_index = 0; pcr_index <= 7; pcr_index++) {
> +		ret = tcg2_measure_event(dev, pcr_index, EV_SEPARATOR,
> +					 sizeof(event), (u8 *)&event);

I assume adding a separator event on all these PCRs is described on the
standard?

> +		if (ret != EFI_SUCCESS)
> +			goto out;
> +	}
> +
> +	tcg2_efi_app_invoked = true;
> +out:
> +	return ret;
> +}
> +
> +/**
> + * efi_tcg2_measure_efi_app_exit() - measure efi app exit
> + *
> + * Return:	status code
> + */
> +efi_status_t EFIAPI efi_tcg2_measure_efi_app_exit(void)
> +{
> +	efi_status_t ret;
> +	struct udevice *dev;
> +
> +	ret = platform_get_tpm2_device(&dev);
> +	if (ret != EFI_SUCCESS)
> +		return ret;
> +
> +	ret = tcg2_measure_event(dev, 4, EV_EFI_ACTION,
> +				 strlen(EFI_RETURNING_FROM_EFI_APPLICATION),

Do we need a NUL terminator on this string or not?


Regards
/Ilias

Re: [PATCH 1/5] efi_loader: increase eventlog buffer size

[Masahisa Kojima]

On Wed, 7 Jul 2021 at 22:47, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
>
>
> On 7/7/21 3:36 PM, Masahisa Kojima wrote:
> > This is a preperation to add eventlog support
> > described in TCG PC Client PFP spec.
> >
> > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > ---
> >   lib/efi_loader/Kconfig | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > index b2ab48a048..a87bf3cc98 100644
> > --- a/lib/efi_loader/Kconfig
> > +++ b/lib/efi_loader/Kconfig
> > @@ -327,7 +327,7 @@ config EFI_TCG2_PROTOCOL
> >   config EFI_TCG2_PROTOCOL_EVENTLOG_SIZE
> >       int "EFI_TCG2_PROTOCOL EventLog size"
> >       depends on EFI_TCG2_PROTOCOL
> > -     default 4096
> > +     default 16384
>
> I found this text in EDK II:
>
> Minimum length(in bytes) of the system preboot TCG event log area(LAML)
> -----------------------------------------------------------------------
>
> For PC Client Implementation spec up to and including 1.2 the minimum
> log size is 64KB. (SecurityPkg/SecurityPkg.dec)

Thank you for your feedback.
I have not checked this.
TCG spec also says "The Log Area Minimum Length for the TCG event log
MUST be at least 64KB." in ACPI chapter.
I will update to set 64KB as default.

Thanks,
Masahisa Kojima

>
> Why should ours be smaller?
>
> Best regards
>
> Heinrich
>
> >       help
> >               Define the size of the EventLog for EFI_TCG2_PROTOCOL. Note that
> >               this is going to be allocated twice. One for the eventlog it self
> >

Re: [PATCH 3/5] efi_loader: add boot variable measurement

[Masahisa Kojima]

On Thu, 8 Jul 2021 at 03:56, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Kojima-san,
> > +{
>
> [...]
>
> > +     u16 *boot_order;
> > +     u16 var_name[] = L"BootOrder";
> > +     u16 boot_name[] = L"Boot0000";
> > +     u16 hexmap[] = L"0123456789ABCDEF";
> > +     u8 *bootvar;
> > +     efi_uintn_t var_data_size;
> > +     u32 count, i;
> > +     efi_status_t ret;
> > +
> > +     boot_order = efi_get_var(var_name, &efi_global_variable_guid,
> > +                              &var_data_size);
> > +     if (!boot_order) {
> > +             log_info("BootOrder not defined\n");
> > +             ret = EFI_NOT_FOUND;
> > +             goto error;
> > +     }
> > +
> > +     ret = tcg2_measure_variable(dev, 1, EV_EFI_VARIABLE_BOOT2, var_name,
> > +                                 &efi_global_variable_guid, var_data_size,
> > +                                 (u8 *)boot_order);
> > +     if (ret != EFI_SUCCESS)
> > +             goto error;
> > +
> > +     count = var_data_size / sizeof(*boot_order);
> > +     for (i = 0; i < count; i++) {
> > +             boot_name[4] = hexmap[(boot_order[i] & 0xf000) >> 12];
> > +             boot_name[5] = hexmap[(boot_order[i] & 0x0f00) >> 8];
> > +             boot_name[6] = hexmap[(boot_order[i] & 0x00f0) >> 4];
> > +             boot_name[7] = hexmap[(boot_order[i] & 0x000f)];
>
> Can you use efi_create_indexed_name() instead?

I have not noticed this utility function, thank you.

>
> [...]
> > +     for (pcr_index = 0; pcr_index <= 7; pcr_index++) {
> > +             ret = tcg2_measure_event(dev, pcr_index, EV_SEPARATOR,
> > +                                      sizeof(event), (u8 *)&event);
>
> I assume adding a separator event on all these PCRs is described on the
> standard?

Yes, TCG spec requires EV_SEPARATOR event prior to the first invocation of
the first Ready to Boot call.
Spec also says EV_SEPARATOR must be the last entry for PCR0, 1, 2, 3, 6.

>
> > +             if (ret != EFI_SUCCESS)
> > +                     goto out;
> > +     }
> > +
> > +     tcg2_efi_app_invoked = true;
> > +out:
> > +     return ret;
> > +}
> > +
> > +/**
> > + * efi_tcg2_measure_efi_app_exit() - measure efi app exit
> > + *
> > + * Return:   status code
> > + */
> > +efi_status_t EFIAPI efi_tcg2_measure_efi_app_exit(void)
> > +{
> > +     efi_status_t ret;
> > +     struct udevice *dev;
> > +
> > +     ret = platform_get_tpm2_device(&dev);
> > +     if (ret != EFI_SUCCESS)
> > +             return ret;
> > +
> > +     ret = tcg2_measure_event(dev, 4, EV_EFI_ACTION,
> > +                              strlen(EFI_RETURNING_FROM_EFI_APPLICATION),
>
> Do we need a NUL terminator on this string or not?

No, TCG spec says
"the actual log entries SHALL NOT include the quote characters
or a NUL terminator."

Thanks,
Masahisa Kojima

>
>
> Regards
> /Ilias

Re: [PATCH 5/5] efi_loader: refactor efi_append_scrtm_version()

[Heinrich Schuchardt]

On 7/7/21 3:36 PM, Masahisa Kojima wrote:
> Refactor efi_append_scrtm_version() to use common
> function for adding eventlog and extending PCR.
>
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> ---
>   lib/efi_loader/efi_tcg2.c | 14 +-------------
>   1 file changed, 1 insertion(+), 13 deletions(-)
>
> diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
> index 823abd8217..00e442cea5 100644
> --- a/lib/efi_loader/efi_tcg2.c
> +++ b/lib/efi_loader/efi_tcg2.c
> @@ -1321,23 +1321,11 @@ out:
>    */
>   static efi_status_t efi_append_scrtm_version(struct udevice *dev)
>   {
> -	struct tpml_digest_values digest_list;
>   	u8 ver[] = U_BOOT_VERSION_STRING;
> -	const int pcr_index = 0;
>   	efi_status_t ret;
>
> -	ret = tcg2_create_digest(ver, sizeof(ver), &digest_list);
> -	if (ret != EFI_SUCCESS)
> -		goto out;
> +	ret = tcg2_measure_event(dev, 0, EV_S_CRTM_VERSION, sizeof(ver), ver);

Must we convert the string to UTF-16? What is required to get a correct
listing of the event in the OS?

Best regards

Heinrich

>
> -	ret = tcg2_pcr_extend(dev, pcr_index, &digest_list);
> -	if (ret != EFI_SUCCESS)
> -		goto out;
> -
> -	ret = tcg2_agile_log_append(pcr_index, EV_S_CRTM_VERSION, &digest_list,
> -				    sizeof(ver), ver);
> -
> -out:
>   	return ret;
>   }
>
>

Re: [PATCH 4/5] efi_loader: add ExitBootServices() measurement

[Heinrich Schuchardt]

On 7/7/21 3:36 PM, Masahisa Kojima wrote:
> TCG PC Client PFP spec requires to measure
> "Exit Boot Services Invocation" if ExitBootServices() is invoked.
> Depending upon the return code from the ExitBootServices() call,
> "Exit Boot Services Returned with Success" or "Exit Boot Services
> Returned with Failure" is also measured.
>
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> ---
>   include/efi_loader.h          |  1 +
>   lib/efi_loader/efi_boottime.c |  5 +++
>   lib/efi_loader/efi_tcg2.c     | 70 +++++++++++++++++++++++++++++++++++
>   3 files changed, 76 insertions(+)
>
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 281ffff30f..e9bd1aac08 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -407,6 +407,7 @@ efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size);
>   efi_status_t efi_init_variables(void);
>   /* Notify ExitBootServices() is called */
>   void efi_variables_boot_exit_notify(void);
> +efi_status_t efi_tcg2_notify_exit_boot_services_failed(void);
>   /* Measure efi application invocation */
>   efi_status_t EFIAPI efi_tcg2_measure_efi_app_invocation(void);
>   /* Measure efi application exit */
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index 2914800c56..6e07ef65bc 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -2181,6 +2181,11 @@ static efi_status_t EFIAPI efi_exit_boot_services(efi_handle_t image_handle,
>   	efi_set_watchdog(0);
>   	WATCHDOG_RESET();
>   out:
> +	if (ret != EFI_SUCCESS) {
> +		if (IS_ENABLED(CONFIG_EFI_TCG2_PROTOCOL))
> +			efi_tcg2_notify_exit_boot_services_failed();
> +	}
> +
>   	return EFI_EXIT(ret);
>   }
>
> diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
> index 6e903e3cb3..823abd8217 100644
> --- a/lib/efi_loader/efi_tcg2.c
> +++ b/lib/efi_loader/efi_tcg2.c
> @@ -1506,6 +1506,67 @@ efi_status_t EFIAPI efi_tcg2_measure_efi_app_exit(void)
>   	return ret;
>   }
>
> +/**
> + * efi_tcg2_notify_exit_boot_services() - ExitBootService callback
> + *
> + * @event:	callback event
> + * @context:	callback context
> + */
> +static void EFIAPI
> +efi_tcg2_notify_exit_boot_services(struct efi_event *event, void *context)
> +{
> +	efi_status_t ret;
> +	struct udevice *dev;
> +
> +	EFI_ENTRY("%p, %p", event, context);
> +
> +	ret = platform_get_tpm2_device(&dev);
> +	if (ret != EFI_SUCCESS)
> +		goto out;
> +
> +	ret = tcg2_measure_event(dev, 5, EV_EFI_ACTION,
> +				 strlen(EFI_EXIT_BOOT_SERVICES_INVOCATION),
> +				 (u8 *)EFI_EXIT_BOOT_SERVICES_INVOCATION);
> +	if (ret != EFI_SUCCESS)
> +		goto out;
> +
> +	ret = tcg2_measure_event(dev, 5, EV_EFI_ACTION,
> +				 strlen(EFI_EXIT_BOOT_SERVICES_SUCCEEDED),
> +				 (u8 *)EFI_EXIT_BOOT_SERVICES_SUCCEEDED);
> +
> +out:
> +	EFI_EXIT(ret);
> +}
> +
> +/**
> + * efi_tcg2_notify_exit_boot_services_failed()
> + *  - notify ExitBootServices() is failed
> + *
> + * Return:	status code
> + */
> +efi_status_t efi_tcg2_notify_exit_boot_services_failed(void)
> +{
> +	struct udevice *dev;
> +	efi_status_t ret;
> +
> +	ret = platform_get_tpm2_device(&dev);
> +	if (ret != EFI_SUCCESS)
> +		goto out;
> +
> +	ret = tcg2_measure_event(dev, 5, EV_EFI_ACTION,
> +				 sizeof(EFI_EXIT_BOOT_SERVICES_INVOCATION),
> +				 (u8 *)EFI_EXIT_BOOT_SERVICES_INVOCATION);
> +	if (ret != EFI_SUCCESS)
> +		goto out;
> +
> +	ret = tcg2_measure_event(dev, 5, EV_EFI_ACTION,
> +				 sizeof(EFI_EXIT_BOOT_SERVICES_FAILED),
> +				 (u8 *)EFI_EXIT_BOOT_SERVICES_FAILED);
> +
> +out:
> +	return ret;
> +}

We must keep out code small. Please, carve out a function to which pass
a the event type and an u8 string and use it where applicable.

Best regards

Heinrich

> +
>   /**
>    * tcg2_measure_secure_boot_variable() - measure secure boot variables
>    *
> @@ -1556,6 +1617,7 @@ efi_status_t efi_tcg2_register(void)
>   {
>   	efi_status_t ret = EFI_SUCCESS;
>   	struct udevice *dev;
> +	struct efi_event *event;
>
>   	ret = platform_get_tpm2_device(&dev);
>   	if (ret != EFI_SUCCESS) {
> @@ -1580,6 +1642,14 @@ efi_status_t efi_tcg2_register(void)
>   		goto fail;
>   	}
>
> +	ret = efi_create_event(EVT_SIGNAL_EXIT_BOOT_SERVICES, TPL_CALLBACK,
> +			       efi_tcg2_notify_exit_boot_services, NULL,
> +			       NULL, &event);
> +	if (ret != EFI_SUCCESS) {
> +		tcg2_uninit();
> +		goto fail;
> +	}
> +
>   	ret = tcg2_measure_secure_boot_variable(dev);
>   	if (ret != EFI_SUCCESS) {
>   		tcg2_uninit();
>

Re: [PATCH 2/5] efi_loader: add secure boot variable measurement

[Heinrich Schuchardt]

On 7/7/21 3:36 PM, Masahisa Kojima wrote:
> TCG PC Client PFP spec requires to measure the secure
> boot policy before validating the UEFI image.
> This commit adds the secure boot variable measurement
> of "SecureBoot", "PK", "KEK", "db" and "dbx".
>
> Note that this implementation assumes that secure boot
> variables are pre-configured and not be set/updated in runtime.
>
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> ---
>   include/efi_tcg2.h        |  20 ++++++
>   lib/efi_loader/efi_tcg2.c | 135 ++++++++++++++++++++++++++++++++++++++
>   2 files changed, 155 insertions(+)
>
> diff --git a/include/efi_tcg2.h b/include/efi_tcg2.h
> index bcfb98168a..8d7b77c087 100644
> --- a/include/efi_tcg2.h
> +++ b/include/efi_tcg2.h
> @@ -142,6 +142,26 @@ struct efi_tcg2_final_events_table {
>   	struct tcg_pcr_event2 event[];
>   };
>
> +/**
> + * struct tdUEFI_VARIABLE_DATA

Please, add a reference to the "TCG PC Client PlatformFirmware Profile
specification".

> + * @variable_name:		The vendorGUID parameter in the
> + *				GetVariable() API.
> + * @unicode_name_length:	The length in CHAR16 of the Unicode name of
> + *				the variable.

Where is this specified as CHAR16 count? I could not find it in the "TCG
PC Client PlatformFirmware Profile specification".

> + * @variable_data_length:	The size of the variable data.
> + * @unicode_name:		The CHAR16 unicode name of the variable
> + *				without NULL-terminator.
> + * @variable_data:		The data parameter of the efi variable
> + *				in the GetVariable() API.
> + */
> +struct efi_tcg2_uefi_variable_data {
> +	efi_guid_t variable_name;
> +	u64 unicode_name_length;
> +	u64 variable_data_length;
> +	u16 unicode_name[1];
> +	u8 variable_data[1];
> +};
> +
>   struct efi_tcg2_protocol {
>   	efi_status_t (EFIAPI * get_capability)(struct efi_tcg2_protocol *this,
>   					       struct efi_tcg2_boot_service_capability *capability);
> diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
> index 1319a8b378..2a248bd62a 100644
> --- a/lib/efi_loader/efi_tcg2.c
> +++ b/lib/efi_loader/efi_tcg2.c
> @@ -78,6 +78,19 @@ static const struct digest_info hash_algo_list[] = {
>   	},
>   };
>
> +struct variable_info {
> +	u16		*name;
> +	const efi_guid_t	*guid;
> +};
> +
> +static struct variable_info secure_variables[] = {
> +	{L"SecureBoot", &efi_global_variable_guid},
> +	{L"PK", &efi_global_variable_guid},
> +	{L"KEK", &efi_global_variable_guid},
> +	{L"db", &efi_guid_image_security_database},
> +	{L"dbx", &efi_guid_image_security_database},

You have to measure BootOrder and Boot#### too.
See TCG PC Client Platform Firmware Profile Specification, March 30, 2016.

> +};
> +
>   #define MAX_HASH_COUNT ARRAY_SIZE(hash_algo_list)
>
>   /**
> @@ -1264,6 +1277,39 @@ free_pool:
>   	return ret;
>   }
>
> +/**
> + * tcg2_measure_event() - common function to add event log and extend PCR
> + *
> + * @dev:		TPM device
> + * @pcr_index:		PCR index
> + * @event_type:		type of event added
> + * @size:		event size
> + * @event:		event data
> + *
> + * Return:	status code
> + */
> +static efi_status_t EFIAPI
> +tcg2_measure_event(struct udevice *dev, u32 pcr_index, u32 event_type,
> +		   u32 size, u8 event[])
> +{
> +	struct tpml_digest_values digest_list;
> +	efi_status_t ret;
> +
> +	ret = tcg2_create_digest(event, size, &digest_list);
> +	if (ret != EFI_SUCCESS)
> +		goto out;
> +
> +	ret = tcg2_pcr_extend(dev, pcr_index, &digest_list);
> +	if (ret != EFI_SUCCESS)
> +		goto out;
> +
> +	ret = tcg2_agile_log_append(pcr_index, event_type, &digest_list,
> +				    size, event);
> +
> +out:
> +	return ret;
> +}
> +
>   /**
>    * efi_append_scrtm_version - Append an S-CRTM EV_S_CRTM_VERSION event on the
>    *			      eventlog and extend the PCRs
> @@ -1294,6 +1340,88 @@ out:
>   	return ret;
>   }
>
> +/**
> + * tcg2_measure_variable() - add variable event log and extend PCR
> + *
> + * @dev:		TPM device
> + * @pcr_index:		PCR index
> + * @event_type:		type of event added
> + * @var_name:		variable name
> + * @guid:		guid
> + * @data_size:		variable data size
> + * @data:		variable data
> + *
> + * Return:	status code
> + */
> +static efi_status_t tcg2_measure_variable(struct udevice *dev, u32 pcr_index,
> +					  u32 event_type, u16 *var_name,
> +					  const efi_guid_t *guid,
> +					  efi_uintn_t data_size, u8 *data)
> +{
> +	u32 event_size;
> +	efi_status_t ret;
> +	struct efi_tcg2_uefi_variable_data *event;
> +
> +	event_size = sizeof(event->variable_name) +
> +		     sizeof(event->unicode_name_length) +
> +		     sizeof(event->variable_data_length) +
> +		     (u16_strlen(var_name) * sizeof(*var_name)) + data_size;

Please, replace by sizeof(*var_name) by sizeof(u16) to improve readability.

> +	event = malloc(event_size);
> +	if (!event)
> +		return EFI_OUT_OF_RESOURCES;
> +
> +	guidcpy(&event->variable_name, guid);
> +	event->unicode_name_length = u16_strlen(var_name);
> +	event->variable_data_length = data_size;
> +	memcpy(event->unicode_name, var_name,
> +	       (event->unicode_name_length * sizeof(*event->unicode_name)));

sizeof(u16)

> +	memcpy((u16 *)event->unicode_name + event->unicode_name_length,
> +	       (u8 *)data, data_size);

memcpy() wants void *. data is already of type u8 *. This conversion can
be removed.

> +	ret = tcg2_measure_event(dev, pcr_index, event_type, event_size,
> +				 (u8 *)event);
> +	free(event);
> +	return ret;
> +}
> +
> +/**
> + * tcg2_measure_secure_boot_variable() - measure secure boot variables
> + *
> + * @dev:	TPM device
> + *
> + * Return:	status code
> + */
> +static efi_status_t tcg2_measure_secure_boot_variable(struct udevice *dev)
> +{
> +	u8 *data;
> +	efi_uintn_t data_size;
> +	u32 count, i;
> +	efi_status_t ret;
> +
> +	count = ARRAY_SIZE(secure_variables);
> +	for (i = 0; i < count; i++) {
> +		data = efi_get_var(secure_variables[i].name,
> +				   secure_variables[i].guid,
> +				   &data_size);

You need to check if data==NULL. There is no guarantee that the
variables exist.

Best regards

Heinrich

> +
> +		ret = tcg2_measure_variable(dev, 7,
> +					    EV_EFI_VARIABLE_DRIVER_CONFIG,
> +					    secure_variables[i].name,
> +					    secure_variables[i].guid,
> +					    data_size, (u8 *)data);
> +		free(data);
> +		if (ret != EFI_SUCCESS)
> +			goto error;
> +	}
> +
> +	/*
> +	 * TODO: add DBT and DBR measurement support when u-boot supports
> +	 * these variables.
> +	 */
> +
> +error:
> +	return ret;
> +}
> +
>   /**
>    * efi_tcg2_register() - register EFI_TCG2_PROTOCOL
>    *
> @@ -1328,6 +1456,13 @@ efi_status_t efi_tcg2_register(void)
>   		tcg2_uninit();
>   		goto fail;
>   	}
> +
> +	ret = tcg2_measure_secure_boot_variable(dev);
> +	if (ret != EFI_SUCCESS) {
> +		tcg2_uninit();
> +		goto fail;
> +	}
> +
>   	return ret;
>
>   fail:
>

Re: [PATCH 3/5] efi_loader: add boot variable measurement

[Heinrich Schuchardt]

On 7/7/21 3:36 PM, Masahisa Kojima wrote:
> TCG PC Client PFP spec requires to measure "Boot####"
> and "BootOrder" variables, EV_SEPARATOR event prior
> to the Ready to Boot invocation.
> Since u-boot does not implement Ready to Boot event,
> these measurements are performed when efi_start_image() is called.
>
> TCG spec also requires to measure "Calling EFI Application from
> Boot Option" for each boot attempt, and "Returning from EFI
> Application from Boot Option" if a boot device returns control
> back to the Boot Manager.
>
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> ---
>   include/efi_loader.h          |   4 ++
>   include/tpm-v2.h              |  18 ++++-
>   lib/efi_loader/efi_boottime.c |  20 ++++++
>   lib/efi_loader/efi_tcg2.c     | 123 ++++++++++++++++++++++++++++++++++
>   4 files changed, 164 insertions(+), 1 deletion(-)
>
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 0a9c82a257..281ffff30f 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -407,6 +407,10 @@ efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size);
>   efi_status_t efi_init_variables(void);
>   /* Notify ExitBootServices() is called */
>   void efi_variables_boot_exit_notify(void);
> +/* Measure efi application invocation */
> +efi_status_t EFIAPI efi_tcg2_measure_efi_app_invocation(void);
> +/* Measure efi application exit */
> +efi_status_t EFIAPI efi_tcg2_measure_efi_app_exit(void);
>   /* Called by bootefi to initialize root node */
>   efi_status_t efi_root_node_register(void);
>   /* Called by bootefi to initialize runtime */
> diff --git a/include/tpm-v2.h b/include/tpm-v2.h
> index 3e48e35861..8a7b7f1874 100644
> --- a/include/tpm-v2.h
> +++ b/include/tpm-v2.h
> @@ -73,7 +73,7 @@ struct udevice;
>   /*
>    * event types, cf.
>    * "TCG PC Client Platform Firmware Profile Specification", Family "2.0"
> - * rev 1.04, June 3, 2019
> + * Level 00 Version 1.05 Revision 23, May 7, 2021
>    */
>   #define EV_EFI_EVENT_BASE			((u32)0x80000000)
>   #define EV_EFI_VARIABLE_DRIVER_CONFIG		((u32)0x80000001)
> @@ -85,8 +85,24 @@ struct udevice;
>   #define EV_EFI_ACTION				((u32)0x80000007)
>   #define EV_EFI_PLATFORM_FIRMWARE_BLOB		((u32)0x80000008)
>   #define EV_EFI_HANDOFF_TABLES			((u32)0x80000009)
> +#define EV_EFI_PLATFORM_FIRMWARE_BLOB2		((u32)0x8000000A)
> +#define EV_EFI_HANDOFF_TABLES2			((u32)0x8000000B)
> +#define EV_EFI_VARIABLE_BOOT2			((u32)0x8000000C)
>   #define EV_EFI_HCRTM_EVENT			((u32)0x80000010)
>   #define EV_EFI_VARIABLE_AUTHORITY		((u32)0x800000E0)
> +#define EV_EFI_SPDM_FIRMWARE_BLOB		((u32)0x800000E1)
> +#define EV_EFI_SPDM_FIRMWARE_CONFIG		((u32)0x800000E2)
> +
> +#define EFI_CALLING_EFI_APPLICATION         \
> +	"Calling EFI Application from Boot Option"
> +#define EFI_RETURNING_FROM_EFI_APPLICATION  \
> +	"Returning from EFI Application from Boot Option"
> +#define EFI_EXIT_BOOT_SERVICES_INVOCATION   \
> +	"Exit Boot Services Invocation"
> +#define EFI_EXIT_BOOT_SERVICES_FAILED       \
> +	"Exit Boot Services Returned with Failure"
> +#define EFI_EXIT_BOOT_SERVICES_SUCCEEDED    \
> +	"Exit Boot Services Returned with Success"

Which spec defines if the string in the event log shall be utf-8 or utf-16?

Best regards

Heinrich

>
>   /* TPMS_TAGGED_PROPERTY Structure */
>   struct tpms_tagged_property {
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index f6d5ba05e3..2914800c56 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -2993,6 +2993,16 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
>   	image_obj->exit_status = &exit_status;
>   	image_obj->exit_jmp = &exit_jmp;
>
> +	if (IS_ENABLED(CONFIG_EFI_TCG2_PROTOCOL)) {
> +		if (image_obj->image_type == IMAGE_SUBSYSTEM_EFI_APPLICATION) {
> +			ret = efi_tcg2_measure_efi_app_invocation();
> +			if (ret != EFI_SUCCESS) {
> +				EFI_PRINT("tcg2 measurement fails(0x%lx)\n",
> +					  ret);
> +			}
> +		}
> +	}
> +
>   	/* call the image! */
>   	if (setjmp(&exit_jmp)) {
>   		/*
> @@ -3251,6 +3261,16 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
>   	    exit_status != EFI_SUCCESS)
>   		efi_delete_image(image_obj, loaded_image_protocol);
>
> +	if (IS_ENABLED(CONFIG_EFI_TCG2_PROTOCOL)) {
> +		if (image_obj->image_type == IMAGE_SUBSYSTEM_EFI_APPLICATION) {
> +			ret = efi_tcg2_measure_efi_app_exit();
> +			if (ret != EFI_SUCCESS) {
> +				EFI_PRINT("tcg2 measurement fails(0x%lx)\n",
> +					  ret);
> +			}
> +		}
> +	}
> +
>   	/* Make sure entry/exit counts for EFI world cross-overs match */
>   	EFI_EXIT(exit_status);
>
> diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
> index 2a248bd62a..6e903e3cb3 100644
> --- a/lib/efi_loader/efi_tcg2.c
> +++ b/lib/efi_loader/efi_tcg2.c
> @@ -35,6 +35,7 @@ struct event_log_buffer {
>   };
>
>   static struct event_log_buffer event_log;
> +static bool tcg2_efi_app_invoked;
>   /*
>    * When requesting TPM2_CAP_TPM_PROPERTIES the value is on a standard offset.
>    * Since the current tpm2_get_capability() response buffers starts at
> @@ -1383,6 +1384,128 @@ static efi_status_t tcg2_measure_variable(struct udevice *dev, u32 pcr_index,
>   	return ret;
>   }
>
> +/**
> + * tcg2_measure_boot_variable() - measure boot variables
> + *
> + * @dev:	TPM device
> + *
> + * Return:	status code
> + */
> +static efi_status_t tcg2_measure_boot_variable(struct udevice *dev)
> +{
> +	u16 *boot_order;
> +	u16 var_name[] = L"BootOrder";
> +	u16 boot_name[] = L"Boot0000";
> +	u16 hexmap[] = L"0123456789ABCDEF";
> +	u8 *bootvar;
> +	efi_uintn_t var_data_size;
> +	u32 count, i;
> +	efi_status_t ret;
> +
> +	boot_order = efi_get_var(var_name, &efi_global_variable_guid,
> +				 &var_data_size);
> +	if (!boot_order) {
> +		log_info("BootOrder not defined\n");
> +		ret = EFI_NOT_FOUND;
> +		goto error;
> +	}
> +
> +	ret = tcg2_measure_variable(dev, 1, EV_EFI_VARIABLE_BOOT2, var_name,
> +				    &efi_global_variable_guid, var_data_size,
> +				    (u8 *)boot_order);
> +	if (ret != EFI_SUCCESS)
> +		goto error;
> +
> +	count = var_data_size / sizeof(*boot_order);
> +	for (i = 0; i < count; i++) {
> +		boot_name[4] = hexmap[(boot_order[i] & 0xf000) >> 12];
> +		boot_name[5] = hexmap[(boot_order[i] & 0x0f00) >> 8];
> +		boot_name[6] = hexmap[(boot_order[i] & 0x00f0) >> 4];
> +		boot_name[7] = hexmap[(boot_order[i] & 0x000f)];
> +
> +		bootvar = efi_get_var(boot_name, &efi_global_variable_guid,
> +				      &var_data_size);
> +
> +		if (!bootvar) {
> +			log_info("%ls not found\n", boot_name);
> +			continue;
> +		}
> +
> +		ret = tcg2_measure_variable(dev, 1, EV_EFI_VARIABLE_BOOT2,
> +					    boot_name,
> +					    &efi_global_variable_guid,
> +					    var_data_size, bootvar);
> +		free(bootvar);
> +		if (ret != EFI_SUCCESS)
> +			goto error;
> +	}
> +
> +error:
> +	free(boot_order);
> +	return ret;
> +}
> +
> +/**
> + * efi_tcg2_measure_efi_app_invocation() - measure efi app invocation
> + *
> + * Return:	status code
> + */
> +efi_status_t EFIAPI efi_tcg2_measure_efi_app_invocation(void)
> +{
> +	efi_status_t ret;
> +	u32 pcr_index;
> +	struct udevice *dev;
> +	u32 event = 0;
> +
> +	if (tcg2_efi_app_invoked)
> +		return EFI_SUCCESS;
> +
> +	ret = platform_get_tpm2_device(&dev);
> +	if (ret != EFI_SUCCESS)
> +		return ret;
> +
> +	ret = tcg2_measure_boot_variable(dev);
> +	if (ret != EFI_SUCCESS)
> +		goto out;
> +
> +	ret = tcg2_measure_event(dev, 4, EV_EFI_ACTION,
> +				 strlen(EFI_CALLING_EFI_APPLICATION),
> +				 (u8 *)EFI_CALLING_EFI_APPLICATION);
> +	if (ret != EFI_SUCCESS)
> +		goto out;
> +
> +	for (pcr_index = 0; pcr_index <= 7; pcr_index++) {
> +		ret = tcg2_measure_event(dev, pcr_index, EV_SEPARATOR,
> +					 sizeof(event), (u8 *)&event);
> +		if (ret != EFI_SUCCESS)
> +			goto out;
> +	}
> +
> +	tcg2_efi_app_invoked = true;
> +out:
> +	return ret;
> +}
> +
> +/**
> + * efi_tcg2_measure_efi_app_exit() - measure efi app exit
> + *
> + * Return:	status code
> + */
> +efi_status_t EFIAPI efi_tcg2_measure_efi_app_exit(void)
> +{
> +	efi_status_t ret;
> +	struct udevice *dev;
> +
> +	ret = platform_get_tpm2_device(&dev);
> +	if (ret != EFI_SUCCESS)
> +		return ret;
> +
> +	ret = tcg2_measure_event(dev, 4, EV_EFI_ACTION,
> +				 strlen(EFI_RETURNING_FROM_EFI_APPLICATION),
> +				 (u8 *)EFI_RETURNING_FROM_EFI_APPLICATION);
> +	return ret;
> +}
> +
>   /**
>    * tcg2_measure_secure_boot_variable() - measure secure boot variables
>    *
>

Re: [PATCH 5/5] efi_loader: refactor efi_append_scrtm_version()

[Masahisa Kojima]

On Fri, 9 Jul 2021 at 02:32, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 7/7/21 3:36 PM, Masahisa Kojima wrote:
> > Refactor efi_append_scrtm_version() to use common
> > function for adding eventlog and extending PCR.
> >
> > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > ---
> >   lib/efi_loader/efi_tcg2.c | 14 +-------------
> >   1 file changed, 1 insertion(+), 13 deletions(-)
> >
> > diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
> > index 823abd8217..00e442cea5 100644
> > --- a/lib/efi_loader/efi_tcg2.c
> > +++ b/lib/efi_loader/efi_tcg2.c
> > @@ -1321,23 +1321,11 @@ out:
> >    */
> >   static efi_status_t efi_append_scrtm_version(struct udevice *dev)
> >   {
> > -     struct tpml_digest_values digest_list;
> >       u8 ver[] = U_BOOT_VERSION_STRING;
> > -     const int pcr_index = 0;
> >       efi_status_t ret;
> >
> > -     ret = tcg2_create_digest(ver, sizeof(ver), &digest_list);
> > -     if (ret != EFI_SUCCESS)
> > -             goto out;
> > +     ret = tcg2_measure_event(dev, 0, EV_S_CRTM_VERSION, sizeof(ver), ver);
>
> Must we convert the string to UTF-16? What is required to get a correct
> listing of the event in the OS?

TCG PC Client spec just says "The event field contains the version string
of the SRTM.".
I think there is no character encoding requirement.

Thanks,
Masahisa Kojima


>
> Best regards
>
> Heinrich
>
> >
> > -     ret = tcg2_pcr_extend(dev, pcr_index, &digest_list);
> > -     if (ret != EFI_SUCCESS)
> > -             goto out;
> > -
> > -     ret = tcg2_agile_log_append(pcr_index, EV_S_CRTM_VERSION, &digest_list,
> > -                                 sizeof(ver), ver);
> > -
> > -out:
> >       return ret;
> >   }
> >
> >
>

Re: [PATCH 2/5] efi_loader: add secure boot variable measurement

[Masahisa Kojima]

On Fri, 9 Jul 2021 at 02:46, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 7/7/21 3:36 PM, Masahisa Kojima wrote:
> > TCG PC Client PFP spec requires to measure the secure
> > boot policy before validating the UEFI image.
> > This commit adds the secure boot variable measurement
> > of "SecureBoot", "PK", "KEK", "db" and "dbx".
> >
> > Note that this implementation assumes that secure boot
> > variables are pre-configured and not be set/updated in runtime.
> >
> > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > ---
> >   include/efi_tcg2.h        |  20 ++++++
> >   lib/efi_loader/efi_tcg2.c | 135 ++++++++++++++++++++++++++++++++++++++
> >   2 files changed, 155 insertions(+)
> >
> > diff --git a/include/efi_tcg2.h b/include/efi_tcg2.h
> > index bcfb98168a..8d7b77c087 100644
> > --- a/include/efi_tcg2.h
> > +++ b/include/efi_tcg2.h
> > @@ -142,6 +142,26 @@ struct efi_tcg2_final_events_table {
> >       struct tcg_pcr_event2 event[];
> >   };
> >
> > +/**
> > + * struct tdUEFI_VARIABLE_DATA
>
> Please, add a reference to the "TCG PC Client PlatformFirmware Profile
> specification".

In addition to this tdUEFI_VARIABLE_DATA structure, other structures
defined in efi_tcg2.h such as tdEFI_TCG2_FINAL_EVENTS_TABLE
are also referencing "TCG PC Client PlatformFirmware Profile specification.
So I will add file-wide reference in the file header.
efi_tcg2.h also includes definitions described in
"TCG EFI Protocol Specification Revision 00.13 March 30, 2016"

>
> > + * @variable_name:           The vendorGUID parameter in the
> > + *                           GetVariable() API.
> > + * @unicode_name_length:     The length in CHAR16 of the Unicode name of
> > + *                           the variable.
>
> Where is this specified as CHAR16 count? I could not find it in the "TCG
> PC Client PlatformFirmware Profile specification".

Would you refer the latest spec?
https://trustedcomputinggroup.org/resource/pc-client-specific-platform-firmware-profile-specification/
(Version 1.05 Revision 23 May 7, 2021), Page 86.

>
> > + * @variable_data_length:    The size of the variable data.
> > + * @unicode_name:            The CHAR16 unicode name of the variable
> > + *                           without NULL-terminator.
> > + * @variable_data:           The data parameter of the efi variable
> > + *                           in the GetVariable() API.
> > + */
> > +struct efi_tcg2_uefi_variable_data {
> > +     efi_guid_t variable_name;
> > +     u64 unicode_name_length;
> > +     u64 variable_data_length;
> > +     u16 unicode_name[1];
> > +     u8 variable_data[1];
> > +};
> > +
> >   struct efi_tcg2_protocol {
> >       efi_status_t (EFIAPI * get_capability)(struct efi_tcg2_protocol *this,
> >                                              struct efi_tcg2_boot_service_capability *capability);
> > diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
> > index 1319a8b378..2a248bd62a 100644
> > --- a/lib/efi_loader/efi_tcg2.c
> > +++ b/lib/efi_loader/efi_tcg2.c
> > @@ -78,6 +78,19 @@ static const struct digest_info hash_algo_list[] = {
> >       },
> >   };
> >
> > +struct variable_info {
> > +     u16             *name;
> > +     const efi_guid_t        *guid;
> > +};
> > +
> > +static struct variable_info secure_variables[] = {
> > +     {L"SecureBoot", &efi_global_variable_guid},
> > +     {L"PK", &efi_global_variable_guid},
> > +     {L"KEK", &efi_global_variable_guid},
> > +     {L"db", &efi_guid_image_security_database},
> > +     {L"dbx", &efi_guid_image_security_database},
>
> You have to measure BootOrder and Boot#### too.
> See TCG PC Client Platform Firmware Profile Specification, March 30, 2016.

These variables measurement is included in a separate patch in this series.
Could you check this?
"[PATCH 3/5] efi_loader: add boot variable measurement"

>
> > +};
> > +
> >   #define MAX_HASH_COUNT ARRAY_SIZE(hash_algo_list)
> >
> >   /**
> > @@ -1264,6 +1277,39 @@ free_pool:
> >       return ret;
> >   }
> >
> > +/**
> > + * tcg2_measure_event() - common function to add event log and extend PCR
> > + *
> > + * @dev:             TPM device
> > + * @pcr_index:               PCR index
> > + * @event_type:              type of event added
> > + * @size:            event size
> > + * @event:           event data
> > + *
> > + * Return:   status code
> > + */
> > +static efi_status_t EFIAPI
> > +tcg2_measure_event(struct udevice *dev, u32 pcr_index, u32 event_type,
> > +                u32 size, u8 event[])
> > +{
> > +     struct tpml_digest_values digest_list;
> > +     efi_status_t ret;
> > +
> > +     ret = tcg2_create_digest(event, size, &digest_list);
> > +     if (ret != EFI_SUCCESS)
> > +             goto out;
> > +
> > +     ret = tcg2_pcr_extend(dev, pcr_index, &digest_list);
> > +     if (ret != EFI_SUCCESS)
> > +             goto out;
> > +
> > +     ret = tcg2_agile_log_append(pcr_index, event_type, &digest_list,
> > +                                 size, event);
> > +
> > +out:
> > +     return ret;
> > +}
> > +
> >   /**
> >    * efi_append_scrtm_version - Append an S-CRTM EV_S_CRTM_VERSION event on the
> >    *                        eventlog and extend the PCRs
> > @@ -1294,6 +1340,88 @@ out:
> >       return ret;
> >   }
> >
> > +/**
> > + * tcg2_measure_variable() - add variable event log and extend PCR
> > + *
> > + * @dev:             TPM device
> > + * @pcr_index:               PCR index
> > + * @event_type:              type of event added
> > + * @var_name:                variable name
> > + * @guid:            guid
> > + * @data_size:               variable data size
> > + * @data:            variable data
> > + *
> > + * Return:   status code
> > + */
> > +static efi_status_t tcg2_measure_variable(struct udevice *dev, u32 pcr_index,
> > +                                       u32 event_type, u16 *var_name,
> > +                                       const efi_guid_t *guid,
> > +                                       efi_uintn_t data_size, u8 *data)
> > +{
> > +     u32 event_size;
> > +     efi_status_t ret;
> > +     struct efi_tcg2_uefi_variable_data *event;
> > +
> > +     event_size = sizeof(event->variable_name) +
> > +                  sizeof(event->unicode_name_length) +
> > +                  sizeof(event->variable_data_length) +
> > +                  (u16_strlen(var_name) * sizeof(*var_name)) + data_size;
>
> Please, replace by sizeof(*var_name) by sizeof(u16) to improve readability.
>
> > +     event = malloc(event_size);
> > +     if (!event)
> > +             return EFI_OUT_OF_RESOURCES;
> > +
> > +     guidcpy(&event->variable_name, guid);
> > +     event->unicode_name_length = u16_strlen(var_name);
> > +     event->variable_data_length = data_size;
> > +     memcpy(event->unicode_name, var_name,
> > +            (event->unicode_name_length * sizeof(*event->unicode_name)));
>
> sizeof(u16)
>
> > +     memcpy((u16 *)event->unicode_name + event->unicode_name_length,
> > +            (u8 *)data, data_size);
>
> memcpy() wants void *. data is already of type u8 *. This conversion can
> be removed.
>
> > +     ret = tcg2_measure_event(dev, pcr_index, event_type, event_size,
> > +                              (u8 *)event);
> > +     free(event);
> > +     return ret;
> > +}
> > +
> > +/**
> > + * tcg2_measure_secure_boot_variable() - measure secure boot variables
> > + *
> > + * @dev:     TPM device
> > + *
> > + * Return:   status code
> > + */
> > +static efi_status_t tcg2_measure_secure_boot_variable(struct udevice *dev)
> > +{
> > +     u8 *data;
> > +     efi_uintn_t data_size;
> > +     u32 count, i;
> > +     efi_status_t ret;
> > +
> > +     count = ARRAY_SIZE(secure_variables);
> > +     for (i = 0; i < count; i++) {
> > +             data = efi_get_var(secure_variables[i].name,
> > +                                secure_variables[i].guid,
> > +                                &data_size);
>
> You need to check if data==NULL. There is no guarantee that the
> variables exist.

Yes, I missed this checking.

Thanks,
Masahisa Kojima

>
> Best regards
>
> Heinrich
>
> > +
> > +             ret = tcg2_measure_variable(dev, 7,
> > +                                         EV_EFI_VARIABLE_DRIVER_CONFIG,
> > +                                         secure_variables[i].name,
> > +                                         secure_variables[i].guid,
> > +                                         data_size, (u8 *)data);
> > +             free(data);
> > +             if (ret != EFI_SUCCESS)
> > +                     goto error;
> > +     }
> > +
> > +     /*
> > +      * TODO: add DBT and DBR measurement support when u-boot supports
> > +      * these variables.
> > +      */
> > +
> > +error:
> > +     return ret;
> > +}
> > +
> >   /**
> >    * efi_tcg2_register() - register EFI_TCG2_PROTOCOL
> >    *
> > @@ -1328,6 +1456,13 @@ efi_status_t efi_tcg2_register(void)
> >               tcg2_uninit();
> >               goto fail;
> >       }
> > +
> > +     ret = tcg2_measure_secure_boot_variable(dev);
> > +     if (ret != EFI_SUCCESS) {
> > +             tcg2_uninit();
> > +             goto fail;
> > +     }
> > +
> >       return ret;
> >
> >   fail:
> >
>

Re: [PATCH 3/5] efi_loader: add boot variable measurement

[Masahisa Kojima]

On Fri, 9 Jul 2021 at 02:46, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 7/7/21 3:36 PM, Masahisa Kojima wrote:
> > TCG PC Client PFP spec requires to measure "Boot####"
> > and "BootOrder" variables, EV_SEPARATOR event prior
> > to the Ready to Boot invocation.
> > Since u-boot does not implement Ready to Boot event,
> > these measurements are performed when efi_start_image() is called.
> >
> > TCG spec also requires to measure "Calling EFI Application from
> > Boot Option" for each boot attempt, and "Returning from EFI
> > Application from Boot Option" if a boot device returns control
> > back to the Boot Manager.
> >
> > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > ---
> >   include/efi_loader.h          |   4 ++
> >   include/tpm-v2.h              |  18 ++++-
> >   lib/efi_loader/efi_boottime.c |  20 ++++++
> >   lib/efi_loader/efi_tcg2.c     | 123 ++++++++++++++++++++++++++++++++++
> >   4 files changed, 164 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > index 0a9c82a257..281ffff30f 100644
> > --- a/include/efi_loader.h
> > +++ b/include/efi_loader.h
> > @@ -407,6 +407,10 @@ efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size);
> >   efi_status_t efi_init_variables(void);
> >   /* Notify ExitBootServices() is called */
> >   void efi_variables_boot_exit_notify(void);
> > +/* Measure efi application invocation */
> > +efi_status_t EFIAPI efi_tcg2_measure_efi_app_invocation(void);
> > +/* Measure efi application exit */
> > +efi_status_t EFIAPI efi_tcg2_measure_efi_app_exit(void);
> >   /* Called by bootefi to initialize root node */
> >   efi_status_t efi_root_node_register(void);
> >   /* Called by bootefi to initialize runtime */
> > diff --git a/include/tpm-v2.h b/include/tpm-v2.h
> > index 3e48e35861..8a7b7f1874 100644
> > --- a/include/tpm-v2.h
> > +++ b/include/tpm-v2.h
> > @@ -73,7 +73,7 @@ struct udevice;
> >   /*
> >    * event types, cf.
> >    * "TCG PC Client Platform Firmware Profile Specification", Family "2.0"
> > - * rev 1.04, June 3, 2019
> > + * Level 00 Version 1.05 Revision 23, May 7, 2021
> >    */
> >   #define EV_EFI_EVENT_BASE                   ((u32)0x80000000)
> >   #define EV_EFI_VARIABLE_DRIVER_CONFIG               ((u32)0x80000001)
> > @@ -85,8 +85,24 @@ struct udevice;
> >   #define EV_EFI_ACTION                               ((u32)0x80000007)
> >   #define EV_EFI_PLATFORM_FIRMWARE_BLOB               ((u32)0x80000008)
> >   #define EV_EFI_HANDOFF_TABLES                       ((u32)0x80000009)
> > +#define EV_EFI_PLATFORM_FIRMWARE_BLOB2               ((u32)0x8000000A)
> > +#define EV_EFI_HANDOFF_TABLES2                       ((u32)0x8000000B)
> > +#define EV_EFI_VARIABLE_BOOT2                        ((u32)0x8000000C)
> >   #define EV_EFI_HCRTM_EVENT                  ((u32)0x80000010)
> >   #define EV_EFI_VARIABLE_AUTHORITY           ((u32)0x800000E0)
> > +#define EV_EFI_SPDM_FIRMWARE_BLOB            ((u32)0x800000E1)
> > +#define EV_EFI_SPDM_FIRMWARE_CONFIG          ((u32)0x800000E2)
> > +
> > +#define EFI_CALLING_EFI_APPLICATION         \
> > +     "Calling EFI Application from Boot Option"
> > +#define EFI_RETURNING_FROM_EFI_APPLICATION  \
> > +     "Returning from EFI Application from Boot Option"
> > +#define EFI_EXIT_BOOT_SERVICES_INVOCATION   \
> > +     "Exit Boot Services Invocation"
> > +#define EFI_EXIT_BOOT_SERVICES_FAILED       \
> > +     "Exit Boot Services Returned with Failure"
> > +#define EFI_EXIT_BOOT_SERVICES_SUCCEEDED    \
> > +     "Exit Boot Services Returned with Success"
>
> Which spec defines if the string in the event log shall be utf-8 or utf-16?

TCG PC Client PFP spec does not clearly define the character encoding.
In my understanding, the string derived from UEFI spec such as
UEFI variable name uses utf-16(CHAR16).
Other strings like "Calling EFI Application from Boot Option" defind in TCG PC
Client spec use 1 byte ASCII encoding.

EDK2 implementation also uses 1 byte ASCII encoding for these strings,
and tpm2-tools::tpm2_eventlog command can handles properly.

Thanks,
Masahisa Kojima

>
> Best regards
>
> Heinrich
>
> >
> >   /* TPMS_TAGGED_PROPERTY Structure */
> >   struct tpms_tagged_property {
> > diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> > index f6d5ba05e3..2914800c56 100644
> > --- a/lib/efi_loader/efi_boottime.c
> > +++ b/lib/efi_loader/efi_boottime.c
> > @@ -2993,6 +2993,16 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
> >       image_obj->exit_status = &exit_status;
> >       image_obj->exit_jmp = &exit_jmp;
> >
> > +     if (IS_ENABLED(CONFIG_EFI_TCG2_PROTOCOL)) {
> > +             if (image_obj->image_type == IMAGE_SUBSYSTEM_EFI_APPLICATION) {
> > +                     ret = efi_tcg2_measure_efi_app_invocation();
> > +                     if (ret != EFI_SUCCESS) {
> > +                             EFI_PRINT("tcg2 measurement fails(0x%lx)\n",
> > +                                       ret);
> > +                     }
> > +             }
> > +     }
> > +
> >       /* call the image! */
> >       if (setjmp(&exit_jmp)) {
> >               /*
> > @@ -3251,6 +3261,16 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
> >           exit_status != EFI_SUCCESS)
> >               efi_delete_image(image_obj, loaded_image_protocol);
> >
> > +     if (IS_ENABLED(CONFIG_EFI_TCG2_PROTOCOL)) {
> > +             if (image_obj->image_type == IMAGE_SUBSYSTEM_EFI_APPLICATION) {
> > +                     ret = efi_tcg2_measure_efi_app_exit();
> > +                     if (ret != EFI_SUCCESS) {
> > +                             EFI_PRINT("tcg2 measurement fails(0x%lx)\n",
> > +                                       ret);
> > +                     }
> > +             }
> > +     }
> > +
> >       /* Make sure entry/exit counts for EFI world cross-overs match */
> >       EFI_EXIT(exit_status);
> >
> > diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
> > index 2a248bd62a..6e903e3cb3 100644
> > --- a/lib/efi_loader/efi_tcg2.c
> > +++ b/lib/efi_loader/efi_tcg2.c
> > @@ -35,6 +35,7 @@ struct event_log_buffer {
> >   };
> >
> >   static struct event_log_buffer event_log;
> > +static bool tcg2_efi_app_invoked;
> >   /*
> >    * When requesting TPM2_CAP_TPM_PROPERTIES the value is on a standard offset.
> >    * Since the current tpm2_get_capability() response buffers starts at
> > @@ -1383,6 +1384,128 @@ static efi_status_t tcg2_measure_variable(struct udevice *dev, u32 pcr_index,
> >       return ret;
> >   }
> >
> > +/**
> > + * tcg2_measure_boot_variable() - measure boot variables
> > + *
> > + * @dev:     TPM device
> > + *
> > + * Return:   status code
> > + */
> > +static efi_status_t tcg2_measure_boot_variable(struct udevice *dev)
> > +{
> > +     u16 *boot_order;
> > +     u16 var_name[] = L"BootOrder";
> > +     u16 boot_name[] = L"Boot0000";
> > +     u16 hexmap[] = L"0123456789ABCDEF";
> > +     u8 *bootvar;
> > +     efi_uintn_t var_data_size;
> > +     u32 count, i;
> > +     efi_status_t ret;
> > +
> > +     boot_order = efi_get_var(var_name, &efi_global_variable_guid,
> > +                              &var_data_size);
> > +     if (!boot_order) {
> > +             log_info("BootOrder not defined\n");
> > +             ret = EFI_NOT_FOUND;
> > +             goto error;
> > +     }
> > +
> > +     ret = tcg2_measure_variable(dev, 1, EV_EFI_VARIABLE_BOOT2, var_name,
> > +                                 &efi_global_variable_guid, var_data_size,
> > +                                 (u8 *)boot_order);
> > +     if (ret != EFI_SUCCESS)
> > +             goto error;
> > +
> > +     count = var_data_size / sizeof(*boot_order);
> > +     for (i = 0; i < count; i++) {
> > +             boot_name[4] = hexmap[(boot_order[i] & 0xf000) >> 12];
> > +             boot_name[5] = hexmap[(boot_order[i] & 0x0f00) >> 8];
> > +             boot_name[6] = hexmap[(boot_order[i] & 0x00f0) >> 4];
> > +             boot_name[7] = hexmap[(boot_order[i] & 0x000f)];
> > +
> > +             bootvar = efi_get_var(boot_name, &efi_global_variable_guid,
> > +                                   &var_data_size);
> > +
> > +             if (!bootvar) {
> > +                     log_info("%ls not found\n", boot_name);
> > +                     continue;
> > +             }
> > +
> > +             ret = tcg2_measure_variable(dev, 1, EV_EFI_VARIABLE_BOOT2,
> > +                                         boot_name,
> > +                                         &efi_global_variable_guid,
> > +                                         var_data_size, bootvar);
> > +             free(bootvar);
> > +             if (ret != EFI_SUCCESS)
> > +                     goto error;
> > +     }
> > +
> > +error:
> > +     free(boot_order);
> > +     return ret;
> > +}
> > +
> > +/**
> > + * efi_tcg2_measure_efi_app_invocation() - measure efi app invocation
> > + *
> > + * Return:   status code
> > + */
> > +efi_status_t EFIAPI efi_tcg2_measure_efi_app_invocation(void)
> > +{
> > +     efi_status_t ret;
> > +     u32 pcr_index;
> > +     struct udevice *dev;
> > +     u32 event = 0;
> > +
> > +     if (tcg2_efi_app_invoked)
> > +             return EFI_SUCCESS;
> > +
> > +     ret = platform_get_tpm2_device(&dev);
> > +     if (ret != EFI_SUCCESS)
> > +             return ret;
> > +
> > +     ret = tcg2_measure_boot_variable(dev);
> > +     if (ret != EFI_SUCCESS)
> > +             goto out;
> > +
> > +     ret = tcg2_measure_event(dev, 4, EV_EFI_ACTION,
> > +                              strlen(EFI_CALLING_EFI_APPLICATION),
> > +                              (u8 *)EFI_CALLING_EFI_APPLICATION);
> > +     if (ret != EFI_SUCCESS)
> > +             goto out;
> > +
> > +     for (pcr_index = 0; pcr_index <= 7; pcr_index++) {
> > +             ret = tcg2_measure_event(dev, pcr_index, EV_SEPARATOR,
> > +                                      sizeof(event), (u8 *)&event);
> > +             if (ret != EFI_SUCCESS)
> > +                     goto out;
> > +     }
> > +
> > +     tcg2_efi_app_invoked = true;
> > +out:
> > +     return ret;
> > +}
> > +
> > +/**
> > + * efi_tcg2_measure_efi_app_exit() - measure efi app exit
> > + *
> > + * Return:   status code
> > + */
> > +efi_status_t EFIAPI efi_tcg2_measure_efi_app_exit(void)
> > +{
> > +     efi_status_t ret;
> > +     struct udevice *dev;
> > +
> > +     ret = platform_get_tpm2_device(&dev);
> > +     if (ret != EFI_SUCCESS)
> > +             return ret;
> > +
> > +     ret = tcg2_measure_event(dev, 4, EV_EFI_ACTION,
> > +                              strlen(EFI_RETURNING_FROM_EFI_APPLICATION),
> > +                              (u8 *)EFI_RETURNING_FROM_EFI_APPLICATION);
> > +     return ret;
> > +}
> > +
> >   /**
> >    * tcg2_measure_secure_boot_variable() - measure secure boot variables
> >    *
> >
>

Re: [PATCH 4/5] efi_loader: add ExitBootServices() measurement

[Masahisa Kojima]

On Fri, 9 Jul 2021 at 02:40, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 7/7/21 3:36 PM, Masahisa Kojima wrote:
> > TCG PC Client PFP spec requires to measure
> > "Exit Boot Services Invocation" if ExitBootServices() is invoked.
> > Depending upon the return code from the ExitBootServices() call,
> > "Exit Boot Services Returned with Success" or "Exit Boot Services
> > Returned with Failure" is also measured.
> >
> > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > ---
> >   include/efi_loader.h          |  1 +
> >   lib/efi_loader/efi_boottime.c |  5 +++
> >   lib/efi_loader/efi_tcg2.c     | 70 +++++++++++++++++++++++++++++++++++
> >   3 files changed, 76 insertions(+)
> >
> > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > index 281ffff30f..e9bd1aac08 100644
> > --- a/include/efi_loader.h
> > +++ b/include/efi_loader.h
> > @@ -407,6 +407,7 @@ efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size);
> >   efi_status_t efi_init_variables(void);
> >   /* Notify ExitBootServices() is called */
> >   void efi_variables_boot_exit_notify(void);
> > +efi_status_t efi_tcg2_notify_exit_boot_services_failed(void);
> >   /* Measure efi application invocation */
> >   efi_status_t EFIAPI efi_tcg2_measure_efi_app_invocation(void);
> >   /* Measure efi application exit */
> > diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> > index 2914800c56..6e07ef65bc 100644
> > --- a/lib/efi_loader/efi_boottime.c
> > +++ b/lib/efi_loader/efi_boottime.c
> > @@ -2181,6 +2181,11 @@ static efi_status_t EFIAPI efi_exit_boot_services(efi_handle_t image_handle,
> >       efi_set_watchdog(0);
> >       WATCHDOG_RESET();
> >   out:
> > +     if (ret != EFI_SUCCESS) {
> > +             if (IS_ENABLED(CONFIG_EFI_TCG2_PROTOCOL))
> > +                     efi_tcg2_notify_exit_boot_services_failed();
> > +     }
> > +
> >       return EFI_EXIT(ret);
> >   }
> >
> > diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
> > index 6e903e3cb3..823abd8217 100644
> > --- a/lib/efi_loader/efi_tcg2.c
> > +++ b/lib/efi_loader/efi_tcg2.c
> > @@ -1506,6 +1506,67 @@ efi_status_t EFIAPI efi_tcg2_measure_efi_app_exit(void)
> >       return ret;
> >   }
> >
> > +/**
> > + * efi_tcg2_notify_exit_boot_services() - ExitBootService callback
> > + *
> > + * @event:   callback event
> > + * @context: callback context
> > + */
> > +static void EFIAPI
> > +efi_tcg2_notify_exit_boot_services(struct efi_event *event, void *context)
> > +{
> > +     efi_status_t ret;
> > +     struct udevice *dev;
> > +
> > +     EFI_ENTRY("%p, %p", event, context);
> > +
> > +     ret = platform_get_tpm2_device(&dev);
> > +     if (ret != EFI_SUCCESS)
> > +             goto out;
> > +
> > +     ret = tcg2_measure_event(dev, 5, EV_EFI_ACTION,
> > +                              strlen(EFI_EXIT_BOOT_SERVICES_INVOCATION),
> > +                              (u8 *)EFI_EXIT_BOOT_SERVICES_INVOCATION);
> > +     if (ret != EFI_SUCCESS)
> > +             goto out;
> > +
> > +     ret = tcg2_measure_event(dev, 5, EV_EFI_ACTION,
> > +                              strlen(EFI_EXIT_BOOT_SERVICES_SUCCEEDED),
> > +                              (u8 *)EFI_EXIT_BOOT_SERVICES_SUCCEEDED);
> > +
> > +out:
> > +     EFI_EXIT(ret);
> > +}
> > +
> > +/**
> > + * efi_tcg2_notify_exit_boot_services_failed()
> > + *  - notify ExitBootServices() is failed
> > + *
> > + * Return:   status code
> > + */
> > +efi_status_t efi_tcg2_notify_exit_boot_services_failed(void)
> > +{
> > +     struct udevice *dev;
> > +     efi_status_t ret;
> > +
> > +     ret = platform_get_tpm2_device(&dev);
> > +     if (ret != EFI_SUCCESS)
> > +             goto out;
> > +
> > +     ret = tcg2_measure_event(dev, 5, EV_EFI_ACTION,
> > +                              sizeof(EFI_EXIT_BOOT_SERVICES_INVOCATION),
> > +                              (u8 *)EFI_EXIT_BOOT_SERVICES_INVOCATION);
> > +     if (ret != EFI_SUCCESS)
> > +             goto out;
> > +
> > +     ret = tcg2_measure_event(dev, 5, EV_EFI_ACTION,
> > +                              sizeof(EFI_EXIT_BOOT_SERVICES_FAILED),
> > +                              (u8 *)EFI_EXIT_BOOT_SERVICES_FAILED);
> > +
> > +out:
> > +     return ret;
> > +}
>
> We must keep out code small. Please, carve out a function to which pass
> a the event type and an u8 string and use it where applicable.

I'm not sure I understand your comment correctly.
pcr_index is also required for carved out function because pcr_index
to extend PCR is different for each EV_EFI_ACTION event.
So the interface of carved out function is almost same as tcg2_measure_event().

I meant to create tcg2_measure_event() as a common sub-function.
to reduce the number of code.

Thanks,
Masahisa Kojima

>
> Best regards
>
> Heinrich
>
> > +
> >   /**
> >    * tcg2_measure_secure_boot_variable() - measure secure boot variables
> >    *
> > @@ -1556,6 +1617,7 @@ efi_status_t efi_tcg2_register(void)
> >   {
> >       efi_status_t ret = EFI_SUCCESS;
> >       struct udevice *dev;
> > +     struct efi_event *event;
> >
> >       ret = platform_get_tpm2_device(&dev);
> >       if (ret != EFI_SUCCESS) {
> > @@ -1580,6 +1642,14 @@ efi_status_t efi_tcg2_register(void)
> >               goto fail;
> >       }
> >
> > +     ret = efi_create_event(EVT_SIGNAL_EXIT_BOOT_SERVICES, TPL_CALLBACK,
> > +                            efi_tcg2_notify_exit_boot_services, NULL,
> > +                            NULL, &event);
> > +     if (ret != EFI_SUCCESS) {
> > +             tcg2_uninit();
> > +             goto fail;
> > +     }
> > +
> >       ret = tcg2_measure_secure_boot_variable(dev);
> >       if (ret != EFI_SUCCESS) {
> >               tcg2_uninit();
> >
>

Re: [PATCH 1/5] efi_loader: increase eventlog buffer size

[Simon Glass]

Hi Masahisa,

On Wed, 7 Jul 2021 at 20:21, Masahisa Kojima <masahisa.kojima@linaro.org> wrote:
>
> On Wed, 7 Jul 2021 at 22:47, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >
> >
> >
> > On 7/7/21 3:36 PM, Masahisa Kojima wrote:
> > > This is a preperation to add eventlog support
> > > described in TCG PC Client PFP spec.
> > >
> > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > > ---
> > >   lib/efi_loader/Kconfig | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > > index b2ab48a048..a87bf3cc98 100644
> > > --- a/lib/efi_loader/Kconfig
> > > +++ b/lib/efi_loader/Kconfig
> > > @@ -327,7 +327,7 @@ config EFI_TCG2_PROTOCOL
> > >   config EFI_TCG2_PROTOCOL_EVENTLOG_SIZE
> > >       int "EFI_TCG2_PROTOCOL EventLog size"
> > >       depends on EFI_TCG2_PROTOCOL
> > > -     default 4096
> > > +     default 16384
> >
> > I found this text in EDK II:
> >
> > Minimum length(in bytes) of the system preboot TCG event log area(LAML)
> > -----------------------------------------------------------------------
> >
> > For PC Client Implementation spec up to and including 1.2 the minimum
> > log size is 64KB. (SecurityPkg/SecurityPkg.dec)
>
> Thank you for your feedback.
> I have not checked this.
> TCG spec also says "The Log Area Minimum Length for the TCG event log
> MUST be at least 64KB." in ACPI chapter.
> I will update to set 64KB as default.
>

Is this the same as the BLOBLISTT_TPM2_TCG_LOG thing? If so, can we
put this in the bloblist? We want to avoid adding code in EFI which is
in U-Boot.


- Simon

> Thanks,
> Masahisa Kojima
>
> >
> > Why should ours be smaller?
> >
> > Best regards
> >
> > Heinrich
> >
> > >       help
> > >               Define the size of the EventLog for EFI_TCG2_PROTOCOL. Note that
> > >               this is going to be allocated twice. One for the eventlog it self
> > >

Re: [PATCH 1/5] efi_loader: increase eventlog buffer size

[Masahisa Kojima]

Hi Simon,

On Sun, 11 Jul 2021 at 09:01, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Masahisa,
>
> On Wed, 7 Jul 2021 at 20:21, Masahisa Kojima <masahisa.kojima@linaro.org> wrote:
> >
> > On Wed, 7 Jul 2021 at 22:47, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > >
> > >
> > >
> > > On 7/7/21 3:36 PM, Masahisa Kojima wrote:
> > > > This is a preperation to add eventlog support
> > > > described in TCG PC Client PFP spec.
> > > >
> > > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > > > ---
> > > >   lib/efi_loader/Kconfig | 2 +-
> > > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > > > index b2ab48a048..a87bf3cc98 100644
> > > > --- a/lib/efi_loader/Kconfig
> > > > +++ b/lib/efi_loader/Kconfig
> > > > @@ -327,7 +327,7 @@ config EFI_TCG2_PROTOCOL
> > > >   config EFI_TCG2_PROTOCOL_EVENTLOG_SIZE
> > > >       int "EFI_TCG2_PROTOCOL EventLog size"
> > > >       depends on EFI_TCG2_PROTOCOL
> > > > -     default 4096
> > > > +     default 16384
> > >
> > > I found this text in EDK II:
> > >
> > > Minimum length(in bytes) of the system preboot TCG event log area(LAML)
> > > -----------------------------------------------------------------------
> > >
> > > For PC Client Implementation spec up to and including 1.2 the minimum
> > > log size is 64KB. (SecurityPkg/SecurityPkg.dec)
> >
> > Thank you for your feedback.
> > I have not checked this.
> > TCG spec also says "The Log Area Minimum Length for the TCG event log
> > MUST be at least 64KB." in ACPI chapter.
> > I will update to set 64KB as default.
> >
>
> Is this the same as the BLOBLISTT_TPM2_TCG_LOG thing? If so, can we
> put this in the bloblist? We want to avoid adding code in EFI which is
> in U-Boot.

I think bloblist is used for data passing from SPL/TPL to u-boot.
Is your comment saying that the eventlog generated
in u-boot(done in efi_tcg2.c with this patch series) should be appended
into the buffer pointed by BLOBLISTT_TPM2_TCG_LOG blob?

Thanks,
Masahisa Kojima

>
>
> - Simon
>
> > Thanks,
> > Masahisa Kojima
> >
> > >
> > > Why should ours be smaller?
> > >
> > > Best regards
> > >
> > > Heinrich
> > >
> > > >       help
> > > >               Define the size of the EventLog for EFI_TCG2_PROTOCOL. Note that
> > > >               this is going to be allocated twice. One for the eventlog it self
> > > >

Re: [PATCH 1/5] efi_loader: increase eventlog buffer size

[Ilias Apalodimas]

On Mon, 12 Jul 2021 at 11:40, Masahisa Kojima
<masahisa.kojima@linaro.org> wrote:
>
> Hi Simon,
>
> On Sun, 11 Jul 2021 at 09:01, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Masahisa,
> >
> > On Wed, 7 Jul 2021 at 20:21, Masahisa Kojima <masahisa.kojima@linaro.org> wrote:
> > >
> > > On Wed, 7 Jul 2021 at 22:47, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > >
> > > >
> > > >
> > > > On 7/7/21 3:36 PM, Masahisa Kojima wrote:
> > > > > This is a preperation to add eventlog support
> > > > > described in TCG PC Client PFP spec.
> > > > >
> > > > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > > > > ---
> > > > >   lib/efi_loader/Kconfig | 2 +-
> > > > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > > > > index b2ab48a048..a87bf3cc98 100644
> > > > > --- a/lib/efi_loader/Kconfig
> > > > > +++ b/lib/efi_loader/Kconfig
> > > > > @@ -327,7 +327,7 @@ config EFI_TCG2_PROTOCOL
> > > > >   config EFI_TCG2_PROTOCOL_EVENTLOG_SIZE
> > > > >       int "EFI_TCG2_PROTOCOL EventLog size"
> > > > >       depends on EFI_TCG2_PROTOCOL
> > > > > -     default 4096
> > > > > +     default 16384
> > > >
> > > > I found this text in EDK II:
> > > >
> > > > Minimum length(in bytes) of the system preboot TCG event log area(LAML)
> > > > -----------------------------------------------------------------------
> > > >
> > > > For PC Client Implementation spec up to and including 1.2 the minimum
> > > > log size is 64KB. (SecurityPkg/SecurityPkg.dec)
> > >
> > > Thank you for your feedback.
> > > I have not checked this.
> > > TCG spec also says "The Log Area Minimum Length for the TCG event log
> > > MUST be at least 64KB." in ACPI chapter.
> > > I will update to set 64KB as default.
> > >
> >
> > Is this the same as the BLOBLISTT_TPM2_TCG_LOG thing? If so, can we
> > put this in the bloblist? We want to avoid adding code in EFI which is
> > in U-Boot.
>
> I think bloblist is used for data passing from SPL/TPL to u-boot.
> Is your comment saying that the eventlog generated
> in u-boot(done in efi_tcg2.c with this patch series) should be appended
> into the buffer pointed by BLOBLISTT_TPM2_TCG_LOG blob?
>

Even in that case the eventlog can't be appended.  The TCG eventlog
hould be copied into EFI memory, since the kernel expects to find it
there.
What we could do is copy the contents of that buffer to the eventlog.
Depending on what that buffer already has (e.g the starting header of
the eventlog), we might need to strip it from the efi_tcg.c code.

Thanks
/Ilias
> Thanks,
> Masahisa Kojima
>
> >
> >
> > - Simon
> >
> > > Thanks,
> > > Masahisa Kojima
> > >
> > > >
> > > > Why should ours be smaller?
> > > >
> > > > Best regards
> > > >
> > > > Heinrich
> > > >
> > > > >       help
> > > > >               Define the size of the EventLog for EFI_TCG2_PROTOCOL. Note that
> > > > >               this is going to be allocated twice. One for the eventlog it self
> > > > >

Re: [PATCH 3/5] efi_loader: add boot variable measurement

[Masahisa Kojima]

Hi Heinrich,

> > > TCG spec also requires to measure "Calling EFI Application from
> > > Boot Option" for each boot attempt, and "Returning from EFI
> > > Application from Boot Option" if a boot device returns control
> > > back to the Boot Manager.

I would like to hear your opinion regarding
"Calling EFI Application from Boot Option" measurement.

My current(v1 patch series) implementation considers
both "bootefi bootmgr" and "bootefi $image_addr" cases,
so I do this "Calling EFI Application from Boot Option" measurement
at efi_boottime.c::efi_start_image().
Do I need to implement only the case UEFI application boot from bootmgr?
If yes, I will move the timing of this measurement at
efi_bootmgr.c::efi_bootmgr_load().

As a reference, in edk2, this measurement is performed in
ready_to_boot event handler, ready_to_boot handler is called
upon the user selects the boot option in boot manager.

What do you think?

Thanks,
Masahisa Kojima





On Fri, 9 Jul 2021 at 11:44, Masahisa Kojima <masahisa.kojima@linaro.org> wrote:
>
> On Fri, 9 Jul 2021 at 02:46, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >
> > On 7/7/21 3:36 PM, Masahisa Kojima wrote:
> > > TCG PC Client PFP spec requires to measure "Boot####"
> > > and "BootOrder" variables, EV_SEPARATOR event prior
> > > to the Ready to Boot invocation.
> > > Since u-boot does not implement Ready to Boot event,
> > > these measurements are performed when efi_start_image() is called.
> > >
> > > TCG spec also requires to measure "Calling EFI Application from
> > > Boot Option" for each boot attempt, and "Returning from EFI
> > > Application from Boot Option" if a boot device returns control
> > > back to the Boot Manager.
> > >
> > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > > ---
> > >   include/efi_loader.h          |   4 ++
> > >   include/tpm-v2.h              |  18 ++++-
> > >   lib/efi_loader/efi_boottime.c |  20 ++++++
> > >   lib/efi_loader/efi_tcg2.c     | 123 ++++++++++++++++++++++++++++++++++
> > >   4 files changed, 164 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > > index 0a9c82a257..281ffff30f 100644
> > > --- a/include/efi_loader.h
> > > +++ b/include/efi_loader.h
> > > @@ -407,6 +407,10 @@ efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size);
> > >   efi_status_t efi_init_variables(void);
> > >   /* Notify ExitBootServices() is called */
> > >   void efi_variables_boot_exit_notify(void);
> > > +/* Measure efi application invocation */
> > > +efi_status_t EFIAPI efi_tcg2_measure_efi_app_invocation(void);
> > > +/* Measure efi application exit */
> > > +efi_status_t EFIAPI efi_tcg2_measure_efi_app_exit(void);
> > >   /* Called by bootefi to initialize root node */
> > >   efi_status_t efi_root_node_register(void);
> > >   /* Called by bootefi to initialize runtime */
> > > diff --git a/include/tpm-v2.h b/include/tpm-v2.h
> > > index 3e48e35861..8a7b7f1874 100644
> > > --- a/include/tpm-v2.h
> > > +++ b/include/tpm-v2.h
> > > @@ -73,7 +73,7 @@ struct udevice;
> > >   /*
> > >    * event types, cf.
> > >    * "TCG PC Client Platform Firmware Profile Specification", Family "2.0"
> > > - * rev 1.04, June 3, 2019
> > > + * Level 00 Version 1.05 Revision 23, May 7, 2021
> > >    */
> > >   #define EV_EFI_EVENT_BASE                   ((u32)0x80000000)
> > >   #define EV_EFI_VARIABLE_DRIVER_CONFIG               ((u32)0x80000001)
> > > @@ -85,8 +85,24 @@ struct udevice;
> > >   #define EV_EFI_ACTION                               ((u32)0x80000007)
> > >   #define EV_EFI_PLATFORM_FIRMWARE_BLOB               ((u32)0x80000008)
> > >   #define EV_EFI_HANDOFF_TABLES                       ((u32)0x80000009)
> > > +#define EV_EFI_PLATFORM_FIRMWARE_BLOB2               ((u32)0x8000000A)
> > > +#define EV_EFI_HANDOFF_TABLES2                       ((u32)0x8000000B)
> > > +#define EV_EFI_VARIABLE_BOOT2                        ((u32)0x8000000C)
> > >   #define EV_EFI_HCRTM_EVENT                  ((u32)0x80000010)
> > >   #define EV_EFI_VARIABLE_AUTHORITY           ((u32)0x800000E0)
> > > +#define EV_EFI_SPDM_FIRMWARE_BLOB            ((u32)0x800000E1)
> > > +#define EV_EFI_SPDM_FIRMWARE_CONFIG          ((u32)0x800000E2)
> > > +
> > > +#define EFI_CALLING_EFI_APPLICATION         \
> > > +     "Calling EFI Application from Boot Option"
> > > +#define EFI_RETURNING_FROM_EFI_APPLICATION  \
> > > +     "Returning from EFI Application from Boot Option"
> > > +#define EFI_EXIT_BOOT_SERVICES_INVOCATION   \
> > > +     "Exit Boot Services Invocation"
> > > +#define EFI_EXIT_BOOT_SERVICES_FAILED       \
> > > +     "Exit Boot Services Returned with Failure"
> > > +#define EFI_EXIT_BOOT_SERVICES_SUCCEEDED    \
> > > +     "Exit Boot Services Returned with Success"
> >
> > Which spec defines if the string in the event log shall be utf-8 or utf-16?
>
> TCG PC Client PFP spec does not clearly define the character encoding.
> In my understanding, the string derived from UEFI spec such as
> UEFI variable name uses utf-16(CHAR16).
> Other strings like "Calling EFI Application from Boot Option" defind in TCG PC
> Client spec use 1 byte ASCII encoding.
>
> EDK2 implementation also uses 1 byte ASCII encoding for these strings,
> and tpm2-tools::tpm2_eventlog command can handles properly.
>
> Thanks,
> Masahisa Kojima
>
> >
> > Best regards
> >
> > Heinrich
> >
> > >
> > >   /* TPMS_TAGGED_PROPERTY Structure */
> > >   struct tpms_tagged_property {
> > > diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> > > index f6d5ba05e3..2914800c56 100644
> > > --- a/lib/efi_loader/efi_boottime.c
> > > +++ b/lib/efi_loader/efi_boottime.c
> > > @@ -2993,6 +2993,16 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
> > >       image_obj->exit_status = &exit_status;
> > >       image_obj->exit_jmp = &exit_jmp;
> > >
> > > +     if (IS_ENABLED(CONFIG_EFI_TCG2_PROTOCOL)) {
> > > +             if (image_obj->image_type == IMAGE_SUBSYSTEM_EFI_APPLICATION) {
> > > +                     ret = efi_tcg2_measure_efi_app_invocation();
> > > +                     if (ret != EFI_SUCCESS) {
> > > +                             EFI_PRINT("tcg2 measurement fails(0x%lx)\n",
> > > +                                       ret);
> > > +                     }
> > > +             }
> > > +     }
> > > +
> > >       /* call the image! */
> > >       if (setjmp(&exit_jmp)) {
> > >               /*
> > > @@ -3251,6 +3261,16 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
> > >           exit_status != EFI_SUCCESS)
> > >               efi_delete_image(image_obj, loaded_image_protocol);
> > >
> > > +     if (IS_ENABLED(CONFIG_EFI_TCG2_PROTOCOL)) {
> > > +             if (image_obj->image_type == IMAGE_SUBSYSTEM_EFI_APPLICATION) {
> > > +                     ret = efi_tcg2_measure_efi_app_exit();
> > > +                     if (ret != EFI_SUCCESS) {
> > > +                             EFI_PRINT("tcg2 measurement fails(0x%lx)\n",
> > > +                                       ret);
> > > +                     }
> > > +             }
> > > +     }
> > > +
> > >       /* Make sure entry/exit counts for EFI world cross-overs match */
> > >       EFI_EXIT(exit_status);
> > >
> > > diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
> > > index 2a248bd62a..6e903e3cb3 100644
> > > --- a/lib/efi_loader/efi_tcg2.c
> > > +++ b/lib/efi_loader/efi_tcg2.c
> > > @@ -35,6 +35,7 @@ struct event_log_buffer {
> > >   };
> > >
> > >   static struct event_log_buffer event_log;
> > > +static bool tcg2_efi_app_invoked;
> > >   /*
> > >    * When requesting TPM2_CAP_TPM_PROPERTIES the value is on a standard offset.
> > >    * Since the current tpm2_get_capability() response buffers starts at
> > > @@ -1383,6 +1384,128 @@ static efi_status_t tcg2_measure_variable(struct udevice *dev, u32 pcr_index,
> > >       return ret;
> > >   }
> > >
> > > +/**
> > > + * tcg2_measure_boot_variable() - measure boot variables
> > > + *
> > > + * @dev:     TPM device
> > > + *
> > > + * Return:   status code
> > > + */
> > > +static efi_status_t tcg2_measure_boot_variable(struct udevice *dev)
> > > +{
> > > +     u16 *boot_order;
> > > +     u16 var_name[] = L"BootOrder";
> > > +     u16 boot_name[] = L"Boot0000";
> > > +     u16 hexmap[] = L"0123456789ABCDEF";
> > > +     u8 *bootvar;
> > > +     efi_uintn_t var_data_size;
> > > +     u32 count, i;
> > > +     efi_status_t ret;
> > > +
> > > +     boot_order = efi_get_var(var_name, &efi_global_variable_guid,
> > > +                              &var_data_size);
> > > +     if (!boot_order) {
> > > +             log_info("BootOrder not defined\n");
> > > +             ret = EFI_NOT_FOUND;
> > > +             goto error;
> > > +     }
> > > +
> > > +     ret = tcg2_measure_variable(dev, 1, EV_EFI_VARIABLE_BOOT2, var_name,
> > > +                                 &efi_global_variable_guid, var_data_size,
> > > +                                 (u8 *)boot_order);
> > > +     if (ret != EFI_SUCCESS)
> > > +             goto error;
> > > +
> > > +     count = var_data_size / sizeof(*boot_order);
> > > +     for (i = 0; i < count; i++) {
> > > +             boot_name[4] = hexmap[(boot_order[i] & 0xf000) >> 12];
> > > +             boot_name[5] = hexmap[(boot_order[i] & 0x0f00) >> 8];
> > > +             boot_name[6] = hexmap[(boot_order[i] & 0x00f0) >> 4];
> > > +             boot_name[7] = hexmap[(boot_order[i] & 0x000f)];
> > > +
> > > +             bootvar = efi_get_var(boot_name, &efi_global_variable_guid,
> > > +                                   &var_data_size);
> > > +
> > > +             if (!bootvar) {
> > > +                     log_info("%ls not found\n", boot_name);
> > > +                     continue;
> > > +             }
> > > +
> > > +             ret = tcg2_measure_variable(dev, 1, EV_EFI_VARIABLE_BOOT2,
> > > +                                         boot_name,
> > > +                                         &efi_global_variable_guid,
> > > +                                         var_data_size, bootvar);
> > > +             free(bootvar);
> > > +             if (ret != EFI_SUCCESS)
> > > +                     goto error;
> > > +     }
> > > +
> > > +error:
> > > +     free(boot_order);
> > > +     return ret;
> > > +}
> > > +
> > > +/**
> > > + * efi_tcg2_measure_efi_app_invocation() - measure efi app invocation
> > > + *
> > > + * Return:   status code
> > > + */
> > > +efi_status_t EFIAPI efi_tcg2_measure_efi_app_invocation(void)
> > > +{
> > > +     efi_status_t ret;
> > > +     u32 pcr_index;
> > > +     struct udevice *dev;
> > > +     u32 event = 0;
> > > +
> > > +     if (tcg2_efi_app_invoked)
> > > +             return EFI_SUCCESS;
> > > +
> > > +     ret = platform_get_tpm2_device(&dev);
> > > +     if (ret != EFI_SUCCESS)
> > > +             return ret;
> > > +
> > > +     ret = tcg2_measure_boot_variable(dev);
> > > +     if (ret != EFI_SUCCESS)
> > > +             goto out;
> > > +
> > > +     ret = tcg2_measure_event(dev, 4, EV_EFI_ACTION,
> > > +                              strlen(EFI_CALLING_EFI_APPLICATION),
> > > +                              (u8 *)EFI_CALLING_EFI_APPLICATION);
> > > +     if (ret != EFI_SUCCESS)
> > > +             goto out;
> > > +
> > > +     for (pcr_index = 0; pcr_index <= 7; pcr_index++) {
> > > +             ret = tcg2_measure_event(dev, pcr_index, EV_SEPARATOR,
> > > +                                      sizeof(event), (u8 *)&event);
> > > +             if (ret != EFI_SUCCESS)
> > > +                     goto out;
> > > +     }
> > > +
> > > +     tcg2_efi_app_invoked = true;
> > > +out:
> > > +     return ret;
> > > +}
> > > +
> > > +/**
> > > + * efi_tcg2_measure_efi_app_exit() - measure efi app exit
> > > + *
> > > + * Return:   status code
> > > + */
> > > +efi_status_t EFIAPI efi_tcg2_measure_efi_app_exit(void)
> > > +{
> > > +     efi_status_t ret;
> > > +     struct udevice *dev;
> > > +
> > > +     ret = platform_get_tpm2_device(&dev);
> > > +     if (ret != EFI_SUCCESS)
> > > +             return ret;
> > > +
> > > +     ret = tcg2_measure_event(dev, 4, EV_EFI_ACTION,
> > > +                              strlen(EFI_RETURNING_FROM_EFI_APPLICATION),
> > > +                              (u8 *)EFI_RETURNING_FROM_EFI_APPLICATION);
> > > +     return ret;
> > > +}
> > > +
> > >   /**
> > >    * tcg2_measure_secure_boot_variable() - measure secure boot variables
> > >    *
> > >
> >

Re: [PATCH 3/5] efi_loader: add boot variable measurement

[Heinrich Schuchardt]

On 13.07.21 10:31, Masahisa Kojima wrote:
> Hi Heinrich,
>
>>>> TCG spec also requires to measure "Calling EFI Application from
>>>> Boot Option" for each boot attempt, and "Returning from EFI
>>>> Application from Boot Option" if a boot device returns control
>>>> back to the Boot Manager.
>
> I would like to hear your opinion regarding
> "Calling EFI Application from Boot Option" measurement.
>
> My current(v1 patch series) implementation considers
> both "bootefi bootmgr" and "bootefi $image_addr" cases,
> so I do this "Calling EFI Application from Boot Option" measurement
> at efi_boottime.c::efi_start_image().
> Do I need to implement only the case UEFI application boot from bootmgr?
> If yes, I will move the timing of this measurement at
> efi_bootmgr.c::efi_bootmgr_load().
>
> As a reference, in edk2, this measurement is performed in
> ready_to_boot event handler, ready_to_boot handler is called
> upon the user selects the boot option in boot manager.

When booting you can call

    bootefi $driver1
    booefii $driver2
    bootefi bootmgr

in sequence.

Any of the binaries can call LoadImage(), StartImage() multiple times to
execute further images. E.g. I am loading iPXE. By default it loads GRUB
from an iSCSI drive but I can choose in the menu or the iPXE console to
invoke another UEFI binary.

I suggest to measure any image no matter how it is invoked. The
measurement must depend on the sequence of invocation.

Best regards

Heinrich

>
> What do you think?
>
> Thanks,
> Masahisa Kojima
>
>
>
>
>
> On Fri, 9 Jul 2021 at 11:44, Masahisa Kojima <masahisa.kojima@linaro.org> wrote:
>>
>> On Fri, 9 Jul 2021 at 02:46, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>
>>> On 7/7/21 3:36 PM, Masahisa Kojima wrote:
>>>> TCG PC Client PFP spec requires to measure "Boot####"
>>>> and "BootOrder" variables, EV_SEPARATOR event prior
>>>> to the Ready to Boot invocation.
>>>> Since u-boot does not implement Ready to Boot event,
>>>> these measurements are performed when efi_start_image() is called.
>>>>
>>>> TCG spec also requires to measure "Calling EFI Application from
>>>> Boot Option" for each boot attempt, and "Returning from EFI
>>>> Application from Boot Option" if a boot device returns control
>>>> back to the Boot Manager.
>>>>
>>>> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
>>>> ---
>>>>    include/efi_loader.h          |   4 ++
>>>>    include/tpm-v2.h              |  18 ++++-
>>>>    lib/efi_loader/efi_boottime.c |  20 ++++++
>>>>    lib/efi_loader/efi_tcg2.c     | 123 ++++++++++++++++++++++++++++++++++
>>>>    4 files changed, 164 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>>>> index 0a9c82a257..281ffff30f 100644
>>>> --- a/include/efi_loader.h
>>>> +++ b/include/efi_loader.h
>>>> @@ -407,6 +407,10 @@ efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size);
>>>>    efi_status_t efi_init_variables(void);
>>>>    /* Notify ExitBootServices() is called */
>>>>    void efi_variables_boot_exit_notify(void);
>>>> +/* Measure efi application invocation */
>>>> +efi_status_t EFIAPI efi_tcg2_measure_efi_app_invocation(void);
>>>> +/* Measure efi application exit */
>>>> +efi_status_t EFIAPI efi_tcg2_measure_efi_app_exit(void);
>>>>    /* Called by bootefi to initialize root node */
>>>>    efi_status_t efi_root_node_register(void);
>>>>    /* Called by bootefi to initialize runtime */
>>>> diff --git a/include/tpm-v2.h b/include/tpm-v2.h
>>>> index 3e48e35861..8a7b7f1874 100644
>>>> --- a/include/tpm-v2.h
>>>> +++ b/include/tpm-v2.h
>>>> @@ -73,7 +73,7 @@ struct udevice;
>>>>    /*
>>>>     * event types, cf.
>>>>     * "TCG PC Client Platform Firmware Profile Specification", Family "2.0"
>>>> - * rev 1.04, June 3, 2019
>>>> + * Level 00 Version 1.05 Revision 23, May 7, 2021
>>>>     */
>>>>    #define EV_EFI_EVENT_BASE                   ((u32)0x80000000)
>>>>    #define EV_EFI_VARIABLE_DRIVER_CONFIG               ((u32)0x80000001)
>>>> @@ -85,8 +85,24 @@ struct udevice;
>>>>    #define EV_EFI_ACTION                               ((u32)0x80000007)
>>>>    #define EV_EFI_PLATFORM_FIRMWARE_BLOB               ((u32)0x80000008)
>>>>    #define EV_EFI_HANDOFF_TABLES                       ((u32)0x80000009)
>>>> +#define EV_EFI_PLATFORM_FIRMWARE_BLOB2               ((u32)0x8000000A)
>>>> +#define EV_EFI_HANDOFF_TABLES2                       ((u32)0x8000000B)
>>>> +#define EV_EFI_VARIABLE_BOOT2                        ((u32)0x8000000C)
>>>>    #define EV_EFI_HCRTM_EVENT                  ((u32)0x80000010)
>>>>    #define EV_EFI_VARIABLE_AUTHORITY           ((u32)0x800000E0)
>>>> +#define EV_EFI_SPDM_FIRMWARE_BLOB            ((u32)0x800000E1)
>>>> +#define EV_EFI_SPDM_FIRMWARE_CONFIG          ((u32)0x800000E2)
>>>> +
>>>> +#define EFI_CALLING_EFI_APPLICATION         \
>>>> +     "Calling EFI Application from Boot Option"
>>>> +#define EFI_RETURNING_FROM_EFI_APPLICATION  \
>>>> +     "Returning from EFI Application from Boot Option"
>>>> +#define EFI_EXIT_BOOT_SERVICES_INVOCATION   \
>>>> +     "Exit Boot Services Invocation"
>>>> +#define EFI_EXIT_BOOT_SERVICES_FAILED       \
>>>> +     "Exit Boot Services Returned with Failure"
>>>> +#define EFI_EXIT_BOOT_SERVICES_SUCCEEDED    \
>>>> +     "Exit Boot Services Returned with Success"
>>>
>>> Which spec defines if the string in the event log shall be utf-8 or utf-16?
>>
>> TCG PC Client PFP spec does not clearly define the character encoding.
>> In my understanding, the string derived from UEFI spec such as
>> UEFI variable name uses utf-16(CHAR16).
>> Other strings like "Calling EFI Application from Boot Option" defind in TCG PC
>> Client spec use 1 byte ASCII encoding.
>>
>> EDK2 implementation also uses 1 byte ASCII encoding for these strings,
>> and tpm2-tools::tpm2_eventlog command can handles properly.
>>
>> Thanks,
>> Masahisa Kojima
>>
>>>
>>> Best regards
>>>
>>> Heinrich
>>>
>>>>
>>>>    /* TPMS_TAGGED_PROPERTY Structure */
>>>>    struct tpms_tagged_property {
>>>> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
>>>> index f6d5ba05e3..2914800c56 100644
>>>> --- a/lib/efi_loader/efi_boottime.c
>>>> +++ b/lib/efi_loader/efi_boottime.c
>>>> @@ -2993,6 +2993,16 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
>>>>        image_obj->exit_status = &exit_status;
>>>>        image_obj->exit_jmp = &exit_jmp;
>>>>
>>>> +     if (IS_ENABLED(CONFIG_EFI_TCG2_PROTOCOL)) {
>>>> +             if (image_obj->image_type == IMAGE_SUBSYSTEM_EFI_APPLICATION) {
>>>> +                     ret = efi_tcg2_measure_efi_app_invocation();
>>>> +                     if (ret != EFI_SUCCESS) {
>>>> +                             EFI_PRINT("tcg2 measurement fails(0x%lx)\n",
>>>> +                                       ret);
>>>> +                     }
>>>> +             }
>>>> +     }
>>>> +
>>>>        /* call the image! */
>>>>        if (setjmp(&exit_jmp)) {
>>>>                /*
>>>> @@ -3251,6 +3261,16 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
>>>>            exit_status != EFI_SUCCESS)
>>>>                efi_delete_image(image_obj, loaded_image_protocol);
>>>>
>>>> +     if (IS_ENABLED(CONFIG_EFI_TCG2_PROTOCOL)) {
>>>> +             if (image_obj->image_type == IMAGE_SUBSYSTEM_EFI_APPLICATION) {
>>>> +                     ret = efi_tcg2_measure_efi_app_exit();
>>>> +                     if (ret != EFI_SUCCESS) {
>>>> +                             EFI_PRINT("tcg2 measurement fails(0x%lx)\n",
>>>> +                                       ret);
>>>> +                     }
>>>> +             }
>>>> +     }
>>>> +
>>>>        /* Make sure entry/exit counts for EFI world cross-overs match */
>>>>        EFI_EXIT(exit_status);
>>>>
>>>> diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
>>>> index 2a248bd62a..6e903e3cb3 100644
>>>> --- a/lib/efi_loader/efi_tcg2.c
>>>> +++ b/lib/efi_loader/efi_tcg2.c
>>>> @@ -35,6 +35,7 @@ struct event_log_buffer {
>>>>    };
>>>>
>>>>    static struct event_log_buffer event_log;
>>>> +static bool tcg2_efi_app_invoked;
>>>>    /*
>>>>     * When requesting TPM2_CAP_TPM_PROPERTIES the value is on a standard offset.
>>>>     * Since the current tpm2_get_capability() response buffers starts at
>>>> @@ -1383,6 +1384,128 @@ static efi_status_t tcg2_measure_variable(struct udevice *dev, u32 pcr_index,
>>>>        return ret;
>>>>    }
>>>>
>>>> +/**
>>>> + * tcg2_measure_boot_variable() - measure boot variables
>>>> + *
>>>> + * @dev:     TPM device
>>>> + *
>>>> + * Return:   status code
>>>> + */
>>>> +static efi_status_t tcg2_measure_boot_variable(struct udevice *dev)
>>>> +{
>>>> +     u16 *boot_order;
>>>> +     u16 var_name[] = L"BootOrder";
>>>> +     u16 boot_name[] = L"Boot0000";
>>>> +     u16 hexmap[] = L"0123456789ABCDEF";
>>>> +     u8 *bootvar;
>>>> +     efi_uintn_t var_data_size;
>>>> +     u32 count, i;
>>>> +     efi_status_t ret;
>>>> +
>>>> +     boot_order = efi_get_var(var_name, &efi_global_variable_guid,
>>>> +                              &var_data_size);
>>>> +     if (!boot_order) {
>>>> +             log_info("BootOrder not defined\n");
>>>> +             ret = EFI_NOT_FOUND;
>>>> +             goto error;
>>>> +     }
>>>> +
>>>> +     ret = tcg2_measure_variable(dev, 1, EV_EFI_VARIABLE_BOOT2, var_name,
>>>> +                                 &efi_global_variable_guid, var_data_size,
>>>> +                                 (u8 *)boot_order);
>>>> +     if (ret != EFI_SUCCESS)
>>>> +             goto error;
>>>> +
>>>> +     count = var_data_size / sizeof(*boot_order);
>>>> +     for (i = 0; i < count; i++) {
>>>> +             boot_name[4] = hexmap[(boot_order[i] & 0xf000) >> 12];
>>>> +             boot_name[5] = hexmap[(boot_order[i] & 0x0f00) >> 8];
>>>> +             boot_name[6] = hexmap[(boot_order[i] & 0x00f0) >> 4];
>>>> +             boot_name[7] = hexmap[(boot_order[i] & 0x000f)];
>>>> +
>>>> +             bootvar = efi_get_var(boot_name, &efi_global_variable_guid,
>>>> +                                   &var_data_size);
>>>> +
>>>> +             if (!bootvar) {
>>>> +                     log_info("%ls not found\n", boot_name);
>>>> +                     continue;
>>>> +             }
>>>> +
>>>> +             ret = tcg2_measure_variable(dev, 1, EV_EFI_VARIABLE_BOOT2,
>>>> +                                         boot_name,
>>>> +                                         &efi_global_variable_guid,
>>>> +                                         var_data_size, bootvar);
>>>> +             free(bootvar);
>>>> +             if (ret != EFI_SUCCESS)
>>>> +                     goto error;
>>>> +     }
>>>> +
>>>> +error:
>>>> +     free(boot_order);
>>>> +     return ret;
>>>> +}
>>>> +
>>>> +/**
>>>> + * efi_tcg2_measure_efi_app_invocation() - measure efi app invocation
>>>> + *
>>>> + * Return:   status code
>>>> + */
>>>> +efi_status_t EFIAPI efi_tcg2_measure_efi_app_invocation(void)
>>>> +{
>>>> +     efi_status_t ret;
>>>> +     u32 pcr_index;
>>>> +     struct udevice *dev;
>>>> +     u32 event = 0;
>>>> +
>>>> +     if (tcg2_efi_app_invoked)
>>>> +             return EFI_SUCCESS;
>>>> +
>>>> +     ret = platform_get_tpm2_device(&dev);
>>>> +     if (ret != EFI_SUCCESS)
>>>> +             return ret;
>>>> +
>>>> +     ret = tcg2_measure_boot_variable(dev);
>>>> +     if (ret != EFI_SUCCESS)
>>>> +             goto out;
>>>> +
>>>> +     ret = tcg2_measure_event(dev, 4, EV_EFI_ACTION,
>>>> +                              strlen(EFI_CALLING_EFI_APPLICATION),
>>>> +                              (u8 *)EFI_CALLING_EFI_APPLICATION);
>>>> +     if (ret != EFI_SUCCESS)
>>>> +             goto out;
>>>> +
>>>> +     for (pcr_index = 0; pcr_index <= 7; pcr_index++) {
>>>> +             ret = tcg2_measure_event(dev, pcr_index, EV_SEPARATOR,
>>>> +                                      sizeof(event), (u8 *)&event);
>>>> +             if (ret != EFI_SUCCESS)
>>>> +                     goto out;
>>>> +     }
>>>> +
>>>> +     tcg2_efi_app_invoked = true;
>>>> +out:
>>>> +     return ret;
>>>> +}
>>>> +
>>>> +/**
>>>> + * efi_tcg2_measure_efi_app_exit() - measure efi app exit
>>>> + *
>>>> + * Return:   status code
>>>> + */
>>>> +efi_status_t EFIAPI efi_tcg2_measure_efi_app_exit(void)
>>>> +{
>>>> +     efi_status_t ret;
>>>> +     struct udevice *dev;
>>>> +
>>>> +     ret = platform_get_tpm2_device(&dev);
>>>> +     if (ret != EFI_SUCCESS)
>>>> +             return ret;
>>>> +
>>>> +     ret = tcg2_measure_event(dev, 4, EV_EFI_ACTION,
>>>> +                              strlen(EFI_RETURNING_FROM_EFI_APPLICATION),
>>>> +                              (u8 *)EFI_RETURNING_FROM_EFI_APPLICATION);
>>>> +     return ret;
>>>> +}
>>>> +
>>>>    /**
>>>>     * tcg2_measure_secure_boot_variable() - measure secure boot variables
>>>>     *
>>>>
>>>

Re: [PATCH 3/5] efi_loader: add boot variable measurement

[AKASHI Takahiro]

On Tue, Jul 13, 2021 at 04:24:52PM +0200, Heinrich Schuchardt wrote:
> 
> 
> On 13.07.21 10:31, Masahisa Kojima wrote:
> > Hi Heinrich,
> > 
> > > > > TCG spec also requires to measure "Calling EFI Application from
> > > > > Boot Option" for each boot attempt, and "Returning from EFI
> > > > > Application from Boot Option" if a boot device returns control
> > > > > back to the Boot Manager.
> > 
> > I would like to hear your opinion regarding
> > "Calling EFI Application from Boot Option" measurement.
> > 
> > My current(v1 patch series) implementation considers
> > both "bootefi bootmgr" and "bootefi $image_addr" cases,
> > so I do this "Calling EFI Application from Boot Option" measurement
> > at efi_boottime.c::efi_start_image().
> > Do I need to implement only the case UEFI application boot from bootmgr?
> > If yes, I will move the timing of this measurement at
> > efi_bootmgr.c::efi_bootmgr_load().
> > 
> > As a reference, in edk2, this measurement is performed in
> > ready_to_boot event handler, ready_to_boot handler is called
> > upon the user selects the boot option in boot manager.
> 
> When booting you can call
> 
>    bootefi $driver1
>    booefii $driver2
>    bootefi bootmgr
> 
> in sequence.
> 
> Any of the binaries can call LoadImage(), StartImage() multiple times to
> execute further images. E.g. I am loading iPXE. By default it loads GRUB
> from an iSCSI drive but I can choose in the menu or the iPXE console to
> invoke another UEFI binary.
> 
> I suggest to measure any image no matter how it is invoked. The
> measurement must depend on the sequence of invocation.

Moreover,
booting from the default path, like /EFI/BOOT/BOOTAA64.EFI,
is only implemented by using bootefi <addr> syntax.

-Takahiro Akashi


> Best regards
> 
> Heinrich
> 
> > 
> > What do you think?
> > 
> > Thanks,
> > Masahisa Kojima
> > 
> > 
> > 
> > 
> > 
> > On Fri, 9 Jul 2021 at 11:44, Masahisa Kojima <masahisa.kojima@linaro.org> wrote:
> > > 
> > > On Fri, 9 Jul 2021 at 02:46, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > 
> > > > On 7/7/21 3:36 PM, Masahisa Kojima wrote:
> > > > > TCG PC Client PFP spec requires to measure "Boot####"
> > > > > and "BootOrder" variables, EV_SEPARATOR event prior
> > > > > to the Ready to Boot invocation.
> > > > > Since u-boot does not implement Ready to Boot event,
> > > > > these measurements are performed when efi_start_image() is called.
> > > > > 
> > > > > TCG spec also requires to measure "Calling EFI Application from
> > > > > Boot Option" for each boot attempt, and "Returning from EFI
> > > > > Application from Boot Option" if a boot device returns control
> > > > > back to the Boot Manager.
> > > > > 
> > > > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > > > > ---
> > > > >    include/efi_loader.h          |   4 ++
> > > > >    include/tpm-v2.h              |  18 ++++-
> > > > >    lib/efi_loader/efi_boottime.c |  20 ++++++
> > > > >    lib/efi_loader/efi_tcg2.c     | 123 ++++++++++++++++++++++++++++++++++
> > > > >    4 files changed, 164 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > > > > index 0a9c82a257..281ffff30f 100644
> > > > > --- a/include/efi_loader.h
> > > > > +++ b/include/efi_loader.h
> > > > > @@ -407,6 +407,10 @@ efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size);
> > > > >    efi_status_t efi_init_variables(void);
> > > > >    /* Notify ExitBootServices() is called */
> > > > >    void efi_variables_boot_exit_notify(void);
> > > > > +/* Measure efi application invocation */
> > > > > +efi_status_t EFIAPI efi_tcg2_measure_efi_app_invocation(void);
> > > > > +/* Measure efi application exit */
> > > > > +efi_status_t EFIAPI efi_tcg2_measure_efi_app_exit(void);
> > > > >    /* Called by bootefi to initialize root node */
> > > > >    efi_status_t efi_root_node_register(void);
> > > > >    /* Called by bootefi to initialize runtime */
> > > > > diff --git a/include/tpm-v2.h b/include/tpm-v2.h
> > > > > index 3e48e35861..8a7b7f1874 100644
> > > > > --- a/include/tpm-v2.h
> > > > > +++ b/include/tpm-v2.h
> > > > > @@ -73,7 +73,7 @@ struct udevice;
> > > > >    /*
> > > > >     * event types, cf.
> > > > >     * "TCG PC Client Platform Firmware Profile Specification", Family "2.0"
> > > > > - * rev 1.04, June 3, 2019
> > > > > + * Level 00 Version 1.05 Revision 23, May 7, 2021
> > > > >     */
> > > > >    #define EV_EFI_EVENT_BASE                   ((u32)0x80000000)
> > > > >    #define EV_EFI_VARIABLE_DRIVER_CONFIG               ((u32)0x80000001)
> > > > > @@ -85,8 +85,24 @@ struct udevice;
> > > > >    #define EV_EFI_ACTION                               ((u32)0x80000007)
> > > > >    #define EV_EFI_PLATFORM_FIRMWARE_BLOB               ((u32)0x80000008)
> > > > >    #define EV_EFI_HANDOFF_TABLES                       ((u32)0x80000009)
> > > > > +#define EV_EFI_PLATFORM_FIRMWARE_BLOB2               ((u32)0x8000000A)
> > > > > +#define EV_EFI_HANDOFF_TABLES2                       ((u32)0x8000000B)
> > > > > +#define EV_EFI_VARIABLE_BOOT2                        ((u32)0x8000000C)
> > > > >    #define EV_EFI_HCRTM_EVENT                  ((u32)0x80000010)
> > > > >    #define EV_EFI_VARIABLE_AUTHORITY           ((u32)0x800000E0)
> > > > > +#define EV_EFI_SPDM_FIRMWARE_BLOB            ((u32)0x800000E1)
> > > > > +#define EV_EFI_SPDM_FIRMWARE_CONFIG          ((u32)0x800000E2)
> > > > > +
> > > > > +#define EFI_CALLING_EFI_APPLICATION         \
> > > > > +     "Calling EFI Application from Boot Option"
> > > > > +#define EFI_RETURNING_FROM_EFI_APPLICATION  \
> > > > > +     "Returning from EFI Application from Boot Option"
> > > > > +#define EFI_EXIT_BOOT_SERVICES_INVOCATION   \
> > > > > +     "Exit Boot Services Invocation"
> > > > > +#define EFI_EXIT_BOOT_SERVICES_FAILED       \
> > > > > +     "Exit Boot Services Returned with Failure"
> > > > > +#define EFI_EXIT_BOOT_SERVICES_SUCCEEDED    \
> > > > > +     "Exit Boot Services Returned with Success"
> > > > 
> > > > Which spec defines if the string in the event log shall be utf-8 or utf-16?
> > > 
> > > TCG PC Client PFP spec does not clearly define the character encoding.
> > > In my understanding, the string derived from UEFI spec such as
> > > UEFI variable name uses utf-16(CHAR16).
> > > Other strings like "Calling EFI Application from Boot Option" defind in TCG PC
> > > Client spec use 1 byte ASCII encoding.
> > > 
> > > EDK2 implementation also uses 1 byte ASCII encoding for these strings,
> > > and tpm2-tools::tpm2_eventlog command can handles properly.
> > > 
> > > Thanks,
> > > Masahisa Kojima
> > > 
> > > > 
> > > > Best regards
> > > > 
> > > > Heinrich
> > > > 
> > > > > 
> > > > >    /* TPMS_TAGGED_PROPERTY Structure */
> > > > >    struct tpms_tagged_property {
> > > > > diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> > > > > index f6d5ba05e3..2914800c56 100644
> > > > > --- a/lib/efi_loader/efi_boottime.c
> > > > > +++ b/lib/efi_loader/efi_boottime.c
> > > > > @@ -2993,6 +2993,16 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
> > > > >        image_obj->exit_status = &exit_status;
> > > > >        image_obj->exit_jmp = &exit_jmp;
> > > > > 
> > > > > +     if (IS_ENABLED(CONFIG_EFI_TCG2_PROTOCOL)) {
> > > > > +             if (image_obj->image_type == IMAGE_SUBSYSTEM_EFI_APPLICATION) {
> > > > > +                     ret = efi_tcg2_measure_efi_app_invocation();
> > > > > +                     if (ret != EFI_SUCCESS) {
> > > > > +                             EFI_PRINT("tcg2 measurement fails(0x%lx)\n",
> > > > > +                                       ret);
> > > > > +                     }
> > > > > +             }
> > > > > +     }
> > > > > +
> > > > >        /* call the image! */
> > > > >        if (setjmp(&exit_jmp)) {
> > > > >                /*
> > > > > @@ -3251,6 +3261,16 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
> > > > >            exit_status != EFI_SUCCESS)
> > > > >                efi_delete_image(image_obj, loaded_image_protocol);
> > > > > 
> > > > > +     if (IS_ENABLED(CONFIG_EFI_TCG2_PROTOCOL)) {
> > > > > +             if (image_obj->image_type == IMAGE_SUBSYSTEM_EFI_APPLICATION) {
> > > > > +                     ret = efi_tcg2_measure_efi_app_exit();
> > > > > +                     if (ret != EFI_SUCCESS) {
> > > > > +                             EFI_PRINT("tcg2 measurement fails(0x%lx)\n",
> > > > > +                                       ret);
> > > > > +                     }
> > > > > +             }
> > > > > +     }
> > > > > +
> > > > >        /* Make sure entry/exit counts for EFI world cross-overs match */
> > > > >        EFI_EXIT(exit_status);
> > > > > 
> > > > > diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
> > > > > index 2a248bd62a..6e903e3cb3 100644
> > > > > --- a/lib/efi_loader/efi_tcg2.c
> > > > > +++ b/lib/efi_loader/efi_tcg2.c
> > > > > @@ -35,6 +35,7 @@ struct event_log_buffer {
> > > > >    };
> > > > > 
> > > > >    static struct event_log_buffer event_log;
> > > > > +static bool tcg2_efi_app_invoked;
> > > > >    /*
> > > > >     * When requesting TPM2_CAP_TPM_PROPERTIES the value is on a standard offset.
> > > > >     * Since the current tpm2_get_capability() response buffers starts at
> > > > > @@ -1383,6 +1384,128 @@ static efi_status_t tcg2_measure_variable(struct udevice *dev, u32 pcr_index,
> > > > >        return ret;
> > > > >    }
> > > > > 
> > > > > +/**
> > > > > + * tcg2_measure_boot_variable() - measure boot variables
> > > > > + *
> > > > > + * @dev:     TPM device
> > > > > + *
> > > > > + * Return:   status code
> > > > > + */
> > > > > +static efi_status_t tcg2_measure_boot_variable(struct udevice *dev)
> > > > > +{
> > > > > +     u16 *boot_order;
> > > > > +     u16 var_name[] = L"BootOrder";
> > > > > +     u16 boot_name[] = L"Boot0000";
> > > > > +     u16 hexmap[] = L"0123456789ABCDEF";
> > > > > +     u8 *bootvar;
> > > > > +     efi_uintn_t var_data_size;
> > > > > +     u32 count, i;
> > > > > +     efi_status_t ret;
> > > > > +
> > > > > +     boot_order = efi_get_var(var_name, &efi_global_variable_guid,
> > > > > +                              &var_data_size);
> > > > > +     if (!boot_order) {
> > > > > +             log_info("BootOrder not defined\n");
> > > > > +             ret = EFI_NOT_FOUND;
> > > > > +             goto error;
> > > > > +     }
> > > > > +
> > > > > +     ret = tcg2_measure_variable(dev, 1, EV_EFI_VARIABLE_BOOT2, var_name,
> > > > > +                                 &efi_global_variable_guid, var_data_size,
> > > > > +                                 (u8 *)boot_order);
> > > > > +     if (ret != EFI_SUCCESS)
> > > > > +             goto error;
> > > > > +
> > > > > +     count = var_data_size / sizeof(*boot_order);
> > > > > +     for (i = 0; i < count; i++) {
> > > > > +             boot_name[4] = hexmap[(boot_order[i] & 0xf000) >> 12];
> > > > > +             boot_name[5] = hexmap[(boot_order[i] & 0x0f00) >> 8];
> > > > > +             boot_name[6] = hexmap[(boot_order[i] & 0x00f0) >> 4];
> > > > > +             boot_name[7] = hexmap[(boot_order[i] & 0x000f)];
> > > > > +
> > > > > +             bootvar = efi_get_var(boot_name, &efi_global_variable_guid,
> > > > > +                                   &var_data_size);
> > > > > +
> > > > > +             if (!bootvar) {
> > > > > +                     log_info("%ls not found\n", boot_name);
> > > > > +                     continue;
> > > > > +             }
> > > > > +
> > > > > +             ret = tcg2_measure_variable(dev, 1, EV_EFI_VARIABLE_BOOT2,
> > > > > +                                         boot_name,
> > > > > +                                         &efi_global_variable_guid,
> > > > > +                                         var_data_size, bootvar);
> > > > > +             free(bootvar);
> > > > > +             if (ret != EFI_SUCCESS)
> > > > > +                     goto error;
> > > > > +     }
> > > > > +
> > > > > +error:
> > > > > +     free(boot_order);
> > > > > +     return ret;
> > > > > +}
> > > > > +
> > > > > +/**
> > > > > + * efi_tcg2_measure_efi_app_invocation() - measure efi app invocation
> > > > > + *
> > > > > + * Return:   status code
> > > > > + */
> > > > > +efi_status_t EFIAPI efi_tcg2_measure_efi_app_invocation(void)
> > > > > +{
> > > > > +     efi_status_t ret;
> > > > > +     u32 pcr_index;
> > > > > +     struct udevice *dev;
> > > > > +     u32 event = 0;
> > > > > +
> > > > > +     if (tcg2_efi_app_invoked)
> > > > > +             return EFI_SUCCESS;
> > > > > +
> > > > > +     ret = platform_get_tpm2_device(&dev);
> > > > > +     if (ret != EFI_SUCCESS)
> > > > > +             return ret;
> > > > > +
> > > > > +     ret = tcg2_measure_boot_variable(dev);
> > > > > +     if (ret != EFI_SUCCESS)
> > > > > +             goto out;
> > > > > +
> > > > > +     ret = tcg2_measure_event(dev, 4, EV_EFI_ACTION,
> > > > > +                              strlen(EFI_CALLING_EFI_APPLICATION),
> > > > > +                              (u8 *)EFI_CALLING_EFI_APPLICATION);
> > > > > +     if (ret != EFI_SUCCESS)
> > > > > +             goto out;
> > > > > +
> > > > > +     for (pcr_index = 0; pcr_index <= 7; pcr_index++) {
> > > > > +             ret = tcg2_measure_event(dev, pcr_index, EV_SEPARATOR,
> > > > > +                                      sizeof(event), (u8 *)&event);
> > > > > +             if (ret != EFI_SUCCESS)
> > > > > +                     goto out;
> > > > > +     }
> > > > > +
> > > > > +     tcg2_efi_app_invoked = true;
> > > > > +out:
> > > > > +     return ret;
> > > > > +}
> > > > > +
> > > > > +/**
> > > > > + * efi_tcg2_measure_efi_app_exit() - measure efi app exit
> > > > > + *
> > > > > + * Return:   status code
> > > > > + */
> > > > > +efi_status_t EFIAPI efi_tcg2_measure_efi_app_exit(void)
> > > > > +{
> > > > > +     efi_status_t ret;
> > > > > +     struct udevice *dev;
> > > > > +
> > > > > +     ret = platform_get_tpm2_device(&dev);
> > > > > +     if (ret != EFI_SUCCESS)
> > > > > +             return ret;
> > > > > +
> > > > > +     ret = tcg2_measure_event(dev, 4, EV_EFI_ACTION,
> > > > > +                              strlen(EFI_RETURNING_FROM_EFI_APPLICATION),
> > > > > +                              (u8 *)EFI_RETURNING_FROM_EFI_APPLICATION);
> > > > > +     return ret;
> > > > > +}
> > > > > +
> > > > >    /**
> > > > >     * tcg2_measure_secure_boot_variable() - measure secure boot variables
> > > > >     *
> > > > > 
> > > > 

Re: [PATCH 3/5] efi_loader: add boot variable measurement

[Masahisa Kojima]

Hi Heinrich, Akashi-san,

Thank you for your comment.
I will keep current implementation.

Thanks,
Masahisa Kojima

On Wed, 14 Jul 2021 at 08:54, AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:
>
> On Tue, Jul 13, 2021 at 04:24:52PM +0200, Heinrich Schuchardt wrote:
> >
> >
> > On 13.07.21 10:31, Masahisa Kojima wrote:
> > > Hi Heinrich,
> > >
> > > > > > TCG spec also requires to measure "Calling EFI Application from
> > > > > > Boot Option" for each boot attempt, and "Returning from EFI
> > > > > > Application from Boot Option" if a boot device returns control
> > > > > > back to the Boot Manager.
> > >
> > > I would like to hear your opinion regarding
> > > "Calling EFI Application from Boot Option" measurement.
> > >
> > > My current(v1 patch series) implementation considers
> > > both "bootefi bootmgr" and "bootefi $image_addr" cases,
> > > so I do this "Calling EFI Application from Boot Option" measurement
> > > at efi_boottime.c::efi_start_image().
> > > Do I need to implement only the case UEFI application boot from bootmgr?
> > > If yes, I will move the timing of this measurement at
> > > efi_bootmgr.c::efi_bootmgr_load().
> > >
> > > As a reference, in edk2, this measurement is performed in
> > > ready_to_boot event handler, ready_to_boot handler is called
> > > upon the user selects the boot option in boot manager.
> >
> > When booting you can call
> >
> >    bootefi $driver1
> >    booefii $driver2
> >    bootefi bootmgr
> >
> > in sequence.
> >
> > Any of the binaries can call LoadImage(), StartImage() multiple times to
> > execute further images. E.g. I am loading iPXE. By default it loads GRUB
> > from an iSCSI drive but I can choose in the menu or the iPXE console to
> > invoke another UEFI binary.
> >
> > I suggest to measure any image no matter how it is invoked. The
> > measurement must depend on the sequence of invocation.
>
> Moreover,
> booting from the default path, like /EFI/BOOT/BOOTAA64.EFI,
> is only implemented by using bootefi <addr> syntax.
>
> -Takahiro Akashi
>
>
> > Best regards
> >
> > Heinrich
> >
> > >
> > > What do you think?
> > >
> > > Thanks,
> > > Masahisa Kojima
> > >
> > >
> > >
> > >
> > >
> > > On Fri, 9 Jul 2021 at 11:44, Masahisa Kojima <masahisa.kojima@linaro.org> wrote:
> > > >
> > > > On Fri, 9 Jul 2021 at 02:46, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > >
> > > > > On 7/7/21 3:36 PM, Masahisa Kojima wrote:
> > > > > > TCG PC Client PFP spec requires to measure "Boot####"
> > > > > > and "BootOrder" variables, EV_SEPARATOR event prior
> > > > > > to the Ready to Boot invocation.
> > > > > > Since u-boot does not implement Ready to Boot event,
> > > > > > these measurements are performed when efi_start_image() is called.
> > > > > >
> > > > > > TCG spec also requires to measure "Calling EFI Application from
> > > > > > Boot Option" for each boot attempt, and "Returning from EFI
> > > > > > Application from Boot Option" if a boot device returns control
> > > > > > back to the Boot Manager.
> > > > > >
> > > > > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > > > > > ---
> > > > > >    include/efi_loader.h          |   4 ++
> > > > > >    include/tpm-v2.h              |  18 ++++-
> > > > > >    lib/efi_loader/efi_boottime.c |  20 ++++++
> > > > > >    lib/efi_loader/efi_tcg2.c     | 123 ++++++++++++++++++++++++++++++++++
> > > > > >    4 files changed, 164 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > > > > > index 0a9c82a257..281ffff30f 100644
> > > > > > --- a/include/efi_loader.h
> > > > > > +++ b/include/efi_loader.h
> > > > > > @@ -407,6 +407,10 @@ efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size);
> > > > > >    efi_status_t efi_init_variables(void);
> > > > > >    /* Notify ExitBootServices() is called */
> > > > > >    void efi_variables_boot_exit_notify(void);
> > > > > > +/* Measure efi application invocation */
> > > > > > +efi_status_t EFIAPI efi_tcg2_measure_efi_app_invocation(void);
> > > > > > +/* Measure efi application exit */
> > > > > > +efi_status_t EFIAPI efi_tcg2_measure_efi_app_exit(void);
> > > > > >    /* Called by bootefi to initialize root node */
> > > > > >    efi_status_t efi_root_node_register(void);
> > > > > >    /* Called by bootefi to initialize runtime */
> > > > > > diff --git a/include/tpm-v2.h b/include/tpm-v2.h
> > > > > > index 3e48e35861..8a7b7f1874 100644
> > > > > > --- a/include/tpm-v2.h
> > > > > > +++ b/include/tpm-v2.h
> > > > > > @@ -73,7 +73,7 @@ struct udevice;
> > > > > >    /*
> > > > > >     * event types, cf.
> > > > > >     * "TCG PC Client Platform Firmware Profile Specification", Family "2.0"
> > > > > > - * rev 1.04, June 3, 2019
> > > > > > + * Level 00 Version 1.05 Revision 23, May 7, 2021
> > > > > >     */
> > > > > >    #define EV_EFI_EVENT_BASE                   ((u32)0x80000000)
> > > > > >    #define EV_EFI_VARIABLE_DRIVER_CONFIG               ((u32)0x80000001)
> > > > > > @@ -85,8 +85,24 @@ struct udevice;
> > > > > >    #define EV_EFI_ACTION                               ((u32)0x80000007)
> > > > > >    #define EV_EFI_PLATFORM_FIRMWARE_BLOB               ((u32)0x80000008)
> > > > > >    #define EV_EFI_HANDOFF_TABLES                       ((u32)0x80000009)
> > > > > > +#define EV_EFI_PLATFORM_FIRMWARE_BLOB2               ((u32)0x8000000A)
> > > > > > +#define EV_EFI_HANDOFF_TABLES2                       ((u32)0x8000000B)
> > > > > > +#define EV_EFI_VARIABLE_BOOT2                        ((u32)0x8000000C)
> > > > > >    #define EV_EFI_HCRTM_EVENT                  ((u32)0x80000010)
> > > > > >    #define EV_EFI_VARIABLE_AUTHORITY           ((u32)0x800000E0)
> > > > > > +#define EV_EFI_SPDM_FIRMWARE_BLOB            ((u32)0x800000E1)
> > > > > > +#define EV_EFI_SPDM_FIRMWARE_CONFIG          ((u32)0x800000E2)
> > > > > > +
> > > > > > +#define EFI_CALLING_EFI_APPLICATION         \
> > > > > > +     "Calling EFI Application from Boot Option"
> > > > > > +#define EFI_RETURNING_FROM_EFI_APPLICATION  \
> > > > > > +     "Returning from EFI Application from Boot Option"
> > > > > > +#define EFI_EXIT_BOOT_SERVICES_INVOCATION   \
> > > > > > +     "Exit Boot Services Invocation"
> > > > > > +#define EFI_EXIT_BOOT_SERVICES_FAILED       \
> > > > > > +     "Exit Boot Services Returned with Failure"
> > > > > > +#define EFI_EXIT_BOOT_SERVICES_SUCCEEDED    \
> > > > > > +     "Exit Boot Services Returned with Success"
> > > > >
> > > > > Which spec defines if the string in the event log shall be utf-8 or utf-16?
> > > >
> > > > TCG PC Client PFP spec does not clearly define the character encoding.
> > > > In my understanding, the string derived from UEFI spec such as
> > > > UEFI variable name uses utf-16(CHAR16).
> > > > Other strings like "Calling EFI Application from Boot Option" defind in TCG PC
> > > > Client spec use 1 byte ASCII encoding.
> > > >
> > > > EDK2 implementation also uses 1 byte ASCII encoding for these strings,
> > > > and tpm2-tools::tpm2_eventlog command can handles properly.
> > > >
> > > > Thanks,
> > > > Masahisa Kojima
> > > >
> > > > >
> > > > > Best regards
> > > > >
> > > > > Heinrich
> > > > >
> > > > > >
> > > > > >    /* TPMS_TAGGED_PROPERTY Structure */
> > > > > >    struct tpms_tagged_property {
> > > > > > diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> > > > > > index f6d5ba05e3..2914800c56 100644
> > > > > > --- a/lib/efi_loader/efi_boottime.c
> > > > > > +++ b/lib/efi_loader/efi_boottime.c
> > > > > > @@ -2993,6 +2993,16 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
> > > > > >        image_obj->exit_status = &exit_status;
> > > > > >        image_obj->exit_jmp = &exit_jmp;
> > > > > >
> > > > > > +     if (IS_ENABLED(CONFIG_EFI_TCG2_PROTOCOL)) {
> > > > > > +             if (image_obj->image_type == IMAGE_SUBSYSTEM_EFI_APPLICATION) {
> > > > > > +                     ret = efi_tcg2_measure_efi_app_invocation();
> > > > > > +                     if (ret != EFI_SUCCESS) {
> > > > > > +                             EFI_PRINT("tcg2 measurement fails(0x%lx)\n",
> > > > > > +                                       ret);
> > > > > > +                     }
> > > > > > +             }
> > > > > > +     }
> > > > > > +
> > > > > >        /* call the image! */
> > > > > >        if (setjmp(&exit_jmp)) {
> > > > > >                /*
> > > > > > @@ -3251,6 +3261,16 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
> > > > > >            exit_status != EFI_SUCCESS)
> > > > > >                efi_delete_image(image_obj, loaded_image_protocol);
> > > > > >
> > > > > > +     if (IS_ENABLED(CONFIG_EFI_TCG2_PROTOCOL)) {
> > > > > > +             if (image_obj->image_type == IMAGE_SUBSYSTEM_EFI_APPLICATION) {
> > > > > > +                     ret = efi_tcg2_measure_efi_app_exit();
> > > > > > +                     if (ret != EFI_SUCCESS) {
> > > > > > +                             EFI_PRINT("tcg2 measurement fails(0x%lx)\n",
> > > > > > +                                       ret);
> > > > > > +                     }
> > > > > > +             }
> > > > > > +     }
> > > > > > +
> > > > > >        /* Make sure entry/exit counts for EFI world cross-overs match */
> > > > > >        EFI_EXIT(exit_status);
> > > > > >
> > > > > > diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
> > > > > > index 2a248bd62a..6e903e3cb3 100644
> > > > > > --- a/lib/efi_loader/efi_tcg2.c
> > > > > > +++ b/lib/efi_loader/efi_tcg2.c
> > > > > > @@ -35,6 +35,7 @@ struct event_log_buffer {
> > > > > >    };
> > > > > >
> > > > > >    static struct event_log_buffer event_log;
> > > > > > +static bool tcg2_efi_app_invoked;
> > > > > >    /*
> > > > > >     * When requesting TPM2_CAP_TPM_PROPERTIES the value is on a standard offset.
> > > > > >     * Since the current tpm2_get_capability() response buffers starts at
> > > > > > @@ -1383,6 +1384,128 @@ static efi_status_t tcg2_measure_variable(struct udevice *dev, u32 pcr_index,
> > > > > >        return ret;
> > > > > >    }
> > > > > >
> > > > > > +/**
> > > > > > + * tcg2_measure_boot_variable() - measure boot variables
> > > > > > + *
> > > > > > + * @dev:     TPM device
> > > > > > + *
> > > > > > + * Return:   status code
> > > > > > + */
> > > > > > +static efi_status_t tcg2_measure_boot_variable(struct udevice *dev)
> > > > > > +{
> > > > > > +     u16 *boot_order;
> > > > > > +     u16 var_name[] = L"BootOrder";
> > > > > > +     u16 boot_name[] = L"Boot0000";
> > > > > > +     u16 hexmap[] = L"0123456789ABCDEF";
> > > > > > +     u8 *bootvar;
> > > > > > +     efi_uintn_t var_data_size;
> > > > > > +     u32 count, i;
> > > > > > +     efi_status_t ret;
> > > > > > +
> > > > > > +     boot_order = efi_get_var(var_name, &efi_global_variable_guid,
> > > > > > +                              &var_data_size);
> > > > > > +     if (!boot_order) {
> > > > > > +             log_info("BootOrder not defined\n");
> > > > > > +             ret = EFI_NOT_FOUND;
> > > > > > +             goto error;
> > > > > > +     }
> > > > > > +
> > > > > > +     ret = tcg2_measure_variable(dev, 1, EV_EFI_VARIABLE_BOOT2, var_name,
> > > > > > +                                 &efi_global_variable_guid, var_data_size,
> > > > > > +                                 (u8 *)boot_order);
> > > > > > +     if (ret != EFI_SUCCESS)
> > > > > > +             goto error;
> > > > > > +
> > > > > > +     count = var_data_size / sizeof(*boot_order);
> > > > > > +     for (i = 0; i < count; i++) {
> > > > > > +             boot_name[4] = hexmap[(boot_order[i] & 0xf000) >> 12];
> > > > > > +             boot_name[5] = hexmap[(boot_order[i] & 0x0f00) >> 8];
> > > > > > +             boot_name[6] = hexmap[(boot_order[i] & 0x00f0) >> 4];
> > > > > > +             boot_name[7] = hexmap[(boot_order[i] & 0x000f)];
> > > > > > +
> > > > > > +             bootvar = efi_get_var(boot_name, &efi_global_variable_guid,
> > > > > > +                                   &var_data_size);
> > > > > > +
> > > > > > +             if (!bootvar) {
> > > > > > +                     log_info("%ls not found\n", boot_name);
> > > > > > +                     continue;
> > > > > > +             }
> > > > > > +
> > > > > > +             ret = tcg2_measure_variable(dev, 1, EV_EFI_VARIABLE_BOOT2,
> > > > > > +                                         boot_name,
> > > > > > +                                         &efi_global_variable_guid,
> > > > > > +                                         var_data_size, bootvar);
> > > > > > +             free(bootvar);
> > > > > > +             if (ret != EFI_SUCCESS)
> > > > > > +                     goto error;
> > > > > > +     }
> > > > > > +
> > > > > > +error:
> > > > > > +     free(boot_order);
> > > > > > +     return ret;
> > > > > > +}
> > > > > > +
> > > > > > +/**
> > > > > > + * efi_tcg2_measure_efi_app_invocation() - measure efi app invocation
> > > > > > + *
> > > > > > + * Return:   status code
> > > > > > + */
> > > > > > +efi_status_t EFIAPI efi_tcg2_measure_efi_app_invocation(void)
> > > > > > +{
> > > > > > +     efi_status_t ret;
> > > > > > +     u32 pcr_index;
> > > > > > +     struct udevice *dev;
> > > > > > +     u32 event = 0;
> > > > > > +
> > > > > > +     if (tcg2_efi_app_invoked)
> > > > > > +             return EFI_SUCCESS;
> > > > > > +
> > > > > > +     ret = platform_get_tpm2_device(&dev);
> > > > > > +     if (ret != EFI_SUCCESS)
> > > > > > +             return ret;
> > > > > > +
> > > > > > +     ret = tcg2_measure_boot_variable(dev);
> > > > > > +     if (ret != EFI_SUCCESS)
> > > > > > +             goto out;
> > > > > > +
> > > > > > +     ret = tcg2_measure_event(dev, 4, EV_EFI_ACTION,
> > > > > > +                              strlen(EFI_CALLING_EFI_APPLICATION),
> > > > > > +                              (u8 *)EFI_CALLING_EFI_APPLICATION);
> > > > > > +     if (ret != EFI_SUCCESS)
> > > > > > +             goto out;
> > > > > > +
> > > > > > +     for (pcr_index = 0; pcr_index <= 7; pcr_index++) {
> > > > > > +             ret = tcg2_measure_event(dev, pcr_index, EV_SEPARATOR,
> > > > > > +                                      sizeof(event), (u8 *)&event);
> > > > > > +             if (ret != EFI_SUCCESS)
> > > > > > +                     goto out;
> > > > > > +     }
> > > > > > +
> > > > > > +     tcg2_efi_app_invoked = true;
> > > > > > +out:
> > > > > > +     return ret;
> > > > > > +}
> > > > > > +
> > > > > > +/**
> > > > > > + * efi_tcg2_measure_efi_app_exit() - measure efi app exit
> > > > > > + *
> > > > > > + * Return:   status code
> > > > > > + */
> > > > > > +efi_status_t EFIAPI efi_tcg2_measure_efi_app_exit(void)
> > > > > > +{
> > > > > > +     efi_status_t ret;
> > > > > > +     struct udevice *dev;
> > > > > > +
> > > > > > +     ret = platform_get_tpm2_device(&dev);
> > > > > > +     if (ret != EFI_SUCCESS)
> > > > > > +             return ret;
> > > > > > +
> > > > > > +     ret = tcg2_measure_event(dev, 4, EV_EFI_ACTION,
> > > > > > +                              strlen(EFI_RETURNING_FROM_EFI_APPLICATION),
> > > > > > +                              (u8 *)EFI_RETURNING_FROM_EFI_APPLICATION);
> > > > > > +     return ret;
> > > > > > +}
> > > > > > +
> > > > > >    /**
> > > > > >     * tcg2_measure_secure_boot_variable() - measure secure boot variables
> > > > > >     *
> > > > > >
> > > > >

Re: [PATCH 1/5] efi_loader: increase eventlog buffer size

[Simon Glass]

Hi Masahisa,

On Mon, 12 Jul 2021 at 02:40, Masahisa Kojima
<masahisa.kojima@linaro.org> wrote:
>
> Hi Simon,
>
> On Sun, 11 Jul 2021 at 09:01, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Masahisa,
> >
> > On Wed, 7 Jul 2021 at 20:21, Masahisa Kojima <masahisa.kojima@linaro.org> wrote:
> > >
> > > On Wed, 7 Jul 2021 at 22:47, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > >
> > > >
> > > >
> > > > On 7/7/21 3:36 PM, Masahisa Kojima wrote:
> > > > > This is a preperation to add eventlog support
> > > > > described in TCG PC Client PFP spec.
> > > > >
> > > > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > > > > ---
> > > > >   lib/efi_loader/Kconfig | 2 +-
> > > > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > > > > index b2ab48a048..a87bf3cc98 100644
> > > > > --- a/lib/efi_loader/Kconfig
> > > > > +++ b/lib/efi_loader/Kconfig
> > > > > @@ -327,7 +327,7 @@ config EFI_TCG2_PROTOCOL
> > > > >   config EFI_TCG2_PROTOCOL_EVENTLOG_SIZE
> > > > >       int "EFI_TCG2_PROTOCOL EventLog size"
> > > > >       depends on EFI_TCG2_PROTOCOL
> > > > > -     default 4096
> > > > > +     default 16384
> > > >
> > > > I found this text in EDK II:
> > > >
> > > > Minimum length(in bytes) of the system preboot TCG event log area(LAML)
> > > > -----------------------------------------------------------------------
> > > >
> > > > For PC Client Implementation spec up to and including 1.2 the minimum
> > > > log size is 64KB. (SecurityPkg/SecurityPkg.dec)
> > >
> > > Thank you for your feedback.
> > > I have not checked this.
> > > TCG spec also says "The Log Area Minimum Length for the TCG event log
> > > MUST be at least 64KB." in ACPI chapter.
> > > I will update to set 64KB as default.
> > >
> >
> > Is this the same as the BLOBLISTT_TPM2_TCG_LOG thing? If so, can we
> > put this in the bloblist? We want to avoid adding code in EFI which is
> > in U-Boot.
>
> I think bloblist is used for data passing from SPL/TPL to u-boot.

It can also be used to place things in memory that end up accessed by
Linux, e.g. ACPI tables. So I think it fits.

> Is your comment saying that the eventlog generated
> in u-boot(done in efi_tcg2.c with this patch series) should be appended
> into the buffer pointed by BLOBLISTT_TPM2_TCG_LOG blob?

I suppose so, but this logic should be implemented in the TPM layer I
think, not just in EFI. Otherwise we end up with another parallel
implementation.

Regards,
Simon

Re: [PATCH 1/5] efi_loader: increase eventlog buffer size

[Simon Glass]

Hi Ilias,

On Mon, 12 Jul 2021 at 03:28, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> On Mon, 12 Jul 2021 at 11:40, Masahisa Kojima
> <masahisa.kojima@linaro.org> wrote:
> >
> > Hi Simon,
> >
> > On Sun, 11 Jul 2021 at 09:01, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Masahisa,
> > >
> > > On Wed, 7 Jul 2021 at 20:21, Masahisa Kojima <masahisa.kojima@linaro.org> wrote:
> > > >
> > > > On Wed, 7 Jul 2021 at 22:47, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > >
> > > > >
> > > > >
> > > > > On 7/7/21 3:36 PM, Masahisa Kojima wrote:
> > > > > > This is a preperation to add eventlog support
> > > > > > described in TCG PC Client PFP spec.
> > > > > >
> > > > > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > > > > > ---
> > > > > >   lib/efi_loader/Kconfig | 2 +-
> > > > > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > > > > > index b2ab48a048..a87bf3cc98 100644
> > > > > > --- a/lib/efi_loader/Kconfig
> > > > > > +++ b/lib/efi_loader/Kconfig
> > > > > > @@ -327,7 +327,7 @@ config EFI_TCG2_PROTOCOL
> > > > > >   config EFI_TCG2_PROTOCOL_EVENTLOG_SIZE
> > > > > >       int "EFI_TCG2_PROTOCOL EventLog size"
> > > > > >       depends on EFI_TCG2_PROTOCOL
> > > > > > -     default 4096
> > > > > > +     default 16384
> > > > >
> > > > > I found this text in EDK II:
> > > > >
> > > > > Minimum length(in bytes) of the system preboot TCG event log area(LAML)
> > > > > -----------------------------------------------------------------------
> > > > >
> > > > > For PC Client Implementation spec up to and including 1.2 the minimum
> > > > > log size is 64KB. (SecurityPkg/SecurityPkg.dec)
> > > >
> > > > Thank you for your feedback.
> > > > I have not checked this.
> > > > TCG spec also says "The Log Area Minimum Length for the TCG event log
> > > > MUST be at least 64KB." in ACPI chapter.
> > > > I will update to set 64KB as default.
> > > >
> > >
> > > Is this the same as the BLOBLISTT_TPM2_TCG_LOG thing? If so, can we
> > > put this in the bloblist? We want to avoid adding code in EFI which is
> > > in U-Boot.
> >
> > I think bloblist is used for data passing from SPL/TPL to u-boot.
> > Is your comment saying that the eventlog generated
> > in u-boot(done in efi_tcg2.c with this patch series) should be appended
> > into the buffer pointed by BLOBLISTT_TPM2_TCG_LOG blob?
> >
>
> Even in that case the eventlog can't be appended.  The TCG eventlog
> hould be copied into EFI memory, since the kernel expects to find it
> there.

Typically bloblist is relocated by U-Boot. There are lots of tables
that must be passed to linux, including ACPI and SMBIOS. With bloblist
they can all be in one place.

> What we could do is copy the contents of that buffer to the eventlog.
> Depending on what that buffer already has (e.g the starting header of
> the eventlog), we might need to strip it from the efi_tcg.c code.

I'm not really sure, but the eventlog is not just EFI thing, right?
The code should be generic.

Regards,
Simon

Re: [PATCH 1/5] efi_loader: increase eventlog buffer size

[Masahisa Kojima]

Hi Simon, Ilias,

On Wed, 14 Jul 2021 at 23:50, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Masahisa,
>
> On Mon, 12 Jul 2021 at 02:40, Masahisa Kojima
> <masahisa.kojima@linaro.org> wrote:
> >
> > Hi Simon,
> >
> > On Sun, 11 Jul 2021 at 09:01, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Masahisa,
> > >
> > > On Wed, 7 Jul 2021 at 20:21, Masahisa Kojima <masahisa.kojima@linaro.org> wrote:
> > > >
> > > > On Wed, 7 Jul 2021 at 22:47, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > >
> > > > >
> > > > >
> > > > > On 7/7/21 3:36 PM, Masahisa Kojima wrote:
> > > > > > This is a preperation to add eventlog support
> > > > > > described in TCG PC Client PFP spec.
> > > > > >
> > > > > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > > > > > ---
> > > > > >   lib/efi_loader/Kconfig | 2 +-
> > > > > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > > > > > index b2ab48a048..a87bf3cc98 100644
> > > > > > --- a/lib/efi_loader/Kconfig
> > > > > > +++ b/lib/efi_loader/Kconfig
> > > > > > @@ -327,7 +327,7 @@ config EFI_TCG2_PROTOCOL
> > > > > >   config EFI_TCG2_PROTOCOL_EVENTLOG_SIZE
> > > > > >       int "EFI_TCG2_PROTOCOL EventLog size"
> > > > > >       depends on EFI_TCG2_PROTOCOL
> > > > > > -     default 4096
> > > > > > +     default 16384
> > > > >
> > > > > I found this text in EDK II:
> > > > >
> > > > > Minimum length(in bytes) of the system preboot TCG event log area(LAML)
> > > > > -----------------------------------------------------------------------
> > > > >
> > > > > For PC Client Implementation spec up to and including 1.2 the minimum
> > > > > log size is 64KB. (SecurityPkg/SecurityPkg.dec)
> > > >
> > > > Thank you for your feedback.
> > > > I have not checked this.
> > > > TCG spec also says "The Log Area Minimum Length for the TCG event log
> > > > MUST be at least 64KB." in ACPI chapter.
> > > > I will update to set 64KB as default.
> > > >
> > >
> > > Is this the same as the BLOBLISTT_TPM2_TCG_LOG thing? If so, can we
> > > put this in the bloblist? We want to avoid adding code in EFI which is
> > > in U-Boot.
> >
> > I think bloblist is used for data passing from SPL/TPL to u-boot.
>
> It can also be used to place things in memory that end up accessed by
> Linux, e.g. ACPI tables. So I think it fits.

I understand bloblist can be used for eventlog, I will check further.

>
> > Is your comment saying that the eventlog generated
> > in u-boot(done in efi_tcg2.c with this patch series) should be appended
> > into the buffer pointed by BLOBLISTT_TPM2_TCG_LOG blob?
>
> I suppose so, but this logic should be implemented in the TPM layer I
> think, not just in EFI. Otherwise we end up with another parallel
> implementation.

Current u-boot/lib/efi_loader/efi_tcg2.c includes the code
not directly related to EFI.
I would like to suggest to divide u-boot/lib/efi_loader/efi_tcg2.c
into two files.

 1)  u-boot/lib/efi_loader/efi_tcg2.c
  This file implements the EFI interfaces required in TCG EFI Protocol
Specification(https://trustedcomputinggroup.org/resource/tcg-efi-protocol-specification/).

 2) u-boot/lib/tcg2.c(new file)
 This file implements the functionality required in TCG PC Client
Platform Firmware Profile
Specification(https://trustedcomputinggroup.org/resource/pc-client-specific-platform-firmware-profile-specification/).
 This file contains the common functions to extend eventlog and PCRs, etc.

In addition, this is different topic, I found some tpm related files
under u-boot/lib directory, I think it better to have new directory
like "tcg" and move tpm related files such as tpm_api.c, tpm-v2.c
and tpm-common.c  into lib/tcg(new directory).

Thanks,
Masahisa Kojima

>
> Regards,
> Simon

Re: [PATCH 1/5] efi_loader: increase eventlog buffer size

[Ilias Apalodimas]

On Wed, Jul 14, 2021 at 08:52:07AM -0600, Simon Glass wrote:
> Hi Ilias,
> 
> On Mon, 12 Jul 2021 at 03:28, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > On Mon, 12 Jul 2021 at 11:40, Masahisa Kojima
> > <masahisa.kojima@linaro.org> wrote:
> > >
> > > Hi Simon,
> > >
> > > On Sun, 11 Jul 2021 at 09:01, Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Masahisa,
> > > >
> > > > On Wed, 7 Jul 2021 at 20:21, Masahisa Kojima <masahisa.kojima@linaro.org> wrote:
> > > > >
> > > > > On Wed, 7 Jul 2021 at 22:47, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > > >
> > > > > >
> > > > > >
> > > > > > On 7/7/21 3:36 PM, Masahisa Kojima wrote:
> > > > > > > This is a preperation to add eventlog support
> > > > > > > described in TCG PC Client PFP spec.
> > > > > > >
> > > > > > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > > > > > > ---
> > > > > > >   lib/efi_loader/Kconfig | 2 +-
> > > > > > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > > > > > > index b2ab48a048..a87bf3cc98 100644
> > > > > > > --- a/lib/efi_loader/Kconfig
> > > > > > > +++ b/lib/efi_loader/Kconfig
> > > > > > > @@ -327,7 +327,7 @@ config EFI_TCG2_PROTOCOL
> > > > > > >   config EFI_TCG2_PROTOCOL_EVENTLOG_SIZE
> > > > > > >       int "EFI_TCG2_PROTOCOL EventLog size"
> > > > > > >       depends on EFI_TCG2_PROTOCOL
> > > > > > > -     default 4096
> > > > > > > +     default 16384
> > > > > >
> > > > > > I found this text in EDK II:
> > > > > >
> > > > > > Minimum length(in bytes) of the system preboot TCG event log area(LAML)
> > > > > > -----------------------------------------------------------------------
> > > > > >
> > > > > > For PC Client Implementation spec up to and including 1.2 the minimum
> > > > > > log size is 64KB. (SecurityPkg/SecurityPkg.dec)
> > > > >
> > > > > Thank you for your feedback.
> > > > > I have not checked this.
> > > > > TCG spec also says "The Log Area Minimum Length for the TCG event log
> > > > > MUST be at least 64KB." in ACPI chapter.
> > > > > I will update to set 64KB as default.
> > > > >
> > > >
> > > > Is this the same as the BLOBLISTT_TPM2_TCG_LOG thing? If so, can we
> > > > put this in the bloblist? We want to avoid adding code in EFI which is
> > > > in U-Boot.
> > >
> > > I think bloblist is used for data passing from SPL/TPL to u-boot.
> > > Is your comment saying that the eventlog generated
> > > in u-boot(done in efi_tcg2.c with this patch series) should be appended
> > > into the buffer pointed by BLOBLISTT_TPM2_TCG_LOG blob?
> > >
> >
> > Even in that case the eventlog can't be appended.  The TCG eventlog
> > hould be copied into EFI memory, since the kernel expects to find it
> > there.
> 
> Typically bloblist is relocated by U-Boot. There are lots of tables
> that must be passed to linux, including ACPI and SMBIOS. With bloblist
> they can all be in one place.


The eventlog must be allocated in EFI memory though. 

> 
> > What we could do is copy the contents of that buffer to the eventlog.
> > Depending on what that buffer already has (e.g the starting header of
> > the eventlog), we might need to strip it from the efi_tcg.c code.
> 
> I'm not really sure, but the eventlog is not just EFI thing, right?
> The code should be generic.

It's purely an EFI construct.  Specifically the entire spec, and even the  log
format for the eventlog are described in 
https://trustedcomputinggroup.org/wp-content/uploads/EFI-Protocol-Specification-rev13-160330final.pdf


cheers
/Ilias
> 
> Regards,
> Simon

Re: [PATCH 1/5] efi_loader: increase eventlog buffer size

[Ilias Apalodimas]

On Thu, Jul 15, 2021 at 02:09:57PM +0900, Masahisa Kojima wrote:
> Hi Simon, Ilias,
> 
> On Wed, 14 Jul 2021 at 23:50, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Masahisa,
> >
> > On Mon, 12 Jul 2021 at 02:40, Masahisa Kojima
> > <masahisa.kojima@linaro.org> wrote:
> > >
> > > Hi Simon,
> > >
> > > On Sun, 11 Jul 2021 at 09:01, Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Masahisa,
> > > >
> > > > On Wed, 7 Jul 2021 at 20:21, Masahisa Kojima <masahisa.kojima@linaro.org> wrote:
> > > > >
> > > > > On Wed, 7 Jul 2021 at 22:47, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > > >
> > > > > >
> > > > > >
> > > > > > On 7/7/21 3:36 PM, Masahisa Kojima wrote:
> > > > > > > This is a preperation to add eventlog support
> > > > > > > described in TCG PC Client PFP spec.
> > > > > > >
> > > > > > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > > > > > > ---
> > > > > > >   lib/efi_loader/Kconfig | 2 +-
> > > > > > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > > > > > > index b2ab48a048..a87bf3cc98 100644
> > > > > > > --- a/lib/efi_loader/Kconfig
> > > > > > > +++ b/lib/efi_loader/Kconfig
> > > > > > > @@ -327,7 +327,7 @@ config EFI_TCG2_PROTOCOL
> > > > > > >   config EFI_TCG2_PROTOCOL_EVENTLOG_SIZE
> > > > > > >       int "EFI_TCG2_PROTOCOL EventLog size"
> > > > > > >       depends on EFI_TCG2_PROTOCOL
> > > > > > > -     default 4096
> > > > > > > +     default 16384
> > > > > >
> > > > > > I found this text in EDK II:
> > > > > >
> > > > > > Minimum length(in bytes) of the system preboot TCG event log area(LAML)
> > > > > > -----------------------------------------------------------------------
> > > > > >
> > > > > > For PC Client Implementation spec up to and including 1.2 the minimum
> > > > > > log size is 64KB. (SecurityPkg/SecurityPkg.dec)
> > > > >
> > > > > Thank you for your feedback.
> > > > > I have not checked this.
> > > > > TCG spec also says "The Log Area Minimum Length for the TCG event log
> > > > > MUST be at least 64KB." in ACPI chapter.
> > > > > I will update to set 64KB as default.
> > > > >
> > > >
> > > > Is this the same as the BLOBLISTT_TPM2_TCG_LOG thing? If so, can we
> > > > put this in the bloblist? We want to avoid adding code in EFI which is
> > > > in U-Boot.
> > >
> > > I think bloblist is used for data passing from SPL/TPL to u-boot.
> >
> > It can also be used to place things in memory that end up accessed by
> > Linux, e.g. ACPI tables. So I think it fits.
> 
> I understand bloblist can be used for eventlog, I will check further.
> 
> >
> > > Is your comment saying that the eventlog generated
> > > in u-boot(done in efi_tcg2.c with this patch series) should be appended
> > > into the buffer pointed by BLOBLISTT_TPM2_TCG_LOG blob?
> >
> > I suppose so, but this logic should be implemented in the TPM layer I
> > think, not just in EFI. Otherwise we end up with another parallel
> > implementation.
> 
> Current u-boot/lib/efi_loader/efi_tcg2.c includes the code
> not directly related to EFI.
> I would like to suggest to divide u-boot/lib/efi_loader/efi_tcg2.c
> into two files.
> 
>  1)  u-boot/lib/efi_loader/efi_tcg2.c
>   This file implements the EFI interfaces required in TCG EFI Protocol
> Specification(https://trustedcomputinggroup.org/resource/tcg-efi-protocol-specification/).
> 

The only problem I see with the way the efi tcg2 eventlog is currently
created, is that it's all done during the efi init.  Ideally this should
happen earlier, especially if SPL or TF-A create their own eventlog.

The status with TF-A atm is that it only creates an eventlog which then
copies to secure and non-secure memory, but it doesnt extend the PCRs. 
We should pick that eventlog from u-boot (or better op-tee), extend the 
PCRs based on the information of the log and then use it as our basis and
start appending events there. 

>  2) u-boot/lib/tcg2.c(new file)
>  This file implements the functionality required in TCG PC Client
> Platform Firmware Profile
> Specification(https://trustedcomputinggroup.org/resource/pc-client-specific-platform-firmware-profile-specification/).
>  This file contains the common functions to extend eventlog and PCRs, etc.

Splitting up the pc client spec bits is probably a good idea. 
What do you have in mind? Moving tcg2_pcr_extend() and
tcg2_agile_log_append() as well, or just the pc client related wrappers?

> 
> In addition, this is different topic, I found some tpm related files
> under u-boot/lib directory, I think it better to have new directory
> like "tcg" and move tpm related files such as tpm_api.c, tpm-v2.c
> and tpm-common.c  into lib/tcg(new directory).

+1

Regards
/Ilias
> 
> Thanks,
> Masahisa Kojima
> 
> >
> > Regards,
> > Simon

Re: [PATCH 1/5] efi_loader: increase eventlog buffer size

[Masahisa Kojima]

On Thu, 15 Jul 2021 at 15:46, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
>
> On Thu, Jul 15, 2021 at 02:09:57PM +0900, Masahisa Kojima wrote:
> > Hi Simon, Ilias,
> >
> > On Wed, 14 Jul 2021 at 23:50, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Masahisa,
> > >
> > > On Mon, 12 Jul 2021 at 02:40, Masahisa Kojima
> > > <masahisa.kojima@linaro.org> wrote:
> > > >
> > > > Hi Simon,
> > > >
> > > > On Sun, 11 Jul 2021 at 09:01, Simon Glass <sjg@chromium.org> wrote:
> > > > >
> > > > > Hi Masahisa,
> > > > >
> > > > > On Wed, 7 Jul 2021 at 20:21, Masahisa Kojima <masahisa.kojima@linaro.org> wrote:
> > > > > >
> > > > > > On Wed, 7 Jul 2021 at 22:47, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > On 7/7/21 3:36 PM, Masahisa Kojima wrote:
> > > > > > > > This is a preperation to add eventlog support
> > > > > > > > described in TCG PC Client PFP spec.
> > > > > > > >
> > > > > > > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > > > > > > > ---
> > > > > > > >   lib/efi_loader/Kconfig | 2 +-
> > > > > > > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > > >
> > > > > > > > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > > > > > > > index b2ab48a048..a87bf3cc98 100644
> > > > > > > > --- a/lib/efi_loader/Kconfig
> > > > > > > > +++ b/lib/efi_loader/Kconfig
> > > > > > > > @@ -327,7 +327,7 @@ config EFI_TCG2_PROTOCOL
> > > > > > > >   config EFI_TCG2_PROTOCOL_EVENTLOG_SIZE
> > > > > > > >       int "EFI_TCG2_PROTOCOL EventLog size"
> > > > > > > >       depends on EFI_TCG2_PROTOCOL
> > > > > > > > -     default 4096
> > > > > > > > +     default 16384
> > > > > > >
> > > > > > > I found this text in EDK II:
> > > > > > >
> > > > > > > Minimum length(in bytes) of the system preboot TCG event log area(LAML)
> > > > > > > -----------------------------------------------------------------------
> > > > > > >
> > > > > > > For PC Client Implementation spec up to and including 1.2 the minimum
> > > > > > > log size is 64KB. (SecurityPkg/SecurityPkg.dec)
> > > > > >
> > > > > > Thank you for your feedback.
> > > > > > I have not checked this.
> > > > > > TCG spec also says "The Log Area Minimum Length for the TCG event log
> > > > > > MUST be at least 64KB." in ACPI chapter.
> > > > > > I will update to set 64KB as default.
> > > > > >
> > > > >
> > > > > Is this the same as the BLOBLISTT_TPM2_TCG_LOG thing? If so, can we
> > > > > put this in the bloblist? We want to avoid adding code in EFI which is
> > > > > in U-Boot.
> > > >
> > > > I think bloblist is used for data passing from SPL/TPL to u-boot.
> > >
> > > It can also be used to place things in memory that end up accessed by
> > > Linux, e.g. ACPI tables. So I think it fits.
> >
> > I understand bloblist can be used for eventlog, I will check further.
> >
> > >
> > > > Is your comment saying that the eventlog generated
> > > > in u-boot(done in efi_tcg2.c with this patch series) should be appended
> > > > into the buffer pointed by BLOBLISTT_TPM2_TCG_LOG blob?
> > >
> > > I suppose so, but this logic should be implemented in the TPM layer I
> > > think, not just in EFI. Otherwise we end up with another parallel
> > > implementation.
> >
> > Current u-boot/lib/efi_loader/efi_tcg2.c includes the code
> > not directly related to EFI.
> > I would like to suggest to divide u-boot/lib/efi_loader/efi_tcg2.c
> > into two files.
> >
> >  1)  u-boot/lib/efi_loader/efi_tcg2.c
> >   This file implements the EFI interfaces required in TCG EFI Protocol
> > Specification(https://trustedcomputinggroup.org/resource/tcg-efi-protocol-specification/).
> >
>
> The only problem I see with the way the efi tcg2 eventlog is currently
> created, is that it's all done during the efi init.  Ideally this should
> happen earlier, especially if SPL or TF-A create their own eventlog.
>
> The status with TF-A atm is that it only creates an eventlog which then
> copies to secure and non-secure memory, but it doesnt extend the PCRs.
> We should pick that eventlog from u-boot (or better op-tee), extend the
> PCRs based on the information of the log and then use it as our basis and
> start appending events there.
>
> >  2) u-boot/lib/tcg2.c(new file)
> >  This file implements the functionality required in TCG PC Client
> > Platform Firmware Profile
> > Specification(https://trustedcomputinggroup.org/resource/pc-client-specific-platform-firmware-profile-specification/).
> >  This file contains the common functions to extend eventlog and PCRs, etc.
>
> Splitting up the pc client spec bits is probably a good idea.
> What do you have in mind? Moving tcg2_pcr_extend() and
> tcg2_agile_log_append() as well, or just the pc client related wrappers?

Sorry but I was confused.
I checked spec again, there are many duplication in TCG EFI Protocol spec
and TCG PC Client PFP spec.
For example,  tdTCG_EfiSpecIdEvent structure is defined in both spec.
On second thought, it is difficult to split the pc client spec into new file,
so I would like to withdraw my suggestion earlier.

Thanks,
Masahisa Kojima


>
> >
> > In addition, this is different topic, I found some tpm related files
> > under u-boot/lib directory, I think it better to have new directory
> > like "tcg" and move tpm related files such as tpm_api.c, tpm-v2.c
> > and tpm-common.c  into lib/tcg(new directory).
>
> +1
>
> Regards
> /Ilias
> >
> > Thanks,
> > Masahisa Kojima
> >
> > >
> > > Regards,
> > > Simon

Re: [PATCH 1/5] efi_loader: increase eventlog buffer size

[Simon Glass]

Hi Ilias,

On Thu, 15 Jul 2021 at 00:20, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> On Wed, Jul 14, 2021 at 08:52:07AM -0600, Simon Glass wrote:
> > Hi Ilias,
> >
> > On Mon, 12 Jul 2021 at 03:28, Ilias Apalodimas
> > <ilias.apalodimas@linaro.org> wrote:
> > >
> > > On Mon, 12 Jul 2021 at 11:40, Masahisa Kojima
> > > <masahisa.kojima@linaro.org> wrote:
> > > >
> > > > Hi Simon,
> > > >
> > > > On Sun, 11 Jul 2021 at 09:01, Simon Glass <sjg@chromium.org> wrote:
> > > > >
> > > > > Hi Masahisa,
> > > > >
> > > > > On Wed, 7 Jul 2021 at 20:21, Masahisa Kojima <masahisa.kojima@linaro.org> wrote:
> > > > > >
> > > > > > On Wed, 7 Jul 2021 at 22:47, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > On 7/7/21 3:36 PM, Masahisa Kojima wrote:
> > > > > > > > This is a preperation to add eventlog support
> > > > > > > > described in TCG PC Client PFP spec.
> > > > > > > >
> > > > > > > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > > > > > > > ---
> > > > > > > >   lib/efi_loader/Kconfig | 2 +-
> > > > > > > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > > >
> > > > > > > > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > > > > > > > index b2ab48a048..a87bf3cc98 100644
> > > > > > > > --- a/lib/efi_loader/Kconfig
> > > > > > > > +++ b/lib/efi_loader/Kconfig
> > > > > > > > @@ -327,7 +327,7 @@ config EFI_TCG2_PROTOCOL
> > > > > > > >   config EFI_TCG2_PROTOCOL_EVENTLOG_SIZE
> > > > > > > >       int "EFI_TCG2_PROTOCOL EventLog size"
> > > > > > > >       depends on EFI_TCG2_PROTOCOL
> > > > > > > > -     default 4096
> > > > > > > > +     default 16384
> > > > > > >
> > > > > > > I found this text in EDK II:
> > > > > > >
> > > > > > > Minimum length(in bytes) of the system preboot TCG event log area(LAML)
> > > > > > > -----------------------------------------------------------------------
> > > > > > >
> > > > > > > For PC Client Implementation spec up to and including 1.2 the minimum
> > > > > > > log size is 64KB. (SecurityPkg/SecurityPkg.dec)
> > > > > >
> > > > > > Thank you for your feedback.
> > > > > > I have not checked this.
> > > > > > TCG spec also says "The Log Area Minimum Length for the TCG event log
> > > > > > MUST be at least 64KB." in ACPI chapter.
> > > > > > I will update to set 64KB as default.
> > > > > >
> > > > >
> > > > > Is this the same as the BLOBLISTT_TPM2_TCG_LOG thing? If so, can we
> > > > > put this in the bloblist? We want to avoid adding code in EFI which is
> > > > > in U-Boot.
> > > >
> > > > I think bloblist is used for data passing from SPL/TPL to u-boot.
> > > > Is your comment saying that the eventlog generated
> > > > in u-boot(done in efi_tcg2.c with this patch series) should be appended
> > > > into the buffer pointed by BLOBLISTT_TPM2_TCG_LOG blob?
> > > >
> > >
> > > Even in that case the eventlog can't be appended.  The TCG eventlog
> > > hould be copied into EFI memory, since the kernel expects to find it
> > > there.
> >
> > Typically bloblist is relocated by U-Boot. There are lots of tables
> > that must be passed to linux, including ACPI and SMBIOS. With bloblist
> > they can all be in one place.
>
>
> The eventlog must be allocated in EFI memory though.

There is really only one memory in U-Boot. I feel that all stuff that
EFI passes on to linux should be in a single bloblist.

>
> >
> > > What we could do is copy the contents of that buffer to the eventlog.
> > > Depending on what that buffer already has (e.g the starting header of
> > > the eventlog), we might need to strip it from the efi_tcg.c code.
> >
> > I'm not really sure, but the eventlog is not just EFI thing, right?
> > The code should be generic.
>
> It's purely an EFI construct.  Specifically the entire spec, and even the  log
> format for the eventlog are described in
> https://trustedcomputinggroup.org/wp-content/uploads/EFI-Protocol-Specification-rev13-160330final.pdf

For some reason I have seen this in ACPI, or something similar.
Perhaps I was getting confused.

We need to find ways to implement EFI things with generic code. I'm
not 100% sure about this particular thing, but since we already create
something similar with ACPI I think we should at least look at doing
it in one place.

Regards,
Simon

Re: [PATCH 1/5] efi_loader: increase eventlog buffer size

[Heinrich Schuchardt]

On 7/15/21 2:57 PM, Simon Glass wrote:
> Hi Ilias,
>
> On Thu, 15 Jul 2021 at 00:20, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
>>
>> On Wed, Jul 14, 2021 at 08:52:07AM -0600, Simon Glass wrote:
>>> Hi Ilias,
>>>
>>> On Mon, 12 Jul 2021 at 03:28, Ilias Apalodimas
>>> <ilias.apalodimas@linaro.org> wrote:
>>>>
>>>> On Mon, 12 Jul 2021 at 11:40, Masahisa Kojima
>>>> <masahisa.kojima@linaro.org> wrote:
>>>>>
>>>>> Hi Simon,
>>>>>
>>>>> On Sun, 11 Jul 2021 at 09:01, Simon Glass <sjg@chromium.org> wrote:
>>>>>>
>>>>>> Hi Masahisa,
>>>>>>
>>>>>> On Wed, 7 Jul 2021 at 20:21, Masahisa Kojima <masahisa.kojima@linaro.org> wrote:
>>>>>>>
>>>>>>> On Wed, 7 Jul 2021 at 22:47, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 7/7/21 3:36 PM, Masahisa Kojima wrote:
>>>>>>>>> This is a preperation to add eventlog support
>>>>>>>>> described in TCG PC Client PFP spec.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
>>>>>>>>> ---
>>>>>>>>>    lib/efi_loader/Kconfig | 2 +-
>>>>>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>>
>>>>>>>>> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
>>>>>>>>> index b2ab48a048..a87bf3cc98 100644
>>>>>>>>> --- a/lib/efi_loader/Kconfig
>>>>>>>>> +++ b/lib/efi_loader/Kconfig
>>>>>>>>> @@ -327,7 +327,7 @@ config EFI_TCG2_PROTOCOL
>>>>>>>>>    config EFI_TCG2_PROTOCOL_EVENTLOG_SIZE
>>>>>>>>>        int "EFI_TCG2_PROTOCOL EventLog size"
>>>>>>>>>        depends on EFI_TCG2_PROTOCOL
>>>>>>>>> -     default 4096
>>>>>>>>> +     default 16384
>>>>>>>>
>>>>>>>> I found this text in EDK II:
>>>>>>>>
>>>>>>>> Minimum length(in bytes) of the system preboot TCG event log area(LAML)
>>>>>>>> -----------------------------------------------------------------------
>>>>>>>>
>>>>>>>> For PC Client Implementation spec up to and including 1.2 the minimum
>>>>>>>> log size is 64KB. (SecurityPkg/SecurityPkg.dec)
>>>>>>>
>>>>>>> Thank you for your feedback.
>>>>>>> I have not checked this.
>>>>>>> TCG spec also says "The Log Area Minimum Length for the TCG event log
>>>>>>> MUST be at least 64KB." in ACPI chapter.
>>>>>>> I will update to set 64KB as default.
>>>>>>>
>>>>>>
>>>>>> Is this the same as the BLOBLISTT_TPM2_TCG_LOG thing? If so, can we
>>>>>> put this in the bloblist? We want to avoid adding code in EFI which is
>>>>>> in U-Boot.
>>>>>
>>>>> I think bloblist is used for data passing from SPL/TPL to u-boot.
>>>>> Is your comment saying that the eventlog generated
>>>>> in u-boot(done in efi_tcg2.c with this patch series) should be appended
>>>>> into the buffer pointed by BLOBLISTT_TPM2_TCG_LOG blob?
>>>>>
>>>>
>>>> Even in that case the eventlog can't be appended.  The TCG eventlog
>>>> hould be copied into EFI memory, since the kernel expects to find it
>>>> there.
>>>
>>> Typically bloblist is relocated by U-Boot. There are lots of tables
>>> that must be passed to linux, including ACPI and SMBIOS. With bloblist
>>> they can all be in one place.
>>
>>
>> The eventlog must be allocated in EFI memory though.
>
> There is really only one memory in U-Boot. I feel that all stuff that
> EFI passes on to linux should be in a single bloblist.

We have should follow existing standards and not invent our own. LInux
is not the only OS booted via U-Boot.

Best regards

Heinrich

>
>>
>>>
>>>> What we could do is copy the contents of that buffer to the eventlog.
>>>> Depending on what that buffer already has (e.g the starting header of
>>>> the eventlog), we might need to strip it from the efi_tcg.c code.
>>>
>>> I'm not really sure, but the eventlog is not just EFI thing, right?
>>> The code should be generic.
>>
>> It's purely an EFI construct.  Specifically the entire spec, and even the  log
>> format for the eventlog are described in
>> https://trustedcomputinggroup.org/wp-content/uploads/EFI-Protocol-Specification-rev13-160330final.pdf
>
> For some reason I have seen this in ACPI, or something similar.
> Perhaps I was getting confused.
>
> We need to find ways to implement EFI things with generic code. I'm
> not 100% sure about this particular thing, but since we already create
> something similar with ACPI I think we should at least look at doing
> it in one place.
>
> Regards,
> Simon
>

Re: [PATCH 1/5] efi_loader: increase eventlog buffer size

[Simon Glass]

Hi Heinrich,

On Thu, 15 Jul 2021 at 08:38, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 7/15/21 2:57 PM, Simon Glass wrote:
> > Hi Ilias,
> >
> > On Thu, 15 Jul 2021 at 00:20, Ilias Apalodimas
> > <ilias.apalodimas@linaro.org> wrote:
> >>
> >> On Wed, Jul 14, 2021 at 08:52:07AM -0600, Simon Glass wrote:
> >>> Hi Ilias,
> >>>
> >>> On Mon, 12 Jul 2021 at 03:28, Ilias Apalodimas
> >>> <ilias.apalodimas@linaro.org> wrote:
> >>>>
> >>>> On Mon, 12 Jul 2021 at 11:40, Masahisa Kojima
> >>>> <masahisa.kojima@linaro.org> wrote:
> >>>>>
> >>>>> Hi Simon,
> >>>>>
> >>>>> On Sun, 11 Jul 2021 at 09:01, Simon Glass <sjg@chromium.org> wrote:
> >>>>>>
> >>>>>> Hi Masahisa,
> >>>>>>
> >>>>>> On Wed, 7 Jul 2021 at 20:21, Masahisa Kojima <masahisa.kojima@linaro.org> wrote:
> >>>>>>>
> >>>>>>> On Wed, 7 Jul 2021 at 22:47, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> On 7/7/21 3:36 PM, Masahisa Kojima wrote:
> >>>>>>>>> This is a preperation to add eventlog support
> >>>>>>>>> described in TCG PC Client PFP spec.
> >>>>>>>>>
> >>>>>>>>> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> >>>>>>>>> ---
> >>>>>>>>>    lib/efi_loader/Kconfig | 2 +-
> >>>>>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>>>>>
> >>>>>>>>> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> >>>>>>>>> index b2ab48a048..a87bf3cc98 100644
> >>>>>>>>> --- a/lib/efi_loader/Kconfig
> >>>>>>>>> +++ b/lib/efi_loader/Kconfig
> >>>>>>>>> @@ -327,7 +327,7 @@ config EFI_TCG2_PROTOCOL
> >>>>>>>>>    config EFI_TCG2_PROTOCOL_EVENTLOG_SIZE
> >>>>>>>>>        int "EFI_TCG2_PROTOCOL EventLog size"
> >>>>>>>>>        depends on EFI_TCG2_PROTOCOL
> >>>>>>>>> -     default 4096
> >>>>>>>>> +     default 16384
> >>>>>>>>
> >>>>>>>> I found this text in EDK II:
> >>>>>>>>
> >>>>>>>> Minimum length(in bytes) of the system preboot TCG event log area(LAML)
> >>>>>>>> -----------------------------------------------------------------------
> >>>>>>>>
> >>>>>>>> For PC Client Implementation spec up to and including 1.2 the minimum
> >>>>>>>> log size is 64KB. (SecurityPkg/SecurityPkg.dec)
> >>>>>>>
> >>>>>>> Thank you for your feedback.
> >>>>>>> I have not checked this.
> >>>>>>> TCG spec also says "The Log Area Minimum Length for the TCG event log
> >>>>>>> MUST be at least 64KB." in ACPI chapter.
> >>>>>>> I will update to set 64KB as default.
> >>>>>>>
> >>>>>>
> >>>>>> Is this the same as the BLOBLISTT_TPM2_TCG_LOG thing? If so, can we
> >>>>>> put this in the bloblist? We want to avoid adding code in EFI which is
> >>>>>> in U-Boot.
> >>>>>
> >>>>> I think bloblist is used for data passing from SPL/TPL to u-boot.
> >>>>> Is your comment saying that the eventlog generated
> >>>>> in u-boot(done in efi_tcg2.c with this patch series) should be appended
> >>>>> into the buffer pointed by BLOBLISTT_TPM2_TCG_LOG blob?
> >>>>>
> >>>>
> >>>> Even in that case the eventlog can't be appended.  The TCG eventlog
> >>>> hould be copied into EFI memory, since the kernel expects to find it
> >>>> there.
> >>>
> >>> Typically bloblist is relocated by U-Boot. There are lots of tables
> >>> that must be passed to linux, including ACPI and SMBIOS. With bloblist
> >>> they can all be in one place.
> >>
> >>
> >> The eventlog must be allocated in EFI memory though.
> >
> > There is really only one memory in U-Boot. I feel that all stuff that
> > EFI passes on to linux should be in a single bloblist.
>
> We have should follow existing standards and not invent our own. LInux
> is not the only OS booted via U-Boot.

Perhaps we can talk about it in the next call. My point is not about
avoiding standards!

What I am saying is that if we put things in a bloblist, and make that
available to Linux (or other OS) via EFI, things should work, but
non-EFI people are happy too. See the ACPI stuff for example - we put
all of those bits in a bloblist, which is really just a contiguous
area of memory. It is more convenient for U-Boot than allocating
memory willy nilly. Plus the 'bloblist' command lets you see what is
there.

Anyway I really don't understand all of this well enough to say what
we should do. I am just passing on hints. There is way too much
'separate' EFI code in U-Boot at present and we need to work to reduce
that and hopefully not add more.

[..]

Regards,
Simon

Re: [PATCH 1/5] efi_loader: increase eventlog buffer size

[Heinrich Schuchardt]

On 15.07.21 17:18, Simon Glass wrote:
> Hi Heinrich,
>
> On Thu, 15 Jul 2021 at 08:38, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 7/15/21 2:57 PM, Simon Glass wrote:
>>> Hi Ilias,
>>>
>>> On Thu, 15 Jul 2021 at 00:20, Ilias Apalodimas
>>> <ilias.apalodimas@linaro.org> wrote:
>>>>
>>>> On Wed, Jul 14, 2021 at 08:52:07AM -0600, Simon Glass wrote:
>>>>> Hi Ilias,
>>>>>
>>>>> On Mon, 12 Jul 2021 at 03:28, Ilias Apalodimas
>>>>> <ilias.apalodimas@linaro.org> wrote:
>>>>>>
>>>>>> On Mon, 12 Jul 2021 at 11:40, Masahisa Kojima
>>>>>> <masahisa.kojima@linaro.org> wrote:
>>>>>>>
>>>>>>> Hi Simon,
>>>>>>>
>>>>>>> On Sun, 11 Jul 2021 at 09:01, Simon Glass <sjg@chromium.org> wrote:
>>>>>>>>
>>>>>>>> Hi Masahisa,
>>>>>>>>
>>>>>>>> On Wed, 7 Jul 2021 at 20:21, Masahisa Kojima <masahisa.kojima@linaro.org> wrote:
>>>>>>>>>
>>>>>>>>> On Wed, 7 Jul 2021 at 22:47, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 7/7/21 3:36 PM, Masahisa Kojima wrote:
>>>>>>>>>>> This is a preperation to add eventlog support
>>>>>>>>>>> described in TCG PC Client PFP spec.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
>>>>>>>>>>> ---
>>>>>>>>>>>     lib/efi_loader/Kconfig | 2 +-
>>>>>>>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
>>>>>>>>>>> index b2ab48a048..a87bf3cc98 100644
>>>>>>>>>>> --- a/lib/efi_loader/Kconfig
>>>>>>>>>>> +++ b/lib/efi_loader/Kconfig
>>>>>>>>>>> @@ -327,7 +327,7 @@ config EFI_TCG2_PROTOCOL
>>>>>>>>>>>     config EFI_TCG2_PROTOCOL_EVENTLOG_SIZE
>>>>>>>>>>>         int "EFI_TCG2_PROTOCOL EventLog size"
>>>>>>>>>>>         depends on EFI_TCG2_PROTOCOL
>>>>>>>>>>> -     default 4096
>>>>>>>>>>> +     default 16384
>>>>>>>>>>
>>>>>>>>>> I found this text in EDK II:
>>>>>>>>>>
>>>>>>>>>> Minimum length(in bytes) of the system preboot TCG event log area(LAML)
>>>>>>>>>> -----------------------------------------------------------------------
>>>>>>>>>>
>>>>>>>>>> For PC Client Implementation spec up to and including 1.2 the minimum
>>>>>>>>>> log size is 64KB. (SecurityPkg/SecurityPkg.dec)
>>>>>>>>>
>>>>>>>>> Thank you for your feedback.
>>>>>>>>> I have not checked this.
>>>>>>>>> TCG spec also says "The Log Area Minimum Length for the TCG event log
>>>>>>>>> MUST be at least 64KB." in ACPI chapter.
>>>>>>>>> I will update to set 64KB as default.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Is this the same as the BLOBLISTT_TPM2_TCG_LOG thing? If so, can we
>>>>>>>> put this in the bloblist? We want to avoid adding code in EFI which is
>>>>>>>> in U-Boot.
>>>>>>>
>>>>>>> I think bloblist is used for data passing from SPL/TPL to u-boot.
>>>>>>> Is your comment saying that the eventlog generated
>>>>>>> in u-boot(done in efi_tcg2.c with this patch series) should be appended
>>>>>>> into the buffer pointed by BLOBLISTT_TPM2_TCG_LOG blob?
>>>>>>>
>>>>>>
>>>>>> Even in that case the eventlog can't be appended.  The TCG eventlog
>>>>>> hould be copied into EFI memory, since the kernel expects to find it
>>>>>> there.
>>>>>
>>>>> Typically bloblist is relocated by U-Boot. There are lots of tables
>>>>> that must be passed to linux, including ACPI and SMBIOS. With bloblist
>>>>> they can all be in one place.
>>>>
>>>>
>>>> The eventlog must be allocated in EFI memory though.
>>>
>>> There is really only one memory in U-Boot. I feel that all stuff that
>>> EFI passes on to linux should be in a single bloblist.
>>
>> We have should follow existing standards and not invent our own. LInux
>> is not the only OS booted via U-Boot.
>
> Perhaps we can talk about it in the next call. My point is not about
> avoiding standards!
>
> What I am saying is that if we put things in a bloblist, and make that
> available to Linux (or other OS) via EFI, things should work, but

Which operating would be aware of your bloblist? Windows, BSD, Haiku?

We want U-Boot to be interchangable with other UEFI firmware like EDK
II. This will only work if we program against the same specs and don't
invent new interfaces.

Best regards

Heinrich

> non-EFI people are happy too. See the ACPI stuff for example - we put
> all of those bits in a bloblist, which is really just a contiguous
> area of memory. It is more convenient for U-Boot than allocating
> memory willy nilly. Plus the 'bloblist' command lets you see what is
> there.
>
> Anyway I really don't understand all of this well enough to say what
> we should do. I am just passing on hints. There is way too much
> 'separate' EFI code in U-Boot at present and we need to work to reduce
> that and hopefully not add more.
>
> [..]
>
> Regards,
> Simon
>

Re: [PATCH 1/5] efi_loader: increase eventlog buffer size

[Simon Glass]

Hi Heinrich,

On Thu, 15 Jul 2021 at 09:35, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
>
>
> On 15.07.21 17:18, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On Thu, 15 Jul 2021 at 08:38, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >> On 7/15/21 2:57 PM, Simon Glass wrote:
> >>> Hi Ilias,
> >>>
> >>> On Thu, 15 Jul 2021 at 00:20, Ilias Apalodimas
> >>> <ilias.apalodimas@linaro.org> wrote:
> >>>>
> >>>> On Wed, Jul 14, 2021 at 08:52:07AM -0600, Simon Glass wrote:
> >>>>> Hi Ilias,
> >>>>>
> >>>>> On Mon, 12 Jul 2021 at 03:28, Ilias Apalodimas
> >>>>> <ilias.apalodimas@linaro.org> wrote:
> >>>>>>
> >>>>>> On Mon, 12 Jul 2021 at 11:40, Masahisa Kojima
> >>>>>> <masahisa.kojima@linaro.org> wrote:
> >>>>>>>
> >>>>>>> Hi Simon,
> >>>>>>>
> >>>>>>> On Sun, 11 Jul 2021 at 09:01, Simon Glass <sjg@chromium.org> wrote:
> >>>>>>>>
> >>>>>>>> Hi Masahisa,
> >>>>>>>>
> >>>>>>>> On Wed, 7 Jul 2021 at 20:21, Masahisa Kojima <masahisa.kojima@linaro.org> wrote:
> >>>>>>>>>
> >>>>>>>>> On Wed, 7 Jul 2021 at 22:47, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> On 7/7/21 3:36 PM, Masahisa Kojima wrote:
> >>>>>>>>>>> This is a preperation to add eventlog support
> >>>>>>>>>>> described in TCG PC Client PFP spec.
> >>>>>>>>>>>
> >>>>>>>>>>> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> >>>>>>>>>>> ---
> >>>>>>>>>>>     lib/efi_loader/Kconfig | 2 +-
> >>>>>>>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>>>>>>>
> >>>>>>>>>>> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> >>>>>>>>>>> index b2ab48a048..a87bf3cc98 100644
> >>>>>>>>>>> --- a/lib/efi_loader/Kconfig
> >>>>>>>>>>> +++ b/lib/efi_loader/Kconfig
> >>>>>>>>>>> @@ -327,7 +327,7 @@ config EFI_TCG2_PROTOCOL
> >>>>>>>>>>>     config EFI_TCG2_PROTOCOL_EVENTLOG_SIZE
> >>>>>>>>>>>         int "EFI_TCG2_PROTOCOL EventLog size"
> >>>>>>>>>>>         depends on EFI_TCG2_PROTOCOL
> >>>>>>>>>>> -     default 4096
> >>>>>>>>>>> +     default 16384
> >>>>>>>>>>
> >>>>>>>>>> I found this text in EDK II:
> >>>>>>>>>>
> >>>>>>>>>> Minimum length(in bytes) of the system preboot TCG event log area(LAML)
> >>>>>>>>>> -----------------------------------------------------------------------
> >>>>>>>>>>
> >>>>>>>>>> For PC Client Implementation spec up to and including 1.2 the minimum
> >>>>>>>>>> log size is 64KB. (SecurityPkg/SecurityPkg.dec)
> >>>>>>>>>
> >>>>>>>>> Thank you for your feedback.
> >>>>>>>>> I have not checked this.
> >>>>>>>>> TCG spec also says "The Log Area Minimum Length for the TCG event log
> >>>>>>>>> MUST be at least 64KB." in ACPI chapter.
> >>>>>>>>> I will update to set 64KB as default.
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> Is this the same as the BLOBLISTT_TPM2_TCG_LOG thing? If so, can we
> >>>>>>>> put this in the bloblist? We want to avoid adding code in EFI which is
> >>>>>>>> in U-Boot.
> >>>>>>>
> >>>>>>> I think bloblist is used for data passing from SPL/TPL to u-boot.
> >>>>>>> Is your comment saying that the eventlog generated
> >>>>>>> in u-boot(done in efi_tcg2.c with this patch series) should be appended
> >>>>>>> into the buffer pointed by BLOBLISTT_TPM2_TCG_LOG blob?
> >>>>>>>
> >>>>>>
> >>>>>> Even in that case the eventlog can't be appended.  The TCG eventlog
> >>>>>> hould be copied into EFI memory, since the kernel expects to find it
> >>>>>> there.
> >>>>>
> >>>>> Typically bloblist is relocated by U-Boot. There are lots of tables
> >>>>> that must be passed to linux, including ACPI and SMBIOS. With bloblist
> >>>>> they can all be in one place.
> >>>>
> >>>>
> >>>> The eventlog must be allocated in EFI memory though.
> >>>
> >>> There is really only one memory in U-Boot. I feel that all stuff that
> >>> EFI passes on to linux should be in a single bloblist.
> >>
> >> We have should follow existing standards and not invent our own. LInux
> >> is not the only OS booted via U-Boot.
> >
> > Perhaps we can talk about it in the next call. My point is not about
> > avoiding standards!
> >
> > What I am saying is that if we put things in a bloblist, and make that
> > available to Linux (or other OS) via EFI, things should work, but
>
> Which operating would be aware of your bloblist? Windows, BSD, Haiku?

None, it is not necessary. The bloblist is a U-Boot construct, a
container for blobs. We can pass a pointer to the blob through the EFI
tables, as we do with ACPI.

>
> We want U-Boot to be interchangable with other UEFI firmware like EDK
> II. This will only work if we program against the same specs and don't
> invent new interfaces.

This is not a new interface. Let's chat about it in a contributor call.

Regards,
Simon

>
> Best regards
>
> Heinrich
>
> > non-EFI people are happy too. See the ACPI stuff for example - we put
> > all of those bits in a bloblist, which is really just a contiguous
> > area of memory. It is more convenient for U-Boot than allocating
> > memory willy nilly. Plus the 'bloblist' command lets you see what is
> > there.
> >
> > Anyway I really don't understand all of this well enough to say what
> > we should do. I am just passing on hints. There is way too much
> > 'separate' EFI code in U-Boot at present and we need to work to reduce
> > that and hopefully not add more.
> >
> > [..]
> >
> > Regards,
> > Simon
> >