Hi Ilias, On Fri, 26 Nov 2021 at 23:55, Ilias Apalodimas <ilias.apalodi...@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.koj...@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