[PATCH v3] efi_loader: check tcg2 protocol installation outside the TCG protocol
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.
Together with above change, this commit also removes the unnecessary tcg2_uninit() call in efi_tcg2_register(), refactors efi_tcg2_register() not to include the process that is not related to the tcg2 protocol registration.
Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org --- Changes in v3: - add static qualifier to is_tcg2_protocol_installed() - simplify is_tcg2_protocol_installed() return handling
Changes in v2: - rebased on top of the TF-A eventlog handover support
include/efi_loader.h | 4 ++ lib/efi_loader/efi_setup.c | 3 ++ lib/efi_loader/efi_tcg2.c | 89 +++++++++++++++++++++++++++++++------- 3 files changed, 81 insertions(+), 15 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h index d52e399841..fe29c10906 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -525,6 +525,10 @@ efi_status_t efi_disk_register(void); efi_status_t efi_rng_register(void); /* Called by efi_init_obj_list() to install EFI_TCG2_PROTOCOL */ efi_status_t efi_tcg2_register(void); +/* Called by efi_init_obj_list() to do the required setup for the measurement */ +efi_status_t efi_tcg2_setup_measurement(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/efi_setup.c b/lib/efi_loader/efi_setup.c index a2338d74af..781d10590d 100644 --- a/lib/efi_loader/efi_setup.c +++ b/lib/efi_loader/efi_setup.c @@ -271,6 +271,9 @@ efi_status_t efi_init_obj_list(void) ret = efi_tcg2_register(); if (ret != EFI_SUCCESS) goto out; + + efi_tcg2_setup_measurement(); + efi_tcg2_do_initial_measurement(); }
/* Secure boot */ diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c index b44eed0ec9..2196af49a6 100644 --- a/lib/efi_loader/efi_tcg2.c +++ b/lib/efi_loader/efi_tcg2.c @@ -153,6 +153,20 @@ static u16 alg_to_len(u16 hash_alg) return 0; }
+/** + * is_tcg2_protocol_installed - chech whether tcg2 protocol is installed + * + * @Return: true if tcg2 protocol is installed, false if not + */ +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; @@ -962,6 +976,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_NOT_READY; + ret = platform_get_tpm2_device(&dev); if (ret != EFI_SUCCESS) return ret; @@ -2140,6 +2157,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_NOT_READY; + if (tcg2_efi_app_invoked) return EFI_SUCCESS;
@@ -2190,6 +2210,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_NOT_READY; + ret = platform_get_tpm2_device(&dev); if (ret != EFI_SUCCESS) return ret; @@ -2214,6 +2237,11 @@ efi_tcg2_notify_exit_boot_services(struct efi_event *event, void *context)
EFI_ENTRY("%p, %p", event, context);
+ if (!is_tcg2_protocol_installed()) { + ret = EFI_NOT_READY; + goto out; + } + event_log.ebs_called = true; ret = platform_get_tpm2_device(&dev); if (ret != EFI_SUCCESS) @@ -2244,6 +2272,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_NOT_READY; + ret = platform_get_tpm2_device(&dev); if (ret != EFI_SUCCESS) goto out; @@ -2313,6 +2344,45 @@ error: return ret; }
+/** + * efi_tcg2_setup_measurement() - setup for the measurement + * + * Return: status code + */ +efi_status_t efi_tcg2_setup_measurement(void) +{ + efi_status_t ret; + struct efi_event *event; + + ret = efi_create_event(EVT_SIGNAL_EXIT_BOOT_SERVICES, TPL_CALLBACK, + efi_tcg2_notify_exit_boot_services, NULL, + NULL, &event); + + 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; + + 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 * @@ -2324,7 +2394,6 @@ efi_status_t efi_tcg2_register(void) { efi_status_t ret = EFI_SUCCESS; struct udevice *dev; - struct efi_event *event; u32 err;
ret = platform_get_tpm2_device(&dev); @@ -2344,6 +2413,10 @@ efi_status_t efi_tcg2_register(void) if (ret != EFI_SUCCESS) goto fail;
+ /* + * efi_add_protocol() is called at last on purpose. + * tcg2_uninit() does not uninstall the tcg2 protocol, but it is intended. + */ ret = efi_add_protocol(efi_root, &efi_guid_tcg2_protocol, (void *)&efi_tcg2_protocol); if (ret != EFI_SUCCESS) { @@ -2351,20 +2424,6 @@ 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(); - goto fail; - } - return ret;
fail:
Hi Kojima-san,
On Fri, Nov 26, 2021 at 10:31:16AM +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.
Together with above change, this commit also removes the unnecessary tcg2_uninit() call in efi_tcg2_register(), refactors efi_tcg2_register() not to include the process that is not related to the tcg2 protocol registration.
Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org
Changes in v3:
- add static qualifier to is_tcg2_protocol_installed()
- simplify is_tcg2_protocol_installed() return handling
Changes in v2:
- rebased on top of the TF-A eventlog handover support
include/efi_loader.h | 4 ++ lib/efi_loader/efi_setup.c | 3 ++ lib/efi_loader/efi_tcg2.c | 89 +++++++++++++++++++++++++++++++------- 3 files changed, 81 insertions(+), 15 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h index d52e399841..fe29c10906 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -525,6 +525,10 @@ efi_status_t efi_disk_register(void); efi_status_t efi_rng_register(void); /* Called by efi_init_obj_list() to install EFI_TCG2_PROTOCOL */ efi_status_t efi_tcg2_register(void); +/* Called by efi_init_obj_list() to do the required setup for the measurement */ +efi_status_t efi_tcg2_setup_measurement(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/efi_setup.c b/lib/efi_loader/efi_setup.c index a2338d74af..781d10590d 100644 --- a/lib/efi_loader/efi_setup.c +++ b/lib/efi_loader/efi_setup.c @@ -271,6 +271,9 @@ efi_status_t efi_init_obj_list(void) ret = efi_tcg2_register(); if (ret != EFI_SUCCESS) goto out;
efi_tcg2_setup_measurement();efi_tcg2_do_initial_measurement();
}
/* Secure boot */ diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c index b44eed0ec9..2196af49a6 100644 --- a/lib/efi_loader/efi_tcg2.c +++ b/lib/efi_loader/efi_tcg2.c @@ -153,6 +153,20 @@ static u16 alg_to_len(u16 hash_alg) return 0; }
+/**
- is_tcg2_protocol_installed - chech whether tcg2 protocol is installed
s/chech/check
- @Return: true if tcg2 protocol is installed, false if not
- */
+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; @@ -962,6 +976,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_NOT_READY;- ret = platform_get_tpm2_device(&dev); if (ret != EFI_SUCCESS) return ret;
@@ -2140,6 +2157,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_NOT_READY;- if (tcg2_efi_app_invoked) return EFI_SUCCESS;
@@ -2190,6 +2210,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_NOT_READY;- ret = platform_get_tpm2_device(&dev); if (ret != EFI_SUCCESS) return ret;
@@ -2214,6 +2237,11 @@ efi_tcg2_notify_exit_boot_services(struct efi_event *event, void *context)
EFI_ENTRY("%p, %p", event, context);
- if (!is_tcg2_protocol_installed()) {
ret = EFI_NOT_READY;goto out;- }
- event_log.ebs_called = true; ret = platform_get_tpm2_device(&dev); if (ret != EFI_SUCCESS)
@@ -2244,6 +2272,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_NOT_READY;- ret = platform_get_tpm2_device(&dev); if (ret != EFI_SUCCESS) goto out;
@@ -2313,6 +2344,45 @@ error: return ret; }
+/**
- efi_tcg2_setup_measurement() - setup for the measurement
- Return: status code
- */
+efi_status_t efi_tcg2_setup_measurement(void) +{
- efi_status_t ret;
- struct efi_event *event;
- ret = efi_create_event(EVT_SIGNAL_EXIT_BOOT_SERVICES, TPL_CALLBACK,
efi_tcg2_notify_exit_boot_services, NULL,NULL, &event);- 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;
- 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;
+}
Unfortunately always returning EFI_SUCCESS from efi_tcg2_register() creates a dependency hell for us. If we want to keep this code don't we need to check is_tcg2_protocol_installed() here as well? If we don't the call chain here is: tcg2_measure_secure_boot_variable() -> tcg2_measure_variable() -> tcg2_measure_event() -> tcg2_agile_log_append(). Won't this still crash if for some reason efi_tcg2_register() failed?
We could also avoid it by adding a similar check to tcg2_agile_log_append(). Or we do something like:
diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c index 117bf9add631..92ea8937cc60 100644 --- a/lib/efi_loader/efi_tcg2.c +++ b/lib/efi_loader/efi_tcg2.c @@ -325,6 +325,16 @@ static efi_status_t tcg2_agile_log_append(u32 pcr_index, u32 event_type, u32 event_size = size + tcg_event_final_size(digest_list); struct efi_tcg2_final_events_table *final_event; efi_status_t ret = EFI_SUCCESS; + /* + * This should never happen. This function should only be invoked if + * the TCG2 protocol has been installed. However since we always + * return EFI_SUCCESS from efi_tcg2_register shield callers against + * crashing and complain + */ + if (!event_log.buffer) { + log_err("EFI TCG2 protocol not installed correctly\n"); + return EFI_INVALID_PARAMETER; + }
/* if ExitBootServices hasn't been called update the normal log */ if (!event_log.ebs_called) { @@ -341,6 +351,10 @@ static efi_status_t tcg2_agile_log_append(u32 pcr_index, u32 event_type, if (!event_log.get_event_called) return ret;
+ if (!event_log.final_buffer) { + log_err("EFI TCG2 protocol not installed correctly\n"); + return EFI_INVALID_PARAMETER; + } /* if GetEventLog has been called update FinalEventLog as well */ if (event_log.final_pos + event_size > TPM2_EVENT_LOG_SIZE) return EFI_VOLUME_FULL;
But at this point I think this is an error waiting to happen. I'd much prefer just refusing to boot if the TCG protocol installation failed.
/**
- efi_tcg2_register() - register EFI_TCG2_PROTOCOL
@@ -2324,7 +2394,6 @@ efi_status_t efi_tcg2_register(void) { efi_status_t ret = EFI_SUCCESS; struct udevice *dev;
struct efi_event *event; u32 err;
ret = platform_get_tpm2_device(&dev);
@@ -2344,6 +2413,10 @@ efi_status_t efi_tcg2_register(void) if (ret != EFI_SUCCESS) goto fail;
- /*
* efi_add_protocol() is called at last on purpose.* tcg2_uninit() does not uninstall the tcg2 protocol, but it is intended.*/@@ -2351,20 +2424,6 @@ 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();goto fail;- }
- return ret;
fail:
2.17.1
return 0;
Regards /Ilias
Hi Ilias,
On Fri, 26 Nov 2021 at 23:55, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Kojima-san,
On Fri, Nov 26, 2021 at 10:31:16AM +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.
Together with above change, this commit also removes the unnecessary tcg2_uninit() call in efi_tcg2_register(), refactors efi_tcg2_register() not to include the process that is not related to the tcg2 protocol registration.
Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org
Changes in v3:
- add static qualifier to is_tcg2_protocol_installed()
- simplify is_tcg2_protocol_installed() return handling
Changes in v2:
- rebased on top of the TF-A eventlog handover support
include/efi_loader.h | 4 ++ lib/efi_loader/efi_setup.c | 3 ++ lib/efi_loader/efi_tcg2.c | 89 +++++++++++++++++++++++++++++++------- 3 files changed, 81 insertions(+), 15 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h index d52e399841..fe29c10906 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -525,6 +525,10 @@ efi_status_t efi_disk_register(void); efi_status_t efi_rng_register(void); /* Called by efi_init_obj_list() to install EFI_TCG2_PROTOCOL */ efi_status_t efi_tcg2_register(void); +/* Called by efi_init_obj_list() to do the required setup for the measurement */ +efi_status_t efi_tcg2_setup_measurement(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/efi_setup.c b/lib/efi_loader/efi_setup.c index a2338d74af..781d10590d 100644 --- a/lib/efi_loader/efi_setup.c +++ b/lib/efi_loader/efi_setup.c @@ -271,6 +271,9 @@ efi_status_t efi_init_obj_list(void) ret = efi_tcg2_register(); if (ret != EFI_SUCCESS) goto out;
efi_tcg2_setup_measurement();efi_tcg2_do_initial_measurement();} /* Secure boot */diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c index b44eed0ec9..2196af49a6 100644 --- a/lib/efi_loader/efi_tcg2.c +++ b/lib/efi_loader/efi_tcg2.c @@ -153,6 +153,20 @@ static u16 alg_to_len(u16 hash_alg) return 0; }
+/**
- is_tcg2_protocol_installed - chech whether tcg2 protocol is installed
s/chech/check
- @Return: true if tcg2 protocol is installed, false if not
- */
+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; @@ -962,6 +976,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_NOT_READY;ret = platform_get_tpm2_device(&dev); if (ret != EFI_SUCCESS) return ret;@@ -2140,6 +2157,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_NOT_READY;if (tcg2_efi_app_invoked) return EFI_SUCCESS;@@ -2190,6 +2210,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_NOT_READY;ret = platform_get_tpm2_device(&dev); if (ret != EFI_SUCCESS) return ret;@@ -2214,6 +2237,11 @@ efi_tcg2_notify_exit_boot_services(struct efi_event *event, void *context)
EFI_ENTRY("%p, %p", event, context);
if (!is_tcg2_protocol_installed()) {ret = EFI_NOT_READY;goto out;}event_log.ebs_called = true; ret = platform_get_tpm2_device(&dev); if (ret != EFI_SUCCESS)@@ -2244,6 +2272,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_NOT_READY;ret = platform_get_tpm2_device(&dev); if (ret != EFI_SUCCESS) goto out;@@ -2313,6 +2344,45 @@ error: return ret; }
+/**
- efi_tcg2_setup_measurement() - setup for the measurement
- Return: status code
- */
+efi_status_t efi_tcg2_setup_measurement(void) +{
efi_status_t ret;struct efi_event *event;ret = efi_create_event(EVT_SIGNAL_EXIT_BOOT_SERVICES, TPL_CALLBACK,efi_tcg2_notify_exit_boot_services, NULL,NULL, &event);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;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;+}
Unfortunately always returning EFI_SUCCESS from efi_tcg2_register() creates a dependency hell for us. If we want to keep this code don't we need to check is_tcg2_protocol_installed() here as well? If we don't the call chain here is: tcg2_measure_secure_boot_variable() -> tcg2_measure_variable() -> tcg2_measure_event() -> tcg2_agile_log_append(). Won't this still crash if for some reason efi_tcg2_register() failed?
Yes, you are correct. efi_tcg2_setup_measurement() and efi_tcg2_do_initial_measurement() expects that efi_tcg2_register() returns the correct error code, instead of EFI_SUCCESS, so this patch will not work as expected.
In addition, I think again that calling is_tcg2_protocol_installed() in the outside of EFI Protocol functions such as tcg2_measure_pe_image() is also wrong. tcg2_measure_pe_image() shall be handled even if TCG2 EFI Protocol is not installed.
We could also avoid it by adding a similar check to tcg2_agile_log_append(). Or we do something like:
diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c index 117bf9add631..92ea8937cc60 100644 --- a/lib/efi_loader/efi_tcg2.c +++ b/lib/efi_loader/efi_tcg2.c @@ -325,6 +325,16 @@ static efi_status_t tcg2_agile_log_append(u32 pcr_index, u32 event_type, u32 event_size = size + tcg_event_final_size(digest_list); struct efi_tcg2_final_events_table *final_event; efi_status_t ret = EFI_SUCCESS;
/** This should never happen. This function should only be invoked if* the TCG2 protocol has been installed. However since we always* return EFI_SUCCESS from efi_tcg2_register shield callers against* crashing and complain*/if (!event_log.buffer) {log_err("EFI TCG2 protocol not installed correctly\n");return EFI_INVALID_PARAMETER;} /* if ExitBootServices hasn't been called update the normal log */ if (!event_log.ebs_called) {@@ -341,6 +351,10 @@ static efi_status_t tcg2_agile_log_append(u32 pcr_index, u32 event_type, if (!event_log.get_event_called) return ret;
if (!event_log.final_buffer) {log_err("EFI TCG2 protocol not installed correctly\n");return EFI_INVALID_PARAMETER;} /* if GetEventLog has been called update FinalEventLog as well */ if (event_log.final_pos + event_size > TPM2_EVENT_LOG_SIZE) return EFI_VOLUME_FULL;But at this point I think this is an error waiting to happen. I'd much prefer just refusing to boot if the TCG protocol installation failed.
I agree. So I think what should we do for this issue is: - Add null check of eventlog buffer in tcg2_agile_log_append() ===> You have already sent this patch. - return appropriate error code in efi_tcg2_register() - remove efi_create_event() and tcg2_measure_secure_boot_variable() from efi_tcg2_register() and create separate function as this patch, to make efi_tcg2_register() implementation simple.
What do you think?
Thanks, Masahisa Kojima
/**
- efi_tcg2_register() - register EFI_TCG2_PROTOCOL
@@ -2324,7 +2394,6 @@ efi_status_t efi_tcg2_register(void) { efi_status_t ret = EFI_SUCCESS; struct udevice *dev;
struct efi_event *event; u32 err; ret = platform_get_tpm2_device(&dev);@@ -2344,6 +2413,10 @@ efi_status_t efi_tcg2_register(void) if (ret != EFI_SUCCESS) goto fail;
/** efi_add_protocol() is called at last on purpose.* tcg2_uninit() does not uninstall the tcg2 protocol, but it is intended.*/ ret = efi_add_protocol(efi_root, &efi_guid_tcg2_protocol, (void *)&efi_tcg2_protocol); if (ret != EFI_SUCCESS) {@@ -2351,20 +2424,6 @@ 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();goto fail;}return ret;fail:
2.17.1
return 0;Regards /Ilias
Hi Kojima-san
[...]
+efi_status_t efi_tcg2_do_initial_measurement(void) +{
efi_status_t ret;struct udevice *dev;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;+}
Unfortunately always returning EFI_SUCCESS from efi_tcg2_register() creates a dependency hell for us. If we want to keep this code don't we need to check is_tcg2_protocol_installed() here as well? If we don't the call chain here is: tcg2_measure_secure_boot_variable() -> tcg2_measure_variable() -> tcg2_measure_event() -> tcg2_agile_log_append(). Won't this still crash if for some reason efi_tcg2_register() failed?
Yes, you are correct. efi_tcg2_setup_measurement() and efi_tcg2_do_initial_measurement() expects that efi_tcg2_register() returns the correct error code, instead of EFI_SUCCESS, so this patch will not work as expected.
In addition, I think again that calling is_tcg2_protocol_installed() in the outside of EFI Protocol functions such as tcg2_measure_pe_image() is also wrong. tcg2_measure_pe_image() shall be handled even if TCG2 EFI Protocol is not installed.
We could also avoid it by adding a similar check to tcg2_agile_log_append(). Or we do something like:
diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c index 117bf9add631..92ea8937cc60 100644 --- a/lib/efi_loader/efi_tcg2.c +++ b/lib/efi_loader/efi_tcg2.c @@ -325,6 +325,16 @@ static efi_status_t tcg2_agile_log_append(u32 pcr_index, u32 event_type, u32 event_size = size + tcg_event_final_size(digest_list); struct efi_tcg2_final_events_table *final_event; efi_status_t ret = EFI_SUCCESS;
/** This should never happen. This function should only be invoked if* the TCG2 protocol has been installed. However since we always* return EFI_SUCCESS from efi_tcg2_register shield callers against* crashing and complain*/if (!event_log.buffer) {log_err("EFI TCG2 protocol not installed correctly\n");return EFI_INVALID_PARAMETER;} /* if ExitBootServices hasn't been called update the normal log */ if (!event_log.ebs_called) {@@ -341,6 +351,10 @@ static efi_status_t tcg2_agile_log_append(u32 pcr_index, u32 event_type, if (!event_log.get_event_called) return ret;
if (!event_log.final_buffer) {log_err("EFI TCG2 protocol not installed correctly\n");return EFI_INVALID_PARAMETER;} /* if GetEventLog has been called update FinalEventLog as well */ if (event_log.final_pos + event_size > TPM2_EVENT_LOG_SIZE) return EFI_VOLUME_FULL;But at this point I think this is an error waiting to happen. I'd much prefer just refusing to boot if the TCG protocol installation failed.
I agree. So I think what should we do for this issue is:
- Add null check of eventlog buffer in tcg2_agile_log_append() ===> You have already sent this patch.
- return appropriate error code in efi_tcg2_register()
- remove efi_create_event() and tcg2_measure_secure_boot_variable() from efi_tcg2_register() and create separate function as this patch, to make efi_tcg2_register() implementation simple.
What do you think?
Yes I think this is the best say forward. Feel free to pick up the null checking patchset.
Thanks, Masahisa Kojima
Cheers /Ilias [...]
participants (2)
-
Ilias Apalodimas -
Masahisa Kojima