tcg2_platform_get_log failing to read address and size of memory-region via ofnode_get_addr_size
Tim Harvey
tharvey at gateworks.com
Wed Mar 27 17:32:17 CET 2024
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?
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. 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?
Best Regards,
Tim
More information about the U-Boot
mailing list