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 17:00:17 CET 2024
Hi Tim,
On Tue, 26 Mar 2024 at 18:15, Tim Harvey <tharvey at gateworks.com> 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?
>
> I have another question that perhaps you may have some feedback on.
> The tpm commands such as pcr_extend, pcr_read currently require a
> 32-byte SHA256 digest and I wish to extend that as my TPM supports
> only SHA1. The tpm2_pcr_extend and tpm2_pcr_read functions were
> extended to function to allow the digest type and length to be passed
> in and I'm wondering what the best way to extend the tpm extend/read
> commands would be to support that.
We could add the argument of the algorithm of the PCR bank we want to
use on the command line. So do_tpm_pcr_read() etc could have an extra
argument for the chosen algorithm.
But we could also auto-detect the enabled PCR banks and extend/print
all of those without adding any extra arguments. That depends on what
functionality you are after (see below)
>
> The tcg2_create_digest function creates a digest based on the
> capabilities of the tpm and the tcg2_pcr_extend loops over those
> calling tpm2_pcr_extend for each digtest supported (and same for
> tcg2_pcr_read looping over tpm2_pcr_read) and I'm assuming TPM's can
> support multiple algos so I suppose a parameter needs to be added to
> the pcr_read and pcr_extend commands. Would you agree with that?
So you want to extend different PCR banks with different measurements?
The TCG spec for measured boot requires all active PCR banks to be
extended with measurements [0]. Specifically, it says
"The function SHALL successfully send the TPM2_PCR_Extend command to the TPM
to extend the PCR indicated by EfiTcgEvent.Header.PCRIndex with the measurement
digest. If the command cannot be sent successfully, the function SHALL return
EFI_DEVICE_ERROR. Firmware SHALL extend digests for all active PCR banks".
That relates with your question above. If we want to stick to the TCG
spec recommendation we don't to pass the algorithm of the
'pcr_extend/read" commands. We just have to teach them to extend/dump
measurements for all active banks
[0] https://trustedcomputinggroup.org/wp-content/uploads/EFI-Protocol-Specification-rev13-160330final.pdf
6.6 EFI_TCG2_PROTOCOL.HashLogExtendEvent
Regards
/Ilias
>
> Best Regards,
>
> Tim
>
> > Thanks
> > /Ilias
> > >
> > > diff --git a/arch/arm/dts/imx8mm-venice-gw73xx.dtsi
> > > b/arch/arm/dts/imx8mm-venice-gw73xx.dtsi
> > > index 7b2130dbdb21..57b3c227ceaf 100644
> > > --- a/arch/arm/dts/imx8mm-venice-gw73xx.dtsi
> > > +++ b/arch/arm/dts/imx8mm-venice-gw73xx.dtsi
> > > @@ -112,6 +112,7 @@
> > > compatible = "tcg,tpm_tis-spi";
> > > reg = <0x1>;
> > > spi-max-frequency = <36000000>;
> > > + memory-region = <&event_log>;
> > > };
> > > };
> > > diff --git a/arch/arm/dts/imx8mm-venice-gw700x.dtsi
> > > b/arch/arm/dts/imx8mm-venice-gw700x.dtsi
> > > index c305e325d007..697fd1148785 100644
> > > --- a/arch/arm/dts/imx8mm-venice-gw700x.dtsi
> > > +++ b/arch/arm/dts/imx8mm-venice-gw700x.dtsi
> > > @@ -13,6 +13,17 @@
> > > reg = <0x0 0x40000000 0 0x80000000>;
> > > };
> > >
> > > + reserved-memory {
> > > + #address-cells = <2>;
> > > + #size-cells = <2>;
> > > + ranges;
> > > +
> > > + event_log: tcg_event_log {
> > > + no-map;
> > > + reg = <0 0x40000000 0x2000>;
> > > + };
> > > + };
> > > +
> > > gpio-keys {
> > > compatible = "gpio-keys";
> > >
> > > And at runtime:
> > > u-boot=> fdt addr $fdtcontroladdr
> > > u-boot=> fdt list /soc at 0/bus at 30800000/spba-bus at 30800000/spi at 30830000/tpm at 1/
> > > tpm at 1 {
> > > compatible = "tcg,tpm_tis-spi";
> > > reg = <0x00000001>;
> > > spi-max-frequency = <0x02255100>;
> > > memory-region = <0x00000025>;
> > > };
> > > u-boot=> fdt list /reserved-memory/
> > > reserved-memory {
> > > #address-cells = <0x00000002>;
> > > #size-cells = <0x00000002>;
> > > ranges;
> > > tcg_event_log {
> > > };
> > > };
> > > u-boot=> fdt list /reserved-memory/tcg_event_log
> > > tcg_event_log {
> > > no-map;
> > > reg = <0x00000000 0x40000000 0x00002000>;
> > > phandle = <0x00000025>;
> > > };
> > >
> > > So why does the following code in tcg2_platform_get_log() return -ENOMEM?
> > >
> > > if (dev_read_phandle_with_args(dev, "memory-region", NULL, 0,
> > > 0, &args))
> > > return -ENODEV;
> > >
> > > a = ofnode_get_addr_size(args.node, "reg", &s);
> > > if (a == FDT_ADDR_T_NONE)
> > > return -ENOMEM;
> > >
> > > debugging shows that dev_read_phandle_with_args returns non-zero but
> > > args.args_count is 0.
You mean dev_read_phandle_with_args() returns 0 right?
> > >
> > > I feel like the construct of using dev_read_phandle_with_args followed
> > > by the ofnode_get_addr_size is just wrong but I don't understand why
> > > nor do I understand how my dt changes differ from what is in
> > > arch/sandbox/dts/test.dts (other than its using address-size=1 which
> > > doesn't appear to be the issue in my testing). The abstraction of the
> > > ofnode and fdt stuff always trip me up... very confusing.
> > >
> > > Can anyone explain the issue here?
> > >
> > > Best Regards,
> > >
> > > Tim
More information about the U-Boot
mailing list