[PATCH 2/3] tpm: make pcr_read command more useful

[Simon Glass]

Hi Ludwig,

On 2026-04-29T11:41:40, Ludwig Nussel <ludwig.nussel at siemens.com> wrote:
> tpm: make pcr_read command more useful
>
> Output now more similar to Linux userspace tool 'tpm2_pcrread'.
>
> Without arguments the command now prints contents of all PCR registers
>
> Signed-off-by: Ludwig Nussel <ludwig.nussel at siemens.com>
>
> cmd/tpm-v2.c | 54 ++++++++++++++++++++++++++++++++++++------------------
>  1 file changed, 36 insertions(+), 18 deletions(-)

> diff --git a/cmd/tpm-v2.c b/cmd/tpm-v2.c
> @@ -150,10 +151,11 @@ static int do_tpm_pcr_read(struct cmd_tbl *cmdtp, int flag, int argc,
>       u32 index, rc;
>       int algo_len;
>       unsigned int updates;
> -     void *data;
> +     u8 *data;
>       int ret;
> +     u32 pcr_start, pcr_end;
>
> -     if (argc < 3 || argc > 4)
> +     if (argc > 4)
>               return CMD_RET_USAGE;

Please mention the new behaviour explicitly in the commit message -
something like "with no arguments all PCRs are read; with only the PCR
index a buffer is allocated internally."

The subject is also a bit vague - how about 'tpm: print all PCRs from pcr_read'

> diff --git a/cmd/tpm-v2.c b/cmd/tpm-v2.c
> @@ -164,28 +166,44 @@ static int do_tpm_pcr_read(struct cmd_tbl *cmdtp, int flag, int argc,
>
>       ret = get_tpm(&dev);
>       if (ret)
> -             return ret;
> +             goto out;
>
>       priv = dev_get_uclass_priv(dev);
>       if (!priv)
> -             return -EINVAL;

...
> +out:
>       return report_return_code(rc);

rc is never assigned on either of these paths, so report_return_code()
reads an uninitialised value. Either drop the goto and return
ret/-EINVAL as before, or assign rc before the jump.

This return -EINVAL bypasses the out: label you just added...

> diff --git a/cmd/tpm-v2.c b/cmd/tpm-v2.c
> @@ -164,28 +166,44 @@ static int do_tpm_pcr_read(struct cmd_tbl *cmdtp, int flag, int argc,
> +     if (argc >= 3)
> +             data = map_sysmem(simple_strtoul(argv[2], NULL, 0), 0);
> +     else
> +             data = malloc(algo_len);

malloc() can fail and the result is dereferenced via tpm2_pcr_read()
without a check. Please bail out cleanly if data is NULL.

> diff --git a/cmd/tpm-v2.c b/cmd/tpm-v2.c
> @@ -164,28 +166,44 @@ static int do_tpm_pcr_read(struct cmd_tbl *cmdtp, int flag, int argc,
> +     printf('%8s:\n', tpm2_algorithm_name(algo));
> +     for (index = pcr_start; index < pcr_end; ++index) {
> +             rc = tpm2_pcr_read(dev, index, priv->pcr_select_min, algo,
> +                                data, algo_len, &updates);
> +             if (!rc) {
> +                     printf("%6u: 0x", index);
> +                     for (int i = 0; i < algo_len; ++i)
> +                             printf('%02X', data[i]);
> +                     printf(" (%u known updates)\n", updates);
> +             }
> +     }

A failing read in the middle of the loop is silently swallowed - the
user sees a gap and then a bogus banner with no error. At minimum
log_err() the failure, and consider whether to keep going or abort.
Also rc from the last iteration is what gets reported, which is
misleading if an earlier PCR failed.

> diff --git a/cmd/tpm-v2.c b/cmd/tpm-v2.c
> @@ -548,7 +566,7 @@ U_BOOT_CMD(tpm2, CONFIG_SYS_MAXARGS, 1, do_tpm, "Issue a TPMv2.x command",
>  "    Extend PCR #<pcr> with digest at <digest_addr> with digest_algo.\n"
>  "    <pcr>: index of the PCR\n"
>  "    <digest_addr>: address of digest of digest_algo type (defaults to SHA256)\n"
> -"pcr_read <pcr> <digest_addr> [<digest_algo>]\n"
> +"pcr_read [<pcr> [<digest_addr> [<digest_algo>]]]\n"
>  "    Read PCR #<pcr> to memory address <digest_addr> with <digest_algo>.\n"
>  "    <pcr>: index of the PCR\n"
>  "    <digest_addr>: address of digest of digest_algo type (defaults to SHA256)\n"

The description still says "Read PCR #<pcr> to memory address
<digest_addr>" - please update it to mention that with no <pcr> all
PCRs are printed, and that <digest_addr> is now optional.

Regards,
Simon