tcg2_platform_get_log failing to read address and size of memory-region via ofnode_get_addr_size

[Ilias Apalodimas]

Hi Tim,

[...]

> > > > > >
> > > > > > 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.
> >
>
> Ilias,
>
> I think the following is needed here:
> diff --git a/lib/tpm-v2.c b/lib/tpm-v2.c
> index 994f8089ba34..6b9d587e491c 100644
> --- a/lib/tpm-v2.c
> +++ b/lib/tpm-v2.c
> @@ -675,8 +675,7 @@ __weak int tcg2_platform_get_log(struct udevice
> *dev, void **addr, u32 *size)
>                 if (dev_read_phandle_with_args(dev, "memory-region", NULL, 0,
>                                                0, &args))
>                         return -ENODEV;
> -
> -               a = ofnode_get_addr_size(args.node, "reg", &s);
> +               a = ofnode_get_addr_size_index(args.node, 0, &s);
>                 if (a == FDT_ADDR_T_NONE)
>                         return -ENOMEM;
>
>
> Best Regards,

Looking at the sandbox config file OF_LIVE is enabled. I haven't
looked into the differences on ofnode_get_addr_size(),
ofnode_get_addr_size_index(), why is the index one needed?

btw, why are you trying to create the eventlog area like this? IIRC
linux doesn't support that. Looking at the latest kernel
'tcg_event_log' is only defined in aspeed bmc's on TPMs so I guess
they have a special usecase for that, hence the code Eddie carried in
U-Boot.
For your case, isn't it preferable to add the log area with
linux,sml-base, linux,sml-size? Linux will try to read the eventlog in
that case.

Thanks
/Ilias
>
> Tim