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
> >
