[PATCH 0/3] fix TCG2 error handling

[PATCH 0/3] fix TCG2 error handling

[Masahisa Kojima]

This series fix the efi_tcg2.c error handling.

Masahisa Kojima (3):
  efi_loader: efi_tcg2_register returns appropriate error
  efi_loader: check tcg2 protocol installation outside the TCG protocol
  efi_loader: correctly handle tcg2_measure_pe_image() error

 include/efi_loader.h              |  2 +
 lib/efi_loader/Kconfig            |  2 +
 lib/efi_loader/efi_image_loader.c | 11 +++-
 lib/efi_loader/efi_setup.c        |  2 +
 lib/efi_loader/efi_tcg2.c         | 85 ++++++++++++++++++++++++-------
 5 files changed, 81 insertions(+), 21 deletions(-)

-- 
2.17.1

[PATCH 1/3] efi_loader: efi_tcg2_register returns appropriate error

[Masahisa Kojima]

This commit modify efi_tcg2_register() to return the
appropriate error.
With this fix, sandbox will not boot because efi_tcg2_register()
fails due to some missing feature in GetCapabilities.
So disable sandbox if EFI_TCG2_PROTOCOL is enabled.

UEFI secure boot variable measurement is not directly related
to TCG2 protocol installation, tcg2_measure_secure_boot_variable()
is moved to the separate function.

Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
---
 include/efi_loader.h       |  2 ++
 lib/efi_loader/Kconfig     |  2 ++
 lib/efi_loader/efi_setup.c |  2 ++
 lib/efi_loader/efi_tcg2.c  | 65 +++++++++++++++++++++++++++-----------
 4 files changed, 53 insertions(+), 18 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index 67c40ca57a..f4860e87fc 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -525,6 +525,8 @@ efi_status_t efi_disk_register(void);
 efi_status_t efi_rng_register(void);
 /* Called by efi_init_obj_list() to install EFI_TCG2_PROTOCOL */
 efi_status_t efi_tcg2_register(void);
+/* Called by efi_init_obj_list() to do initial measurement */
+efi_status_t efi_tcg2_do_initial_measurement(void);
 /* measure the pe-coff image, extend PCR and add Event Log */
 efi_status_t tcg2_measure_pe_image(void *efi, u64 efi_size,
 				   struct efi_loaded_image_obj *handle,
diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
index 700dc838dd..24f9a2bb75 100644
--- a/lib/efi_loader/Kconfig
+++ b/lib/efi_loader/Kconfig
@@ -308,6 +308,8 @@ config EFI_TCG2_PROTOCOL
 	bool "EFI_TCG2_PROTOCOL support"
 	default y
 	depends on TPM_V2
+	# Sandbox TPM currently fails on GetCapabilities needed for TCG2
+	depends on !SANDBOX
 	select SHA1
 	select SHA256
 	select SHA384
diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
index 1aba71cd96..f58a4afa7f 100644
--- a/lib/efi_loader/efi_setup.c
+++ b/lib/efi_loader/efi_setup.c
@@ -241,6 +241,8 @@ efi_status_t efi_init_obj_list(void)
 		ret = efi_tcg2_register();
 		if (ret != EFI_SUCCESS)
 			goto out;
+
+		efi_tcg2_do_initial_measurement();
 	}
 
 	/* Secure boot */
diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
index 5f71b188a0..6dbdd35f29 100644
--- a/lib/efi_loader/efi_tcg2.c
+++ b/lib/efi_loader/efi_tcg2.c
@@ -153,6 +153,15 @@ static u16 alg_to_len(u16 hash_alg)
 	return 0;
 }
 
+static bool is_tcg2_protocol_installed(void)
+{
+	struct efi_handler *handler;
+	efi_status_t ret;
+
+	ret = efi_search_protocol(efi_root, &efi_guid_tcg2_protocol, &handler);
+	return ret == EFI_SUCCESS;
+}
+
 static u32 tcg_event_final_size(struct tpml_digest_values *digest_list)
 {
 	u32 len;
@@ -1664,6 +1673,14 @@ void tcg2_uninit(void)
 	event_log.buffer = NULL;
 	efi_free_pool(event_log.final_buffer);
 	event_log.final_buffer = NULL;
+
+	if (!is_tcg2_protocol_installed())
+		return;
+
+	ret = efi_remove_protocol(efi_root, &efi_guid_tcg2_protocol,
+				  (void *)&efi_tcg2_protocol);
+	if (ret != EFI_SUCCESS)
+		log_err("Failed to remove EFI TCG2 protocol\n");
 }
 
 /**
@@ -2345,12 +2362,37 @@ error:
 	return ret;
 }
 
+/**
+ * efi_tcg2_do_initial_measurement() - do initial measurement
+ *
+ * Return:	status code
+ */
+efi_status_t efi_tcg2_do_initial_measurement(void)
+{
+	efi_status_t ret;
+	struct udevice *dev;
+
+	if (!is_tcg2_protocol_installed())
+		return EFI_SUCCESS;
+
+	ret = platform_get_tpm2_device(&dev);
+	if (ret != EFI_SUCCESS)
+		goto out;
+
+	ret = tcg2_measure_secure_boot_variable(dev);
+	if (ret != EFI_SUCCESS)
+		goto out;
+
+out:
+	return ret;
+}
+
 /**
  * efi_tcg2_register() - register EFI_TCG2_PROTOCOL
  *
  * If a TPM2 device is available, the TPM TCG2 Protocol is registered
  *
- * Return:	An error status is only returned if adding the protocol fails.
+ * Return:	status code
  */
 efi_status_t efi_tcg2_register(void)
 {
@@ -2373,8 +2415,10 @@ efi_status_t efi_tcg2_register(void)
 	}
 
 	ret = efi_init_event_log();
-	if (ret != EFI_SUCCESS)
+	if (ret != EFI_SUCCESS) {
+		tcg2_uninit();
 		goto fail;
+	}
 
 	ret = efi_add_protocol(efi_root, &efi_guid_tcg2_protocol,
 			       (void *)&efi_tcg2_protocol);
@@ -2391,24 +2435,9 @@ efi_status_t efi_tcg2_register(void)
 		goto fail;
 	}
 
-	ret = tcg2_measure_secure_boot_variable(dev);
-	if (ret != EFI_SUCCESS) {
-		tcg2_uninit();
-		goto fail;
-	}
-
 	return ret;
 
 fail:
 	log_err("Cannot install EFI_TCG2_PROTOCOL\n");
-	/*
-	 * Return EFI_SUCCESS and don't stop the EFI subsystem.
-	 * That's done for 2 reasons
-	 * - If the protocol is not installed the PCRs won't be extended.  So
-	 *   someone later in the boot flow will notice that and take the
-	 *   necessary actions.
-	 * - The TPM sandbox is limited and we won't be able to run any efi
-	 *   related tests with TCG2 enabled
-	 */
-	return EFI_SUCCESS;
+	return ret;
 }
-- 
2.17.1

[PATCH 2/3] efi_loader: check tcg2 protocol installation outside the TCG protocol

[Masahisa Kojima]

There are functions that calls tcg2_agile_log_append() outside
of the TCG protocol invocation (e.g tcg2_measure_pe_image).
These functions must to check that TCG2 protocol is installed.
If not, measurement shall be skipped.

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

diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
index 6dbdd35f29..2b7b7cbbae 100644
--- a/lib/efi_loader/efi_tcg2.c
+++ b/lib/efi_loader/efi_tcg2.c
@@ -972,6 +972,9 @@ efi_status_t tcg2_measure_pe_image(void *efi, u64 efi_size,
 	IMAGE_NT_HEADERS32 *nt;
 	struct efi_handler *handler;
 
+	if (!is_tcg2_protocol_installed())
+		return EFI_SUCCESS;
+
 	ret = platform_get_tpm2_device(&dev);
 	if (ret != EFI_SUCCESS)
 		return ret;
@@ -2189,6 +2192,9 @@ efi_status_t efi_tcg2_measure_efi_app_invocation(struct efi_loaded_image_obj *ha
 	u32 event = 0;
 	struct smbios_entry *entry;
 
+	if (!is_tcg2_protocol_installed())
+		return EFI_SUCCESS;
+
 	if (tcg2_efi_app_invoked)
 		return EFI_SUCCESS;
 
@@ -2239,6 +2245,9 @@ efi_status_t efi_tcg2_measure_efi_app_exit(void)
 	efi_status_t ret;
 	struct udevice *dev;
 
+	if (!is_tcg2_protocol_installed())
+		return EFI_SUCCESS;
+
 	ret = platform_get_tpm2_device(&dev);
 	if (ret != EFI_SUCCESS)
 		return ret;
@@ -2264,6 +2273,12 @@ efi_tcg2_notify_exit_boot_services(struct efi_event *event, void *context)
 	EFI_ENTRY("%p, %p", event, context);
 
 	event_log.ebs_called = true;
+
+	if (!is_tcg2_protocol_installed()) {
+		ret = EFI_SUCCESS;
+		goto out;
+	}
+
 	ret = platform_get_tpm2_device(&dev);
 	if (ret != EFI_SUCCESS)
 		goto out;
@@ -2293,6 +2308,9 @@ efi_status_t efi_tcg2_notify_exit_boot_services_failed(void)
 	struct udevice *dev;
 	efi_status_t ret;
 
+	if (!is_tcg2_protocol_installed())
+		return EFI_SUCCESS;
+
 	ret = platform_get_tpm2_device(&dev);
 	if (ret != EFI_SUCCESS)
 		goto out;
-- 
2.17.1

[PATCH 3/3] efi_loader: correctly handle tcg2_measure_pe_image() error

[Masahisa Kojima]

When the TCG2 protocol is installed in efi_tcg2_register(),
TPM2 device must be present.
tcg2_measure_pe_image() expects that TCP2 protocol is installed
and TPM device is available. If TCG2 Protocol is installed but
TPM device is not found, tcg2_measure_pe_image() returns
EFI_SECURITY_VIOLATION efi_load_image() ends with failure.

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

diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
index eb95580538..426f096574 100644
--- a/lib/efi_loader/efi_image_loader.c
+++ b/lib/efi_loader/efi_image_loader.c
@@ -934,9 +934,16 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle,
 
 #if CONFIG_IS_ENABLED(EFI_TCG2_PROTOCOL)
 	/* Measure an PE/COFF image */
-	if (tcg2_measure_pe_image(efi, efi_size, handle,
-				  loaded_image_info))
+	ret = tcg2_measure_pe_image(efi, efi_size, handle, loaded_image_info);
+	if (ret == EFI_SECURITY_VIOLATION) {
+		/*
+		 * TCG2 Protocol is installed but no TPM device found,
+		 * this is not expected.
+		 */
 		log_err("PE image measurement failed\n");
+		goto err;
+	}
+
 #endif
 
 	/* Copy PE headers */
diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
index 2b7b7cbbae..c19f73dc10 100644
--- a/lib/efi_loader/efi_tcg2.c
+++ b/lib/efi_loader/efi_tcg2.c
@@ -977,7 +977,7 @@ efi_status_t tcg2_measure_pe_image(void *efi, u64 efi_size,
 
 	ret = platform_get_tpm2_device(&dev);
 	if (ret != EFI_SUCCESS)
-		return ret;
+		return EFI_SECURITY_VIOLATION;
 
 	switch (handle->image_type) {
 	case IMAGE_SUBSYSTEM_EFI_APPLICATION:
-- 
2.17.1

Re: [PATCH 1/3] efi_loader: efi_tcg2_register returns appropriate error

[Ilias Apalodimas]

On Fri, Dec 03, 2021 at 12:58:13PM +0900, Masahisa Kojima wrote:
> This commit modify efi_tcg2_register() to return the
> appropriate error.
> With this fix, sandbox will not boot because efi_tcg2_register()
> fails due to some missing feature in GetCapabilities.
> So disable sandbox if EFI_TCG2_PROTOCOL is enabled.
> 
> UEFI secure boot variable measurement is not directly related
> to TCG2 protocol installation, tcg2_measure_secure_boot_variable()
> is moved to the separate function.
> 
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> ---
>  include/efi_loader.h       |  2 ++
>  lib/efi_loader/Kconfig     |  2 ++
>  lib/efi_loader/efi_setup.c |  2 ++
>  lib/efi_loader/efi_tcg2.c  | 65 +++++++++++++++++++++++++++-----------
>  4 files changed, 53 insertions(+), 18 deletions(-)
> 
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 67c40ca57a..f4860e87fc 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -525,6 +525,8 @@ efi_status_t efi_disk_register(void);
>  efi_status_t efi_rng_register(void);
>  /* Called by efi_init_obj_list() to install EFI_TCG2_PROTOCOL */
>  efi_status_t efi_tcg2_register(void);
> +/* Called by efi_init_obj_list() to do initial measurement */
> +efi_status_t efi_tcg2_do_initial_measurement(void);
>  /* measure the pe-coff image, extend PCR and add Event Log */
>  efi_status_t tcg2_measure_pe_image(void *efi, u64 efi_size,
>  				   struct efi_loaded_image_obj *handle,
> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> index 700dc838dd..24f9a2bb75 100644
> --- a/lib/efi_loader/Kconfig
> +++ b/lib/efi_loader/Kconfig
> @@ -308,6 +308,8 @@ config EFI_TCG2_PROTOCOL
>  	bool "EFI_TCG2_PROTOCOL support"
>  	default y
>  	depends on TPM_V2
> +	# Sandbox TPM currently fails on GetCapabilities needed for TCG2
> +	depends on !SANDBOX
>  	select SHA1
>  	select SHA256
>  	select SHA384
> diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
> index 1aba71cd96..f58a4afa7f 100644
> --- a/lib/efi_loader/efi_setup.c
> +++ b/lib/efi_loader/efi_setup.c
> @@ -241,6 +241,8 @@ efi_status_t efi_init_obj_list(void)
>  		ret = efi_tcg2_register();
>  		if (ret != EFI_SUCCESS)
>  			goto out;
> +
> +		efi_tcg2_do_initial_measurement();
>  	}
>  
>  	/* Secure boot */
> diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
> index 5f71b188a0..6dbdd35f29 100644
> --- a/lib/efi_loader/efi_tcg2.c
> +++ b/lib/efi_loader/efi_tcg2.c
> @@ -153,6 +153,15 @@ static u16 alg_to_len(u16 hash_alg)
>  	return 0;
>  }
>  
> +static bool is_tcg2_protocol_installed(void)
> +{
> +	struct efi_handler *handler;
> +	efi_status_t ret;
> +
> +	ret = efi_search_protocol(efi_root, &efi_guid_tcg2_protocol, &handler);
> +	return ret == EFI_SUCCESS;
> +}
> +
>  static u32 tcg_event_final_size(struct tpml_digest_values *digest_list)
>  {
>  	u32 len;
> @@ -1664,6 +1673,14 @@ void tcg2_uninit(void)
>  	event_log.buffer = NULL;
>  	efi_free_pool(event_log.final_buffer);
>  	event_log.final_buffer = NULL;
> +
> +	if (!is_tcg2_protocol_installed())
> +		return;
> +
> +	ret = efi_remove_protocol(efi_root, &efi_guid_tcg2_protocol,
> +				  (void *)&efi_tcg2_protocol);
> +	if (ret != EFI_SUCCESS)
> +		log_err("Failed to remove EFI TCG2 protocol\n");
>  }
>  
>  /**
> @@ -2345,12 +2362,37 @@ error:
>  	return ret;
>  }
>  
> +/**
> + * efi_tcg2_do_initial_measurement() - do initial measurement
> + *
> + * Return:	status code
> + */
> +efi_status_t efi_tcg2_do_initial_measurement(void)
> +{
> +	efi_status_t ret;
> +	struct udevice *dev;
> +
> +	if (!is_tcg2_protocol_installed())
> +		return EFI_SUCCESS;
> +
> +	ret = platform_get_tpm2_device(&dev);
> +	if (ret != EFI_SUCCESS)
> +		goto out;
> +

Would it make more sense to return a security violation here and treat this
error similarly to patch [3/3]?

> +	ret = tcg2_measure_secure_boot_variable(dev);
> +	if (ret != EFI_SUCCESS)
> +		goto out;
> +
> +out:
> +	return ret;
> +}
> +
>  /**
>   * efi_tcg2_register() - register EFI_TCG2_PROTOCOL
>   *
>   * If a TPM2 device is available, the TPM TCG2 Protocol is registered
>   *
> - * Return:	An error status is only returned if adding the protocol fails.
> + * Return:	status code
>   */
>  efi_status_t efi_tcg2_register(void)
>  {
> @@ -2373,8 +2415,10 @@ efi_status_t efi_tcg2_register(void)
>  	}
>  
>  	ret = efi_init_event_log();
> -	if (ret != EFI_SUCCESS)
> +	if (ret != EFI_SUCCESS) {
> +		tcg2_uninit();
>  		goto fail;
> +	}
>  
>  	ret = efi_add_protocol(efi_root, &efi_guid_tcg2_protocol,
>  			       (void *)&efi_tcg2_protocol);
> @@ -2391,24 +2435,9 @@ efi_status_t efi_tcg2_register(void)
>  		goto fail;
>  	}
>  
> -	ret = tcg2_measure_secure_boot_variable(dev);
> -	if (ret != EFI_SUCCESS) {
> -		tcg2_uninit();
> -		goto fail;
> -	}
> -
>  	return ret;
>  
>  fail:
>  	log_err("Cannot install EFI_TCG2_PROTOCOL\n");
> -	/*
> -	 * Return EFI_SUCCESS and don't stop the EFI subsystem.
> -	 * That's done for 2 reasons
> -	 * - If the protocol is not installed the PCRs won't be extended.  So
> -	 *   someone later in the boot flow will notice that and take the
> -	 *   necessary actions.
> -	 * - The TPM sandbox is limited and we won't be able to run any efi
> -	 *   related tests with TCG2 enabled
> -	 */
> -	return EFI_SUCCESS;
> +	return ret;
>  }
> -- 
> 2.17.1
> 

Cheers
/Ilias

Re: [PATCH 2/3] efi_loader: check tcg2 protocol installation outside the TCG protocol

[Ilias Apalodimas]

On Fri, Dec 03, 2021 at 12:58:14PM +0900, Masahisa Kojima wrote:
> There are functions that calls tcg2_agile_log_append() outside
> of the TCG protocol invocation (e.g tcg2_measure_pe_image).
> These functions must to check that TCG2 protocol is installed.
> If not, measurement shall be skipped.
>
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> ---
>  lib/efi_loader/efi_tcg2.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
> index 6dbdd35f29..2b7b7cbbae 100644
> --- a/lib/efi_loader/efi_tcg2.c
> +++ b/lib/efi_loader/efi_tcg2.c
> @@ -972,6 +972,9 @@ efi_status_t tcg2_measure_pe_image(void *efi, u64 efi_size,
>       IMAGE_NT_HEADERS32 *nt;
>       struct efi_handler *handler;
>
> +     if (!is_tcg2_protocol_installed())
> +             return EFI_SUCCESS;
> +
>       ret = platform_get_tpm2_device(&dev);
>       if (ret != EFI_SUCCESS)
>               return ret;
> @@ -2189,6 +2192,9 @@ efi_status_t efi_tcg2_measure_efi_app_invocation(struct efi_loaded_image_obj *ha
>       u32 event = 0;
>       struct smbios_entry *entry;
>
> +     if (!is_tcg2_protocol_installed())
> +             return EFI_SUCCESS;
> +
>       if (tcg2_efi_app_invoked)
>               return EFI_SUCCESS;
>
> @@ -2239,6 +2245,9 @@ efi_status_t efi_tcg2_measure_efi_app_exit(void)
>       efi_status_t ret;
>       struct udevice *dev;
>
> +     if (!is_tcg2_protocol_installed())
> +             return EFI_SUCCESS;
> +
>       ret = platform_get_tpm2_device(&dev);
>       if (ret != EFI_SUCCESS)
>               return ret;
> @@ -2264,6 +2273,12 @@ efi_tcg2_notify_exit_boot_services(struct efi_event *event, void *context)
>       EFI_ENTRY("%p, %p", event, context);
>
>       event_log.ebs_called = true;
> +
> +     if (!is_tcg2_protocol_installed()) {
> +             ret = EFI_SUCCESS;
> +             goto out;
> +     }
> +
>       ret = platform_get_tpm2_device(&dev);
>       if (ret != EFI_SUCCESS)
>               goto out;
> @@ -2293,6 +2308,9 @@ efi_status_t efi_tcg2_notify_exit_boot_services_failed(void)
>       struct udevice *dev;
>       efi_status_t ret;
>
> +     if (!is_tcg2_protocol_installed())
> +             return EFI_SUCCESS;
> +
>       ret = platform_get_tpm2_device(&dev);
>       if (ret != EFI_SUCCESS)
>               goto out;
> --
> 2.17.1
>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Re: [PATCH 3/3] efi_loader: correctly handle tcg2_measure_pe_image() error

[Ilias Apalodimas]

Heinrich does this approach work for you till we fix the DM-EFI
integration?

At least it tries to cover some cases were the efi protocol is installed
(which means the tpm was there in the beginning), but later on is removed

On Fri, Dec 03, 2021 at 12:58:15PM +0900, Masahisa Kojima wrote:
> When the TCG2 protocol is installed in efi_tcg2_register(),
> TPM2 device must be present.
> tcg2_measure_pe_image() expects that TCP2 protocol is installed
> and TPM device is available. If TCG2 Protocol is installed but
> TPM device is not found, tcg2_measure_pe_image() returns
> EFI_SECURITY_VIOLATION efi_load_image() ends with failure.
> 
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> ---
>  lib/efi_loader/efi_image_loader.c | 11 +++++++++--
>  lib/efi_loader/efi_tcg2.c         |  2 +-
>  2 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
> index eb95580538..426f096574 100644
> --- a/lib/efi_loader/efi_image_loader.c
> +++ b/lib/efi_loader/efi_image_loader.c
> @@ -934,9 +934,16 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle,
>  
>  #if CONFIG_IS_ENABLED(EFI_TCG2_PROTOCOL)
>  	/* Measure an PE/COFF image */
> -	if (tcg2_measure_pe_image(efi, efi_size, handle,
> -				  loaded_image_info))
> +	ret = tcg2_measure_pe_image(efi, efi_size, handle, loaded_image_info);
> +	if (ret == EFI_SECURITY_VIOLATION) {
> +		/*
> +		 * TCG2 Protocol is installed but no TPM device found,
> +		 * this is not expected.
> +		 */
>  		log_err("PE image measurement failed\n");
> +		goto err;
> +	}
> +
>  #endif
>  
>  	/* Copy PE headers */
> diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
> index 2b7b7cbbae..c19f73dc10 100644
> --- a/lib/efi_loader/efi_tcg2.c
> +++ b/lib/efi_loader/efi_tcg2.c
> @@ -977,7 +977,7 @@ efi_status_t tcg2_measure_pe_image(void *efi, u64 efi_size,
>  
>  	ret = platform_get_tpm2_device(&dev);
>  	if (ret != EFI_SUCCESS)
> -		return ret;
> +		return EFI_SECURITY_VIOLATION;
>  
>  	switch (handle->image_type) {
>  	case IMAGE_SUBSYSTEM_EFI_APPLICATION:
> -- 
> 2.17.1
> 

Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Re: [PATCH 1/3] efi_loader: efi_tcg2_register returns appropriate error

[Masahisa Kojima]

On Mon, 6 Dec 2021 at 23:08, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> On Fri, Dec 03, 2021 at 12:58:13PM +0900, Masahisa Kojima wrote:
> > This commit modify efi_tcg2_register() to return the
> > appropriate error.
> > With this fix, sandbox will not boot because efi_tcg2_register()
> > fails due to some missing feature in GetCapabilities.
> > So disable sandbox if EFI_TCG2_PROTOCOL is enabled.
> >
> > UEFI secure boot variable measurement is not directly related
> > to TCG2 protocol installation, tcg2_measure_secure_boot_variable()
> > is moved to the separate function.
> >
> > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > ---
> >  include/efi_loader.h       |  2 ++
> >  lib/efi_loader/Kconfig     |  2 ++
> >  lib/efi_loader/efi_setup.c |  2 ++
> >  lib/efi_loader/efi_tcg2.c  | 65 +++++++++++++++++++++++++++-----------
> >  4 files changed, 53 insertions(+), 18 deletions(-)
> >
> > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > index 67c40ca57a..f4860e87fc 100644
> > --- a/include/efi_loader.h
> > +++ b/include/efi_loader.h
> > @@ -525,6 +525,8 @@ efi_status_t efi_disk_register(void);
> >  efi_status_t efi_rng_register(void);
> >  /* Called by efi_init_obj_list() to install EFI_TCG2_PROTOCOL */
> >  efi_status_t efi_tcg2_register(void);
> > +/* Called by efi_init_obj_list() to do initial measurement */
> > +efi_status_t efi_tcg2_do_initial_measurement(void);
> >  /* measure the pe-coff image, extend PCR and add Event Log */
> >  efi_status_t tcg2_measure_pe_image(void *efi, u64 efi_size,
> >                                  struct efi_loaded_image_obj *handle,
> > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > index 700dc838dd..24f9a2bb75 100644
> > --- a/lib/efi_loader/Kconfig
> > +++ b/lib/efi_loader/Kconfig
> > @@ -308,6 +308,8 @@ config EFI_TCG2_PROTOCOL
> >       bool "EFI_TCG2_PROTOCOL support"
> >       default y
> >       depends on TPM_V2
> > +     # Sandbox TPM currently fails on GetCapabilities needed for TCG2
> > +     depends on !SANDBOX
> >       select SHA1
> >       select SHA256
> >       select SHA384
> > diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
> > index 1aba71cd96..f58a4afa7f 100644
> > --- a/lib/efi_loader/efi_setup.c
> > +++ b/lib/efi_loader/efi_setup.c
> > @@ -241,6 +241,8 @@ efi_status_t efi_init_obj_list(void)
> >               ret = efi_tcg2_register();
> >               if (ret != EFI_SUCCESS)
> >                       goto out;
> > +
> > +             efi_tcg2_do_initial_measurement();
> >       }
> >
> >       /* Secure boot */
> > diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
> > index 5f71b188a0..6dbdd35f29 100644
> > --- a/lib/efi_loader/efi_tcg2.c
> > +++ b/lib/efi_loader/efi_tcg2.c
> > @@ -153,6 +153,15 @@ static u16 alg_to_len(u16 hash_alg)
> >       return 0;
> >  }
> >
> > +static bool is_tcg2_protocol_installed(void)
> > +{
> > +     struct efi_handler *handler;
> > +     efi_status_t ret;
> > +
> > +     ret = efi_search_protocol(efi_root, &efi_guid_tcg2_protocol, &handler);
> > +     return ret == EFI_SUCCESS;
> > +}
> > +
> >  static u32 tcg_event_final_size(struct tpml_digest_values *digest_list)
> >  {
> >       u32 len;
> > @@ -1664,6 +1673,14 @@ void tcg2_uninit(void)
> >       event_log.buffer = NULL;
> >       efi_free_pool(event_log.final_buffer);
> >       event_log.final_buffer = NULL;
> > +
> > +     if (!is_tcg2_protocol_installed())
> > +             return;
> > +
> > +     ret = efi_remove_protocol(efi_root, &efi_guid_tcg2_protocol,
> > +                               (void *)&efi_tcg2_protocol);
> > +     if (ret != EFI_SUCCESS)
> > +             log_err("Failed to remove EFI TCG2 protocol\n");
> >  }
> >
> >  /**
> > @@ -2345,12 +2362,37 @@ error:
> >       return ret;
> >  }
> >
> > +/**
> > + * efi_tcg2_do_initial_measurement() - do initial measurement
> > + *
> > + * Return:   status code
> > + */
> > +efi_status_t efi_tcg2_do_initial_measurement(void)
> > +{
> > +     efi_status_t ret;
> > +     struct udevice *dev;
> > +
> > +     if (!is_tcg2_protocol_installed())
> > +             return EFI_SUCCESS;
> > +
> > +     ret = platform_get_tpm2_device(&dev);
> > +     if (ret != EFI_SUCCESS)
> > +             goto out;
> > +
>
> Would it make more sense to return a security violation here and treat this
> error similarly to patch [3/3]?

Yes, I agree. I also add the similar check for
efi_tcg2_measure_efi_app_invocation().

Thanks,
Masahisa Kojima

>
> > +     ret = tcg2_measure_secure_boot_variable(dev);
> > +     if (ret != EFI_SUCCESS)
> > +             goto out;
> > +
> > +out:
> > +     return ret;
> > +}
> > +
> >  /**
> >   * efi_tcg2_register() - register EFI_TCG2_PROTOCOL
> >   *
> >   * If a TPM2 device is available, the TPM TCG2 Protocol is registered
> >   *
> > - * Return:   An error status is only returned if adding the protocol fails.
> > + * Return:   status code
> >   */
> >  efi_status_t efi_tcg2_register(void)
> >  {
> > @@ -2373,8 +2415,10 @@ efi_status_t efi_tcg2_register(void)
> >       }
> >
> >       ret = efi_init_event_log();
> > -     if (ret != EFI_SUCCESS)
> > +     if (ret != EFI_SUCCESS) {
> > +             tcg2_uninit();
> >               goto fail;
> > +     }
> >
> >       ret = efi_add_protocol(efi_root, &efi_guid_tcg2_protocol,
> >                              (void *)&efi_tcg2_protocol);
> > @@ -2391,24 +2435,9 @@ efi_status_t efi_tcg2_register(void)
> >               goto fail;
> >       }
> >
> > -     ret = tcg2_measure_secure_boot_variable(dev);
> > -     if (ret != EFI_SUCCESS) {
> > -             tcg2_uninit();
> > -             goto fail;
> > -     }
> > -
> >       return ret;
> >
> >  fail:
> >       log_err("Cannot install EFI_TCG2_PROTOCOL\n");
> > -     /*
> > -      * Return EFI_SUCCESS and don't stop the EFI subsystem.
> > -      * That's done for 2 reasons
> > -      * - If the protocol is not installed the PCRs won't be extended.  So
> > -      *   someone later in the boot flow will notice that and take the
> > -      *   necessary actions.
> > -      * - The TPM sandbox is limited and we won't be able to run any efi
> > -      *   related tests with TCG2 enabled
> > -      */
> > -     return EFI_SUCCESS;
> > +     return ret;
> >  }
> > --
> > 2.17.1
> >
>
> Cheers
> /Ilias