diff mbox series

efi_loader: Measure the loaded DTB

Message ID 20221207151110.529106-1-etienne.carriere@linaro.org
State Superseded
Headers show
Series efi_loader: Measure the loaded DTB | expand

Commit Message

Etienne Carriere Dec. 7, 2022, 3:11 p.m. UTC
Measures the DTB passed to the EFI application upon new boolean config
switch CONFIG_EFI_TCG2_PROTOCOL_MEASURE_DTB. For platforms where the
content of the DTB passed to the OS can change across reboots, there is
not point measuring it hence the config switch to allow platform to not
embed this feature.

Co-developed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
---
 cmd/bootefi.c             |  9 +++++++++
 include/efi_loader.h      |  2 ++
 include/efi_tcg2.h        | 10 ++++++++++
 include/tpm-v2.h          |  2 ++
 lib/efi_loader/Kconfig    | 12 ++++++++++++
 lib/efi_loader/efi_tcg2.c | 36 ++++++++++++++++++++++++++++++++++++
 6 files changed, 71 insertions(+)

Comments

Heinrich Schuchardt Dec. 8, 2022, 4:12 a.m. UTC | #1
On 12/7/22 16:11, Etienne Carriere wrote:
> Measures the DTB passed to the EFI application upon new boolean config
> switch CONFIG_EFI_TCG2_PROTOCOL_MEASURE_DTB. For platforms where the
> content of the DTB passed to the OS can change across reboots, there is
> not point measuring it hence the config switch to allow platform to not
> embed this feature.
>
> Co-developed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> ---
>   cmd/bootefi.c             |  9 +++++++++
>   include/efi_loader.h      |  2 ++
>   include/efi_tcg2.h        | 10 ++++++++++
>   include/tpm-v2.h          |  2 ++
>   lib/efi_loader/Kconfig    | 12 ++++++++++++
>   lib/efi_loader/efi_tcg2.c | 36 ++++++++++++++++++++++++++++++++++++
>   6 files changed, 71 insertions(+)
>
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index 2a7d42925d..56e4a1909f 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -315,6 +315,15 @@ efi_status_t efi_install_fdt(void *fdt)
>   		return EFI_LOAD_ERROR;
>   	}
>
> +	/* Measure the installed DTB */
> +	if (CONFIG_IS_ENABLED(EFI_TCG2_PROTOCOL_MEASURE_DTB)) {
> +		ret = efi_tcg2_measure_dtb(fdt);

Why measure here and not in efi_tcg2_measure_efi_app_invocation() where
we measure the SMBIOS table every time when an app is started with
StartImage()? Which may mean measuring it multiple times like when
starting iPXE, when starting GRUB, and again when starting Linux.

What is the value of measuring the device-tree here when GRUB or
systemd-boot afterwards load a different device-tree or modify the
provided device-tree?

> +		if (ret == EFI_SECURITY_VIOLATION) {
> +			log_err("ERROR: failed to measure DTB\n");
> +			return ret;
> +		}
> +	}
> +
>   	/* Prepare device tree for payload */
>   	ret = copy_fdt(&fdt);
>   	if (ret) {
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 0899e293e5..7538b6b828 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -530,6 +530,8 @@ efi_status_t efi_tcg2_notify_exit_boot_services_failed(void);
>   efi_status_t efi_tcg2_measure_efi_app_invocation(struct efi_loaded_image_obj *handle);
>   /* Measure efi application exit */
>   efi_status_t efi_tcg2_measure_efi_app_exit(void);
> +/* Measure DTB */
> +efi_status_t efi_tcg2_measure_dtb(void *fdt);
>   /* Called by bootefi to initialize root node */
>   efi_status_t efi_root_node_register(void);
>   /* Called by bootefi to initialize runtime */
> diff --git a/include/efi_tcg2.h b/include/efi_tcg2.h
> index 874306dc11..b1c3abd097 100644
> --- a/include/efi_tcg2.h
> +++ b/include/efi_tcg2.h
> @@ -233,6 +233,16 @@ struct efi_gpt_data {
>   	gpt_entry partitions[];
>   } __packed;
>
> +/**
> + * struct tdUEFI_PLATFORM_FIRMWARE_BLOB2
> + * @blob_description_size:	Byte size of @data
> + * @data:			Description data
> + */
> +struct uefi_platform_firmware_blob2 {
> +	u8 blob_description_size;
> +	u8 data[];
> +} __packed;
> +
>   struct efi_tcg2_protocol {
>   	efi_status_t (EFIAPI * get_capability)(struct efi_tcg2_protocol *this,
>   					       struct efi_tcg2_boot_service_capability *capability);
> diff --git a/include/tpm-v2.h b/include/tpm-v2.h
> index 737e57551d..2df3dad553 100644
> --- a/include/tpm-v2.h
> +++ b/include/tpm-v2.h
> @@ -105,6 +105,8 @@ struct udevice;
>   	"Exit Boot Services Returned with Failure"
>   #define EFI_EXIT_BOOT_SERVICES_SUCCEEDED    \
>   	"Exit Boot Services Returned with Success"
> +#define EFI_DTB_EVENT_STRING \
> +	"DTB DATA"
>
>   /* TPMS_TAGGED_PROPERTY Structure */
>   struct tpms_tagged_property {
> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> index e2b643871b..e490236d14 100644
> --- a/lib/efi_loader/Kconfig
> +++ b/lib/efi_loader/Kconfig
> @@ -337,6 +337,18 @@ config EFI_TCG2_PROTOCOL_EVENTLOG_SIZE
>   		this is going to be allocated twice. One for the eventlog it self
>   		and one for the configuration table that is required from the spec
>
> +config EFI_TCG2_PROTOCOL_MEASURE_DTB
> +	bool "Measure DTB with EFI_TCG2_PROTOCOL"
> +	depends on EFI_TCG2_PROTOCOL
> +	default n

This line has no effect.

> +	help
> +	  When enabled, the DTB image passed to the booted EFI image is
> +	  measured using EFI TCG2 protocol. Do not enable this feature if

%s/using/using the/

> +	  the passed DTB contains data that change across platform reboots
> +	  and cannot be used has a predictable measurement. Otherwise

How should the user know this? Shall we create dependencies to other
Kconfig symbols? e.g.

     depends on !NET_RANDOM_ETHADDR

> +	  this feature allows better measurement of the system boot
> +	  sequence.
> +
>   config EFI_LOAD_FILE2_INITRD
>   	bool "EFI_FILE_LOAD2_PROTOCOL for Linux initial ramdisk"
>   	default y
> diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
> index a525ebf75b..51c9d80828 100644
> --- a/lib/efi_loader/efi_tcg2.c
> +++ b/lib/efi_loader/efi_tcg2.c
> @@ -2175,6 +2175,42 @@ out1:
>   	return ret;
>   }
>
> +/**
> + * efi_tcg2_measure_dtb() - measure the dtb used to boot our OS
> + *
> + * @fdt: pointer to the device tree blob
> + *
> + * Return:	status code
> + */
> +efi_status_t efi_tcg2_measure_dtb(void *fdt)
> +{
> +	efi_status_t ret;
> +	struct uefi_platform_firmware_blob2 *blob;
> +	struct udevice *dev;
> +	u32 event_size;
> +
> +	if (!is_tcg2_protocol_installed())
> +		return EFI_SUCCESS;
> +
> +	ret = platform_get_tpm2_device(&dev);
> +	if (ret != EFI_SUCCESS)
> +		return EFI_SECURITY_VIOLATION;
> +
> +	event_size = sizeof(*blob) + sizeof(EFI_DTB_EVENT_STRING) + fdt_totalsize(fdt);
> +	blob = calloc(1, event_size);
> +	if (!blob)
> +		return EFI_OUT_OF_RESOURCES;
> +
> +	blob->blob_description_size = sizeof(EFI_DTB_EVENT_STRING);
> +	memcpy(blob->data, EFI_DTB_EVENT_STRING, blob->blob_description_size);
> +	memcpy(blob->data + blob->blob_description_size, fdt, fdt_totalsize(fdt));
> +
> +	ret = tcg2_measure_event(dev, 0, EV_POST_CODE, event_size, (u8 *)blob);

We should ignore "free space" surrounding blocks. This free space is
expected to contain random data. E.g in copy_fdt() we do not zero out
the memory after the strings block.

On the RISC-V architecture we always write the boot-hart ID into the
device-tree to be backward compatible to old kernels. Which other
device-tree information is random and needs to be ignored when measuring?

Is there a standard defining into which PCR the DTB shall be measured?
Why did you choose pcr_index=0?

Why measure device-trees but not ACPI tables?

Best regards

Heinrich

> +
> +	free(blob);
> +	return ret;
> +}
> +
>   /**
>    * efi_tcg2_measure_efi_app_invocation() - measure efi app invocation
>    *
Ilias Apalodimas Dec. 8, 2022, 8:01 a.m. UTC | #2
Hi Heinrich

On Thu, Dec 08, 2022 at 05:12:26AM +0100, Heinrich Schuchardt wrote:
> On 12/7/22 16:11, Etienne Carriere wrote:
> > Measures the DTB passed to the EFI application upon new boolean config
> > switch CONFIG_EFI_TCG2_PROTOCOL_MEASURE_DTB. For platforms where the
> > content of the DTB passed to the OS can change across reboots, there is
> > not point measuring it hence the config switch to allow platform to not
> > embed this feature.
> > 
> > Co-developed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> > ---
> >   cmd/bootefi.c             |  9 +++++++++
> >   include/efi_loader.h      |  2 ++
> >   include/efi_tcg2.h        | 10 ++++++++++
> >   include/tpm-v2.h          |  2 ++
> >   lib/efi_loader/Kconfig    | 12 ++++++++++++
> >   lib/efi_loader/efi_tcg2.c | 36 ++++++++++++++++++++++++++++++++++++
> >   6 files changed, 71 insertions(+)
> > 
> > diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> > index 2a7d42925d..56e4a1909f 100644
> > --- a/cmd/bootefi.c
> > +++ b/cmd/bootefi.c
> > @@ -315,6 +315,15 @@ efi_status_t efi_install_fdt(void *fdt)
> >   		return EFI_LOAD_ERROR;
> >   	}
> > 
> > +	/* Measure the installed DTB */
> > +	if (CONFIG_IS_ENABLED(EFI_TCG2_PROTOCOL_MEASURE_DTB)) {
> > +		ret = efi_tcg2_measure_dtb(fdt);
> 
> Why measure here and not in efi_tcg2_measure_efi_app_invocation() where
> we measure the SMBIOS table every time when an app is started with
> StartImage()? Which may mean measuring it multiple times like when
> starting iPXE, when starting GRUB, and again when starting Linux.

IIRC the spec says you should try and measure things once.  On top of that
it has no wording for DTB,  but for ACPI tables it says they should be measured prior
to any fix-ups.  We pretty much copied the recommendations for ACPI apart
from when we measure the data.  For ACPI it says "ACPI flash data prior to
any modifications" [0] but since U-Boot allows us to change the loaded DTB we
tried to measure the one we end up installing in the config table.

> 
> What is the value of measuring the device-tree here when GRUB or
> systemd-boot afterwards load a different device-tree or modify the
> provided device-tree?

If they modify if there's some usage since you, in theory, trust and
hopefully verified those binaries and the modifications they are about to
perform.  
If they end up replacing it there's no point in measuring, but that's exactly why
this is under a Kconfig option.  If they load a different DTB then they are responsible
for measuring it?

[...]

> > +			log_err("ERROR: failed to measure DTB\n");
> > +			return ret;
> > +		}
> > +	}
> > +
> 
> %s/using/using the/
> 
> > +	  the passed DTB contains data that change across platform reboots
> > +	  and cannot be used has a predictable measurement. Otherwise
> 
> How should the user know this? Shall we create dependencies to other
> Kconfig symbols? e.g.
> 
>     depends on !NET_RANDOM_ETHADDR
> 

That's not a bad idea.  Maybe we can add the ones that make sense and keep
extending it if we find more values affect the 'randomness'
The whole point of having a discrete Kconfig on that is that we don't
really know what kind of random things people end up dumping on their DTB.

> > +	  this feature allows better measurement of the system boot
> > +	  sequence.
> > +
> >   config EFI_LOAD_FILE2_INITRD
> >   	bool "EFI_FILE_LOAD2_PROTOCOL for Linux initial ramdisk"
> >   	default y
> > diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
> > index a525ebf75b..51c9d80828 100644
> > --- a/lib/efi_loader/efi_tcg2.c
> > +++ b/lib/efi_loader/efi_tcg2.c
> > @@ -2175,6 +2175,42 @@ out1:
> >   	return ret;
> >   }
> > 
> > +/**
> > + * efi_tcg2_measure_dtb() - measure the dtb used to boot our OS
> > + *
> > + * @fdt: pointer to the device tree blob
> > + *
> > + * Return:	status code
> > + */
> > +efi_status_t efi_tcg2_measure_dtb(void *fdt)
> > +{
> > +	efi_status_t ret;
> > +	struct uefi_platform_firmware_blob2 *blob;
> > +	struct udevice *dev;
> > +	u32 event_size;
> > +
> > +	if (!is_tcg2_protocol_installed())
> > +		return EFI_SUCCESS;
> > +
> > +	ret = platform_get_tpm2_device(&dev);
> > +	if (ret != EFI_SUCCESS)
> > +		return EFI_SECURITY_VIOLATION;
> > +
> > +	event_size = sizeof(*blob) + sizeof(EFI_DTB_EVENT_STRING) + fdt_totalsize(fdt);
> > +	blob = calloc(1, event_size);
> > +	if (!blob)
> > +		return EFI_OUT_OF_RESOURCES;
> > +
> > +	blob->blob_description_size = sizeof(EFI_DTB_EVENT_STRING);
> > +	memcpy(blob->data, EFI_DTB_EVENT_STRING, blob->blob_description_size);
> > +	memcpy(blob->data + blob->blob_description_size, fdt, fdt_totalsize(fdt));
> > +
> > +	ret = tcg2_measure_event(dev, 0, EV_POST_CODE, event_size, (u8 *)blob);
> 
> We should ignore "free space" surrounding blocks. This free space is
> expected to contain random data. E.g in copy_fdt() we do not zero out
> the memory after the strings block.
> 
> On the RISC-V architecture we always write the boot-hart ID into the
> device-tree to be backward compatible to old kernels. Which other
> device-tree information is random and needs to be ignored when measuring?
> 
> Is there a standard defining into which PCR the DTB shall be measured?
> Why did you choose pcr_index=0?

The TCG spec doesn't have any wording regarding DTB atm.  PCR0 is what's
defined in the spec for ACPI tables

[0] https://trustedcomputinggroup.org/wp-content/uploads/TCG_PCClient_PFP_r1p05_v23_pub.pdf (page 29)

> 
> Why measure device-trees but not ACPI tables?

You can measure those as well and we probably should,  but this patch is
only trying to address DTBs.

Regards
/Ilias

> 
> Best regards
> 
> Heinrich
> 
> > +
> > +	free(blob);
> > +	return ret;
> > +}
> > +
> >   /**
> >    * efi_tcg2_measure_efi_app_invocation() - measure efi app invocation
> >    *
>
Heinrich Schuchardt Dec. 8, 2022, 9:05 a.m. UTC | #3
Am 8. Dezember 2022 09:01:26 MEZ schrieb Ilias Apalodimas <ilias.apalodimas@linaro.org>:
>Hi Heinrich
>
>On Thu, Dec 08, 2022 at 05:12:26AM +0100, Heinrich Schuchardt wrote:
>> On 12/7/22 16:11, Etienne Carriere wrote:
>> > Measures the DTB passed to the EFI application upon new boolean config
>> > switch CONFIG_EFI_TCG2_PROTOCOL_MEASURE_DTB. For platforms where the
>> > content of the DTB passed to the OS can change across reboots, there is
>> > not point measuring it hence the config switch to allow platform to not
>> > embed this feature.
>> > 
>> > Co-developed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>> > Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
>> > ---
>> >   cmd/bootefi.c             |  9 +++++++++
>> >   include/efi_loader.h      |  2 ++
>> >   include/efi_tcg2.h        | 10 ++++++++++
>> >   include/tpm-v2.h          |  2 ++
>> >   lib/efi_loader/Kconfig    | 12 ++++++++++++
>> >   lib/efi_loader/efi_tcg2.c | 36 ++++++++++++++++++++++++++++++++++++
>> >   6 files changed, 71 insertions(+)
>> > 
>> > diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>> > index 2a7d42925d..56e4a1909f 100644
>> > --- a/cmd/bootefi.c
>> > +++ b/cmd/bootefi.c
>> > @@ -315,6 +315,15 @@ efi_status_t efi_install_fdt(void *fdt)
>> >   		return EFI_LOAD_ERROR;
>> >   	}
>> > 
>> > +	/* Measure the installed DTB */
>> > +	if (CONFIG_IS_ENABLED(EFI_TCG2_PROTOCOL_MEASURE_DTB)) {
>> > +		ret = efi_tcg2_measure_dtb(fdt);
>> 
>> Why measure here and not in efi_tcg2_measure_efi_app_invocation() where
>> we measure the SMBIOS table every time when an app is started with
>> StartImage()? Which may mean measuring it multiple times like when
>> starting iPXE, when starting GRUB, and again when starting Linux.
>
>IIRC the spec says you should try and measure things once.  On top of that
>it has no wording for DTB,  but for ACPI tables it says they should be measured prior
>to any fix-ups.  We pretty much copied the recommendations for ACPI apart
>from when we measure the data.  For ACPI it says "ACPI flash data prior to
>any modifications" [0] but since U-Boot allows us to change the loaded DTB we
>tried to measure the one we end up installing in the config table.

If for ACPI it is sufficient to measure before fixups, doing the same for dtbs should be adequate. This would avoid variable parts like random MAC addresses, boot-hart ID, kaslr seed but should include applied overlays. So just measuring the dtb passed into bootefi or the fallback dtb.

Why do we measure SMBIOS multiple times?


>
>> 
>> What is the value of measuring the device-tree here when GRUB or
>> systemd-boot afterwards load a different device-tree or modify the
>> provided device-tree?
>
>If they modify if there's some usage since you, in theory, trust and
>hopefully verified those binaries and the modifications they are about to
>perform.  
>If they end up replacing it there's no point in measuring, but that's exactly why
>this is under a Kconfig option.  If they load a different DTB then they are responsible
>for measuring it?
>
>[...]
>
>> > +			log_err("ERROR: failed to measure DTB\n");
>> > +			return ret;
>> > +		}
>> > +	}
>> > +
>> 
>> %s/using/using the/
>> 
>> > +	  the passed DTB contains data that change across platform reboots
>> > +	  and cannot be used has a predictable measurement. Otherwise
>> 
>> How should the user know this? Shall we create dependencies to other
>> Kconfig symbols? e.g.
>> 
>>     depends on !NET_RANDOM_ETHADDR
>> 
>
>That's not a bad idea.  Maybe we can add the ones that make sense and keep
>extending it if we find more values affect the 'randomness'
>The whole point of having a discrete Kconfig on that is that we don't
>really know what kind of random things people end up dumping on their DTB.
>
>> > +	  this feature allows better measurement of the system boot
>> > +	  sequence.
>> > +
>> >   config EFI_LOAD_FILE2_INITRD
>> >   	bool "EFI_FILE_LOAD2_PROTOCOL for Linux initial ramdisk"
>> >   	default y
>> > diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
>> > index a525ebf75b..51c9d80828 100644
>> > --- a/lib/efi_loader/efi_tcg2.c
>> > +++ b/lib/efi_loader/efi_tcg2.c
>> > @@ -2175,6 +2175,42 @@ out1:
>> >   	return ret;
>> >   }
>> > 
>> > +/**
>> > + * efi_tcg2_measure_dtb() - measure the dtb used to boot our OS
>> > + *
>> > + * @fdt: pointer to the device tree blob
>> > + *
>> > + * Return:	status code
>> > + */
>> > +efi_status_t efi_tcg2_measure_dtb(void *fdt)
>> > +{
>> > +	efi_status_t ret;
>> > +	struct uefi_platform_firmware_blob2 *blob;
>> > +	struct udevice *dev;
>> > +	u32 event_size;
>> > +
>> > +	if (!is_tcg2_protocol_installed())
>> > +		return EFI_SUCCESS;
>> > +
>> > +	ret = platform_get_tpm2_device(&dev);
>> > +	if (ret != EFI_SUCCESS)
>> > +		return EFI_SECURITY_VIOLATION;
>> > +
>> > +	event_size = sizeof(*blob) + sizeof(EFI_DTB_EVENT_STRING) + fdt_totalsize(fdt);
>> > +	blob = calloc(1, event_size);
>> > +	if (!blob)
>> > +		return EFI_OUT_OF_RESOURCES;
>> > +
>> > +	blob->blob_description_size = sizeof(EFI_DTB_EVENT_STRING);
>> > +	memcpy(blob->data, EFI_DTB_EVENT_STRING, blob->blob_description_size);
>> > +	memcpy(blob->data + blob->blob_description_size, fdt, fdt_totalsize(fdt));
>> > +
>> > +	ret = tcg2_measure_event(dev, 0, EV_POST_CODE, event_size, (u8 *)blob);
>> 
>> We should ignore "free space" surrounding blocks. This free space is
>> expected to contain random data. E.g in copy_fdt() we do not zero out
>> the memory after the strings block.

What are your thoughts on ignoring non-coding areas? This would avoid a lot of randomness.


>> 
>> On the RISC-V architecture we always write the boot-hart ID into the
>> device-tree to be backward compatible to old kernels. Which other
>> device-tree information is random and needs to be ignored when measuring?
>> 
>> Is there a standard defining into which PCR the DTB shall be measured?
>> Why did you choose pcr_index=0?
>
>The TCG spec doesn't have any wording regarding DTB atm.  PCR0 is what's
>defined in the spec for ACPI tables

That could be worth a code comment.

Best regards

Heinrich

>
>[0] https://trustedcomputinggroup.org/wp-content/uploads/TCG_PCClient_PFP_r1p05_v23_pub.pdf (page 29)
>
>> 
>> Why measure device-trees but not ACPI tables?
>
>You can measure those as well and we probably should,  but this patch is
>only trying to address DTBs.
>
>Regards
>/Ilias
>
>> 
>> Best regards
>> 
>> Heinrich
>> 
>> > +
>> > +	free(blob);
>> > +	return ret;
>> > +}
>> > +
>> >   /**
>> >    * efi_tcg2_measure_efi_app_invocation() - measure efi app invocation
>> >    *
>>
Masahisa Kojima Dec. 8, 2022, 9:33 a.m. UTC | #4
Hi Heinrich,

On Thu, 8 Dec 2022 at 18:05, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> Am 8. Dezember 2022 09:01:26 MEZ schrieb Ilias Apalodimas <ilias.apalodimas@linaro.org>:
> >Hi Heinrich
> >
> >On Thu, Dec 08, 2022 at 05:12:26AM +0100, Heinrich Schuchardt wrote:
> >> On 12/7/22 16:11, Etienne Carriere wrote:
> >> > Measures the DTB passed to the EFI application upon new boolean config
> >> > switch CONFIG_EFI_TCG2_PROTOCOL_MEASURE_DTB. For platforms where the
> >> > content of the DTB passed to the OS can change across reboots, there is
> >> > not point measuring it hence the config switch to allow platform to not
> >> > embed this feature.
> >> >
> >> > Co-developed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> >> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> >> > Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> >> > ---
> >> >   cmd/bootefi.c             |  9 +++++++++
> >> >   include/efi_loader.h      |  2 ++
> >> >   include/efi_tcg2.h        | 10 ++++++++++
> >> >   include/tpm-v2.h          |  2 ++
> >> >   lib/efi_loader/Kconfig    | 12 ++++++++++++
> >> >   lib/efi_loader/efi_tcg2.c | 36 ++++++++++++++++++++++++++++++++++++
> >> >   6 files changed, 71 insertions(+)
> >> >
> >> > diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> >> > index 2a7d42925d..56e4a1909f 100644
> >> > --- a/cmd/bootefi.c
> >> > +++ b/cmd/bootefi.c
> >> > @@ -315,6 +315,15 @@ efi_status_t efi_install_fdt(void *fdt)
> >> >            return EFI_LOAD_ERROR;
> >> >    }
> >> >
> >> > +  /* Measure the installed DTB */
> >> > +  if (CONFIG_IS_ENABLED(EFI_TCG2_PROTOCOL_MEASURE_DTB)) {
> >> > +          ret = efi_tcg2_measure_dtb(fdt);
> >>
> >> Why measure here and not in efi_tcg2_measure_efi_app_invocation() where
> >> we measure the SMBIOS table every time when an app is started with
> >> StartImage()? Which may mean measuring it multiple times like when
> >> starting iPXE, when starting GRUB, and again when starting Linux.
> >
> >IIRC the spec says you should try and measure things once.  On top of that
> >it has no wording for DTB,  but for ACPI tables it says they should be measured prior
> >to any fix-ups.  We pretty much copied the recommendations for ACPI apart
> >from when we measure the data.  For ACPI it says "ACPI flash data prior to
> >any modifications" [0] but since U-Boot allows us to change the loaded DTB we
> >tried to measure the one we end up installing in the config table.
>
> If for ACPI it is sufficient to measure before fixups, doing the same for dtbs should be adequate. This would avoid variable parts like random MAC addresses, boot-hart ID, kaslr seed but should include applied overlays. So just measuring the dtb passed into bootefi or the fallback dtb.
>
> Why do we measure SMBIOS multiple times?

SMBIOS is measured only once, there is a flag "tcg2_efi_app_invoked
"to avoid multiple measurements.

---
efi_status_t efi_tcg2_measure_efi_app_invocation(struct
efi_loaded_image_obj *handle)
{

<snip>

    if (!is_tcg2_protocol_installed())
        return EFI_SUCCESS;

    if (tcg2_efi_app_invoked)
        return EFI_SUCCESS;

<snip>

    tcg2_efi_app_invoked = true;
out:
    return ret;
}
---

Thanks,
Masahisa Kojima

>
>
> >
> >>
> >> What is the value of measuring the device-tree here when GRUB or
> >> systemd-boot afterwards load a different device-tree or modify the
> >> provided device-tree?
> >
> >If they modify if there's some usage since you, in theory, trust and
> >hopefully verified those binaries and the modifications they are about to
> >perform.
> >If they end up replacing it there's no point in measuring, but that's exactly why
> >this is under a Kconfig option.  If they load a different DTB then they are responsible
> >for measuring it?
> >
> >[...]
> >
> >> > +                  log_err("ERROR: failed to measure DTB\n");
> >> > +                  return ret;
> >> > +          }
> >> > +  }
> >> > +
> >>
> >> %s/using/using the/
> >>
> >> > +    the passed DTB contains data that change across platform reboots
> >> > +    and cannot be used has a predictable measurement. Otherwise
> >>
> >> How should the user know this? Shall we create dependencies to other
> >> Kconfig symbols? e.g.
> >>
> >>     depends on !NET_RANDOM_ETHADDR
> >>
> >
> >That's not a bad idea.  Maybe we can add the ones that make sense and keep
> >extending it if we find more values affect the 'randomness'
> >The whole point of having a discrete Kconfig on that is that we don't
> >really know what kind of random things people end up dumping on their DTB.
> >
> >> > +    this feature allows better measurement of the system boot
> >> > +    sequence.
> >> > +
> >> >   config EFI_LOAD_FILE2_INITRD
> >> >    bool "EFI_FILE_LOAD2_PROTOCOL for Linux initial ramdisk"
> >> >    default y
> >> > diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
> >> > index a525ebf75b..51c9d80828 100644
> >> > --- a/lib/efi_loader/efi_tcg2.c
> >> > +++ b/lib/efi_loader/efi_tcg2.c
> >> > @@ -2175,6 +2175,42 @@ out1:
> >> >    return ret;
> >> >   }
> >> >
> >> > +/**
> >> > + * efi_tcg2_measure_dtb() - measure the dtb used to boot our OS
> >> > + *
> >> > + * @fdt: pointer to the device tree blob
> >> > + *
> >> > + * Return:        status code
> >> > + */
> >> > +efi_status_t efi_tcg2_measure_dtb(void *fdt)
> >> > +{
> >> > +  efi_status_t ret;
> >> > +  struct uefi_platform_firmware_blob2 *blob;
> >> > +  struct udevice *dev;
> >> > +  u32 event_size;
> >> > +
> >> > +  if (!is_tcg2_protocol_installed())
> >> > +          return EFI_SUCCESS;
> >> > +
> >> > +  ret = platform_get_tpm2_device(&dev);
> >> > +  if (ret != EFI_SUCCESS)
> >> > +          return EFI_SECURITY_VIOLATION;
> >> > +
> >> > +  event_size = sizeof(*blob) + sizeof(EFI_DTB_EVENT_STRING) + fdt_totalsize(fdt);
> >> > +  blob = calloc(1, event_size);
> >> > +  if (!blob)
> >> > +          return EFI_OUT_OF_RESOURCES;
> >> > +
> >> > +  blob->blob_description_size = sizeof(EFI_DTB_EVENT_STRING);
> >> > +  memcpy(blob->data, EFI_DTB_EVENT_STRING, blob->blob_description_size);
> >> > +  memcpy(blob->data + blob->blob_description_size, fdt, fdt_totalsize(fdt));
> >> > +
> >> > +  ret = tcg2_measure_event(dev, 0, EV_POST_CODE, event_size, (u8 *)blob);
> >>
> >> We should ignore "free space" surrounding blocks. This free space is
> >> expected to contain random data. E.g in copy_fdt() we do not zero out
> >> the memory after the strings block.
>
> What are your thoughts on ignoring non-coding areas? This would avoid a lot of randomness.
>
>
> >>
> >> On the RISC-V architecture we always write the boot-hart ID into the
> >> device-tree to be backward compatible to old kernels. Which other
> >> device-tree information is random and needs to be ignored when measuring?
> >>
> >> Is there a standard defining into which PCR the DTB shall be measured?
> >> Why did you choose pcr_index=0?
> >
> >The TCG spec doesn't have any wording regarding DTB atm.  PCR0 is what's
> >defined in the spec for ACPI tables
>
> That could be worth a code comment.
>
> Best regards
>
> Heinrich
>
> >
> >[0] https://trustedcomputinggroup.org/wp-content/uploads/TCG_PCClient_PFP_r1p05_v23_pub.pdf (page 29)
> >
> >>
> >> Why measure device-trees but not ACPI tables?
> >
> >You can measure those as well and we probably should,  but this patch is
> >only trying to address DTBs.
> >
> >Regards
> >/Ilias
> >
> >>
> >> Best regards
> >>
> >> Heinrich
> >>
> >> > +
> >> > +  free(blob);
> >> > +  return ret;
> >> > +}
> >> > +
> >> >   /**
> >> >    * efi_tcg2_measure_efi_app_invocation() - measure efi app invocation
> >> >    *
> >>
>
Etienne Carriere Dec. 9, 2022, 7:31 a.m. UTC | #5
Hello Heinrich and all,

Thanks for the feedback.

On Thu, 8 Dec 2022 at 10:10, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> Am 8. Dezember 2022 09:01:26 MEZ schrieb Ilias Apalodimas <ilias.apalodimas@linaro.org>:
> >Hi Heinrich
> >
> >On Thu, Dec 08, 2022 at 05:12:26AM +0100, Heinrich Schuchardt wrote:
> >> On 12/7/22 16:11, Etienne Carriere wrote:
> >> > Measures the DTB passed to the EFI application upon new boolean config
> >> > switch CONFIG_EFI_TCG2_PROTOCOL_MEASURE_DTB. For platforms where the
> >> > content of the DTB passed to the OS can change across reboots, there is
> >> > not point measuring it hence the config switch to allow platform to not
> >> > embed this feature.
> >> >
> >> > Co-developed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> >> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> >> > Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> >> > ---
> >> >   cmd/bootefi.c             |  9 +++++++++
> >> >   include/efi_loader.h      |  2 ++
> >> >   include/efi_tcg2.h        | 10 ++++++++++
> >> >   include/tpm-v2.h          |  2 ++
> >> >   lib/efi_loader/Kconfig    | 12 ++++++++++++
> >> >   lib/efi_loader/efi_tcg2.c | 36 ++++++++++++++++++++++++++++++++++++
> >> >   6 files changed, 71 insertions(+)
> >> >
> >> > diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> >> > index 2a7d42925d..56e4a1909f 100644
> >> > --- a/cmd/bootefi.c
> >> > +++ b/cmd/bootefi.c
> >> > @@ -315,6 +315,15 @@ efi_status_t efi_install_fdt(void *fdt)
> >> >            return EFI_LOAD_ERROR;
> >> >    }
> >> >
> >> > +  /* Measure the installed DTB */
> >> > +  if (CONFIG_IS_ENABLED(EFI_TCG2_PROTOCOL_MEASURE_DTB)) {
> >> > +          ret = efi_tcg2_measure_dtb(fdt);
> >>
> >> Why measure here and not in efi_tcg2_measure_efi_app_invocation() where
> >> we measure the SMBIOS table every time when an app is started with
> >> StartImage()? Which may mean measuring it multiple times like when
> >> starting iPXE, when starting GRUB, and again when starting Linux.
> >
> >IIRC the spec says you should try and measure things once.  On top of that
> >it has no wording for DTB,  but for ACPI tables it says they should be measured prior
> >to any fix-ups.  We pretty much copied the recommendations for ACPI apart
> >from when we measure the data.  For ACPI it says "ACPI flash data prior to
> >any modifications" [0] but since U-Boot allows us to change the loaded DTB we
> >tried to measure the one we end up installing in the config table.
>
> If for ACPI it is sufficient to measure before fixups, doing the same for dtbs should be adequate. This would avoid variable parts like random MAC addresses, boot-hart ID, kaslr seed but should include applied overlays. So just measuring the dtb passed into bootefi or the fallback dtb.

I agree this feature is interesting when the DTB passed was initially
loaded from an external image. Measuring the loaded image(s ?) when
loaded would make more sense but there DTB is can be load from various
commands from generic or custom bootcmd script, commands like
"tftpboot ${fdt_addr_r} dtb/${efi_fdtfile}"  or load_efi_dtb variable
from  include/config_distro_bootcmd.h. We cannot run a generic
measurement a those places.


>
> Why do we measure SMBIOS multiple times?
>
>
> >
> >>
> >> What is the value of measuring the device-tree here when GRUB or
> >> systemd-boot afterwards load a different device-tree or modify the
> >> provided device-tree?
> >
> >If they modify if there's some usage since you, in theory, trust and
> >hopefully verified those binaries and the modifications they are about to
> >perform.
> >If they end up replacing it there's no point in measuring, but that's exactly why
> >this is under a Kconfig option.  If they load a different DTB then they are responsible
> >for measuring it?
> >
> >[...]
> >
> >> > +                  log_err("ERROR: failed to measure DTB\n");
> >> > +                  return ret;
> >> > +          }
> >> > +  }
> >> > +
> >>
> >> %s/using/using the/

acked, thanks.

> >>
> >> > +    the passed DTB contains data that change across platform reboots
> >> > +    and cannot be used has a predictable measurement. Otherwise
> >>
> >> How should the user know this? Shall we create dependencies to other
> >> Kconfig symbols? e.g.
> >>
> >>     depends on !NET_RANDOM_ETHADDR
> >>
> >
> >That's not a bad idea.  Maybe we can add the ones that make sense and keep
> >extending it if we find more values affect the 'randomness'
> >The whole point of having a discrete Kconfig on that is that we don't
> >really know what kind of random things people end up dumping on their DTB.
> >
> >> > +    this feature allows better measurement of the system boot
> >> > +    sequence.
> >> > +
> >> >   config EFI_LOAD_FILE2_INITRD
> >> >    bool "EFI_FILE_LOAD2_PROTOCOL for Linux initial ramdisk"
> >> >    default y
> >> > diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
> >> > index a525ebf75b..51c9d80828 100644
> >> > --- a/lib/efi_loader/efi_tcg2.c
> >> > +++ b/lib/efi_loader/efi_tcg2.c
> >> > @@ -2175,6 +2175,42 @@ out1:
> >> >    return ret;
> >> >   }
> >> >
> >> > +/**
> >> > + * efi_tcg2_measure_dtb() - measure the dtb used to boot our OS
> >> > + *
> >> > + * @fdt: pointer to the device tree blob
> >> > + *
> >> > + * Return:        status code
> >> > + */
> >> > +efi_status_t efi_tcg2_measure_dtb(void *fdt)
> >> > +{
> >> > +  efi_status_t ret;
> >> > +  struct uefi_platform_firmware_blob2 *blob;
> >> > +  struct udevice *dev;
> >> > +  u32 event_size;
> >> > +
> >> > +  if (!is_tcg2_protocol_installed())
> >> > +          return EFI_SUCCESS;
> >> > +
> >> > +  ret = platform_get_tpm2_device(&dev);
> >> > +  if (ret != EFI_SUCCESS)
> >> > +          return EFI_SECURITY_VIOLATION;
> >> > +
> >> > +  event_size = sizeof(*blob) + sizeof(EFI_DTB_EVENT_STRING) + fdt_totalsize(fdt);
> >> > +  blob = calloc(1, event_size);
> >> > +  if (!blob)
> >> > +          return EFI_OUT_OF_RESOURCES;
> >> > +
> >> > +  blob->blob_description_size = sizeof(EFI_DTB_EVENT_STRING);
> >> > +  memcpy(blob->data, EFI_DTB_EVENT_STRING, blob->blob_description_size);
> >> > +  memcpy(blob->data + blob->blob_description_size, fdt, fdt_totalsize(fdt));
> >> > +
> >> > +  ret = tcg2_measure_event(dev, 0, EV_POST_CODE, event_size, (u8 *)blob);
> >>
> >> We should ignore "free space" surrounding blocks. This free space is
> >> expected to contain random data. E.g in copy_fdt() we do not zero out
> >> the memory after the strings block.
>
> What are your thoughts on ignoring non-coding areas? This would avoid a lot of randomness.

Hmmm, right. To measure here, copy_fdt() should be modified.

>
>
> >>
> >> On the RISC-V architecture we always write the boot-hart ID into the
> >> device-tree to be backward compatible to old kernels. Which other
> >> device-tree information is random and needs to be ignored when measuring?
> >>
> >> Is there a standard defining into which PCR the DTB shall be measured?
> >> Why did you choose pcr_index=0?
> >
> >The TCG spec doesn't have any wording regarding DTB atm.  PCR0 is what's
> >defined in the spec for ACPI tables
>
> That could be worth a code comment.

Ok.

>
> Best regards
>
> Heinrich
>
> >
> >[0] https://trustedcomputinggroup.org/wp-content/uploads/TCG_PCClient_PFP_r1p05_v23_pub.pdf (page 29)
> >
> >>
> >> Why measure device-trees but not ACPI tables?
> >
> >You can measure those as well and we probably should,  but this patch is
> >only trying to address DTBs.
> >
> >Regards
> >/Ilias
> >
> >>
> >> Best regards
> >>
> >> Heinrich
> >>
> >> > +
> >> > +  free(blob);
> >> > +  return ret;
> >> > +}
> >> > +
> >> >   /**
> >> >    * efi_tcg2_measure_efi_app_invocation() - measure efi app invocation
> >> >    *
> >>
>
Heinrich Schuchardt Dec. 9, 2022, 8:04 a.m. UTC | #6
On 12/9/22 08:31, Etienne Carriere wrote:
> Hello Heinrich and all,
>
> Thanks for the feedback.
>
> On Thu, 8 Dec 2022 at 10:10, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> Am 8. Dezember 2022 09:01:26 MEZ schrieb Ilias Apalodimas <ilias.apalodimas@linaro.org>:
>>> Hi Heinrich
>>>
>>> On Thu, Dec 08, 2022 at 05:12:26AM +0100, Heinrich Schuchardt wrote:
>>>> On 12/7/22 16:11, Etienne Carriere wrote:
>>>>> Measures the DTB passed to the EFI application upon new boolean config
>>>>> switch CONFIG_EFI_TCG2_PROTOCOL_MEASURE_DTB. For platforms where the
>>>>> content of the DTB passed to the OS can change across reboots, there is
>>>>> not point measuring it hence the config switch to allow platform to not
>>>>> embed this feature.
>>>>>
>>>>> Co-developed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>>>>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>>>>> Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
>>>>> ---
>>>>>    cmd/bootefi.c             |  9 +++++++++
>>>>>    include/efi_loader.h      |  2 ++
>>>>>    include/efi_tcg2.h        | 10 ++++++++++
>>>>>    include/tpm-v2.h          |  2 ++
>>>>>    lib/efi_loader/Kconfig    | 12 ++++++++++++
>>>>>    lib/efi_loader/efi_tcg2.c | 36 ++++++++++++++++++++++++++++++++++++
>>>>>    6 files changed, 71 insertions(+)
>>>>>
>>>>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>>>>> index 2a7d42925d..56e4a1909f 100644
>>>>> --- a/cmd/bootefi.c
>>>>> +++ b/cmd/bootefi.c
>>>>> @@ -315,6 +315,15 @@ efi_status_t efi_install_fdt(void *fdt)
>>>>>             return EFI_LOAD_ERROR;
>>>>>     }
>>>>>
>>>>> +  /* Measure the installed DTB */
>>>>> +  if (CONFIG_IS_ENABLED(EFI_TCG2_PROTOCOL_MEASURE_DTB)) {
>>>>> +          ret = efi_tcg2_measure_dtb(fdt);
>>>>
>>>> Why measure here and not in efi_tcg2_measure_efi_app_invocation() where
>>>> we measure the SMBIOS table every time when an app is started with
>>>> StartImage()? Which may mean measuring it multiple times like when
>>>> starting iPXE, when starting GRUB, and again when starting Linux.
>>>
>>> IIRC the spec says you should try and measure things once.  On top of that
>>> it has no wording for DTB,  but for ACPI tables it says they should be measured prior
>>> to any fix-ups.  We pretty much copied the recommendations for ACPI apart
>> >from when we measure the data.  For ACPI it says "ACPI flash data prior to
>>> any modifications" [0] but since U-Boot allows us to change the loaded DTB we
>>> tried to measure the one we end up installing in the config table.
>>
>> If for ACPI it is sufficient to measure before fixups, doing the same for dtbs should be adequate. This would avoid variable parts like random MAC addresses, boot-hart ID, kaslr seed but should include applied overlays. So just measuring the dtb passed into bootefi or the fallback dtb.
>
> I agree this feature is interesting when the DTB passed was initially
> loaded from an external image. Measuring the loaded image(s ?) when
> loaded would make more sense but there DTB is can be load from various
> commands from generic or custom bootcmd script, commands like
> "tftpboot ${fdt_addr_r} dtb/${efi_fdtfile}"  or load_efi_dtb variable
> from  include/config_distro_bootcmd.h. We cannot run a generic
> measurement a those places.

All of these cases run through efi_install_fdt(). You could measure
after copy_fdt() and before image_setup_libfdt().

>
>
>>
>> Why do we measure SMBIOS multiple times?
>>
>>
>>>
>>>>
>>>> What is the value of measuring the device-tree here when GRUB or
>>>> systemd-boot afterwards load a different device-tree or modify the
>>>> provided device-tree?
>>>
>>> If they modify if there's some usage since you, in theory, trust and
>>> hopefully verified those binaries and the modifications they are about to
>>> perform.
>>> If they end up replacing it there's no point in measuring, but that's exactly why
>>> this is under a Kconfig option.  If they load a different DTB then they are responsible
>>> for measuring it?
>>>
>>> [...]
>>>
>>>>> +                  log_err("ERROR: failed to measure DTB\n");
>>>>> +                  return ret;
>>>>> +          }
>>>>> +  }
>>>>> +
>>>>
>>>> %s/using/using the/
>
> acked, thanks.
>
>>>>
>>>>> +    the passed DTB contains data that change across platform reboots
>>>>> +    and cannot be used has a predictable measurement. Otherwise
>>>>
>>>> How should the user know this? Shall we create dependencies to other
>>>> Kconfig symbols? e.g.
>>>>
>>>>      depends on !NET_RANDOM_ETHADDR
>>>>
>>>
>>> That's not a bad idea.  Maybe we can add the ones that make sense and keep
>>> extending it if we find more values affect the 'randomness'
>>> The whole point of having a discrete Kconfig on that is that we don't
>>> really know what kind of random things people end up dumping on their DTB.
>>>
>>>>> +    this feature allows better measurement of the system boot
>>>>> +    sequence.
>>>>> +
>>>>>    config EFI_LOAD_FILE2_INITRD
>>>>>     bool "EFI_FILE_LOAD2_PROTOCOL for Linux initial ramdisk"
>>>>>     default y
>>>>> diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
>>>>> index a525ebf75b..51c9d80828 100644
>>>>> --- a/lib/efi_loader/efi_tcg2.c
>>>>> +++ b/lib/efi_loader/efi_tcg2.c
>>>>> @@ -2175,6 +2175,42 @@ out1:
>>>>>     return ret;
>>>>>    }
>>>>>
>>>>> +/**
>>>>> + * efi_tcg2_measure_dtb() - measure the dtb used to boot our OS
>>>>> + *
>>>>> + * @fdt: pointer to the device tree blob
>>>>> + *
>>>>> + * Return:        status code
>>>>> + */
>>>>> +efi_status_t efi_tcg2_measure_dtb(void *fdt)
>>>>> +{
>>>>> +  efi_status_t ret;
>>>>> +  struct uefi_platform_firmware_blob2 *blob;
>>>>> +  struct udevice *dev;
>>>>> +  u32 event_size;
>>>>> +
>>>>> +  if (!is_tcg2_protocol_installed())
>>>>> +          return EFI_SUCCESS;
>>>>> +
>>>>> +  ret = platform_get_tpm2_device(&dev);
>>>>> +  if (ret != EFI_SUCCESS)
>>>>> +          return EFI_SECURITY_VIOLATION;
>>>>> +
>>>>> +  event_size = sizeof(*blob) + sizeof(EFI_DTB_EVENT_STRING) + fdt_totalsize(fdt);
>>>>> +  blob = calloc(1, event_size);
>>>>> +  if (!blob)
>>>>> +          return EFI_OUT_OF_RESOURCES;
>>>>> +
>>>>> +  blob->blob_description_size = sizeof(EFI_DTB_EVENT_STRING);
>>>>> +  memcpy(blob->data, EFI_DTB_EVENT_STRING, blob->blob_description_size);
>>>>> +  memcpy(blob->data + blob->blob_description_size, fdt, fdt_totalsize(fdt));
>>>>> +
>>>>> +  ret = tcg2_measure_event(dev, 0, EV_POST_CODE, event_size, (u8 *)blob);
>>>>
>>>> We should ignore "free space" surrounding blocks. This free space is
>>>> expected to contain random data. E.g in copy_fdt() we do not zero out
>>>> the memory after the strings block.
>>
>> What are your thoughts on ignoring non-coding areas? This would avoid a lot of randomness.
>
> Hmmm, right. To measure here, copy_fdt() should be modified.

I would prefer not to change the content of the loaded fdt but to just
restrict measurement to

* fdt_header, fdt_header + 40
* off_mem_rsvmap, off_mem_rsvmap + 16
* off_dt_struct, off_dt_struct + size_dt_struct
* off_dt_strings, off_dt_strings + size_dt_strings

Just start the hash (sha256_starts) and then call the hash update
function (sha256_update) for each of these. Use the hash end function
(sha256_finish) to obtain the result.

This all can be done in a new function called after copy_fdt().

Best regards

Heinrich

>
>>
>>
>>>>
>>>> On the RISC-V architecture we always write the boot-hart ID into the
>>>> device-tree to be backward compatible to old kernels. Which other
>>>> device-tree information is random and needs to be ignored when measuring?
>>>>
>>>> Is there a standard defining into which PCR the DTB shall be measured?
>>>> Why did you choose pcr_index=0?
>>>
>>> The TCG spec doesn't have any wording regarding DTB atm.  PCR0 is what's
>>> defined in the spec for ACPI tables
>>
>> That could be worth a code comment.
>
> Ok.
>
>>
>> Best regards
>>
>> Heinrich
>>
>>>
>>> [0] https://trustedcomputinggroup.org/wp-content/uploads/TCG_PCClient_PFP_r1p05_v23_pub.pdf (page 29)
>>>
>>>>
>>>> Why measure device-trees but not ACPI tables?
>>>
>>> You can measure those as well and we probably should,  but this patch is
>>> only trying to address DTBs.
>>>
>>> Regards
>>> /Ilias
>>>
>>>>
>>>> Best regards
>>>>
>>>> Heinrich
>>>>
>>>>> +
>>>>> +  free(blob);
>>>>> +  return ret;
>>>>> +}
>>>>> +
>>>>>    /**
>>>>>     * efi_tcg2_measure_efi_app_invocation() - measure efi app invocation
>>>>>     *
>>>>
>>
diff mbox series

Patch

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 2a7d42925d..56e4a1909f 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -315,6 +315,15 @@  efi_status_t efi_install_fdt(void *fdt)
 		return EFI_LOAD_ERROR;
 	}
 
+	/* Measure the installed DTB */
+	if (CONFIG_IS_ENABLED(EFI_TCG2_PROTOCOL_MEASURE_DTB)) {
+		ret = efi_tcg2_measure_dtb(fdt);
+		if (ret == EFI_SECURITY_VIOLATION) {
+			log_err("ERROR: failed to measure DTB\n");
+			return ret;
+		}
+	}
+
 	/* Prepare device tree for payload */
 	ret = copy_fdt(&fdt);
 	if (ret) {
diff --git a/include/efi_loader.h b/include/efi_loader.h
index 0899e293e5..7538b6b828 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -530,6 +530,8 @@  efi_status_t efi_tcg2_notify_exit_boot_services_failed(void);
 efi_status_t efi_tcg2_measure_efi_app_invocation(struct efi_loaded_image_obj *handle);
 /* Measure efi application exit */
 efi_status_t efi_tcg2_measure_efi_app_exit(void);
+/* Measure DTB */
+efi_status_t efi_tcg2_measure_dtb(void *fdt);
 /* Called by bootefi to initialize root node */
 efi_status_t efi_root_node_register(void);
 /* Called by bootefi to initialize runtime */
diff --git a/include/efi_tcg2.h b/include/efi_tcg2.h
index 874306dc11..b1c3abd097 100644
--- a/include/efi_tcg2.h
+++ b/include/efi_tcg2.h
@@ -233,6 +233,16 @@  struct efi_gpt_data {
 	gpt_entry partitions[];
 } __packed;
 
+/**
+ * struct tdUEFI_PLATFORM_FIRMWARE_BLOB2
+ * @blob_description_size:	Byte size of @data
+ * @data:			Description data
+ */
+struct uefi_platform_firmware_blob2 {
+	u8 blob_description_size;
+	u8 data[];
+} __packed;
+
 struct efi_tcg2_protocol {
 	efi_status_t (EFIAPI * get_capability)(struct efi_tcg2_protocol *this,
 					       struct efi_tcg2_boot_service_capability *capability);
diff --git a/include/tpm-v2.h b/include/tpm-v2.h
index 737e57551d..2df3dad553 100644
--- a/include/tpm-v2.h
+++ b/include/tpm-v2.h
@@ -105,6 +105,8 @@  struct udevice;
 	"Exit Boot Services Returned with Failure"
 #define EFI_EXIT_BOOT_SERVICES_SUCCEEDED    \
 	"Exit Boot Services Returned with Success"
+#define EFI_DTB_EVENT_STRING \
+	"DTB DATA"
 
 /* TPMS_TAGGED_PROPERTY Structure */
 struct tpms_tagged_property {
diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
index e2b643871b..e490236d14 100644
--- a/lib/efi_loader/Kconfig
+++ b/lib/efi_loader/Kconfig
@@ -337,6 +337,18 @@  config EFI_TCG2_PROTOCOL_EVENTLOG_SIZE
 		this is going to be allocated twice. One for the eventlog it self
 		and one for the configuration table that is required from the spec
 
+config EFI_TCG2_PROTOCOL_MEASURE_DTB
+	bool "Measure DTB with EFI_TCG2_PROTOCOL"
+	depends on EFI_TCG2_PROTOCOL
+	default n
+	help
+	  When enabled, the DTB image passed to the booted EFI image is
+	  measured using EFI TCG2 protocol. Do not enable this feature if
+	  the passed DTB contains data that change across platform reboots
+	  and cannot be used has a predictable measurement. Otherwise
+	  this feature allows better measurement of the system boot
+	  sequence.
+
 config EFI_LOAD_FILE2_INITRD
 	bool "EFI_FILE_LOAD2_PROTOCOL for Linux initial ramdisk"
 	default y
diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
index a525ebf75b..51c9d80828 100644
--- a/lib/efi_loader/efi_tcg2.c
+++ b/lib/efi_loader/efi_tcg2.c
@@ -2175,6 +2175,42 @@  out1:
 	return ret;
 }
 
+/**
+ * efi_tcg2_measure_dtb() - measure the dtb used to boot our OS
+ *
+ * @fdt: pointer to the device tree blob
+ *
+ * Return:	status code
+ */
+efi_status_t efi_tcg2_measure_dtb(void *fdt)
+{
+	efi_status_t ret;
+	struct uefi_platform_firmware_blob2 *blob;
+	struct udevice *dev;
+	u32 event_size;
+
+	if (!is_tcg2_protocol_installed())
+		return EFI_SUCCESS;
+
+	ret = platform_get_tpm2_device(&dev);
+	if (ret != EFI_SUCCESS)
+		return EFI_SECURITY_VIOLATION;
+
+	event_size = sizeof(*blob) + sizeof(EFI_DTB_EVENT_STRING) + fdt_totalsize(fdt);
+	blob = calloc(1, event_size);
+	if (!blob)
+		return EFI_OUT_OF_RESOURCES;
+
+	blob->blob_description_size = sizeof(EFI_DTB_EVENT_STRING);
+	memcpy(blob->data, EFI_DTB_EVENT_STRING, blob->blob_description_size);
+	memcpy(blob->data + blob->blob_description_size, fdt, fdt_totalsize(fdt));
+
+	ret = tcg2_measure_event(dev, 0, EV_POST_CODE, event_size, (u8 *)blob);
+
+	free(blob);
+	return ret;
+}
+
 /**
  * efi_tcg2_measure_efi_app_invocation() - measure efi app invocation
  *