On Wed, Mar 27, 2024 at 10:06 AM Ilias Apalodimas
<ilias.apalodi...@linaro.org> wrote:
>
> On Wed, 27 Mar 2024 at 18:32, Tim Harvey <thar...@gateworks.com> wrote:
> >
> > On Wed, Mar 27, 2024 at 8:46 AM Ilias Apalodimas
> > <ilias.apalodi...@linaro.org> wrote:
> > >
> > > Hi both,
> > >
> > > On Wed, 27 Mar 2024 at 17:22, Eddie James <eaja...@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.apalodi...@linaro.org> wrote:
> > > > >> Hi Tim,
> > > > >>
> > > > >> On Tue, 26 Mar 2024 at 03:15, Tim Harvey <thar...@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 <thar...@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 <thar...@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@40000000 {
> > + no-map;
> > + reg = <0x40000000 0x2000>;
> > + };
> > + };
> > +
> > led-controller {
> > compatible = "gpio-leds";
> > pinctrl-names = "default";
> > @@ -91,6 +102,7 @@
> > tpm@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@1 ofnode=00000000000054d0 valid=1 is_np=0
> > of_live_active=0
> > tcg2_platform_get_log tpm@1 args:0
> > tcg2_platform_get_log tpm@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,
Tim
