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

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