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

[Simon Glass <sjg@chromium.org>]

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?

> +       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;


> +               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

> +"    <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?

> +
> +       return 0;
> +}
> --
> 2.14.1
>

Regards,
Simon