[PATCH v3] efi_loader: check tcg2 protocol installation outside the TCG protocol

[Ilias Apalodimas]

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
[...]