Re: [PATCH] efi_loader: Measure the loaded DTB

[Masahisa Kojima <masahisa.kojima@linaro.org>]

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
> >> >    *
> >>
>