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.
However, PCR5 is used for more than that and there might be a need to use
a PCR with a more limited scope which measures our initramfs and
LoadOptions.
So add a measurement in PCR9 (which we already use for our initrd) and
extend it with the LoadOption measurements
Signed-off-by: Ilias Apalodimas <[email protected]>
---
.../firmware/efi/libstub/efi-stub-helper.c | 21 +++++++++++++++++++
drivers/firmware/efi/libstub/efistub.h | 1 +
2 files changed, 22 insertions(+)
diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
index 3ef4867344b9..5b03248527c6 100644
--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ 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);
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
#define EV_EVENT_TAG 0x00000006U
#define EFI_TCG2_EVENT_HEADER_VERSION 0x1
--
2.34.1
On Fri, 16 Sept 2022 at 10:15, Ilias Apalodimas
<[email protected]> 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.
> However, PCR5 is used for more than that and there might be a need to use
> a PCR with a more limited scope which measures our initramfs and
> LoadOptions.
>
> So add a measurement in PCR9 (which we already use for our initrd) and
> extend it with the LoadOption measurements
>
> Signed-off-by: Ilias Apalodimas <[email protected]>
> ---
> .../firmware/efi/libstub/efi-stub-helper.c | 21 +++++++++++++++++++
> drivers/firmware/efi/libstub/efistub.h | 1 +
> 2 files changed, 22 insertions(+)
>
> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
> index 3ef4867344b9..5b03248527c6 100644
> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> +++ 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.
> 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?
> #define EV_EVENT_TAG 0x00000006U
> #define EFI_TCG2_EVENT_HEADER_VERSION 0x1
>
> --
> 2.34.1
>
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
> <[email protected]> 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
> >