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