On Tue, Mar 26, 2024 at 2:24 AM Ilias Apalodimas <[email protected]> wrote: > > Hi Tim, > > On Tue, 26 Mar 2024 at 03:15, Tim Harvey <[email protected]> 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 <[email protected]> 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 <[email protected]> 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. 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? 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@0/bus@30800000/spba-bus@30800000/spi@30830000/tpm@1/ > > tpm@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. > > > > 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
