Re: [PATCH 2/8] tpm: Require a digest source when extending the PCR

[Simon Glass <sjg@chromium.org>]

Hi Ilias,

On Tue, 7 Jun 2022 at 02:42, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> On Mon, Feb 28, 2022 at 05:11:19PM -0700, Simon Glass wrote:
> > This feature is used for measured boot. It is not currently supported in
> > the TPM drivers, but add it to the API so that code which expects it can
> > signal its request.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> >  cmd/tpm-v1.c      |  3 ++-
> >  cmd/tpm_test.c    |  5 +++--
> >  include/tpm_api.h |  8 +++++---
> >  lib/tpm-v2.c      |  2 ++
> >  lib/tpm_api.c     | 14 ++++++++++----
> >  5 files changed, 22 insertions(+), 10 deletions(-)

(long pause as I forgot about this and only just discovered it was not applied)

> >
> > diff --git a/cmd/tpm-v1.c b/cmd/tpm-v1.c
> > index bf238a9f2e..0869b70775 100644
> > --- a/cmd/tpm-v1.c
> > +++ b/cmd/tpm-v1.c
> > @@ -131,7 +131,8 @@ static int do_tpm_extend(struct cmd_tbl *cmdtp, int flag, int argc,
> >               return CMD_RET_FAILURE;
> >       }
> >
> > -     rc = tpm_pcr_extend(dev, index, in_digest, out_digest);
> > +     rc = tpm_pcr_extend(dev, index, in_digest, sizeof(in_digest),
> > +                         out_digest, "test");
>
> Where is the output value of an extended PCR needed in measured boot?
> IMHO this out_digest seems pointless.  I'd be happier if we just completely
> removed it and make the v2 variant look like v1 more.

It is used by the tpm command to display the digest value.

>
> >       if (!rc) {
> >               puts("PCR value after execution of the command:\n");
> >               print_byte_string(out_digest, sizeof(out_digest));
> > diff --git a/cmd/tpm_test.c b/cmd/tpm_test.c
> > index a3ccb12f53..b35eae81dc 100644
> > --- a/cmd/tpm_test.c
> > +++ b/cmd/tpm_test.c
> > @@ -91,7 +91,8 @@ static int test_early_extend(struct udevice *dev)
> >       tpm_init(dev);
> >       TPM_CHECK(tpm_startup(dev, TPM_ST_CLEAR));
> >       TPM_CHECK(tpm_continue_self_test(dev));
> > -     TPM_CHECK(tpm_pcr_extend(dev, 1, value_in, value_out));
> > +     TPM_CHECK(tpm_pcr_extend(dev, 1, value_in, sizeof(value_in), value_out,
> > +                              "test"));
> >       printf("done\n");
> >       return 0;
> >  }
> > @@ -438,7 +439,7 @@ static int test_timing(struct udevice *dev)
> >                  100);
> >       TTPM_CHECK(tpm_nv_read_value(dev, INDEX0, (uint8_t *)&x, sizeof(x)),
> >                  100);
> > -     TTPM_CHECK(tpm_pcr_extend(dev, 0, in, out), 200);
> > +     TTPM_CHECK(tpm_pcr_extend(dev, 0, in, sizeof(in), out, "test"), 200);
> >       TTPM_CHECK(tpm_set_global_lock(dev), 50);
> >       TTPM_CHECK(tpm_tsc_physical_presence(dev, PHYS_PRESENCE), 100);
> >       printf("done\n");
> > diff --git a/include/tpm_api.h b/include/tpm_api.h
> > index 11aa14eb79..3c8e48bc25 100644
> > --- a/include/tpm_api.h
> > +++ b/include/tpm_api.h
> > @@ -81,14 +81,16 @@ u32 tpm_nv_write_value(struct udevice *dev, u32 index, const void *data,
> >   *
> >   * @param dev                TPM device
> >   * @param index              index of the PCR
> > - * @param in_digest  160-bit value representing the event to be
> > + * @param in_digest  160/256-bit value representing the event to be
> >   *                   recorded
> > - * @param out_digest 160-bit PCR value after execution of the
> > + * @param size               size of digest in bytes
> > + * @param out_digest 160/256-bit PCR value after execution of the
> >   *                   command
> > + * @param name               additional info about where the digest comes from
> >   * Return: return code of the operation
> >   */
> >  u32 tpm_pcr_extend(struct udevice *dev, u32 index, const void *in_digest,
> > -                void *out_digest);
> > +                uint size, void *out_digest, const char *name);
> >
> >  /**
> >   * Issue a TPM_PCRRead command.
> > diff --git a/lib/tpm-v2.c b/lib/tpm-v2.c
> > index 1bf627853a..6058f2e1e4 100644
> > --- a/lib/tpm-v2.c
> > +++ b/lib/tpm-v2.c
> > @@ -157,6 +157,8 @@ u32 tpm2_pcr_extend(struct udevice *dev, u32 index, u32 algorithm,
> >       };
> >       int ret;
> >
> > +     if (!digest)
> > +             return -EINVAL;
> >       /*
> >        * Fill the command structure starting from the first buffer:
> >        *     - the digest
> > diff --git a/lib/tpm_api.c b/lib/tpm_api.c
> > index 4ac4612c81..a8d3731d3a 100644
> > --- a/lib/tpm_api.c
> > +++ b/lib/tpm_api.c
> > @@ -140,15 +140,21 @@ u32 tpm_write_lock(struct udevice *dev, u32 index)
> >  }
> >
> >  u32 tpm_pcr_extend(struct udevice *dev, u32 index, const void *in_digest,
> > -                void *out_digest)
> > +                uint size, void *out_digest, const char *name)
> >  {
> > -     if (tpm_is_v1(dev))
> > +     if (tpm_is_v1(dev)) {
> > +             if (size != PCR_DIGEST_LENGTH || !out_digest)
> > +                     return -EINVAL;
> >               return tpm1_extend(dev, index, in_digest, out_digest);
> > -     else if (tpm_is_v2(dev))
> > +     } else if (tpm_is_v2(dev)) {
> > +             if (size != TPM2_SHA256_DIGEST_SIZE)
> > +                     return -EINVAL;
>
> Why are we limiting this?  This is supposed to be dictated by the PCR bank
> configuration of each hardware

OK I can drop this. Normally we use SHA256 for TPMv2 and I'm not sure
we suppose anything else. But we don't have to....

>
> >               return tpm2_pcr_extend(dev, index, TPM2_ALG_SHA256, in_digest,
> >                                      TPM2_DIGEST_LEN);
> > -     else
> > +             /* @name is ignored as we do not support measured boot */
> > +     } else {
> >               return -ENOSYS;
> > +     }
> >  }
> >
> >  u32 tpm_pcr_read(struct udevice *dev, u32 index, void *data, size_t count)
> > --
> > 2.35.1.574.g5d30c73bfb-goog

Regards,
Simon