Hi Simon,
On Wed, 2 May 2018 20:32:13 -0600, Simon Glass <s...@chromium.org> wrote:
> Hi Miquel,
>
> On 2 May 2018 at 02:59, Miquel Raynal <miquel.ray...@bootlin.com> wrote:
> > Add support for the TPM2_GetCapability command.
> >
> > Change the command file and the help accordingly.
> >
> > Signed-off-by: Miquel Raynal <miquel.ray...@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 <s...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
