[PATCH v3] efi_loader: check tcg2 protocol installation outside the TCG protocol
Masahisa Kojima
masahisa.kojima at linaro.org
Mon Nov 29 04:03:22 CET 2021
Hi Ilias,
On Fri, 26 Nov 2021 at 23:55, Ilias Apalodimas
<ilias.apalodimas at 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 at 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
More information about the U-Boot
mailing list