[PATCH] efi_loader: Measure the loaded DTB
Masahisa Kojima
masahisa.kojima at linaro.org
Thu Dec 8 10:33:48 CET 2022
Hi Heinrich,
On Thu, 8 Dec 2022 at 18:05, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> Am 8. Dezember 2022 09:01:26 MEZ schrieb Ilias Apalodimas <ilias.apalodimas at 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 at linaro.org>
> >> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>
> >> > Signed-off-by: Etienne Carriere <etienne.carriere at 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
> >> > *
> >>
>
More information about the U-Boot
mailing list