[PATCH] efi_loader: Measure the loaded DTB
Heinrich Schuchardt
xypron.glpk at gmx.de
Fri Dec 9 09:04:08 CET 2022
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 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.
>
> 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
>>>>> *
>>>>
>>
More information about the U-Boot
mailing list