On Wed, 27 Mar 2024 at 18:32, Tim Harvey tharvey@gateworks.com wrote:
On Wed, Mar 27, 2024 at 8:46 AM Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi both,
On Wed, 27 Mar 2024 at 17:22, Eddie James eajames@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@linaro.org wrote:
Hi Tim,
On Tue, 26 Mar 2024 at 03:15, Tim Harvey tharvey@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@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@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.
Thanks /Ilias
Best Regards,
Tim