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

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

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