[PATCH 1/2] efi/libstub: refactor the initrd measuring functions

[PATCH 1/2] efi/libstub: refactor the initrd measuring functions

[Ilias Apalodimas]

Currently, from the efi-stub, we are only measuring the loaded initrd.
A following patch is introducing measurements of extra components.

The current functions are limited in measuring an initrd only, so swap
the code around a bit,  move the struct into the stub header files and
add an extra argument containing the tagged event we are about to measure

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 .../firmware/efi/libstub/efi-stub-helper.c    | 82 +++++++++----------
 drivers/firmware/efi/libstub/efistub.h        |  6 ++
 2 files changed, 46 insertions(+), 42 deletions(-)

diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
index 3d972061c1b0..3ef4867344b9 100644
--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -334,6 +334,28 @@ void efi_apply_loadoptions_quirk(const void **load_options, int *load_options_si
 	*load_options_size = load_option_unpacked.optional_data_size;
 }
 
+static
+void efi_measure_tagged_event(unsigned long load_addr, unsigned long load_size,
+			      const struct efi_measured_event *event)
+{
+	efi_guid_t tcg2_guid = EFI_TCG2_PROTOCOL_GUID;
+	efi_tcg2_protocol_t *tcg2 = NULL;
+	efi_status_t status;
+
+	efi_bs_call(locate_protocol, &tcg2_guid, NULL, (void **)&tcg2);
+	if (tcg2) {
+		status = efi_call_proto(tcg2, hash_log_extend_event,
+					0, load_addr, load_size,
+					&event->event_data);
+		if (status != EFI_SUCCESS)
+			efi_warn("Failed to measure data: 0x%lx\n",
+				 status);
+		else
+			efi_info("Measured %s into PCR %d\n", event->tagged_event_data,
+				 event->event_data.event_header.pcr_index);
+	}
+}
+
 /*
  * Convert the unicode UEFI command line to ASCII to pass to kernel.
  * Size of memory allocated return in *cmd_line_len.
@@ -625,47 +647,6 @@ efi_status_t efi_load_initrd_cmdline(efi_loaded_image_t *image,
 				    load_addr, load_size);
 }
 
-static const struct {
-	efi_tcg2_event_t	event_data;
-	efi_tcg2_tagged_event_t tagged_event;
-	u8			tagged_event_data[];
-} initrd_tcg2_event = {
-	{
-		sizeof(initrd_tcg2_event) + sizeof("Linux initrd"),
-		{
-			sizeof(initrd_tcg2_event.event_data.event_header),
-			EFI_TCG2_EVENT_HEADER_VERSION,
-			9,
-			EV_EVENT_TAG,
-		},
-	},
-	{
-		INITRD_EVENT_TAG_ID,
-		sizeof("Linux initrd"),
-	},
-	{ "Linux initrd" },
-};
-
-static void efi_measure_initrd(unsigned long load_addr, unsigned long load_size)
-{
-	efi_guid_t tcg2_guid = EFI_TCG2_PROTOCOL_GUID;
-	efi_tcg2_protocol_t *tcg2 = NULL;
-	efi_status_t status;
-
-	efi_bs_call(locate_protocol, &tcg2_guid, NULL, (void **)&tcg2);
-	if (tcg2) {
-		status = efi_call_proto(tcg2, hash_log_extend_event,
-					0, load_addr, load_size,
-					&initrd_tcg2_event.event_data);
-		if (status != EFI_SUCCESS)
-			efi_warn("Failed to measure initrd data: 0x%lx\n",
-				 status);
-		else
-			efi_info("Measured initrd data into PCR %d\n",
-				 initrd_tcg2_event.event_data.event_header.pcr_index);
-	}
-}
-
 /**
  * efi_load_initrd() - Load initial RAM disk
  * @image:	EFI loaded image protocol
@@ -683,6 +664,22 @@ efi_status_t efi_load_initrd(efi_loaded_image_t *image,
 			     unsigned long hard_limit)
 {
 	efi_status_t status;
+	static const struct efi_measured_event initrd_tcg2_event = {
+		{
+			sizeof(initrd_tcg2_event) + sizeof("Linux initrd"),
+			{
+				sizeof(initrd_tcg2_event.event_data.event_header),
+				EFI_TCG2_EVENT_HEADER_VERSION,
+				9,
+				EV_EVENT_TAG,
+			},
+		},
+		{
+			INITRD_EVENT_TAG_ID,
+			sizeof("Linux initrd"),
+		},
+		{ "Linux initrd" },
+	};
 
 	if (efi_noinitrd) {
 		*load_addr = *load_size = 0;
@@ -692,7 +689,8 @@ efi_status_t efi_load_initrd(efi_loaded_image_t *image,
 		if (status == EFI_SUCCESS) {
 			efi_info("Loaded initrd from LINUX_EFI_INITRD_MEDIA_GUID device path\n");
 			if (*load_size > 0)
-				efi_measure_initrd(*load_addr, *load_size);
+				efi_measure_tagged_event(*load_addr, *load_size,
+							 &initrd_tcg2_event);
 		} else if (status == EFI_NOT_FOUND) {
 			status = efi_load_initrd_cmdline(image, load_addr, load_size,
 							 soft_limit, hard_limit);
diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
index b0ae0a454404..cb7eb5ed9f14 100644
--- a/drivers/firmware/efi/libstub/efistub.h
+++ b/drivers/firmware/efi/libstub/efistub.h
@@ -765,6 +765,12 @@ typedef struct efi_tcg2_event efi_tcg2_event_t;
 typedef struct efi_tcg2_tagged_event efi_tcg2_tagged_event_t;
 typedef union efi_tcg2_protocol efi_tcg2_protocol_t;
 
+struct efi_measured_event {
+	efi_tcg2_event_t	event_data;
+	efi_tcg2_tagged_event_t tagged_event;
+	u8			tagged_event_data[];
+};
+
 union efi_tcg2_protocol {
 	struct {
 		void *get_capability;
-- 
2.34.1

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

[Ilias Apalodimas]

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 <ilias.apalodimas@linaro.org>
---
 .../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

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

[Ard Biesheuvel]

On Fri, 16 Sept 2022 at 10:15, Ilias Apalodimas
<ilias.apalodimas@linaro.org> 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 <ilias.apalodimas@linaro.org>
> ---
>  .../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
>

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@linaro.org> 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
> >