[U-Boot] [PATCH v3 13/25] tpm: add TPM2_GetCapability command support

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Simon,

On Wed, 2 May 2018 20:32:13 -0600, Simon Glass <sjg@chromium.org> wrote:

> Hi Miquel,
> 
> On 2 May 2018 at 02:59, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > Add support for the TPM2_GetCapability command.
> >
> > Change the command file and the help accordingly.
> >
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  cmd/tpm-v2.c     | 41 +++++++++++++++++++++++++++++++++++++++++
> >  include/tpm-v2.h | 15 +++++++++++++++
> >  lib/tpm-v2.c     | 25 +++++++++++++++++++++++++
> >  3 files changed, 81 insertions(+)  
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> nits below
> 
> >
> > diff --git a/cmd/tpm-v2.c b/cmd/tpm-v2.c
> > index a61d751b4a..39fd9f0064 100644
> > --- a/cmd/tpm-v2.c
> > +++ b/cmd/tpm-v2.c
> > @@ -108,6 +108,39 @@ static int do_tpm_pcr_read(cmd_tbl_t *cmdtp, int flag, int argc,
> >         return report_return_code(rc);
> >  }
> >
> > +static int do_tpm_get_capability(cmd_tbl_t *cmdtp, int flag, int argc,
> > +                                char * const argv[])
> > +{
> > +       u32 capability, property, rc;
> > +       u8 *data;
> > +       size_t count;
> > +       int i, j;
> > +
> > +       if (argc != 5)
> > +               return CMD_RET_USAGE;
> > +
> > +       capability = simple_strtoul(argv[1], NULL, 0);
> > +       property = simple_strtoul(argv[2], NULL, 0);
> > +       data = (void *)simple_strtoul(argv[3], NULL, 0);  
> 
> map_sysmem() again - can you please fix globally in your patches?

Done.

> 
> > +       count = simple_strtoul(argv[4], NULL, 0);
> > +
> > +       rc = tpm2_get_capability(capability, property, data, count);
> > +       if (!rc) {  
> 
> How about:
> 
> if (rc)
>    return report_return_code(rc);
> printf()...
> ...
> 
> return 0;

Sure, but I used a goto statement as the print use the 'data' pointer
that need to be "unmapped" in both cases.

> 
> 
> > +               printf("Capabilities read from TPM:\n");
> > +               for (i = 0; i < count; i++) {
> > +                       printf("Property 0x");
> > +                       for (j = 0; j < 4; j++)
> > +                               printf("%02x", data[(i * 8) + j]);
> > +                       printf(": 0x");
> > +                       for (j = 4; j < 8; j++)
> > +                               printf("%02x", data[(i * 8) + j]);
> > +                       printf("\n");
> > +               }
> > +       }
> > +
> > +       return report_return_code(rc);
> > +}
> > +
> >  static cmd_tbl_t tpm2_commands[] = {
> >         U_BOOT_CMD_MKENT(info, 0, 1, do_tpm_info, "", ""),
> >         U_BOOT_CMD_MKENT(init, 0, 1, do_tpm_init, "", ""),
> > @@ -116,6 +149,7 @@ static cmd_tbl_t tpm2_commands[] = {
> >         U_BOOT_CMD_MKENT(clear, 0, 1, do_tpm2_clear, "", ""),
> >         U_BOOT_CMD_MKENT(pcr_extend, 0, 1, do_tpm2_pcr_extend, "", ""),
> >         U_BOOT_CMD_MKENT(pcr_read, 0, 1, do_tpm_pcr_read, "", ""),
> > +       U_BOOT_CMD_MKENT(get_capability, 0, 1, do_tpm_get_capability, "", ""),
> >  };
> >
> >  cmd_tbl_t *get_tpm_commands(unsigned int *size)
> > @@ -155,4 +189,11 @@ U_BOOT_CMD(tpm, CONFIG_SYS_MAXARGS, 1, do_tpm, "Issue a TPMv2.x command",
> >  "    Read PCR #<pcr> to memory address <digest_addr>.\n"
> >  "    <pcr>: index of the PCR\n"
> >  "    <digest_addr>: address to store the a 32-byte SHA256 digest\n"
> > +"get_capability <capability> <property> <addr> <count>\n"
> > +"    Read and display <count> entries indexed by <capability>/<property>.\n"
> > +"    Values are 4-byte long and are written at <addr>.\n"  
> 
> 4 bytes long

Ok.

> 
> > +"    <capability>: capability\n"
> > +"    <property>: property\n"
> > +"    <addr>: address to store <count> entries of 4 bytes\n"
> > +"    <count>: number of entries to retrieve\n"
> >  );
> > diff --git a/include/tpm-v2.h b/include/tpm-v2.h
> > index 54d14df365..07cd3a5e99 100644
> > --- a/include/tpm-v2.h
> > +++ b/include/tpm-v2.h
> > @@ -175,4 +175,19 @@ u32 tpm2_pcr_extend(u32 index, const uint8_t *digest);
> >   */
> >  u32 tpm2_pcr_read(u32 index, void *data, unsigned int *updates);
> >
> > +/**
> > + * Issue a TPM2_GetCapability command.  This implementation is limited
> > + * to query property index that is 4-byte wide.
> > + *
> > + * @param capability   Partition of capabilities
> > + * @param property     Further definition of capability, which is
> > + *                     limited to be 4-byte wide
> > + * @param buf          Output buffer for capability information
> > + * @param prop_count   Size of output buffer
> > + *
> > + * @return return code of the operation
> > + */
> > +u32 tpm2_get_capability(u32 capability, u32 property, void *buf,
> > +                       size_t prop_count);
> > +
> >  #endif /* __TPM_V2_H */
> > diff --git a/lib/tpm-v2.c b/lib/tpm-v2.c
> > index d557b08f8b..c567c943df 100644
> > --- a/lib/tpm-v2.c
> > +++ b/lib/tpm-v2.c
> > @@ -158,3 +158,28 @@ u32 tpm2_pcr_read(u32 index, void *data, unsigned int *updates)
> >
> >         return 0;
> >  }
> > +
> > +u32 tpm2_get_capability(u32 capability, u32 property, void *buf,
> > +                       size_t prop_count)
> > +{
> > +       u8 command_v2[COMMAND_BUFFER_SIZE] = {
> > +               tpm_u16(TPM2_ST_NO_SESSIONS),           /* TAG */
> > +               tpm_u32(22),                            /* Length */
> > +               tpm_u32(TPM2_CC_GET_CAPABILITY),        /* Command code */
> > +
> > +               tpm_u32(capability),                    /* Capability */
> > +               tpm_u32(property),                      /* Property */
> > +               tpm_u32(prop_count),                    /* Property count */
> > +       };
> > +       u8 response[COMMAND_BUFFER_SIZE];
> > +       size_t response_len = COMMAND_BUFFER_SIZE;
> > +       int ret;
> > +
> > +       ret = tpm_sendrecv_command(command_v2, response, &response_len);
> > +       if (ret)
> > +               return ret;
> > +
> > +       memcpy(buf, &response[19], response_len - 19);  
> 
> What is 19 here? It is confusing to have open-coded values with no
> meaning. Is it sizeof() something, or a number in the spec?

It is hardcoded in the spec. It is a sum of sizeof(), indeed, I can
make it more readable.
 
> 
> > +
> > +       return 0;
> > +}
> > --
> > 2.14.1
> >  
> 
> Regards,
> Simon



-- 
Miquel Raynal, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com