Hi Heinrich, On Thu, 8 Dec 2022 at 18:05, Heinrich Schuchardt <xypron.g...@gmx.de> wrote: > > Am 8. Dezember 2022 09:01:26 MEZ schrieb Ilias Apalodimas > <ilias.apalodi...@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.apalodi...@linaro.org> > >> > Signed-off-by: Ilias Apalodimas <ilias.apalodi...@linaro.org> > >> > Signed-off-by: Etienne Carriere <etienne.carri...@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 > >> > * > >> >