Re: [PATCH 2/2] efi/libstub: measure EFI LoadOptions

[Ilias Apalodimas]

Hi Ard, 

On Fri, Sep 16, 2022 at 10:26:46AM +0200, Ard Biesheuvel wrote:
> On Fri, 16 Sept 2022 at 10:15, Ilias Apalodimas
> <ilias.apalodimas@xxxxxxxxxx> wrote:
> >
> > The EFI TCG spec, in §10.2.6 Measuring UEFI Variables and UEFI GPT Data,
> > is  measuring the entire UEFI_LOAD_OPTION (in PCR5).  As a result boot
> > variables that point to the same UEFI application but with different
> > optional data,  will have distinct measurements.
> >
> 
> That is not the main problem. The main problem is that
> LoadImage()/StartImage() may be used to invoke things beyond Boot####
> options, and at StartImage() time, the load options could be anything.
> So not measuring the load options when the image is actually being
> invoked is a huge oversight.

Fair enough, I'll update the description

> 
> 
> > However, PCR5 is used for more than that and there might be a need to use
> > +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> > @@ -370,6 +370,27 @@ char *efi_convert_cmdline(efi_loaded_image_t *image, int *cmd_line_len)
> >         int options_bytes = 0, safe_options_bytes = 0;  /* UTF-8 bytes */
> >         bool in_quote = false;
> >         efi_status_t status;
> > +       static const struct efi_measured_event load_options_tcg2_event = {
> > +               {
> > +                       sizeof(load_options_tcg2_event) + sizeof("Load Options"),
> > +                       {
> > +                               sizeof(load_options_tcg2_event.event_data.event_header),
> > +                               EFI_TCG2_EVENT_HEADER_VERSION,
> > +                               9,
> > +                               EV_EVENT_TAG,
> > +                       },
> > +               },
> > +               {
> > +                       LOAD_OPTIONS_EVENT_TAG_ID,
> > +                       sizeof("Load Options"),
> > +               },
> > +               { "Load Options" },
> > +       };
> > +
> > +       if (options_chars > 0)
> > +               efi_measure_tagged_event((unsigned long) options,
> > +                                        (unsigned long) options_chars,
> > +                                        &load_options_tcg2_event);
> >
> 
> The name 'options_chars' is a bit misleading here, as it is actually
> the size in bytes at this point.

True but any suggestions on how to fix this? Rename the declaration?

> 
> >         efi_apply_loadoptions_quirk((const void **)&options, &options_chars);
> >         options_chars /= sizeof(*options);
> > diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
> > index cb7eb5ed9f14..e3605b383964 100644
> > --- a/drivers/firmware/efi/libstub/efistub.h
> > +++ b/drivers/firmware/efi/libstub/efistub.h
> > @@ -741,6 +741,7 @@ union apple_properties_protocol {
> >  typedef u32 efi_tcg2_event_log_format;
> >
> >  #define INITRD_EVENT_TAG_ID 0x8F3B22ECU
> > +#define LOAD_OPTIONS_EVENT_TAG_ID 0x8F3B22EDU
> 
> Is this an arbitrarily chosen value?

Yea.  As far as events are concerned I've found 2 event types:
- EV_IPL: This event is deprecated for platform
  firmware. It may be used by Boot Manager
  Code to measure events
- EV_EVENT_TAG: Used for PCRs defined for OS and
  application usage.  Defined for use by Host Platform Operating
  System or Software.

The latter seemed better for our case and it must include a 
'struct TCG_PCClientTaggedEvent' in the event. 
The first member of that struct is the ID which is a unique identifier
defined by the measuring OS or application. 

Cheers
/Ilias

> 
> >  #define EV_EVENT_TAG 0x00000006U
> >  #define EFI_TCG2_EVENT_HEADER_VERSION  0x1
> >
> > --
> > 2.34.1
> >