tcg2_platform_get_log failing to read address and size of memory-region via ofnode_get_addr_size
Ilias Apalodimas
ilias.apalodimas at linaro.org
Wed Mar 27 18:05:37 CET 2024
On Wed, 27 Mar 2024 at 18:32, Tim Harvey <tharvey at gateworks.com> wrote:
>
> On Wed, Mar 27, 2024 at 8:46 AM Ilias Apalodimas
> <ilias.apalodimas at linaro.org> wrote:
> >
> > Hi both,
> >
> > On Wed, 27 Mar 2024 at 17:22, Eddie James <eajames at linux.ibm.com> wrote:
> > >
> > >
> > > On 3/26/24 11:15, Tim Harvey wrote:
> > > > On Tue, Mar 26, 2024 at 2:24 AM Ilias Apalodimas
> > > > <ilias.apalodimas at linaro.org> wrote:
> > > >> Hi Tim,
> > > >>
> > > >> On Tue, 26 Mar 2024 at 03:15, Tim Harvey <tharvey at gateworks.com> wrote:
> > > >>> Greetings,
> > > >>>
> > > >>> I'm unable to understand why tcg2_platform_get_log is failing to read
> > > >>> a memory region.
> > > >>>
> > > >>> For example the following diffs:
> > > >> I am not really sure what those nodes are supposed to do in sandbox.
> > > >> Pehaps Eddie remembers.
> > > >> What exactly are you trying to achieve here? Read the eventlog from TF-A?
> > > >>
> > > > Hi Ilias,
> > > >
> > > > I was trying to get measured boot (CONFIG_MEASURED_BOOT=y) working on
> > > > a tpm on my board but ran into an issue when I couldn't get the
> > > > memory-region I added for testing to be recognized with the current
> > > > code in tcg2_platform_get_log().
> > > >
> > > > I wonder if an event log should be required for measured boot - it
> > > > sounds like that was something required for EFI, so I was thinking of
> > > > submitting the following:
> > > > commit b3f336c2f863168219a93cd1c7ac922396e0fad5 (HEAD -> master-venice)
> > > > Author: Tim Harvey <tharvey at gateworks.com>
> > > > Date: Tue Mar 26 08:49:07 2024 -0700
> > > >
> > > > tpm: allow measured boot without an event log
> > > >
> > > > Currently an event log is required for measured boot. Remove this
> > > > requirement.
> > > >
> > > > Signed-off-by: Tim Harvey <tharvey at gateworks.com>
> > > >
> > > > diff --git a/lib/tpm-v2.c b/lib/tpm-v2.c
> > > > index 68eaaa639f89..994f8089ba34 100644
> > > > --- a/lib/tpm-v2.c
> > > > +++ b/lib/tpm-v2.c
> > > > @@ -175,17 +175,19 @@ static int tcg2_log_append_check(struct
> > > > tcg2_event_log *elog, u32 pcr_index,
> > > > u32 event_size;
> > > > u8 *log;
> > > >
> > > > - event_size = size + tcg2_event_get_size(digest_list);
> > > > - if (elog->log_position + event_size > elog->log_size) {
> > > > - printf("%s: log too large: %u + %u > %u\n", __func__,
> > > > - elog->log_position, event_size, elog->log_size);
> > > > - return -ENOBUFS;
> > > > - }
> > > > + if (elog->log_size) {
> > > > + event_size = size + tcg2_event_get_size(digest_list);
> > > > + if (elog->log_position + event_size > elog->log_size) {
> > > > + printf("%s: log too large: %u + %u > %u\n", __func__,
> > > > + elog->log_position, event_size, elog->log_size);
> > > > + return -ENOBUFS;
> > > > + }
> > > >
> > > > - log = elog->log + elog->log_position;
> > > > - elog->log_position += event_size;
> > > > + log = elog->log + elog->log_position;
> > > > + elog->log_position += event_size;
> > > >
> > > > - tcg2_log_append(pcr_index, event_type, digest_list, size, event, log);
> > > > + tcg2_log_append(pcr_index, event_type, digest_list,
> > > > size, event, log);
> > > > + }
> > > >
> > > > return 0;
> > > > }
> > > > @@ -613,10 +615,8 @@ int tcg2_measurement_init(struct udevice **dev,
> > > > struct tcg2_event_log *elog,
> > > > return rc;
> > > >
> > > > rc = tcg2_log_prepare_buffer(*dev, elog, ignore_existing_log);
> > > > - if (rc) {
> > > > + if (rc)
> > > > tcg2_measurement_term(*dev, elog, true);
> > > > - return rc;
> > > > - }
> > > >
> > > > rc = tcg2_measure_event(*dev, elog, 0, EV_S_CRTM_VERSION,
> > > > strlen(version_string) + 1,
> > > >
> > > > Would you agree with removing the requirement for the event log?
> > >
> > >
> > > No, the log is required, otherwise it's fairly meaningless work. You
> > > need the log in your OS to verify the contents of the TPM.
> >
> > It's the other way around. You trust the TPM and replay the event log
> > in memory to verify it's correct.
> > That being said, I do agree the event log is pretty useful when trying
> > to understand how and what the platform measured. In any case, I'd
> > rather fix any issues rather than sidestep them.
> >
>
> Why do you need a log to verify the contents of the TPM? If the PCR's
> are not correct you can't get your secrets from the TPM and if they
> are you can regardless of a log. Where is this log requirement in the
> TCG specification?
Yes, as I said you *validate* the eventlog by looking at the TPM PCRs,
not the other way around.
The problem with the TCG spec is
- EFI_TCG2_PROTOCOL.GetEventLog can only returns either EFI_SUCCESS or
EFI_INVALID_PARAMETER. There's no EFI_UNSUPPORTED
- EFI_TCG2_PROTOCOL.HashLogExtendEvent has a flag
(EFI_TCG2_EXTEND_ONLY) which allows you to only extend PCRs and not
log them
But I can't find anywhere in the spec a statement that says "the
eventlog is optional"
>
> Please see Linux commit 805fa88e0780b ("tpm: Don't make log failures
> fatal") which has a commit log of:
>
> If a TPM is in disabled state, it's reasonable for it to have an empty
> log.
Yes, an empty log. Not missing a log overall. Which makes sense if the
TPM is disabled.
> Bailing out of probe in this case means that the PPI interface
> isn't available, so there's no way to then enable the TPM from the OS.
> In general it seems reasonable to ignore log errors - they shouldn't
> interfere with any other TPM functionality.
>
> That last sentence makes sense to me; Sure the log may be 'useful' to
> some but I feel like it's not a requirement and it certainly is not a
> requirement for Linux.
>
> > The return value of ofnode_get_addr_size() depends on a couple Kconfig
> > options. Any chance those differ from the ones Eddie is using?
> >
>
> I have the same dt changes. I think this has something to do with the
> whole are we using of or fdt, and is of live:
>
> $ git diff
> diff --git a/arch/arm/dts/imx8mm-venice-gw72xx.dtsi
> b/arch/arm/dts/imx8mm-venice-gw72xx.dtsi
> index 97ed34a3c586..5752a38c7b4c 100644
> --- a/arch/arm/dts/imx8mm-venice-gw72xx.dtsi
> +++ b/arch/arm/dts/imx8mm-venice-gw72xx.dtsi
> @@ -14,6 +14,17 @@
> usb1 = &usbotg2;
> };
>
> + reserved-memory {
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges;
> +
> + event_log: tcg_event_log at 40000000 {
> + no-map;
> + reg = <0x40000000 0x2000>;
> + };
> + };
> +
> led-controller {
> compatible = "gpio-leds";
> pinctrl-names = "default";
> @@ -91,6 +102,7 @@
> tpm at 1 {
> compatible = "tcg,tpm_tis-spi";
> reg = <0x1>;
> + memory-region = <&event_log>;
> spi-max-frequency = <36000000>;
> };
> };
> diff --git a/lib/tpm-v2.c b/lib/tpm-v2.c
> index 994f8089ba34..0bc08cebed2f 100644
> --- a/lib/tpm-v2.c
> +++ b/lib/tpm-v2.c
> @@ -672,11 +673,14 @@ __weak int tcg2_platform_get_log(struct udevice
> *dev, void **addr, u32 *size)
> phys_addr_t a;
> fdt_size_t s;
>
> +printf("%s %s ofnode=%px valid=%d is_np=%d of_live_active=%d\n",
> __func__, dev->name, dev_ofnode(dev).np, ofnode_valid(dev_ofnode(de
> v)), ofnode_is_np(dev_ofnode(dev)), of_live_active());
> if (dev_read_phandle_with_args(dev, "memory-region", NULL, 0,
> 0, &args))
> return -ENODEV;
> +printf("%s %s args:%d\n", __func__, dev->name, args.args_count);
>
> a = ofnode_get_addr_size(args.node, "reg", &s);
> +printf("%s %s a:%px\n", __func__, dev->name, (void*)a);
> if (a == FDT_ADDR_T_NONE)
> return -ENOMEM;
>
> This shows the following:
> tcg2_platform_get_log tpm at 1 ofnode=00000000000054d0 valid=1 is_np=0
> of_live_active=0
> tcg2_platform_get_log tpm at 1 args:0
> tcg2_platform_get_log tpm at 1 a:ffffffffffffffff
> ^^^ can't get address of reg
>
> so we have a valid of_node in the dev structure but its not a np?
>
> dev_read_phandle_with_args() is returning success but was not able to
> parse any args, so we should not use 'args.node' as the arg of
> ofnode_get_addr_size(args.node, "reg", &s);
>
> I think the mix of dev_read_phandle_with_args and ofnode_get_addr_size
> is wrong. U-Boot is full of constructs like this that are extremely
> confusing... missing fdt with of and the concept of if its live or
> not. I'm hoping Simon can shed some light on this and maybe give or
> point to a primer on all the of/dt/live stuff?
Thanks that helps. I'll try to reproduce it with sandbox and/or QEMU
with OF_LIVE and see if I get something.
Thanks
/Ilias
>
> Best Regards,
>
> Tim
More information about the U-Boot
mailing list