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?

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

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.

The return value of ofnode_get_addr_size() depends on a couple Kconfig options. Any chance those differ from the ones Eddie is using?

Thanks /Ilias

Here is the device tree reserved memory stuff we're using, perhaps it will help.

diff --git a/arch/arm/dts/ast2600-p10bmc.dts b/arch/arm/dts/ast2600-p10bmc.dts index 1d0f88bf96..8fbfeaa0d7 100755 --- a/arch/arm/dts/ast2600-p10bmc.dts +++ b/arch/arm/dts/ast2600-p10bmc.dts @@ -13,6 +13,17 @@ reg = <0x80000000 0x40000000>; };

•   reserved-memory {
  

•           #address-cells = <1>;
  

•           #size-cells = <1>;
  

•           ranges;
  

•           event_log: tcg_event_log@b3d00000 {
  

•                   no-map;
  

•                   reg = <0xb3d00000 0x100000>;
  

•           };
  

•   };
  

•    chosen {
             stdout-path = &uart5;
     };
  

@@ -113,6 +124,7 @@ tpm@2e { compatible = "nuvoton,npct75x"; reg = <0x2e>;

•           memory-region = <&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

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

Hi Tim,

[...]

...

...

...

...

...

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,

Looking at the sandbox config file OF_LIVE is enabled. I haven't looked into the differences on ofnode_get_addr_size(), ofnode_get_addr_size_index(), why is the index one needed?

btw, why are you trying to create the eventlog area like this? IIRC linux doesn't support that. Looking at the latest kernel 'tcg_event_log' is only defined in aspeed bmc's on TPMs so I guess they have a special usecase for that, hence the code Eddie carried in U-Boot. For your case, isn't it preferable to add the log area with linux,sml-base, linux,sml-size? Linux will try to read the eventlog in that case.

Thanks /Ilias

Tim

Hi Tim,

On Tue, 26 Mar 2024 at 18:15, Tim Harvey tharvey@gateworks.com 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?

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-Specificat... 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@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.

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