| Message ID | 20240405001722.2876240-1-tharvey@gateworks.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Ilias Apalodimas |
| Headers | show |
| Series | [v3] tpm-v2: allow algoirthm name to be configured for pcr_read and pcr_extend | expand |
Hi Tim, Thanks for the patch I'll be away next week, I'll try to find time and take a closer look. The pipeline [0] shows some TPM related failures [0] https://source.denx.de/u-boot/custodians/u-boot-tpm/-/commit/9b4be64e41454e17269a968397933eeff300c380 Cheers /Ilias On Fri, 5 Apr 2024 at 03:17, Tim Harvey <tharvey@gateworks.com> wrote: > > For pcr_read and pcr_extend commands allow the digest algorithm to be > specified by an additional argument. If not specified it will default to > SHA256 for backwards compatibility. > > A follow-on to this could be to extend all PCR banks with the detected > algo when the <digest_algo> argument is 'auto'. > > Signed-off-by: Tim Harvey <tharvey@gateworks.com> > --- > v3: > - replace tpm2_supported_algorithms with struct and use this to relate hash algoirthm > details > v2: > - use tpm2_algorithm_to_len > - use enum tpm2_algorithms > - make function names and parameter names more consistent with existing > tpm-v2 functions > - fix various spelling errors > --- > cmd/tpm-v2.c | 49 ++++++++++++++++++++-------- > include/tpm-v2.h | 67 ++++++++++++++++++++++++++++++++++++++- > lib/efi_loader/efi_tcg2.c | 4 +-- > lib/tpm-v2.c | 62 +++++++++++++++++++++++------------- > 4 files changed, 143 insertions(+), 39 deletions(-) > > diff --git a/cmd/tpm-v2.c b/cmd/tpm-v2.c > index 7e479b9dfe36..2343b4d9cb9e 100644 > --- a/cmd/tpm-v2.c > +++ b/cmd/tpm-v2.c > @@ -99,11 +99,19 @@ static int do_tpm2_pcr_extend(struct cmd_tbl *cmdtp, int flag, int argc, > struct tpm_chip_priv *priv; > u32 index = simple_strtoul(argv[1], NULL, 0); > void *digest = map_sysmem(simple_strtoul(argv[2], NULL, 0), 0); > + int algo = TPM2_ALG_SHA256; > + int algo_len; > int ret; > u32 rc; > > - if (argc != 3) > + if (argc < 3 || argc > 4) > return CMD_RET_USAGE; > + if (argc == 4) { > + algo = tpm2_name_to_algorithm(argv[3]); > + if (algo < 0) > + return CMD_RET_FAILURE; > + } > + algo_len = tpm2_algorithm_to_len(algo); > > ret = get_tpm(&dev); > if (ret) > @@ -116,8 +124,12 @@ static int do_tpm2_pcr_extend(struct cmd_tbl *cmdtp, int flag, int argc, > if (index >= priv->pcr_count) > return -EINVAL; > > - rc = tpm2_pcr_extend(dev, index, TPM2_ALG_SHA256, digest, > - TPM2_DIGEST_LEN); > + rc = tpm2_pcr_extend(dev, index, algo, digest, algo_len); > + if (!rc) { > + printf("PCR #%u extended with %d byte %s digest\n", index, > + algo_len, tpm2_algorithm_name(algo)); > + print_byte_string(digest, algo_len); > + } > > unmap_sysmem(digest); > > @@ -127,15 +139,23 @@ static int do_tpm2_pcr_extend(struct cmd_tbl *cmdtp, int flag, int argc, > static int do_tpm_pcr_read(struct cmd_tbl *cmdtp, int flag, int argc, > char *const argv[]) > { > + enum tpm2_algorithms algo = TPM2_ALG_SHA256; > struct udevice *dev; > struct tpm_chip_priv *priv; > u32 index, rc; > + int algo_len; > unsigned int updates; > void *data; > int ret; > > - if (argc != 3) > + if (argc < 3 || argc > 4) > return CMD_RET_USAGE; > + if (argc == 4) { > + algo = tpm2_name_to_algorithm(argv[3]); > + if (algo < 0) > + return CMD_RET_FAILURE; > + } > + algo_len = tpm2_algorithm_to_len(algo); > > ret = get_tpm(&dev); > if (ret) > @@ -151,11 +171,12 @@ static int do_tpm_pcr_read(struct cmd_tbl *cmdtp, int flag, int argc, > > data = map_sysmem(simple_strtoul(argv[2], NULL, 0), 0); > > - rc = tpm2_pcr_read(dev, index, priv->pcr_select_min, TPM2_ALG_SHA256, > - data, TPM2_DIGEST_LEN, &updates); > + rc = tpm2_pcr_read(dev, index, priv->pcr_select_min, algo, > + data, algo_len, &updates); > if (!rc) { > - printf("PCR #%u content (%u known updates):\n", index, updates); > - print_byte_string(data, TPM2_DIGEST_LEN); > + printf("PCR #%u %s %d byte content (%u known updates):\n", index, > + tpm2_algorithm_name(algo), algo_len, updates); > + print_byte_string(data, algo_len); > } > > unmap_sysmem(data); > @@ -415,14 +436,14 @@ U_BOOT_CMD(tpm2, CONFIG_SYS_MAXARGS, 1, do_tpm, "Issue a TPMv2.x command", > " <hierarchy> is one of:\n" > " * TPM2_RH_LOCKOUT\n" > " * TPM2_RH_PLATFORM\n" > -"pcr_extend <pcr> <digest_addr>\n" > -" Extend PCR #<pcr> with digest at <digest_addr>.\n" > +"pcr_extend <pcr> <digest_addr> [<digest_algo>]\n" > +" Extend PCR #<pcr> with digest at <digest_addr> with digest_algo.\n" > " <pcr>: index of the PCR\n" > -" <digest_addr>: address of a 32-byte SHA256 digest\n" > -"pcr_read <pcr> <digest_addr>\n" > -" Read PCR #<pcr> to memory address <digest_addr>.\n" > +" <digest_addr>: address of digest of digest_algo type (defaults to SHA256)\n" > +"pcr_read <pcr> <digest_addr> [<digest_algo>]\n" > +" Read PCR #<pcr> to memory address <digest_addr> with <digest_algo>.\n" > " <pcr>: index of the PCR\n" > -" <digest_addr>: address to store the a 32-byte SHA256 digest\n" > +" <digest_addr>: address of digest of digest_algo type (defaults to SHA256)\n" > "get_capability <capability> <property> <addr> <count>\n" > " Read and display <count> entries indexed by <capability>/<property>.\n" > " Values are 4 bytes long and are written at <addr>.\n" > diff --git a/include/tpm-v2.h b/include/tpm-v2.h > index 33dd103767c4..263f9529e55d 100644 > --- a/include/tpm-v2.h > +++ b/include/tpm-v2.h > @@ -386,7 +386,54 @@ enum tpm2_algorithms { > TPM2_ALG_SM3_256 = 0x12, > }; > > -extern const enum tpm2_algorithms tpm2_supported_algorithms[4]; > +/** > + * struct digest_info - details of supported digests > + * > + * @hash_name: hash name > + * @hash_alg: hash algorithm id > + * @hash_mask: hash registry mask > + * @hash_len: hash digest length > + */ > +struct digest_info { > + const char *hash_name; > + u16 hash_alg; > + u32 hash_mask; > + u16 hash_len; > +}; > + > +/* Algorithm Registry */ > +#define TCG2_BOOT_HASH_ALG_SHA1 0x00000001 > +#define TCG2_BOOT_HASH_ALG_SHA256 0x00000002 > +#define TCG2_BOOT_HASH_ALG_SHA384 0x00000004 > +#define TCG2_BOOT_HASH_ALG_SHA512 0x00000008 > +#define TCG2_BOOT_HASH_ALG_SM3_256 0x00000010 > + > +static const struct digest_info hash_algo_list[] = { > + { > + "sha1", > + TPM2_ALG_SHA1, > + TCG2_BOOT_HASH_ALG_SHA1, > + TPM2_SHA1_DIGEST_SIZE, > + }, > + { > + "sha256", > + TPM2_ALG_SHA256, > + TCG2_BOOT_HASH_ALG_SHA256, > + TPM2_SHA256_DIGEST_SIZE, > + }, > + { > + "sha384", > + TPM2_ALG_SHA384, > + TCG2_BOOT_HASH_ALG_SHA384, > + TPM2_SHA384_DIGEST_SIZE, > + }, > + { > + "sha512", > + TPM2_ALG_SHA512, > + TCG2_BOOT_HASH_ALG_SHA512, > + TPM2_SHA512_DIGEST_SIZE, > + }, > +}; > > static inline u16 tpm2_algorithm_to_len(enum tpm2_algorithms a) > { > @@ -965,4 +1012,22 @@ u32 tpm2_enable_nvcommits(struct udevice *dev, uint vendor_cmd, > */ > u32 tpm2_auto_start(struct udevice *dev); > > +/** > + * tpm2_name_to_algorithm() - Return an algorithm id given a supported > + * algorithm name > + * > + * @name: algorithm name > + * Return: enum tpm2_algorithms or -EINVAL > + */ > +enum tpm2_algorithms tpm2_name_to_algorithm(const char *name); > + > +/** > + * tpm2_algorithm_name() - Return an algorithm name string for a > + * supported algorithm id > + * > + * @algorithm_id: algorithm defined in enum tpm2_algorithms > + * Return: algorithm name string or "" > + */ > +const char *tpm2_algorithm_name(enum tpm2_algorithms); > + > #endif /* __TPM_V2_H */ > diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c > index b07e0099c27e..cf5d8de018a7 100644 > --- a/lib/efi_loader/efi_tcg2.c > +++ b/lib/efi_loader/efi_tcg2.c > @@ -414,8 +414,8 @@ static efi_status_t tcg2_hash_pe_image(void *efi, u64 efi_size, > } > > digest_list->count = 0; > - for (i = 0; i < ARRAY_SIZE(tpm2_supported_algorithms); i++) { > - u16 hash_alg = tpm2_supported_algorithms[i]; > + for (i = 0; i < ARRAY_SIZE(hash_algo_list); i++) { > + u16 hash_alg = hash_algo_list[i].hash_alg; > > if (!(active & tpm2_algorithm_to_mask(hash_alg))) > continue; > diff --git a/lib/tpm-v2.c b/lib/tpm-v2.c > index 68eaaa639f89..83f490c82541 100644 > --- a/lib/tpm-v2.c > +++ b/lib/tpm-v2.c > @@ -22,13 +22,6 @@ > > #include "tpm-utils.h" > > -const enum tpm2_algorithms tpm2_supported_algorithms[4] = { > - TPM2_ALG_SHA1, > - TPM2_ALG_SHA256, > - TPM2_ALG_SHA384, > - TPM2_ALG_SHA512, > -}; > - > int tcg2_get_active_pcr_banks(struct udevice *dev, u32 *active_pcr_banks) > { > u32 supported = 0; > @@ -82,14 +75,14 @@ int tcg2_create_digest(struct udevice *dev, const u8 *input, u32 length, > return rc; > > digest_list->count = 0; > - for (i = 0; i < ARRAY_SIZE(tpm2_supported_algorithms); ++i) { > + for (i = 0; i < ARRAY_SIZE(hash_algo_list); ++i) { > u32 mask = > - tpm2_algorithm_to_mask(tpm2_supported_algorithms[i]); > + tpm2_algorithm_to_mask(hash_algo_list[i].hash_alg); > > if (!(active & mask)) > continue; > > - switch (tpm2_supported_algorithms[i]) { > + switch (hash_algo_list[i].hash_alg) { > case TPM2_ALG_SHA1: > sha1_starts(&ctx); > sha1_update(&ctx, input, length); > @@ -116,12 +109,12 @@ int tcg2_create_digest(struct udevice *dev, const u8 *input, u32 length, > break; > default: > printf("%s: unsupported algorithm %x\n", __func__, > - tpm2_supported_algorithms[i]); > + hash_algo_list[i].hash_alg); > continue; > } > > digest_list->digests[digest_list->count].hash_alg = > - tpm2_supported_algorithms[i]; > + hash_algo_list[i].hash_alg; > memcpy(&digest_list->digests[digest_list->count].digest, final, > len); > digest_list->count++; > @@ -208,13 +201,13 @@ static int tcg2_log_init(struct udevice *dev, struct tcg2_event_log *elog) > return rc; > > event_size = offsetof(struct tcg_efi_spec_id_event, digest_sizes); > - for (i = 0; i < ARRAY_SIZE(tpm2_supported_algorithms); ++i) { > - mask = tpm2_algorithm_to_mask(tpm2_supported_algorithms[i]); > + for (i = 0; i < ARRAY_SIZE(hash_algo_list); ++i) { > + mask = tpm2_algorithm_to_mask(hash_algo_list[i].hash_alg); > > if (!(active & mask)) > continue; > > - switch (tpm2_supported_algorithms[i]) { > + switch (hash_algo_list[i].hash_alg) { > case TPM2_ALG_SHA1: > case TPM2_ALG_SHA256: > case TPM2_ALG_SHA384: > @@ -253,17 +246,17 @@ static int tcg2_log_init(struct udevice *dev, struct tcg2_event_log *elog) > put_unaligned_le32(count, &ev->number_of_algorithms); > > count = 0; > - for (i = 0; i < ARRAY_SIZE(tpm2_supported_algorithms); ++i) { > - mask = tpm2_algorithm_to_mask(tpm2_supported_algorithms[i]); > + for (i = 0; i < ARRAY_SIZE(hash_algo_list); ++i) { > + mask = tpm2_algorithm_to_mask(hash_algo_list[i].hash_alg); > > if (!(active & mask)) > continue; > > - len = tpm2_algorithm_to_len(tpm2_supported_algorithms[i]); > + len = hash_algo_list[i].hash_len; > if (!len) > continue; > > - put_unaligned_le16(tpm2_supported_algorithms[i], > + put_unaligned_le16(hash_algo_list[i].hash_alg, > &ev->digest_sizes[count].algorithm_id); > put_unaligned_le16(len, &ev->digest_sizes[count].digest_size); > count++; > @@ -304,7 +297,7 @@ static int tcg2_replay_eventlog(struct tcg2_event_log *elog, > pos = offsetof(struct tcg_pcr_event2, digests) + > offsetof(struct tpml_digest_values, count); > count = get_unaligned_le32(log + pos); > - if (count > ARRAY_SIZE(tpm2_supported_algorithms) || > + if (count > ARRAY_SIZE(hash_algo_list) || > (digest_list->count && digest_list->count != count)) > return 0; > > @@ -407,7 +400,7 @@ static int tcg2_log_parse(struct udevice *dev, struct tcg2_event_log *elog) > return 0; > > count = get_unaligned_le32(&event->number_of_algorithms); > - if (count > ARRAY_SIZE(tpm2_supported_algorithms)) > + if (count > ARRAY_SIZE(hash_algo_list)) > return 0; > > calc_size = offsetof(struct tcg_efi_spec_id_event, digest_sizes) + > @@ -1110,7 +1103,7 @@ int tpm2_get_pcr_info(struct udevice *dev, u32 *supported_pcr, u32 *active_pcr, > * We only support 5 algorithms for now so check against that > * instead of TPM2_NUM_PCR_BANKS > */ > - if (pcrs.count > ARRAY_SIZE(tpm2_supported_algorithms) || > + if (pcrs.count > ARRAY_SIZE(hash_algo_list) || > pcrs.count < 1) { > printf("%s: too many pcrs: %u\n", __func__, pcrs.count); > return -EMSGSIZE; > @@ -1555,3 +1548,28 @@ u32 tpm2_enable_nvcommits(struct udevice *dev, uint vendor_cmd, > > return 0; > } > + > +enum tpm2_algorithms tpm2_name_to_algorithm(const char *name) > +{ > + size_t i; > + > + for (i = 0; i < ARRAY_SIZE(hash_algo_list); ++i) { > + if (!strcasecmp(name, hash_algo_list[i].hash_name)) > + return hash_algo_list[i].hash_alg; > + } > + printf("%s: unsupported algorithm %s\n", __func__, name); > + > + return -EINVAL; > +} > + > +const char *tpm2_algorithm_name(enum tpm2_algorithms algo) > +{ > + size_t i; > + > + for (i = 0; i < ARRAY_SIZE(hash_algo_list); ++i) { > + if (hash_algo_list[i].hash_alg == algo) > + return hash_algo_list[i].hash_name; > + } > + > + return ""; > +} > -- > 2.25.1 >
On Sat, Apr 6, 2024 at 9:33 AM Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Hi Tim, > > Thanks for the patch > > I'll be away next week, I'll try to find time and take a closer look. > The pipeline [0] shows some TPM related failures > > [0] https://source.denx.de/u-boot/custodians/u-boot-tpm/-/commit/9b4be64e41454e17269a968397933eeff300c380 > Hi Ilias, I changed the output of 'tpm pcr_read' so that it shows the algo and size causing the test in test/py/tests/test_tpm2.py to fail: @@ -151,11 +171,12 @@ static int do_tpm_pcr_read(struct cmd_tbl *cmdtp, int flag, int argc, data = map_sysmem(simple_strtoul(argv[2], NULL, 0), 0); - rc = tpm2_pcr_read(dev, index, priv->pcr_select_min, TPM2_ALG_SHA256, - data, TPM2_DIGEST_LEN, &updates); + rc = tpm2_pcr_read(dev, index, priv->pcr_select_min, algo, + data, algo_len, &updates); if (!rc) { - printf("PCR #%u content (%u known updates):\n", index, updates); - print_byte_string(data, TPM2_DIGEST_LEN); + printf("PCR #%u %s %d byte content (%u known updates):\n", index, + tpm2_algorithm_name(algo), algo_len, updates); + print_byte_string(data, algo_len); } failure: E AssertionError: assert 'PCR #10 content' in 'PCR #10 sha256 32 byte content (723 known updates):\r\r\n 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00\r\r\n 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00' So I suppose I need to update test/py/tests/test_tpm2.py as well. Would I update test/py/tests/test_tpm2.py in the same patch as the one that causes the failure? How do I go about running the tests manually to make sure I've addressed it? Best Regards, Tim > Cheers > /Ilias > > On Fri, 5 Apr 2024 at 03:17, Tim Harvey <tharvey@gateworks.com> wrote: > > > > For pcr_read and pcr_extend commands allow the digest algorithm to be > > specified by an additional argument. If not specified it will default to > > SHA256 for backwards compatibility. > > > > A follow-on to this could be to extend all PCR banks with the detected > > algo when the <digest_algo> argument is 'auto'. > > > > Signed-off-by: Tim Harvey <tharvey@gateworks.com> > > --- > > v3: > > - replace tpm2_supported_algorithms with struct and use this to relate hash algoirthm > > details > > v2: > > - use tpm2_algorithm_to_len > > - use enum tpm2_algorithms > > - make function names and parameter names more consistent with existing > > tpm-v2 functions > > - fix various spelling errors > > --- > > cmd/tpm-v2.c | 49 ++++++++++++++++++++-------- > > include/tpm-v2.h | 67 ++++++++++++++++++++++++++++++++++++++- > > lib/efi_loader/efi_tcg2.c | 4 +-- > > lib/tpm-v2.c | 62 +++++++++++++++++++++++------------- > > 4 files changed, 143 insertions(+), 39 deletions(-) > > > > diff --git a/cmd/tpm-v2.c b/cmd/tpm-v2.c > > index 7e479b9dfe36..2343b4d9cb9e 100644 > > --- a/cmd/tpm-v2.c > > +++ b/cmd/tpm-v2.c > > @@ -99,11 +99,19 @@ static int do_tpm2_pcr_extend(struct cmd_tbl *cmdtp, int flag, int argc, > > struct tpm_chip_priv *priv; > > u32 index = simple_strtoul(argv[1], NULL, 0); > > void *digest = map_sysmem(simple_strtoul(argv[2], NULL, 0), 0); > > + int algo = TPM2_ALG_SHA256; > > + int algo_len; > > int ret; > > u32 rc; > > > > - if (argc != 3) > > + if (argc < 3 || argc > 4) > > return CMD_RET_USAGE; > > + if (argc == 4) { > > + algo = tpm2_name_to_algorithm(argv[3]); > > + if (algo < 0) > > + return CMD_RET_FAILURE; > > + } > > + algo_len = tpm2_algorithm_to_len(algo); > > > > ret = get_tpm(&dev); > > if (ret) > > @@ -116,8 +124,12 @@ static int do_tpm2_pcr_extend(struct cmd_tbl *cmdtp, int flag, int argc, > > if (index >= priv->pcr_count) > > return -EINVAL; > > > > - rc = tpm2_pcr_extend(dev, index, TPM2_ALG_SHA256, digest, > > - TPM2_DIGEST_LEN); > > + rc = tpm2_pcr_extend(dev, index, algo, digest, algo_len); > > + if (!rc) { > > + printf("PCR #%u extended with %d byte %s digest\n", index, > > + algo_len, tpm2_algorithm_name(algo)); > > + print_byte_string(digest, algo_len); > > + } > > > > unmap_sysmem(digest); > > > > @@ -127,15 +139,23 @@ static int do_tpm2_pcr_extend(struct cmd_tbl *cmdtp, int flag, int argc, > > static int do_tpm_pcr_read(struct cmd_tbl *cmdtp, int flag, int argc, > > char *const argv[]) > > { > > + enum tpm2_algorithms algo = TPM2_ALG_SHA256; > > struct udevice *dev; > > struct tpm_chip_priv *priv; > > u32 index, rc; > > + int algo_len; > > unsigned int updates; > > void *data; > > int ret; > > > > - if (argc != 3) > > + if (argc < 3 || argc > 4) > > return CMD_RET_USAGE; > > + if (argc == 4) { > > + algo = tpm2_name_to_algorithm(argv[3]); > > + if (algo < 0) > > + return CMD_RET_FAILURE; > > + } > > + algo_len = tpm2_algorithm_to_len(algo); > > > > ret = get_tpm(&dev); > > if (ret) > > @@ -151,11 +171,12 @@ static int do_tpm_pcr_read(struct cmd_tbl *cmdtp, int flag, int argc, > > > > data = map_sysmem(simple_strtoul(argv[2], NULL, 0), 0); > > > > - rc = tpm2_pcr_read(dev, index, priv->pcr_select_min, TPM2_ALG_SHA256, > > - data, TPM2_DIGEST_LEN, &updates); > > + rc = tpm2_pcr_read(dev, index, priv->pcr_select_min, algo, > > + data, algo_len, &updates); > > if (!rc) { > > - printf("PCR #%u content (%u known updates):\n", index, updates); > > - print_byte_string(data, TPM2_DIGEST_LEN); > > + printf("PCR #%u %s %d byte content (%u known updates):\n", index, > > + tpm2_algorithm_name(algo), algo_len, updates); > > + print_byte_string(data, algo_len); > > } > > > > unmap_sysmem(data); > > @@ -415,14 +436,14 @@ U_BOOT_CMD(tpm2, CONFIG_SYS_MAXARGS, 1, do_tpm, "Issue a TPMv2.x command", > > " <hierarchy> is one of:\n" > > " * TPM2_RH_LOCKOUT\n" > > " * TPM2_RH_PLATFORM\n" > > -"pcr_extend <pcr> <digest_addr>\n" > > -" Extend PCR #<pcr> with digest at <digest_addr>.\n" > > +"pcr_extend <pcr> <digest_addr> [<digest_algo>]\n" > > +" Extend PCR #<pcr> with digest at <digest_addr> with digest_algo.\n" > > " <pcr>: index of the PCR\n" > > -" <digest_addr>: address of a 32-byte SHA256 digest\n" > > -"pcr_read <pcr> <digest_addr>\n" > > -" Read PCR #<pcr> to memory address <digest_addr>.\n" > > +" <digest_addr>: address of digest of digest_algo type (defaults to SHA256)\n" > > +"pcr_read <pcr> <digest_addr> [<digest_algo>]\n" > > +" Read PCR #<pcr> to memory address <digest_addr> with <digest_algo>.\n" > > " <pcr>: index of the PCR\n" > > -" <digest_addr>: address to store the a 32-byte SHA256 digest\n" > > +" <digest_addr>: address of digest of digest_algo type (defaults to SHA256)\n" > > "get_capability <capability> <property> <addr> <count>\n" > > " Read and display <count> entries indexed by <capability>/<property>.\n" > > " Values are 4 bytes long and are written at <addr>.\n" > > diff --git a/include/tpm-v2.h b/include/tpm-v2.h > > index 33dd103767c4..263f9529e55d 100644 > > --- a/include/tpm-v2.h > > +++ b/include/tpm-v2.h > > @@ -386,7 +386,54 @@ enum tpm2_algorithms { > > TPM2_ALG_SM3_256 = 0x12, > > }; > > > > -extern const enum tpm2_algorithms tpm2_supported_algorithms[4]; > > +/** > > + * struct digest_info - details of supported digests > > + * > > + * @hash_name: hash name > > + * @hash_alg: hash algorithm id > > + * @hash_mask: hash registry mask > > + * @hash_len: hash digest length > > + */ > > +struct digest_info { > > + const char *hash_name; > > + u16 hash_alg; > > + u32 hash_mask; > > + u16 hash_len; > > +}; > > + > > +/* Algorithm Registry */ > > +#define TCG2_BOOT_HASH_ALG_SHA1 0x00000001 > > +#define TCG2_BOOT_HASH_ALG_SHA256 0x00000002 > > +#define TCG2_BOOT_HASH_ALG_SHA384 0x00000004 > > +#define TCG2_BOOT_HASH_ALG_SHA512 0x00000008 > > +#define TCG2_BOOT_HASH_ALG_SM3_256 0x00000010 > > + > > +static const struct digest_info hash_algo_list[] = { > > + { > > + "sha1", > > + TPM2_ALG_SHA1, > > + TCG2_BOOT_HASH_ALG_SHA1, > > + TPM2_SHA1_DIGEST_SIZE, > > + }, > > + { > > + "sha256", > > + TPM2_ALG_SHA256, > > + TCG2_BOOT_HASH_ALG_SHA256, > > + TPM2_SHA256_DIGEST_SIZE, > > + }, > > + { > > + "sha384", > > + TPM2_ALG_SHA384, > > + TCG2_BOOT_HASH_ALG_SHA384, > > + TPM2_SHA384_DIGEST_SIZE, > > + }, > > + { > > + "sha512", > > + TPM2_ALG_SHA512, > > + TCG2_BOOT_HASH_ALG_SHA512, > > + TPM2_SHA512_DIGEST_SIZE, > > + }, > > +}; > > > > static inline u16 tpm2_algorithm_to_len(enum tpm2_algorithms a) > > { > > @@ -965,4 +1012,22 @@ u32 tpm2_enable_nvcommits(struct udevice *dev, uint vendor_cmd, > > */ > > u32 tpm2_auto_start(struct udevice *dev); > > > > +/** > > + * tpm2_name_to_algorithm() - Return an algorithm id given a supported > > + * algorithm name > > + * > > + * @name: algorithm name > > + * Return: enum tpm2_algorithms or -EINVAL > > + */ > > +enum tpm2_algorithms tpm2_name_to_algorithm(const char *name); > > + > > +/** > > + * tpm2_algorithm_name() - Return an algorithm name string for a > > + * supported algorithm id > > + * > > + * @algorithm_id: algorithm defined in enum tpm2_algorithms > > + * Return: algorithm name string or "" > > + */ > > +const char *tpm2_algorithm_name(enum tpm2_algorithms); > > + > > #endif /* __TPM_V2_H */ > > diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c > > index b07e0099c27e..cf5d8de018a7 100644 > > --- a/lib/efi_loader/efi_tcg2.c > > +++ b/lib/efi_loader/efi_tcg2.c > > @@ -414,8 +414,8 @@ static efi_status_t tcg2_hash_pe_image(void *efi, u64 efi_size, > > } > > > > digest_list->count = 0; > > - for (i = 0; i < ARRAY_SIZE(tpm2_supported_algorithms); i++) { > > - u16 hash_alg = tpm2_supported_algorithms[i]; > > + for (i = 0; i < ARRAY_SIZE(hash_algo_list); i++) { > > + u16 hash_alg = hash_algo_list[i].hash_alg; > > > > if (!(active & tpm2_algorithm_to_mask(hash_alg))) > > continue; > > diff --git a/lib/tpm-v2.c b/lib/tpm-v2.c > > index 68eaaa639f89..83f490c82541 100644 > > --- a/lib/tpm-v2.c > > +++ b/lib/tpm-v2.c > > @@ -22,13 +22,6 @@ > > > > #include "tpm-utils.h" > > > > -const enum tpm2_algorithms tpm2_supported_algorithms[4] = { > > - TPM2_ALG_SHA1, > > - TPM2_ALG_SHA256, > > - TPM2_ALG_SHA384, > > - TPM2_ALG_SHA512, > > -}; > > - > > int tcg2_get_active_pcr_banks(struct udevice *dev, u32 *active_pcr_banks) > > { > > u32 supported = 0; > > @@ -82,14 +75,14 @@ int tcg2_create_digest(struct udevice *dev, const u8 *input, u32 length, > > return rc; > > > > digest_list->count = 0; > > - for (i = 0; i < ARRAY_SIZE(tpm2_supported_algorithms); ++i) { > > + for (i = 0; i < ARRAY_SIZE(hash_algo_list); ++i) { > > u32 mask = > > - tpm2_algorithm_to_mask(tpm2_supported_algorithms[i]); > > + tpm2_algorithm_to_mask(hash_algo_list[i].hash_alg); > > > > if (!(active & mask)) > > continue; > > > > - switch (tpm2_supported_algorithms[i]) { > > + switch (hash_algo_list[i].hash_alg) { > > case TPM2_ALG_SHA1: > > sha1_starts(&ctx); > > sha1_update(&ctx, input, length); > > @@ -116,12 +109,12 @@ int tcg2_create_digest(struct udevice *dev, const u8 *input, u32 length, > > break; > > default: > > printf("%s: unsupported algorithm %x\n", __func__, > > - tpm2_supported_algorithms[i]); > > + hash_algo_list[i].hash_alg); > > continue; > > } > > > > digest_list->digests[digest_list->count].hash_alg = > > - tpm2_supported_algorithms[i]; > > + hash_algo_list[i].hash_alg; > > memcpy(&digest_list->digests[digest_list->count].digest, final, > > len); > > digest_list->count++; > > @@ -208,13 +201,13 @@ static int tcg2_log_init(struct udevice *dev, struct tcg2_event_log *elog) > > return rc; > > > > event_size = offsetof(struct tcg_efi_spec_id_event, digest_sizes); > > - for (i = 0; i < ARRAY_SIZE(tpm2_supported_algorithms); ++i) { > > - mask = tpm2_algorithm_to_mask(tpm2_supported_algorithms[i]); > > + for (i = 0; i < ARRAY_SIZE(hash_algo_list); ++i) { > > + mask = tpm2_algorithm_to_mask(hash_algo_list[i].hash_alg); > > > > if (!(active & mask)) > > continue; > > > > - switch (tpm2_supported_algorithms[i]) { > > + switch (hash_algo_list[i].hash_alg) { > > case TPM2_ALG_SHA1: > > case TPM2_ALG_SHA256: > > case TPM2_ALG_SHA384: > > @@ -253,17 +246,17 @@ static int tcg2_log_init(struct udevice *dev, struct tcg2_event_log *elog) > > put_unaligned_le32(count, &ev->number_of_algorithms); > > > > count = 0; > > - for (i = 0; i < ARRAY_SIZE(tpm2_supported_algorithms); ++i) { > > - mask = tpm2_algorithm_to_mask(tpm2_supported_algorithms[i]); > > + for (i = 0; i < ARRAY_SIZE(hash_algo_list); ++i) { > > + mask = tpm2_algorithm_to_mask(hash_algo_list[i].hash_alg); > > > > if (!(active & mask)) > > continue; > > > > - len = tpm2_algorithm_to_len(tpm2_supported_algorithms[i]); > > + len = hash_algo_list[i].hash_len; > > if (!len) > > continue; > > > > - put_unaligned_le16(tpm2_supported_algorithms[i], > > + put_unaligned_le16(hash_algo_list[i].hash_alg, > > &ev->digest_sizes[count].algorithm_id); > > put_unaligned_le16(len, &ev->digest_sizes[count].digest_size); > > count++; > > @@ -304,7 +297,7 @@ static int tcg2_replay_eventlog(struct tcg2_event_log *elog, > > pos = offsetof(struct tcg_pcr_event2, digests) + > > offsetof(struct tpml_digest_values, count); > > count = get_unaligned_le32(log + pos); > > - if (count > ARRAY_SIZE(tpm2_supported_algorithms) || > > + if (count > ARRAY_SIZE(hash_algo_list) || > > (digest_list->count && digest_list->count != count)) > > return 0; > > > > @@ -407,7 +400,7 @@ static int tcg2_log_parse(struct udevice *dev, struct tcg2_event_log *elog) > > return 0; > > > > count = get_unaligned_le32(&event->number_of_algorithms); > > - if (count > ARRAY_SIZE(tpm2_supported_algorithms)) > > + if (count > ARRAY_SIZE(hash_algo_list)) > > return 0; > > > > calc_size = offsetof(struct tcg_efi_spec_id_event, digest_sizes) + > > @@ -1110,7 +1103,7 @@ int tpm2_get_pcr_info(struct udevice *dev, u32 *supported_pcr, u32 *active_pcr, > > * We only support 5 algorithms for now so check against that > > * instead of TPM2_NUM_PCR_BANKS > > */ > > - if (pcrs.count > ARRAY_SIZE(tpm2_supported_algorithms) || > > + if (pcrs.count > ARRAY_SIZE(hash_algo_list) || > > pcrs.count < 1) { > > printf("%s: too many pcrs: %u\n", __func__, pcrs.count); > > return -EMSGSIZE; > > @@ -1555,3 +1548,28 @@ u32 tpm2_enable_nvcommits(struct udevice *dev, uint vendor_cmd, > > > > return 0; > > } > > + > > +enum tpm2_algorithms tpm2_name_to_algorithm(const char *name) > > +{ > > + size_t i; > > + > > + for (i = 0; i < ARRAY_SIZE(hash_algo_list); ++i) { > > + if (!strcasecmp(name, hash_algo_list[i].hash_name)) > > + return hash_algo_list[i].hash_alg; > > + } > > + printf("%s: unsupported algorithm %s\n", __func__, name); > > + > > + return -EINVAL; > > +} > > + > > +const char *tpm2_algorithm_name(enum tpm2_algorithms algo) > > +{ > > + size_t i; > > + > > + for (i = 0; i < ARRAY_SIZE(hash_algo_list); ++i) { > > + if (hash_algo_list[i].hash_alg == algo) > > + return hash_algo_list[i].hash_name; > > + } > > + > > + return ""; > > +} > > -- > > 2.25.1 > >
Hi Tim, On Fri, 19 Apr 2024 at 20:13, Tim Harvey <tharvey@gateworks.com> wrote: > > On Sat, Apr 6, 2024 at 9:33 AM Ilias Apalodimas > <ilias.apalodimas@linaro.org> wrote: > > > > Hi Tim, > > > > Thanks for the patch > > > > I'll be away next week, I'll try to find time and take a closer look. > > The pipeline [0] shows some TPM related failures > > > > [0] https://source.denx.de/u-boot/custodians/u-boot-tpm/-/commit/9b4be64e41454e17269a968397933eeff300c380 > > > > Hi Ilias, > > I changed the output of 'tpm pcr_read' so that it shows the algo and > size causing the test in test/py/tests/test_tpm2.py to fail: > @@ -151,11 +171,12 @@ static int do_tpm_pcr_read(struct cmd_tbl > *cmdtp, int flag, int argc, > > data = map_sysmem(simple_strtoul(argv[2], NULL, 0), 0); > > - rc = tpm2_pcr_read(dev, index, priv->pcr_select_min, TPM2_ALG_SHA256, > - data, TPM2_DIGEST_LEN, &updates); > + rc = tpm2_pcr_read(dev, index, priv->pcr_select_min, algo, > + data, algo_len, &updates); > if (!rc) { > - printf("PCR #%u content (%u known updates):\n", index, updates); > - print_byte_string(data, TPM2_DIGEST_LEN); > + printf("PCR #%u %s %d byte content (%u known > updates):\n", index, > + tpm2_algorithm_name(algo), algo_len, updates); > + print_byte_string(data, algo_len); > } > > failure: > E AssertionError: assert 'PCR #10 content' in 'PCR #10 sha256 32 > byte content (723 known updates):\r\r\n 00 00 00 00 00 00 00 00 00 00 > 00 00 00 00 00 00\r\r\n 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 00' > > So I suppose I need to update test/py/tests/test_tpm2.py as well. Yes > > Would I update test/py/tests/test_tpm2.py in the same patch as the one > that causes the failure? Yes please, I'd like patches merged that won't break the CI > > How do I go about running the tests manually to make sure I've addressed it? You can send a PR against U-Boots repo (in github) Cheers /Ilias > > Best Regards, > > Tim > > > Cheers > > /Ilias > > > > On Fri, 5 Apr 2024 at 03:17, Tim Harvey <tharvey@gateworks.com> wrote: > > > > > > For pcr_read and pcr_extend commands allow the digest algorithm to be > > > specified by an additional argument. If not specified it will default to > > > SHA256 for backwards compatibility. > > > > > > A follow-on to this could be to extend all PCR banks with the detected > > > algo when the <digest_algo> argument is 'auto'. > > > > > > Signed-off-by: Tim Harvey <tharvey@gateworks.com> > > > --- > > > v3: > > > - replace tpm2_supported_algorithms with struct and use this to relate hash algoirthm > > > details > > > v2: > > > - use tpm2_algorithm_to_len > > > - use enum tpm2_algorithms > > > - make function names and parameter names more consistent with existing > > > tpm-v2 functions > > > - fix various spelling errors > > > --- > > > cmd/tpm-v2.c | 49 ++++++++++++++++++++-------- > > > include/tpm-v2.h | 67 ++++++++++++++++++++++++++++++++++++++- > > > lib/efi_loader/efi_tcg2.c | 4 +-- > > > lib/tpm-v2.c | 62 +++++++++++++++++++++++------------- > > > 4 files changed, 143 insertions(+), 39 deletions(-) > > > > > > diff --git a/cmd/tpm-v2.c b/cmd/tpm-v2.c > > > index 7e479b9dfe36..2343b4d9cb9e 100644 > > > --- a/cmd/tpm-v2.c > > > +++ b/cmd/tpm-v2.c > > > @@ -99,11 +99,19 @@ static int do_tpm2_pcr_extend(struct cmd_tbl *cmdtp, int flag, int argc, > > > struct tpm_chip_priv *priv; > > > u32 index = simple_strtoul(argv[1], NULL, 0); > > > void *digest = map_sysmem(simple_strtoul(argv[2], NULL, 0), 0); > > > + int algo = TPM2_ALG_SHA256; > > > + int algo_len; > > > int ret; > > > u32 rc; > > > > > > - if (argc != 3) > > > + if (argc < 3 || argc > 4) > > > return CMD_RET_USAGE; > > > + if (argc == 4) { > > > + algo = tpm2_name_to_algorithm(argv[3]); > > > + if (algo < 0) > > > + return CMD_RET_FAILURE; > > > + } > > > + algo_len = tpm2_algorithm_to_len(algo); > > > > > > ret = get_tpm(&dev); > > > if (ret) > > > @@ -116,8 +124,12 @@ static int do_tpm2_pcr_extend(struct cmd_tbl *cmdtp, int flag, int argc, > > > if (index >= priv->pcr_count) > > > return -EINVAL; > > > > > > - rc = tpm2_pcr_extend(dev, index, TPM2_ALG_SHA256, digest, > > > - TPM2_DIGEST_LEN); > > > + rc = tpm2_pcr_extend(dev, index, algo, digest, algo_len); > > > + if (!rc) { > > > + printf("PCR #%u extended with %d byte %s digest\n", index, > > > + algo_len, tpm2_algorithm_name(algo)); > > > + print_byte_string(digest, algo_len); > > > + } > > > > > > unmap_sysmem(digest); > > > > > > @@ -127,15 +139,23 @@ static int do_tpm2_pcr_extend(struct cmd_tbl *cmdtp, int flag, int argc, > > > static int do_tpm_pcr_read(struct cmd_tbl *cmdtp, int flag, int argc, > > > char *const argv[]) > > > { > > > + enum tpm2_algorithms algo = TPM2_ALG_SHA256; > > > struct udevice *dev; > > > struct tpm_chip_priv *priv; > > > u32 index, rc; > > > + int algo_len; > > > unsigned int updates; > > > void *data; > > > int ret; > > > > > > - if (argc != 3) > > > + if (argc < 3 || argc > 4) > > > return CMD_RET_USAGE; > > > + if (argc == 4) { > > > + algo = tpm2_name_to_algorithm(argv[3]); > > > + if (algo < 0) > > > + return CMD_RET_FAILURE; > > > + } > > > + algo_len = tpm2_algorithm_to_len(algo); > > > > > > ret = get_tpm(&dev); > > > if (ret) > > > @@ -151,11 +171,12 @@ static int do_tpm_pcr_read(struct cmd_tbl *cmdtp, int flag, int argc, > > > > > > data = map_sysmem(simple_strtoul(argv[2], NULL, 0), 0); > > > > > > - rc = tpm2_pcr_read(dev, index, priv->pcr_select_min, TPM2_ALG_SHA256, > > > - data, TPM2_DIGEST_LEN, &updates); > > > + rc = tpm2_pcr_read(dev, index, priv->pcr_select_min, algo, > > > + data, algo_len, &updates); > > > if (!rc) { > > > - printf("PCR #%u content (%u known updates):\n", index, updates); > > > - print_byte_string(data, TPM2_DIGEST_LEN); > > > + printf("PCR #%u %s %d byte content (%u known updates):\n", index, > > > + tpm2_algorithm_name(algo), algo_len, updates); > > > + print_byte_string(data, algo_len); > > > } > > > > > > unmap_sysmem(data); > > > @@ -415,14 +436,14 @@ U_BOOT_CMD(tpm2, CONFIG_SYS_MAXARGS, 1, do_tpm, "Issue a TPMv2.x command", > > > " <hierarchy> is one of:\n" > > > " * TPM2_RH_LOCKOUT\n" > > > " * TPM2_RH_PLATFORM\n" > > > -"pcr_extend <pcr> <digest_addr>\n" > > > -" Extend PCR #<pcr> with digest at <digest_addr>.\n" > > > +"pcr_extend <pcr> <digest_addr> [<digest_algo>]\n" > > > +" Extend PCR #<pcr> with digest at <digest_addr> with digest_algo.\n" > > > " <pcr>: index of the PCR\n" > > > -" <digest_addr>: address of a 32-byte SHA256 digest\n" > > > -"pcr_read <pcr> <digest_addr>\n" > > > -" Read PCR #<pcr> to memory address <digest_addr>.\n" > > > +" <digest_addr>: address of digest of digest_algo type (defaults to SHA256)\n" > > > +"pcr_read <pcr> <digest_addr> [<digest_algo>]\n" > > > +" Read PCR #<pcr> to memory address <digest_addr> with <digest_algo>.\n" > > > " <pcr>: index of the PCR\n" > > > -" <digest_addr>: address to store the a 32-byte SHA256 digest\n" > > > +" <digest_addr>: address of digest of digest_algo type (defaults to SHA256)\n" > > > "get_capability <capability> <property> <addr> <count>\n" > > > " Read and display <count> entries indexed by <capability>/<property>.\n" > > > " Values are 4 bytes long and are written at <addr>.\n" > > > diff --git a/include/tpm-v2.h b/include/tpm-v2.h > > > index 33dd103767c4..263f9529e55d 100644 > > > --- a/include/tpm-v2.h > > > +++ b/include/tpm-v2.h > > > @@ -386,7 +386,54 @@ enum tpm2_algorithms { > > > TPM2_ALG_SM3_256 = 0x12, > > > }; > > > > > > -extern const enum tpm2_algorithms tpm2_supported_algorithms[4]; > > > +/** > > > + * struct digest_info - details of supported digests > > > + * > > > + * @hash_name: hash name > > > + * @hash_alg: hash algorithm id > > > + * @hash_mask: hash registry mask > > > + * @hash_len: hash digest length > > > + */ > > > +struct digest_info { > > > + const char *hash_name; > > > + u16 hash_alg; > > > + u32 hash_mask; > > > + u16 hash_len; > > > +}; > > > + > > > +/* Algorithm Registry */ > > > +#define TCG2_BOOT_HASH_ALG_SHA1 0x00000001 > > > +#define TCG2_BOOT_HASH_ALG_SHA256 0x00000002 > > > +#define TCG2_BOOT_HASH_ALG_SHA384 0x00000004 > > > +#define TCG2_BOOT_HASH_ALG_SHA512 0x00000008 > > > +#define TCG2_BOOT_HASH_ALG_SM3_256 0x00000010 > > > + > > > +static const struct digest_info hash_algo_list[] = { > > > + { > > > + "sha1", > > > + TPM2_ALG_SHA1, > > > + TCG2_BOOT_HASH_ALG_SHA1, > > > + TPM2_SHA1_DIGEST_SIZE, > > > + }, > > > + { > > > + "sha256", > > > + TPM2_ALG_SHA256, > > > + TCG2_BOOT_HASH_ALG_SHA256, > > > + TPM2_SHA256_DIGEST_SIZE, > > > + }, > > > + { > > > + "sha384", > > > + TPM2_ALG_SHA384, > > > + TCG2_BOOT_HASH_ALG_SHA384, > > > + TPM2_SHA384_DIGEST_SIZE, > > > + }, > > > + { > > > + "sha512", > > > + TPM2_ALG_SHA512, > > > + TCG2_BOOT_HASH_ALG_SHA512, > > > + TPM2_SHA512_DIGEST_SIZE, > > > + }, > > > +}; > > > > > > static inline u16 tpm2_algorithm_to_len(enum tpm2_algorithms a) > > > { > > > @@ -965,4 +1012,22 @@ u32 tpm2_enable_nvcommits(struct udevice *dev, uint vendor_cmd, > > > */ > > > u32 tpm2_auto_start(struct udevice *dev); > > > > > > +/** > > > + * tpm2_name_to_algorithm() - Return an algorithm id given a supported > > > + * algorithm name > > > + * > > > + * @name: algorithm name > > > + * Return: enum tpm2_algorithms or -EINVAL > > > + */ > > > +enum tpm2_algorithms tpm2_name_to_algorithm(const char *name); > > > + > > > +/** > > > + * tpm2_algorithm_name() - Return an algorithm name string for a > > > + * supported algorithm id > > > + * > > > + * @algorithm_id: algorithm defined in enum tpm2_algorithms > > > + * Return: algorithm name string or "" > > > + */ > > > +const char *tpm2_algorithm_name(enum tpm2_algorithms); > > > + > > > #endif /* __TPM_V2_H */ > > > diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c > > > index b07e0099c27e..cf5d8de018a7 100644 > > > --- a/lib/efi_loader/efi_tcg2.c > > > +++ b/lib/efi_loader/efi_tcg2.c > > > @@ -414,8 +414,8 @@ static efi_status_t tcg2_hash_pe_image(void *efi, u64 efi_size, > > > } > > > > > > digest_list->count = 0; > > > - for (i = 0; i < ARRAY_SIZE(tpm2_supported_algorithms); i++) { > > > - u16 hash_alg = tpm2_supported_algorithms[i]; > > > + for (i = 0; i < ARRAY_SIZE(hash_algo_list); i++) { > > > + u16 hash_alg = hash_algo_list[i].hash_alg; > > > > > > if (!(active & tpm2_algorithm_to_mask(hash_alg))) > > > continue; > > > diff --git a/lib/tpm-v2.c b/lib/tpm-v2.c > > > index 68eaaa639f89..83f490c82541 100644 > > > --- a/lib/tpm-v2.c > > > +++ b/lib/tpm-v2.c > > > @@ -22,13 +22,6 @@ > > > > > > #include "tpm-utils.h" > > > > > > -const enum tpm2_algorithms tpm2_supported_algorithms[4] = { > > > - TPM2_ALG_SHA1, > > > - TPM2_ALG_SHA256, > > > - TPM2_ALG_SHA384, > > > - TPM2_ALG_SHA512, > > > -}; > > > - > > > int tcg2_get_active_pcr_banks(struct udevice *dev, u32 *active_pcr_banks) > > > { > > > u32 supported = 0; > > > @@ -82,14 +75,14 @@ int tcg2_create_digest(struct udevice *dev, const u8 *input, u32 length, > > > return rc; > > > > > > digest_list->count = 0; > > > - for (i = 0; i < ARRAY_SIZE(tpm2_supported_algorithms); ++i) { > > > + for (i = 0; i < ARRAY_SIZE(hash_algo_list); ++i) { > > > u32 mask = > > > - tpm2_algorithm_to_mask(tpm2_supported_algorithms[i]); > > > + tpm2_algorithm_to_mask(hash_algo_list[i].hash_alg); > > > > > > if (!(active & mask)) > > > continue; > > > > > > - switch (tpm2_supported_algorithms[i]) { > > > + switch (hash_algo_list[i].hash_alg) { > > > case TPM2_ALG_SHA1: > > > sha1_starts(&ctx); > > > sha1_update(&ctx, input, length); > > > @@ -116,12 +109,12 @@ int tcg2_create_digest(struct udevice *dev, const u8 *input, u32 length, > > > break; > > > default: > > > printf("%s: unsupported algorithm %x\n", __func__, > > > - tpm2_supported_algorithms[i]); > > > + hash_algo_list[i].hash_alg); > > > continue; > > > } > > > > > > digest_list->digests[digest_list->count].hash_alg = > > > - tpm2_supported_algorithms[i]; > > > + hash_algo_list[i].hash_alg; > > > memcpy(&digest_list->digests[digest_list->count].digest, final, > > > len); > > > digest_list->count++; > > > @@ -208,13 +201,13 @@ static int tcg2_log_init(struct udevice *dev, struct tcg2_event_log *elog) > > > return rc; > > > > > > event_size = offsetof(struct tcg_efi_spec_id_event, digest_sizes); > > > - for (i = 0; i < ARRAY_SIZE(tpm2_supported_algorithms); ++i) { > > > - mask = tpm2_algorithm_to_mask(tpm2_supported_algorithms[i]); > > > + for (i = 0; i < ARRAY_SIZE(hash_algo_list); ++i) { > > > + mask = tpm2_algorithm_to_mask(hash_algo_list[i].hash_alg); > > > > > > if (!(active & mask)) > > > continue; > > > > > > - switch (tpm2_supported_algorithms[i]) { > > > + switch (hash_algo_list[i].hash_alg) { > > > case TPM2_ALG_SHA1: > > > case TPM2_ALG_SHA256: > > > case TPM2_ALG_SHA384: > > > @@ -253,17 +246,17 @@ static int tcg2_log_init(struct udevice *dev, struct tcg2_event_log *elog) > > > put_unaligned_le32(count, &ev->number_of_algorithms); > > > > > > count = 0; > > > - for (i = 0; i < ARRAY_SIZE(tpm2_supported_algorithms); ++i) { > > > - mask = tpm2_algorithm_to_mask(tpm2_supported_algorithms[i]); > > > + for (i = 0; i < ARRAY_SIZE(hash_algo_list); ++i) { > > > + mask = tpm2_algorithm_to_mask(hash_algo_list[i].hash_alg); > > > > > > if (!(active & mask)) > > > continue; > > > > > > - len = tpm2_algorithm_to_len(tpm2_supported_algorithms[i]); > > > + len = hash_algo_list[i].hash_len; > > > if (!len) > > > continue; > > > > > > - put_unaligned_le16(tpm2_supported_algorithms[i], > > > + put_unaligned_le16(hash_algo_list[i].hash_alg, > > > &ev->digest_sizes[count].algorithm_id); > > > put_unaligned_le16(len, &ev->digest_sizes[count].digest_size); > > > count++; > > > @@ -304,7 +297,7 @@ static int tcg2_replay_eventlog(struct tcg2_event_log *elog, > > > pos = offsetof(struct tcg_pcr_event2, digests) + > > > offsetof(struct tpml_digest_values, count); > > > count = get_unaligned_le32(log + pos); > > > - if (count > ARRAY_SIZE(tpm2_supported_algorithms) || > > > + if (count > ARRAY_SIZE(hash_algo_list) || > > > (digest_list->count && digest_list->count != count)) > > > return 0; > > > > > > @@ -407,7 +400,7 @@ static int tcg2_log_parse(struct udevice *dev, struct tcg2_event_log *elog) > > > return 0; > > > > > > count = get_unaligned_le32(&event->number_of_algorithms); > > > - if (count > ARRAY_SIZE(tpm2_supported_algorithms)) > > > + if (count > ARRAY_SIZE(hash_algo_list)) > > > return 0; > > > > > > calc_size = offsetof(struct tcg_efi_spec_id_event, digest_sizes) + > > > @@ -1110,7 +1103,7 @@ int tpm2_get_pcr_info(struct udevice *dev, u32 *supported_pcr, u32 *active_pcr, > > > * We only support 5 algorithms for now so check against that > > > * instead of TPM2_NUM_PCR_BANKS > > > */ > > > - if (pcrs.count > ARRAY_SIZE(tpm2_supported_algorithms) || > > > + if (pcrs.count > ARRAY_SIZE(hash_algo_list) || > > > pcrs.count < 1) { > > > printf("%s: too many pcrs: %u\n", __func__, pcrs.count); > > > return -EMSGSIZE; > > > @@ -1555,3 +1548,28 @@ u32 tpm2_enable_nvcommits(struct udevice *dev, uint vendor_cmd, > > > > > > return 0; > > > } > > > + > > > +enum tpm2_algorithms tpm2_name_to_algorithm(const char *name) > > > +{ > > > + size_t i; > > > + > > > + for (i = 0; i < ARRAY_SIZE(hash_algo_list); ++i) { > > > + if (!strcasecmp(name, hash_algo_list[i].hash_name)) > > > + return hash_algo_list[i].hash_alg; > > > + } > > > + printf("%s: unsupported algorithm %s\n", __func__, name); > > > + > > > + return -EINVAL; > > > +} > > > + > > > +const char *tpm2_algorithm_name(enum tpm2_algorithms algo) > > > +{ > > > + size_t i; > > > + > > > + for (i = 0; i < ARRAY_SIZE(hash_algo_list); ++i) { > > > + if (hash_algo_list[i].hash_alg == algo) > > > + return hash_algo_list[i].hash_name; > > > + } > > > + > > > + return ""; > > > +} > > > -- > > > 2.25.1 > > >
Also quickly looking at this, you need a new function for tpm2_algorithm_to_mask() (look below) On Fri, 19 Apr 2024 at 20:20, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Hi Tim, > > On Fri, 19 Apr 2024 at 20:13, Tim Harvey <tharvey@gateworks.com> wrote: > > > > On Sat, Apr 6, 2024 at 9:33 AM Ilias Apalodimas > > <ilias.apalodimas@linaro.org> wrote: > > > > > > Hi Tim, > > > > > > Thanks for the patch > > > > > > I'll be away next week, I'll try to find time and take a closer look. > > > The pipeline [0] shows some TPM related failures > > > > > > [0] https://source.denx.de/u-boot/custodians/u-boot-tpm/-/commit/9b4be64e41454e17269a968397933eeff300c380 > > > > > > > Hi Ilias, > > > > I changed the output of 'tpm pcr_read' so that it shows the algo and > > size causing the test in test/py/tests/test_tpm2.py to fail: > > @@ -151,11 +171,12 @@ static int do_tpm_pcr_read(struct cmd_tbl > > *cmdtp, int flag, int argc, > > > > data = map_sysmem(simple_strtoul(argv[2], NULL, 0), 0); > > > > - rc = tpm2_pcr_read(dev, index, priv->pcr_select_min, TPM2_ALG_SHA256, > > - data, TPM2_DIGEST_LEN, &updates); > > + rc = tpm2_pcr_read(dev, index, priv->pcr_select_min, algo, > > + data, algo_len, &updates); > > if (!rc) { > > - printf("PCR #%u content (%u known updates):\n", index, updates); > > - print_byte_string(data, TPM2_DIGEST_LEN); > > + printf("PCR #%u %s %d byte content (%u known > > updates):\n", index, > > + tpm2_algorithm_name(algo), algo_len, updates); > > + print_byte_string(data, algo_len); > > } > > > > failure: > > E AssertionError: assert 'PCR #10 content' in 'PCR #10 sha256 32 > > byte content (723 known updates):\r\r\n 00 00 00 00 00 00 00 00 00 00 > > 00 00 00 00 00 00\r\r\n 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > > 00' > > > > So I suppose I need to update test/py/tests/test_tpm2.py as well. > > Yes > > > > > Would I update test/py/tests/test_tpm2.py in the same patch as the one > > that causes the failure? > > Yes please, I'd like patches merged that won't break the CI > > > > > How do I go about running the tests manually to make sure I've addressed it? > > You can send a PR against U-Boots repo (in github) > > Cheers > /Ilias > > > > Best Regards, > > > > Tim > > > > > Cheers > > > /Ilias > > > > > > On Fri, 5 Apr 2024 at 03:17, Tim Harvey <tharvey@gateworks.com> wrote: > > > > > > > > For pcr_read and pcr_extend commands allow the digest algorithm to be > > > > specified by an additional argument. If not specified it will default to > > > > SHA256 for backwards compatibility. > > > > > > > > A follow-on to this could be to extend all PCR banks with the detected > > > > algo when the <digest_algo> argument is 'auto'. > > > > > > > > Signed-off-by: Tim Harvey <tharvey@gateworks.com> > > > > --- > > > > v3: > > > > - replace tpm2_supported_algorithms with struct and use this to relate hash algoirthm > > > > details > > > > v2: > > > > - use tpm2_algorithm_to_len > > > > - use enum tpm2_algorithms > > > > - make function names and parameter names more consistent with existing > > > > tpm-v2 functions > > > > - fix various spelling errors > > > > --- > > > > cmd/tpm-v2.c | 49 ++++++++++++++++++++-------- > > > > include/tpm-v2.h | 67 ++++++++++++++++++++++++++++++++++++++- > > > > lib/efi_loader/efi_tcg2.c | 4 +-- > > > > lib/tpm-v2.c | 62 +++++++++++++++++++++++------------- > > > > 4 files changed, 143 insertions(+), 39 deletions(-) > > > > > > > > diff --git a/cmd/tpm-v2.c b/cmd/tpm-v2.c > > > > index 7e479b9dfe36..2343b4d9cb9e 100644 > > > > --- a/cmd/tpm-v2.c > > > > +++ b/cmd/tpm-v2.c > > > > @@ -99,11 +99,19 @@ static int do_tpm2_pcr_extend(struct cmd_tbl *cmdtp, int flag, int argc, > > > > struct tpm_chip_priv *priv; > > > > u32 index = simple_strtoul(argv[1], NULL, 0); > > > > void *digest = map_sysmem(simple_strtoul(argv[2], NULL, 0), 0); > > > > + int algo = TPM2_ALG_SHA256; > > > > + int algo_len; > > > > int ret; > > > > u32 rc; > > > > > > > > - if (argc != 3) > > > > + if (argc < 3 || argc > 4) > > > > return CMD_RET_USAGE; > > > > + if (argc == 4) { > > > > + algo = tpm2_name_to_algorithm(argv[3]); > > > > + if (algo < 0) > > > > + return CMD_RET_FAILURE; > > > > + } > > > > + algo_len = tpm2_algorithm_to_len(algo); > > > > > > > > ret = get_tpm(&dev); > > > > if (ret) > > > > @@ -116,8 +124,12 @@ static int do_tpm2_pcr_extend(struct cmd_tbl *cmdtp, int flag, int argc, > > > > if (index >= priv->pcr_count) > > > > return -EINVAL; > > > > > > > > - rc = tpm2_pcr_extend(dev, index, TPM2_ALG_SHA256, digest, > > > > - TPM2_DIGEST_LEN); > > > > + rc = tpm2_pcr_extend(dev, index, algo, digest, algo_len); > > > > + if (!rc) { > > > > + printf("PCR #%u extended with %d byte %s digest\n", index, > > > > + algo_len, tpm2_algorithm_name(algo)); > > > > + print_byte_string(digest, algo_len); > > > > + } > > > > > > > > unmap_sysmem(digest); > > > > > > > > @@ -127,15 +139,23 @@ static int do_tpm2_pcr_extend(struct cmd_tbl *cmdtp, int flag, int argc, > > > > static int do_tpm_pcr_read(struct cmd_tbl *cmdtp, int flag, int argc, > > > > char *const argv[]) > > > > { > > > > + enum tpm2_algorithms algo = TPM2_ALG_SHA256; > > > > struct udevice *dev; > > > > struct tpm_chip_priv *priv; > > > > u32 index, rc; > > > > + int algo_len; > > > > unsigned int updates; > > > > void *data; > > > > int ret; > > > > > > > > - if (argc != 3) > > > > + if (argc < 3 || argc > 4) > > > > return CMD_RET_USAGE; > > > > + if (argc == 4) { > > > > + algo = tpm2_name_to_algorithm(argv[3]); > > > > + if (algo < 0) > > > > + return CMD_RET_FAILURE; > > > > + } > > > > + algo_len = tpm2_algorithm_to_len(algo); > > > > > > > > ret = get_tpm(&dev); > > > > if (ret) > > > > @@ -151,11 +171,12 @@ static int do_tpm_pcr_read(struct cmd_tbl *cmdtp, int flag, int argc, > > > > > > > > data = map_sysmem(simple_strtoul(argv[2], NULL, 0), 0); > > > > > > > > - rc = tpm2_pcr_read(dev, index, priv->pcr_select_min, TPM2_ALG_SHA256, > > > > - data, TPM2_DIGEST_LEN, &updates); > > > > + rc = tpm2_pcr_read(dev, index, priv->pcr_select_min, algo, > > > > + data, algo_len, &updates); > > > > if (!rc) { > > > > - printf("PCR #%u content (%u known updates):\n", index, updates); > > > > - print_byte_string(data, TPM2_DIGEST_LEN); > > > > + printf("PCR #%u %s %d byte content (%u known updates):\n", index, > > > > + tpm2_algorithm_name(algo), algo_len, updates); > > > > + print_byte_string(data, algo_len); > > > > } > > > > > > > > unmap_sysmem(data); > > > > @@ -415,14 +436,14 @@ U_BOOT_CMD(tpm2, CONFIG_SYS_MAXARGS, 1, do_tpm, "Issue a TPMv2.x command", > > > > " <hierarchy> is one of:\n" > > > > " * TPM2_RH_LOCKOUT\n" > > > > " * TPM2_RH_PLATFORM\n" > > > > -"pcr_extend <pcr> <digest_addr>\n" > > > > -" Extend PCR #<pcr> with digest at <digest_addr>.\n" > > > > +"pcr_extend <pcr> <digest_addr> [<digest_algo>]\n" > > > > +" Extend PCR #<pcr> with digest at <digest_addr> with digest_algo.\n" > > > > " <pcr>: index of the PCR\n" > > > > -" <digest_addr>: address of a 32-byte SHA256 digest\n" > > > > -"pcr_read <pcr> <digest_addr>\n" > > > > -" Read PCR #<pcr> to memory address <digest_addr>.\n" > > > > +" <digest_addr>: address of digest of digest_algo type (defaults to SHA256)\n" > > > > +"pcr_read <pcr> <digest_addr> [<digest_algo>]\n" > > > > +" Read PCR #<pcr> to memory address <digest_addr> with <digest_algo>.\n" > > > > " <pcr>: index of the PCR\n" > > > > -" <digest_addr>: address to store the a 32-byte SHA256 digest\n" > > > > +" <digest_addr>: address of digest of digest_algo type (defaults to SHA256)\n" > > > > "get_capability <capability> <property> <addr> <count>\n" > > > > " Read and display <count> entries indexed by <capability>/<property>.\n" > > > > " Values are 4 bytes long and are written at <addr>.\n" > > > > diff --git a/include/tpm-v2.h b/include/tpm-v2.h > > > > index 33dd103767c4..263f9529e55d 100644 > > > > --- a/include/tpm-v2.h > > > > +++ b/include/tpm-v2.h > > > > @@ -386,7 +386,54 @@ enum tpm2_algorithms { > > > > TPM2_ALG_SM3_256 = 0x12, > > > > }; > > > > > > > > -extern const enum tpm2_algorithms tpm2_supported_algorithms[4]; > > > > +/** > > > > + * struct digest_info - details of supported digests > > > > + * > > > > + * @hash_name: hash name > > > > + * @hash_alg: hash algorithm id > > > > + * @hash_mask: hash registry mask > > > > + * @hash_len: hash digest length > > > > + */ > > > > +struct digest_info { > > > > + const char *hash_name; > > > > + u16 hash_alg; > > > > + u32 hash_mask; > > > > + u16 hash_len; > > > > +}; > > > > + > > > > +/* Algorithm Registry */ > > > > +#define TCG2_BOOT_HASH_ALG_SHA1 0x00000001 > > > > +#define TCG2_BOOT_HASH_ALG_SHA256 0x00000002 > > > > +#define TCG2_BOOT_HASH_ALG_SHA384 0x00000004 > > > > +#define TCG2_BOOT_HASH_ALG_SHA512 0x00000008 > > > > +#define TCG2_BOOT_HASH_ALG_SM3_256 0x00000010 > > > > + > > > > +static const struct digest_info hash_algo_list[] = { > > > > + { > > > > + "sha1", > > > > + TPM2_ALG_SHA1, > > > > + TCG2_BOOT_HASH_ALG_SHA1, > > > > + TPM2_SHA1_DIGEST_SIZE, > > > > + }, > > > > + { > > > > + "sha256", > > > > + TPM2_ALG_SHA256, > > > > + TCG2_BOOT_HASH_ALG_SHA256, > > > > + TPM2_SHA256_DIGEST_SIZE, > > > > + }, > > > > + { > > > > + "sha384", > > > > + TPM2_ALG_SHA384, > > > > + TCG2_BOOT_HASH_ALG_SHA384, > > > > + TPM2_SHA384_DIGEST_SIZE, > > > > + }, > > > > + { > > > > + "sha512", > > > > + TPM2_ALG_SHA512, > > > > + TCG2_BOOT_HASH_ALG_SHA512, > > > > + TPM2_SHA512_DIGEST_SIZE, > > > > + }, > > > > +}; > > > > > > > > static inline u16 tpm2_algorithm_to_len(enum tpm2_algorithms a) > > > > { > > > > @@ -965,4 +1012,22 @@ u32 tpm2_enable_nvcommits(struct udevice *dev, uint vendor_cmd, > > > > */ > > > > u32 tpm2_auto_start(struct udevice *dev); > > > > > > > > +/** > > > > + * tpm2_name_to_algorithm() - Return an algorithm id given a supported > > > > + * algorithm name > > > > + * > > > > + * @name: algorithm name > > > > + * Return: enum tpm2_algorithms or -EINVAL > > > > + */ > > > > +enum tpm2_algorithms tpm2_name_to_algorithm(const char *name); > > > > + > > > > +/** > > > > + * tpm2_algorithm_name() - Return an algorithm name string for a > > > > + * supported algorithm id > > > > + * > > > > + * @algorithm_id: algorithm defined in enum tpm2_algorithms > > > > + * Return: algorithm name string or "" > > > > + */ > > > > +const char *tpm2_algorithm_name(enum tpm2_algorithms); > > > > + > > > > #endif /* __TPM_V2_H */ > > > > diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c > > > > index b07e0099c27e..cf5d8de018a7 100644 > > > > --- a/lib/efi_loader/efi_tcg2.c > > > > +++ b/lib/efi_loader/efi_tcg2.c > > > > @@ -414,8 +414,8 @@ static efi_status_t tcg2_hash_pe_image(void *efi, u64 efi_size, > > > > } > > > > > > > > digest_list->count = 0; > > > > - for (i = 0; i < ARRAY_SIZE(tpm2_supported_algorithms); i++) { > > > > - u16 hash_alg = tpm2_supported_algorithms[i]; > > > > + for (i = 0; i < ARRAY_SIZE(hash_algo_list); i++) { > > > > + u16 hash_alg = hash_algo_list[i].hash_alg; > > > > > > > > if (!(active & tpm2_algorithm_to_mask(hash_alg))) > > > > continue; > > > > diff --git a/lib/tpm-v2.c b/lib/tpm-v2.c > > > > index 68eaaa639f89..83f490c82541 100644 > > > > --- a/lib/tpm-v2.c > > > > +++ b/lib/tpm-v2.c > > > > @@ -22,13 +22,6 @@ > > > > > > > > #include "tpm-utils.h" > > > > > > > > -const enum tpm2_algorithms tpm2_supported_algorithms[4] = { > > > > - TPM2_ALG_SHA1, > > > > - TPM2_ALG_SHA256, > > > > - TPM2_ALG_SHA384, > > > > - TPM2_ALG_SHA512, > > > > -}; The current tpm2_algorithm_to_mask() operates on those values and bits shifts based on the enum. Since you remove the enum above, you must also change the function implementation and return hash_algo_list.hash_mask Cheers /Ilias > > > > - > > > > int tcg2_get_active_pcr_banks(struct udevice *dev, u32 *active_pcr_banks) > > > > { > > > > u32 supported = 0; > > > > @@ -82,14 +75,14 @@ int tcg2_create_digest(struct udevice *dev, const u8 *input, u32 length, > > > > return rc; > > > > > > > > digest_list->count = 0; > > > > - for (i = 0; i < ARRAY_SIZE(tpm2_supported_algorithms); ++i) { > > > > + for (i = 0; i < ARRAY_SIZE(hash_algo_list); ++i) { > > > > u32 mask = > > > > - tpm2_algorithm_to_mask(tpm2_supported_algorithms[i]); > > > > + tpm2_algorithm_to_mask(hash_algo_list[i].hash_alg); > > > > > > > > if (!(active & mask))
On Fri, Apr 19, 2024 at 10:37 AM Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Also quickly looking at this, you need a new function for > tpm2_algorithm_to_mask() (look below) > > On Fri, 19 Apr 2024 at 20:20, Ilias Apalodimas > <ilias.apalodimas@linaro.org> wrote: > > > > Hi Tim, > > > > On Fri, 19 Apr 2024 at 20:13, Tim Harvey <tharvey@gateworks.com> wrote: > > > > > > On Sat, Apr 6, 2024 at 9:33 AM Ilias Apalodimas > > > <ilias.apalodimas@linaro.org> wrote: > > > > > > > > Hi Tim, > > > > > > > > Thanks for the patch > > > > > > > > I'll be away next week, I'll try to find time and take a closer look. > > > > The pipeline [0] shows some TPM related failures > > > > > > > > [0] https://source.denx.de/u-boot/custodians/u-boot-tpm/-/commit/9b4be64e41454e17269a968397933eeff300c380 > > > > > > > > > > Hi Ilias, > > > > > > I changed the output of 'tpm pcr_read' so that it shows the algo and > > > size causing the test in test/py/tests/test_tpm2.py to fail: > > > @@ -151,11 +171,12 @@ static int do_tpm_pcr_read(struct cmd_tbl > > > *cmdtp, int flag, int argc, > > > > > > data = map_sysmem(simple_strtoul(argv[2], NULL, 0), 0); > > > > > > - rc = tpm2_pcr_read(dev, index, priv->pcr_select_min, TPM2_ALG_SHA256, > > > - data, TPM2_DIGEST_LEN, &updates); > > > + rc = tpm2_pcr_read(dev, index, priv->pcr_select_min, algo, > > > + data, algo_len, &updates); > > > if (!rc) { > > > - printf("PCR #%u content (%u known updates):\n", index, updates); > > > - print_byte_string(data, TPM2_DIGEST_LEN); > > > + printf("PCR #%u %s %d byte content (%u known > > > updates):\n", index, > > > + tpm2_algorithm_name(algo), algo_len, updates); > > > + print_byte_string(data, algo_len); > > > } > > > > > > failure: > > > E AssertionError: assert 'PCR #10 content' in 'PCR #10 sha256 32 > > > byte content (723 known updates):\r\r\n 00 00 00 00 00 00 00 00 00 00 > > > 00 00 00 00 00 00\r\r\n 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > > > 00' > > > > > > So I suppose I need to update test/py/tests/test_tpm2.py as well. > > > > Yes > > > > > > > > Would I update test/py/tests/test_tpm2.py in the same patch as the one > > > that causes the failure? > > > > Yes please, I'd like patches merged that won't break the CI > > > > > > > > How do I go about running the tests manually to make sure I've addressed it? > > > > You can send a PR against U-Boots repo (in github) > > > > Cheers > > /Ilias > > > > > > Best Regards, > > > > > > Tim > > > > > > > Cheers > > > > /Ilias > > > > > > > > On Fri, 5 Apr 2024 at 03:17, Tim Harvey <tharvey@gateworks.com> wrote: > > > > > > > > > > For pcr_read and pcr_extend commands allow the digest algorithm to be > > > > > specified by an additional argument. If not specified it will default to > > > > > SHA256 for backwards compatibility. > > > > > > > > > > A follow-on to this could be to extend all PCR banks with the detected > > > > > algo when the <digest_algo> argument is 'auto'. > > > > > > > > > > Signed-off-by: Tim Harvey <tharvey@gateworks.com> > > > > > --- > > > > > v3: > > > > > - replace tpm2_supported_algorithms with struct and use this to relate hash algoirthm > > > > > details > > > > > v2: > > > > > - use tpm2_algorithm_to_len > > > > > - use enum tpm2_algorithms > > > > > - make function names and parameter names more consistent with existing > > > > > tpm-v2 functions > > > > > - fix various spelling errors > > > > > --- > > > > > cmd/tpm-v2.c | 49 ++++++++++++++++++++-------- > > > > > include/tpm-v2.h | 67 ++++++++++++++++++++++++++++++++++++++- > > > > > lib/efi_loader/efi_tcg2.c | 4 +-- > > > > > lib/tpm-v2.c | 62 +++++++++++++++++++++++------------- > > > > > 4 files changed, 143 insertions(+), 39 deletions(-) > > > > > > > > > > diff --git a/cmd/tpm-v2.c b/cmd/tpm-v2.c > > > > > index 7e479b9dfe36..2343b4d9cb9e 100644 > > > > > --- a/cmd/tpm-v2.c > > > > > +++ b/cmd/tpm-v2.c > > > > > @@ -99,11 +99,19 @@ static int do_tpm2_pcr_extend(struct cmd_tbl *cmdtp, int flag, int argc, > > > > > struct tpm_chip_priv *priv; > > > > > u32 index = simple_strtoul(argv[1], NULL, 0); > > > > > void *digest = map_sysmem(simple_strtoul(argv[2], NULL, 0), 0); > > > > > + int algo = TPM2_ALG_SHA256; > > > > > + int algo_len; > > > > > int ret; > > > > > u32 rc; > > > > > > > > > > - if (argc != 3) > > > > > + if (argc < 3 || argc > 4) > > > > > return CMD_RET_USAGE; > > > > > + if (argc == 4) { > > > > > + algo = tpm2_name_to_algorithm(argv[3]); > > > > > + if (algo < 0) > > > > > + return CMD_RET_FAILURE; > > > > > + } > > > > > + algo_len = tpm2_algorithm_to_len(algo); > > > > > > > > > > ret = get_tpm(&dev); > > > > > if (ret) > > > > > @@ -116,8 +124,12 @@ static int do_tpm2_pcr_extend(struct cmd_tbl *cmdtp, int flag, int argc, > > > > > if (index >= priv->pcr_count) > > > > > return -EINVAL; > > > > > > > > > > - rc = tpm2_pcr_extend(dev, index, TPM2_ALG_SHA256, digest, > > > > > - TPM2_DIGEST_LEN); > > > > > + rc = tpm2_pcr_extend(dev, index, algo, digest, algo_len); > > > > > + if (!rc) { > > > > > + printf("PCR #%u extended with %d byte %s digest\n", index, > > > > > + algo_len, tpm2_algorithm_name(algo)); > > > > > + print_byte_string(digest, algo_len); > > > > > + } > > > > > > > > > > unmap_sysmem(digest); > > > > > > > > > > @@ -127,15 +139,23 @@ static int do_tpm2_pcr_extend(struct cmd_tbl *cmdtp, int flag, int argc, > > > > > static int do_tpm_pcr_read(struct cmd_tbl *cmdtp, int flag, int argc, > > > > > char *const argv[]) > > > > > { > > > > > + enum tpm2_algorithms algo = TPM2_ALG_SHA256; > > > > > struct udevice *dev; > > > > > struct tpm_chip_priv *priv; > > > > > u32 index, rc; > > > > > + int algo_len; > > > > > unsigned int updates; > > > > > void *data; > > > > > int ret; > > > > > > > > > > - if (argc != 3) > > > > > + if (argc < 3 || argc > 4) > > > > > return CMD_RET_USAGE; > > > > > + if (argc == 4) { > > > > > + algo = tpm2_name_to_algorithm(argv[3]); > > > > > + if (algo < 0) > > > > > + return CMD_RET_FAILURE; > > > > > + } > > > > > + algo_len = tpm2_algorithm_to_len(algo); > > > > > > > > > > ret = get_tpm(&dev); > > > > > if (ret) > > > > > @@ -151,11 +171,12 @@ static int do_tpm_pcr_read(struct cmd_tbl *cmdtp, int flag, int argc, > > > > > > > > > > data = map_sysmem(simple_strtoul(argv[2], NULL, 0), 0); > > > > > > > > > > - rc = tpm2_pcr_read(dev, index, priv->pcr_select_min, TPM2_ALG_SHA256, > > > > > - data, TPM2_DIGEST_LEN, &updates); > > > > > + rc = tpm2_pcr_read(dev, index, priv->pcr_select_min, algo, > > > > > + data, algo_len, &updates); > > > > > if (!rc) { > > > > > - printf("PCR #%u content (%u known updates):\n", index, updates); > > > > > - print_byte_string(data, TPM2_DIGEST_LEN); > > > > > + printf("PCR #%u %s %d byte content (%u known updates):\n", index, > > > > > + tpm2_algorithm_name(algo), algo_len, updates); > > > > > + print_byte_string(data, algo_len); > > > > > } > > > > > > > > > > unmap_sysmem(data); > > > > > @@ -415,14 +436,14 @@ U_BOOT_CMD(tpm2, CONFIG_SYS_MAXARGS, 1, do_tpm, "Issue a TPMv2.x command", > > > > > " <hierarchy> is one of:\n" > > > > > " * TPM2_RH_LOCKOUT\n" > > > > > " * TPM2_RH_PLATFORM\n" > > > > > -"pcr_extend <pcr> <digest_addr>\n" > > > > > -" Extend PCR #<pcr> with digest at <digest_addr>.\n" > > > > > +"pcr_extend <pcr> <digest_addr> [<digest_algo>]\n" > > > > > +" Extend PCR #<pcr> with digest at <digest_addr> with digest_algo.\n" > > > > > " <pcr>: index of the PCR\n" > > > > > -" <digest_addr>: address of a 32-byte SHA256 digest\n" > > > > > -"pcr_read <pcr> <digest_addr>\n" > > > > > -" Read PCR #<pcr> to memory address <digest_addr>.\n" > > > > > +" <digest_addr>: address of digest of digest_algo type (defaults to SHA256)\n" > > > > > +"pcr_read <pcr> <digest_addr> [<digest_algo>]\n" > > > > > +" Read PCR #<pcr> to memory address <digest_addr> with <digest_algo>.\n" > > > > > " <pcr>: index of the PCR\n" > > > > > -" <digest_addr>: address to store the a 32-byte SHA256 digest\n" > > > > > +" <digest_addr>: address of digest of digest_algo type (defaults to SHA256)\n" > > > > > "get_capability <capability> <property> <addr> <count>\n" > > > > > " Read and display <count> entries indexed by <capability>/<property>.\n" > > > > > " Values are 4 bytes long and are written at <addr>.\n" > > > > > diff --git a/include/tpm-v2.h b/include/tpm-v2.h > > > > > index 33dd103767c4..263f9529e55d 100644 > > > > > --- a/include/tpm-v2.h > > > > > +++ b/include/tpm-v2.h > > > > > @@ -386,7 +386,54 @@ enum tpm2_algorithms { > > > > > TPM2_ALG_SM3_256 = 0x12, > > > > > }; > > > > > > > > > > -extern const enum tpm2_algorithms tpm2_supported_algorithms[4]; > > > > > +/** > > > > > + * struct digest_info - details of supported digests > > > > > + * > > > > > + * @hash_name: hash name > > > > > + * @hash_alg: hash algorithm id > > > > > + * @hash_mask: hash registry mask > > > > > + * @hash_len: hash digest length > > > > > + */ > > > > > +struct digest_info { > > > > > + const char *hash_name; > > > > > + u16 hash_alg; > > > > > + u32 hash_mask; > > > > > + u16 hash_len; > > > > > +}; > > > > > + > > > > > +/* Algorithm Registry */ > > > > > +#define TCG2_BOOT_HASH_ALG_SHA1 0x00000001 > > > > > +#define TCG2_BOOT_HASH_ALG_SHA256 0x00000002 > > > > > +#define TCG2_BOOT_HASH_ALG_SHA384 0x00000004 > > > > > +#define TCG2_BOOT_HASH_ALG_SHA512 0x00000008 > > > > > +#define TCG2_BOOT_HASH_ALG_SM3_256 0x00000010 > > > > > + > > > > > +static const struct digest_info hash_algo_list[] = { > > > > > + { > > > > > + "sha1", > > > > > + TPM2_ALG_SHA1, > > > > > + TCG2_BOOT_HASH_ALG_SHA1, > > > > > + TPM2_SHA1_DIGEST_SIZE, > > > > > + }, > > > > > + { > > > > > + "sha256", > > > > > + TPM2_ALG_SHA256, > > > > > + TCG2_BOOT_HASH_ALG_SHA256, > > > > > + TPM2_SHA256_DIGEST_SIZE, > > > > > + }, > > > > > + { > > > > > + "sha384", > > > > > + TPM2_ALG_SHA384, > > > > > + TCG2_BOOT_HASH_ALG_SHA384, > > > > > + TPM2_SHA384_DIGEST_SIZE, > > > > > + }, > > > > > + { > > > > > + "sha512", > > > > > + TPM2_ALG_SHA512, > > > > > + TCG2_BOOT_HASH_ALG_SHA512, > > > > > + TPM2_SHA512_DIGEST_SIZE, > > > > > + }, > > > > > +}; > > > > > > > > > > static inline u16 tpm2_algorithm_to_len(enum tpm2_algorithms a) > > > > > { > > > > > @@ -965,4 +1012,22 @@ u32 tpm2_enable_nvcommits(struct udevice *dev, uint vendor_cmd, > > > > > */ > > > > > u32 tpm2_auto_start(struct udevice *dev); > > > > > > > > > > +/** > > > > > + * tpm2_name_to_algorithm() - Return an algorithm id given a supported > > > > > + * algorithm name > > > > > + * > > > > > + * @name: algorithm name > > > > > + * Return: enum tpm2_algorithms or -EINVAL > > > > > + */ > > > > > +enum tpm2_algorithms tpm2_name_to_algorithm(const char *name); > > > > > + > > > > > +/** > > > > > + * tpm2_algorithm_name() - Return an algorithm name string for a > > > > > + * supported algorithm id > > > > > + * > > > > > + * @algorithm_id: algorithm defined in enum tpm2_algorithms > > > > > + * Return: algorithm name string or "" > > > > > + */ > > > > > +const char *tpm2_algorithm_name(enum tpm2_algorithms); > > > > > + > > > > > #endif /* __TPM_V2_H */ > > > > > diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c > > > > > index b07e0099c27e..cf5d8de018a7 100644 > > > > > --- a/lib/efi_loader/efi_tcg2.c > > > > > +++ b/lib/efi_loader/efi_tcg2.c > > > > > @@ -414,8 +414,8 @@ static efi_status_t tcg2_hash_pe_image(void *efi, u64 efi_size, > > > > > } > > > > > > > > > > digest_list->count = 0; > > > > > - for (i = 0; i < ARRAY_SIZE(tpm2_supported_algorithms); i++) { > > > > > - u16 hash_alg = tpm2_supported_algorithms[i]; > > > > > + for (i = 0; i < ARRAY_SIZE(hash_algo_list); i++) { > > > > > + u16 hash_alg = hash_algo_list[i].hash_alg; > > > > > > > > > > if (!(active & tpm2_algorithm_to_mask(hash_alg))) > > > > > continue; > > > > > diff --git a/lib/tpm-v2.c b/lib/tpm-v2.c > > > > > index 68eaaa639f89..83f490c82541 100644 > > > > > --- a/lib/tpm-v2.c > > > > > +++ b/lib/tpm-v2.c > > > > > @@ -22,13 +22,6 @@ > > > > > > > > > > #include "tpm-utils.h" > > > > > > > > > > -const enum tpm2_algorithms tpm2_supported_algorithms[4] = { > > > > > - TPM2_ALG_SHA1, > > > > > - TPM2_ALG_SHA256, > > > > > - TPM2_ALG_SHA384, > > > > > - TPM2_ALG_SHA512, > > > > > -}; > > The current tpm2_algorithm_to_mask() operates on those values and bits > shifts based on the enum. Since you remove the enum above, you must > also change the function implementation and return > hash_algo_list.hash_mask > That's already taken care of as the calls to tpm2_algorithm_to_mask() in my patch were adjusted from: tpm2_algorithm_to_mask(tpm2_supported_algorithms[i]) to tpm2_algorithm_to_mask(hash_algo_list[i].hash_alg) The hash_alg is the value from the previous enum. Best Regards, Tim > Cheers > /Ilias > > > > > > > > - > > > > > int tcg2_get_active_pcr_banks(struct udevice *dev, u32 *active_pcr_banks) > > > > > { > > > > > u32 supported = 0; > > > > > @@ -82,14 +75,14 @@ int tcg2_create_digest(struct udevice *dev, const u8 *input, u32 length, > > > > > return rc; > > > > > > > > > > digest_list->count = 0; > > > > > - for (i = 0; i < ARRAY_SIZE(tpm2_supported_algorithms); ++i) { > > > > > + for (i = 0; i < ARRAY_SIZE(hash_algo_list); ++i) { > > > > > u32 mask = > > > > > - tpm2_algorithm_to_mask(tpm2_supported_algorithms[i]); > > > > > + tpm2_algorithm_to_mask(hash_algo_list[i].hash_alg); > > > > > > > > > > if (!(active & mask))
On Fri, 19 Apr 2024 at 20:52, Tim Harvey <tharvey@gateworks.com> wrote: > > On Fri, Apr 19, 2024 at 10:37 AM Ilias Apalodimas > <ilias.apalodimas@linaro.org> wrote: > > > > Also quickly looking at this, you need a new function for > > tpm2_algorithm_to_mask() (look below) > > > > On Fri, 19 Apr 2024 at 20:20, Ilias Apalodimas > > <ilias.apalodimas@linaro.org> wrote: > > > > > > Hi Tim, > > > > > > On Fri, 19 Apr 2024 at 20:13, Tim Harvey <tharvey@gateworks.com> wrote: > > > > > > > > On Sat, Apr 6, 2024 at 9:33 AM Ilias Apalodimas > > > > <ilias.apalodimas@linaro.org> wrote: > > > > > > > > > > Hi Tim, > > > > > > > > > > Thanks for the patch > > > > > > > > > > I'll be away next week, I'll try to find time and take a closer look. > > > > > The pipeline [0] shows some TPM related failures > > > > > > > > > > [0] https://source.denx.de/u-boot/custodians/u-boot-tpm/-/commit/9b4be64e41454e17269a968397933eeff300c380 > > > > > > > > > > > > > Hi Ilias, > > > > > > > > I changed the output of 'tpm pcr_read' so that it shows the algo and > > > > size causing the test in test/py/tests/test_tpm2.py to fail: > > > > @@ -151,11 +171,12 @@ static int do_tpm_pcr_read(struct cmd_tbl > > > > *cmdtp, int flag, int argc, > > > > > > > > data = map_sysmem(simple_strtoul(argv[2], NULL, 0), 0); > > > > > > > > - rc = tpm2_pcr_read(dev, index, priv->pcr_select_min, TPM2_ALG_SHA256, > > > > - data, TPM2_DIGEST_LEN, &updates); > > > > + rc = tpm2_pcr_read(dev, index, priv->pcr_select_min, algo, > > > > + data, algo_len, &updates); > > > > if (!rc) { > > > > - printf("PCR #%u content (%u known updates):\n", index, updates); > > > > - print_byte_string(data, TPM2_DIGEST_LEN); > > > > + printf("PCR #%u %s %d byte content (%u known > > > > updates):\n", index, > > > > + tpm2_algorithm_name(algo), algo_len, updates); > > > > + print_byte_string(data, algo_len); > > > > } > > > > > > > > failure: > > > > E AssertionError: assert 'PCR #10 content' in 'PCR #10 sha256 32 > > > > byte content (723 known updates):\r\r\n 00 00 00 00 00 00 00 00 00 00 > > > > 00 00 00 00 00 00\r\r\n 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > > > > 00' > > > > > > > > So I suppose I need to update test/py/tests/test_tpm2.py as well. > > > > > > Yes > > > > > > > > > > > Would I update test/py/tests/test_tpm2.py in the same patch as the one > > > > that causes the failure? > > > > > > Yes please, I'd like patches merged that won't break the CI > > > > > > > > > > > How do I go about running the tests manually to make sure I've addressed it? > > > > > > You can send a PR against U-Boots repo (in github) > > > > > > Cheers > > > /Ilias > > > > > > > > Best Regards, > > > > > > > > Tim > > > > > > > > > Cheers > > > > > /Ilias > > > > > > > > > > On Fri, 5 Apr 2024 at 03:17, Tim Harvey <tharvey@gateworks.com> wrote: > > > > > > > > > > > > For pcr_read and pcr_extend commands allow the digest algorithm to be > > > > > > specified by an additional argument. If not specified it will default to > > > > > > SHA256 for backwards compatibility. > > > > > > > > > > > > A follow-on to this could be to extend all PCR banks with the detected > > > > > > algo when the <digest_algo> argument is 'auto'. > > > > > > > > > > > > Signed-off-by: Tim Harvey <tharvey@gateworks.com> > > > > > > --- > > > > > > v3: > > > > > > - replace tpm2_supported_algorithms with struct and use this to relate hash algoirthm > > > > > > details > > > > > > v2: > > > > > > - use tpm2_algorithm_to_len > > > > > > - use enum tpm2_algorithms > > > > > > - make function names and parameter names more consistent with existing > > > > > > tpm-v2 functions > > > > > > - fix various spelling errors > > > > > > --- > > > > > > cmd/tpm-v2.c | 49 ++++++++++++++++++++-------- > > > > > > include/tpm-v2.h | 67 ++++++++++++++++++++++++++++++++++++++- > > > > > > lib/efi_loader/efi_tcg2.c | 4 +-- > > > > > > lib/tpm-v2.c | 62 +++++++++++++++++++++++------------- > > > > > > 4 files changed, 143 insertions(+), 39 deletions(-) > > > > > > > > > > > > diff --git a/cmd/tpm-v2.c b/cmd/tpm-v2.c > > > > > > index 7e479b9dfe36..2343b4d9cb9e 100644 > > > > > > --- a/cmd/tpm-v2.c > > > > > > +++ b/cmd/tpm-v2.c > > > > > > @@ -99,11 +99,19 @@ static int do_tpm2_pcr_extend(struct cmd_tbl *cmdtp, int flag, int argc, > > > > > > struct tpm_chip_priv *priv; > > > > > > u32 index = simple_strtoul(argv[1], NULL, 0); > > > > > > void *digest = map_sysmem(simple_strtoul(argv[2], NULL, 0), 0); > > > > > > + int algo = TPM2_ALG_SHA256; > > > > > > + int algo_len; > > > > > > int ret; > > > > > > u32 rc; > > > > > > > > > > > > - if (argc != 3) > > > > > > + if (argc < 3 || argc > 4) > > > > > > return CMD_RET_USAGE; > > > > > > + if (argc == 4) { > > > > > > + algo = tpm2_name_to_algorithm(argv[3]); > > > > > > + if (algo < 0) > > > > > > + return CMD_RET_FAILURE; > > > > > > + } > > > > > > + algo_len = tpm2_algorithm_to_len(algo); > > > > > > > > > > > > ret = get_tpm(&dev); > > > > > > if (ret) > > > > > > @@ -116,8 +124,12 @@ static int do_tpm2_pcr_extend(struct cmd_tbl *cmdtp, int flag, int argc, > > > > > > if (index >= priv->pcr_count) > > > > > > return -EINVAL; > > > > > > > > > > > > - rc = tpm2_pcr_extend(dev, index, TPM2_ALG_SHA256, digest, > > > > > > - TPM2_DIGEST_LEN); > > > > > > + rc = tpm2_pcr_extend(dev, index, algo, digest, algo_len); > > > > > > + if (!rc) { > > > > > > + printf("PCR #%u extended with %d byte %s digest\n", index, > > > > > > + algo_len, tpm2_algorithm_name(algo)); > > > > > > + print_byte_string(digest, algo_len); > > > > > > + } > > > > > > > > > > > > unmap_sysmem(digest); > > > > > > > > > > > > @@ -127,15 +139,23 @@ static int do_tpm2_pcr_extend(struct cmd_tbl *cmdtp, int flag, int argc, > > > > > > static int do_tpm_pcr_read(struct cmd_tbl *cmdtp, int flag, int argc, > > > > > > char *const argv[]) > > > > > > { > > > > > > + enum tpm2_algorithms algo = TPM2_ALG_SHA256; > > > > > > struct udevice *dev; > > > > > > struct tpm_chip_priv *priv; > > > > > > u32 index, rc; > > > > > > + int algo_len; > > > > > > unsigned int updates; > > > > > > void *data; > > > > > > int ret; > > > > > > > > > > > > - if (argc != 3) > > > > > > + if (argc < 3 || argc > 4) > > > > > > return CMD_RET_USAGE; > > > > > > + if (argc == 4) { > > > > > > + algo = tpm2_name_to_algorithm(argv[3]); > > > > > > + if (algo < 0) > > > > > > + return CMD_RET_FAILURE; > > > > > > + } > > > > > > + algo_len = tpm2_algorithm_to_len(algo); > > > > > > > > > > > > ret = get_tpm(&dev); > > > > > > if (ret) > > > > > > @@ -151,11 +171,12 @@ static int do_tpm_pcr_read(struct cmd_tbl *cmdtp, int flag, int argc, > > > > > > > > > > > > data = map_sysmem(simple_strtoul(argv[2], NULL, 0), 0); > > > > > > > > > > > > - rc = tpm2_pcr_read(dev, index, priv->pcr_select_min, TPM2_ALG_SHA256, > > > > > > - data, TPM2_DIGEST_LEN, &updates); > > > > > > + rc = tpm2_pcr_read(dev, index, priv->pcr_select_min, algo, > > > > > > + data, algo_len, &updates); > > > > > > if (!rc) { > > > > > > - printf("PCR #%u content (%u known updates):\n", index, updates); > > > > > > - print_byte_string(data, TPM2_DIGEST_LEN); > > > > > > + printf("PCR #%u %s %d byte content (%u known updates):\n", index, > > > > > > + tpm2_algorithm_name(algo), algo_len, updates); > > > > > > + print_byte_string(data, algo_len); > > > > > > } > > > > > > > > > > > > unmap_sysmem(data); > > > > > > @@ -415,14 +436,14 @@ U_BOOT_CMD(tpm2, CONFIG_SYS_MAXARGS, 1, do_tpm, "Issue a TPMv2.x command", > > > > > > " <hierarchy> is one of:\n" > > > > > > " * TPM2_RH_LOCKOUT\n" > > > > > > " * TPM2_RH_PLATFORM\n" > > > > > > -"pcr_extend <pcr> <digest_addr>\n" > > > > > > -" Extend PCR #<pcr> with digest at <digest_addr>.\n" > > > > > > +"pcr_extend <pcr> <digest_addr> [<digest_algo>]\n" > > > > > > +" Extend PCR #<pcr> with digest at <digest_addr> with digest_algo.\n" > > > > > > " <pcr>: index of the PCR\n" > > > > > > -" <digest_addr>: address of a 32-byte SHA256 digest\n" > > > > > > -"pcr_read <pcr> <digest_addr>\n" > > > > > > -" Read PCR #<pcr> to memory address <digest_addr>.\n" > > > > > > +" <digest_addr>: address of digest of digest_algo type (defaults to SHA256)\n" > > > > > > +"pcr_read <pcr> <digest_addr> [<digest_algo>]\n" > > > > > > +" Read PCR #<pcr> to memory address <digest_addr> with <digest_algo>.\n" > > > > > > " <pcr>: index of the PCR\n" > > > > > > -" <digest_addr>: address to store the a 32-byte SHA256 digest\n" > > > > > > +" <digest_addr>: address of digest of digest_algo type (defaults to SHA256)\n" > > > > > > "get_capability <capability> <property> <addr> <count>\n" > > > > > > " Read and display <count> entries indexed by <capability>/<property>.\n" > > > > > > " Values are 4 bytes long and are written at <addr>.\n" > > > > > > diff --git a/include/tpm-v2.h b/include/tpm-v2.h > > > > > > index 33dd103767c4..263f9529e55d 100644 > > > > > > --- a/include/tpm-v2.h > > > > > > +++ b/include/tpm-v2.h > > > > > > @@ -386,7 +386,54 @@ enum tpm2_algorithms { > > > > > > TPM2_ALG_SM3_256 = 0x12, > > > > > > }; > > > > > > > > > > > > -extern const enum tpm2_algorithms tpm2_supported_algorithms[4]; > > > > > > +/** > > > > > > + * struct digest_info - details of supported digests > > > > > > + * > > > > > > + * @hash_name: hash name > > > > > > + * @hash_alg: hash algorithm id > > > > > > + * @hash_mask: hash registry mask > > > > > > + * @hash_len: hash digest length > > > > > > + */ > > > > > > +struct digest_info { > > > > > > + const char *hash_name; > > > > > > + u16 hash_alg; > > > > > > + u32 hash_mask; > > > > > > + u16 hash_len; > > > > > > +}; > > > > > > + > > > > > > +/* Algorithm Registry */ > > > > > > +#define TCG2_BOOT_HASH_ALG_SHA1 0x00000001 > > > > > > +#define TCG2_BOOT_HASH_ALG_SHA256 0x00000002 > > > > > > +#define TCG2_BOOT_HASH_ALG_SHA384 0x00000004 > > > > > > +#define TCG2_BOOT_HASH_ALG_SHA512 0x00000008 > > > > > > +#define TCG2_BOOT_HASH_ALG_SM3_256 0x00000010 > > > > > > + > > > > > > +static const struct digest_info hash_algo_list[] = { > > > > > > + { > > > > > > + "sha1", > > > > > > + TPM2_ALG_SHA1, > > > > > > + TCG2_BOOT_HASH_ALG_SHA1, > > > > > > + TPM2_SHA1_DIGEST_SIZE, > > > > > > + }, > > > > > > + { > > > > > > + "sha256", > > > > > > + TPM2_ALG_SHA256, > > > > > > + TCG2_BOOT_HASH_ALG_SHA256, > > > > > > + TPM2_SHA256_DIGEST_SIZE, > > > > > > + }, > > > > > > + { > > > > > > + "sha384", > > > > > > + TPM2_ALG_SHA384, > > > > > > + TCG2_BOOT_HASH_ALG_SHA384, > > > > > > + TPM2_SHA384_DIGEST_SIZE, > > > > > > + }, > > > > > > + { > > > > > > + "sha512", > > > > > > + TPM2_ALG_SHA512, > > > > > > + TCG2_BOOT_HASH_ALG_SHA512, > > > > > > + TPM2_SHA512_DIGEST_SIZE, > > > > > > + }, > > > > > > +}; > > > > > > > > > > > > static inline u16 tpm2_algorithm_to_len(enum tpm2_algorithms a) > > > > > > { > > > > > > @@ -965,4 +1012,22 @@ u32 tpm2_enable_nvcommits(struct udevice *dev, uint vendor_cmd, > > > > > > */ > > > > > > u32 tpm2_auto_start(struct udevice *dev); > > > > > > > > > > > > +/** > > > > > > + * tpm2_name_to_algorithm() - Return an algorithm id given a supported > > > > > > + * algorithm name > > > > > > + * > > > > > > + * @name: algorithm name > > > > > > + * Return: enum tpm2_algorithms or -EINVAL > > > > > > + */ > > > > > > +enum tpm2_algorithms tpm2_name_to_algorithm(const char *name); > > > > > > + > > > > > > +/** > > > > > > + * tpm2_algorithm_name() - Return an algorithm name string for a > > > > > > + * supported algorithm id > > > > > > + * > > > > > > + * @algorithm_id: algorithm defined in enum tpm2_algorithms > > > > > > + * Return: algorithm name string or "" > > > > > > + */ > > > > > > +const char *tpm2_algorithm_name(enum tpm2_algorithms); > > > > > > + > > > > > > #endif /* __TPM_V2_H */ > > > > > > diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c > > > > > > index b07e0099c27e..cf5d8de018a7 100644 > > > > > > --- a/lib/efi_loader/efi_tcg2.c > > > > > > +++ b/lib/efi_loader/efi_tcg2.c > > > > > > @@ -414,8 +414,8 @@ static efi_status_t tcg2_hash_pe_image(void *efi, u64 efi_size, > > > > > > } > > > > > > > > > > > > digest_list->count = 0; > > > > > > - for (i = 0; i < ARRAY_SIZE(tpm2_supported_algorithms); i++) { > > > > > > - u16 hash_alg = tpm2_supported_algorithms[i]; > > > > > > + for (i = 0; i < ARRAY_SIZE(hash_algo_list); i++) { > > > > > > + u16 hash_alg = hash_algo_list[i].hash_alg; > > > > > > > > > > > > if (!(active & tpm2_algorithm_to_mask(hash_alg))) > > > > > > continue; > > > > > > diff --git a/lib/tpm-v2.c b/lib/tpm-v2.c > > > > > > index 68eaaa639f89..83f490c82541 100644 > > > > > > --- a/lib/tpm-v2.c > > > > > > +++ b/lib/tpm-v2.c > > > > > > @@ -22,13 +22,6 @@ > > > > > > > > > > > > #include "tpm-utils.h" > > > > > > > > > > > > -const enum tpm2_algorithms tpm2_supported_algorithms[4] = { > > > > > > - TPM2_ALG_SHA1, > > > > > > - , > > > > > > - TPM2_ALG_SHA384, > > > > > > - TPM2_ALG_SHA512, > > > > > > -}; > > > > The current tpm2_algorithm_to_mask() operates on those values and bits > > shifts based on the enum. Since you remove the enum above, you must > > also change the function implementation and return > > hash_algo_list.hash_mask > > > > That's already taken care of as the calls to tpm2_algorithm_to_mask() > in my patch were adjusted from: > tpm2_algorithm_to_mask(tpm2_supported_algorithms[i]) > to > tpm2_algorithm_to_mask(hash_algo_list[i].hash_alg) > > The hash_alg is the value from the previous enum. The previous enums were defining TPM2_ALG_SHA1=1, TPM2_ALG_SHA256=2 etc The current values are different. On top of that the .hash_mask entry of digest_info is never used. So you need to change tpm2_algorithm_to_mask() and directly return the hash_mask Cheers /Ilias > > Best Regards, > > Tim > > > Cheers > > /Ilias > > > > > > > > > > > > - > > > > > > int tcg2_get_active_pcr_banks(struct udevice *dev, u32 *active_pcr_banks) > > > > > > { > > > > > > u32 supported = 0; > > > > > > @@ -82,14 +75,14 @@ int tcg2_create_digest(struct udevice *dev, const u8 *input, u32 length, > > > > > > return rc; > > > > > > > > > > > > digest_list->count = 0; > > > > > > - for (i = 0; i < ARRAY_SIZE(tpm2_supported_algorithms); ++i) { > > > > > > + for (i = 0; i < ARRAY_SIZE(hash_algo_list); ++i) { > > > > > > u32 mask = > > > > > > - tpm2_algorithm_to_mask(tpm2_supported_algorithms[i]); > > > > > > + tpm2_algorithm_to_mask(hash_algo_list[i].hash_alg); > > > > > > > > > > > > if (!(active & mask))
On Fri, Apr 19, 2024 at 1:04 PM Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > On Fri, 19 Apr 2024 at 20:52, Tim Harvey <tharvey@gateworks.com> wrote: > > > > On Fri, Apr 19, 2024 at 10:37 AM Ilias Apalodimas > > <ilias.apalodimas@linaro.org> wrote: > > > > > > Also quickly looking at this, you need a new function for > > > tpm2_algorithm_to_mask() (look below) > > > > > > On Fri, 19 Apr 2024 at 20:20, Ilias Apalodimas > > > <ilias.apalodimas@linaro.org> wrote: > > > > > > > > Hi Tim, > > > > > > > > On Fri, 19 Apr 2024 at 20:13, Tim Harvey <tharvey@gateworks.com> wrote: > > > > > > > > > > On Sat, Apr 6, 2024 at 9:33 AM Ilias Apalodimas > > > > > <ilias.apalodimas@linaro.org> wrote: > > > > > > > > > > > > Hi Tim, > > > > > > > > > > > > Thanks for the patch > > > > > > > > > > > > I'll be away next week, I'll try to find time and take a closer look. > > > > > > The pipeline [0] shows some TPM related failures > > > > > > > > > > > > [0] https://source.denx.de/u-boot/custodians/u-boot-tpm/-/commit/9b4be64e41454e17269a968397933eeff300c380 > > > > > > > > > > > > > > > > Hi Ilias, > > > > > > > > > > I changed the output of 'tpm pcr_read' so that it shows the algo and > > > > > size causing the test in test/py/tests/test_tpm2.py to fail: > > > > > @@ -151,11 +171,12 @@ static int do_tpm_pcr_read(struct cmd_tbl > > > > > *cmdtp, int flag, int argc, > > > > > > > > > > data = map_sysmem(simple_strtoul(argv[2], NULL, 0), 0); > > > > > > > > > > - rc = tpm2_pcr_read(dev, index, priv->pcr_select_min, TPM2_ALG_SHA256, > > > > > - data, TPM2_DIGEST_LEN, &updates); > > > > > + rc = tpm2_pcr_read(dev, index, priv->pcr_select_min, algo, > > > > > + data, algo_len, &updates); > > > > > if (!rc) { > > > > > - printf("PCR #%u content (%u known updates):\n", index, updates); > > > > > - print_byte_string(data, TPM2_DIGEST_LEN); > > > > > + printf("PCR #%u %s %d byte content (%u known > > > > > updates):\n", index, > > > > > + tpm2_algorithm_name(algo), algo_len, updates); > > > > > + print_byte_string(data, algo_len); > > > > > } > > > > > > > > > > failure: > > > > > E AssertionError: assert 'PCR #10 content' in 'PCR #10 sha256 32 > > > > > byte content (723 known updates):\r\r\n 00 00 00 00 00 00 00 00 00 00 > > > > > 00 00 00 00 00 00\r\r\n 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > > > > > 00' > > > > > > > > > > So I suppose I need to update test/py/tests/test_tpm2.py as well. > > > > > > > > Yes > > > > > > > > > > > > > > Would I update test/py/tests/test_tpm2.py in the same patch as the one > > > > > that causes the failure? > > > > > > > > Yes please, I'd like patches merged that won't break the CI > > > > > > > > > > > > > > How do I go about running the tests manually to make sure I've addressed it? > > > > > > > > You can send a PR against U-Boots repo (in github) > > > > > > > > Cheers > > > > /Ilias > > > > > > > > > > Best Regards, > > > > > > > > > > Tim > > > > > > > > > > > Cheers > > > > > > /Ilias > > > > > > > > > > > > On Fri, 5 Apr 2024 at 03:17, Tim Harvey <tharvey@gateworks.com> wrote: > > > > > > > > > > > > > > For pcr_read and pcr_extend commands allow the digest algorithm to be > > > > > > > specified by an additional argument. If not specified it will default to > > > > > > > SHA256 for backwards compatibility. > > > > > > > > > > > > > > A follow-on to this could be to extend all PCR banks with the detected > > > > > > > algo when the <digest_algo> argument is 'auto'. > > > > > > > > > > > > > > Signed-off-by: Tim Harvey <tharvey@gateworks.com> > > > > > > > --- > > > > > > > v3: > > > > > > > - replace tpm2_supported_algorithms with struct and use this to relate hash algoirthm > > > > > > > details > > > > > > > v2: > > > > > > > - use tpm2_algorithm_to_len > > > > > > > - use enum tpm2_algorithms > > > > > > > - make function names and parameter names more consistent with existing > > > > > > > tpm-v2 functions > > > > > > > - fix various spelling errors > > > > > > > --- > > > > > > > cmd/tpm-v2.c | 49 ++++++++++++++++++++-------- > > > > > > > include/tpm-v2.h | 67 ++++++++++++++++++++++++++++++++++++++- > > > > > > > lib/efi_loader/efi_tcg2.c | 4 +-- > > > > > > > lib/tpm-v2.c | 62 +++++++++++++++++++++++------------- > > > > > > > 4 files changed, 143 insertions(+), 39 deletions(-) > > > > > > > > > > > > > > diff --git a/cmd/tpm-v2.c b/cmd/tpm-v2.c > > > > > > > index 7e479b9dfe36..2343b4d9cb9e 100644 > > > > > > > --- a/cmd/tpm-v2.c > > > > > > > +++ b/cmd/tpm-v2.c > > > > > > > @@ -99,11 +99,19 @@ static int do_tpm2_pcr_extend(struct cmd_tbl *cmdtp, int flag, int argc, > > > > > > > struct tpm_chip_priv *priv; > > > > > > > u32 index = simple_strtoul(argv[1], NULL, 0); > > > > > > > void *digest = map_sysmem(simple_strtoul(argv[2], NULL, 0), 0); > > > > > > > + int algo = TPM2_ALG_SHA256; > > > > > > > + int algo_len; > > > > > > > int ret; > > > > > > > u32 rc; > > > > > > > > > > > > > > - if (argc != 3) > > > > > > > + if (argc < 3 || argc > 4) > > > > > > > return CMD_RET_USAGE; > > > > > > > + if (argc == 4) { > > > > > > > + algo = tpm2_name_to_algorithm(argv[3]); > > > > > > > + if (algo < 0) > > > > > > > + return CMD_RET_FAILURE; > > > > > > > + } > > > > > > > + algo_len = tpm2_algorithm_to_len(algo); > > > > > > > > > > > > > > ret = get_tpm(&dev); > > > > > > > if (ret) > > > > > > > @@ -116,8 +124,12 @@ static int do_tpm2_pcr_extend(struct cmd_tbl *cmdtp, int flag, int argc, > > > > > > > if (index >= priv->pcr_count) > > > > > > > return -EINVAL; > > > > > > > > > > > > > > - rc = tpm2_pcr_extend(dev, index, TPM2_ALG_SHA256, digest, > > > > > > > - TPM2_DIGEST_LEN); > > > > > > > + rc = tpm2_pcr_extend(dev, index, algo, digest, algo_len); > > > > > > > + if (!rc) { > > > > > > > + printf("PCR #%u extended with %d byte %s digest\n", index, > > > > > > > + algo_len, tpm2_algorithm_name(algo)); > > > > > > > + print_byte_string(digest, algo_len); > > > > > > > + } > > > > > > > > > > > > > > unmap_sysmem(digest); > > > > > > > > > > > > > > @@ -127,15 +139,23 @@ static int do_tpm2_pcr_extend(struct cmd_tbl *cmdtp, int flag, int argc, > > > > > > > static int do_tpm_pcr_read(struct cmd_tbl *cmdtp, int flag, int argc, > > > > > > > char *const argv[]) > > > > > > > { > > > > > > > + enum tpm2_algorithms algo = TPM2_ALG_SHA256; > > > > > > > struct udevice *dev; > > > > > > > struct tpm_chip_priv *priv; > > > > > > > u32 index, rc; > > > > > > > + int algo_len; > > > > > > > unsigned int updates; > > > > > > > void *data; > > > > > > > int ret; > > > > > > > > > > > > > > - if (argc != 3) > > > > > > > + if (argc < 3 || argc > 4) > > > > > > > return CMD_RET_USAGE; > > > > > > > + if (argc == 4) { > > > > > > > + algo = tpm2_name_to_algorithm(argv[3]); > > > > > > > + if (algo < 0) > > > > > > > + return CMD_RET_FAILURE; > > > > > > > + } > > > > > > > + algo_len = tpm2_algorithm_to_len(algo); > > > > > > > > > > > > > > ret = get_tpm(&dev); > > > > > > > if (ret) > > > > > > > @@ -151,11 +171,12 @@ static int do_tpm_pcr_read(struct cmd_tbl *cmdtp, int flag, int argc, > > > > > > > > > > > > > > data = map_sysmem(simple_strtoul(argv[2], NULL, 0), 0); > > > > > > > > > > > > > > - rc = tpm2_pcr_read(dev, index, priv->pcr_select_min, TPM2_ALG_SHA256, > > > > > > > - data, TPM2_DIGEST_LEN, &updates); > > > > > > > + rc = tpm2_pcr_read(dev, index, priv->pcr_select_min, algo, > > > > > > > + data, algo_len, &updates); > > > > > > > if (!rc) { > > > > > > > - printf("PCR #%u content (%u known updates):\n", index, updates); > > > > > > > - print_byte_string(data, TPM2_DIGEST_LEN); > > > > > > > + printf("PCR #%u %s %d byte content (%u known updates):\n", index, > > > > > > > + tpm2_algorithm_name(algo), algo_len, updates); > > > > > > > + print_byte_string(data, algo_len); > > > > > > > } > > > > > > > > > > > > > > unmap_sysmem(data); > > > > > > > @@ -415,14 +436,14 @@ U_BOOT_CMD(tpm2, CONFIG_SYS_MAXARGS, 1, do_tpm, "Issue a TPMv2.x command", > > > > > > > " <hierarchy> is one of:\n" > > > > > > > " * TPM2_RH_LOCKOUT\n" > > > > > > > " * TPM2_RH_PLATFORM\n" > > > > > > > -"pcr_extend <pcr> <digest_addr>\n" > > > > > > > -" Extend PCR #<pcr> with digest at <digest_addr>.\n" > > > > > > > +"pcr_extend <pcr> <digest_addr> [<digest_algo>]\n" > > > > > > > +" Extend PCR #<pcr> with digest at <digest_addr> with digest_algo.\n" > > > > > > > " <pcr>: index of the PCR\n" > > > > > > > -" <digest_addr>: address of a 32-byte SHA256 digest\n" > > > > > > > -"pcr_read <pcr> <digest_addr>\n" > > > > > > > -" Read PCR #<pcr> to memory address <digest_addr>.\n" > > > > > > > +" <digest_addr>: address of digest of digest_algo type (defaults to SHA256)\n" > > > > > > > +"pcr_read <pcr> <digest_addr> [<digest_algo>]\n" > > > > > > > +" Read PCR #<pcr> to memory address <digest_addr> with <digest_algo>.\n" > > > > > > > " <pcr>: index of the PCR\n" > > > > > > > -" <digest_addr>: address to store the a 32-byte SHA256 digest\n" > > > > > > > +" <digest_addr>: address of digest of digest_algo type (defaults to SHA256)\n" > > > > > > > "get_capability <capability> <property> <addr> <count>\n" > > > > > > > " Read and display <count> entries indexed by <capability>/<property>.\n" > > > > > > > " Values are 4 bytes long and are written at <addr>.\n" > > > > > > > diff --git a/include/tpm-v2.h b/include/tpm-v2.h > > > > > > > index 33dd103767c4..263f9529e55d 100644 > > > > > > > --- a/include/tpm-v2.h > > > > > > > +++ b/include/tpm-v2.h > > > > > > > @@ -386,7 +386,54 @@ enum tpm2_algorithms { > > > > > > > TPM2_ALG_SM3_256 = 0x12, > > > > > > > }; > > > > > > > > > > > > > > -extern const enum tpm2_algorithms tpm2_supported_algorithms[4]; > > > > > > > +/** > > > > > > > + * struct digest_info - details of supported digests > > > > > > > + * > > > > > > > + * @hash_name: hash name > > > > > > > + * @hash_alg: hash algorithm id > > > > > > > + * @hash_mask: hash registry mask > > > > > > > + * @hash_len: hash digest length > > > > > > > + */ > > > > > > > +struct digest_info { > > > > > > > + const char *hash_name; > > > > > > > + u16 hash_alg; > > > > > > > + u32 hash_mask; > > > > > > > + u16 hash_len; > > > > > > > +}; > > > > > > > + > > > > > > > +/* Algorithm Registry */ > > > > > > > +#define TCG2_BOOT_HASH_ALG_SHA1 0x00000001 > > > > > > > +#define TCG2_BOOT_HASH_ALG_SHA256 0x00000002 > > > > > > > +#define TCG2_BOOT_HASH_ALG_SHA384 0x00000004 > > > > > > > +#define TCG2_BOOT_HASH_ALG_SHA512 0x00000008 > > > > > > > +#define TCG2_BOOT_HASH_ALG_SM3_256 0x00000010 > > > > > > > + > > > > > > > +static const struct digest_info hash_algo_list[] = { > > > > > > > + { > > > > > > > + "sha1", > > > > > > > + TPM2_ALG_SHA1, > > > > > > > + TCG2_BOOT_HASH_ALG_SHA1, > > > > > > > + TPM2_SHA1_DIGEST_SIZE, > > > > > > > + }, > > > > > > > + { > > > > > > > + "sha256", > > > > > > > + TPM2_ALG_SHA256, > > > > > > > + TCG2_BOOT_HASH_ALG_SHA256, > > > > > > > + TPM2_SHA256_DIGEST_SIZE, > > > > > > > + }, > > > > > > > + { > > > > > > > + "sha384", > > > > > > > + TPM2_ALG_SHA384, > > > > > > > + TCG2_BOOT_HASH_ALG_SHA384, > > > > > > > + TPM2_SHA384_DIGEST_SIZE, > > > > > > > + }, > > > > > > > + { > > > > > > > + "sha512", > > > > > > > + TPM2_ALG_SHA512, > > > > > > > + TCG2_BOOT_HASH_ALG_SHA512, > > > > > > > + TPM2_SHA512_DIGEST_SIZE, > > > > > > > + }, > > > > > > > +}; > > > > > > > > > > > > > > static inline u16 tpm2_algorithm_to_len(enum tpm2_algorithms a) > > > > > > > { > > > > > > > @@ -965,4 +1012,22 @@ u32 tpm2_enable_nvcommits(struct udevice *dev, uint vendor_cmd, > > > > > > > */ > > > > > > > u32 tpm2_auto_start(struct udevice *dev); > > > > > > > > > > > > > > +/** > > > > > > > + * tpm2_name_to_algorithm() - Return an algorithm id given a supported > > > > > > > + * algorithm name > > > > > > > + * > > > > > > > + * @name: algorithm name > > > > > > > + * Return: enum tpm2_algorithms or -EINVAL > > > > > > > + */ > > > > > > > +enum tpm2_algorithms tpm2_name_to_algorithm(const char *name); > > > > > > > + > > > > > > > +/** > > > > > > > + * tpm2_algorithm_name() - Return an algorithm name string for a > > > > > > > + * supported algorithm id > > > > > > > + * > > > > > > > + * @algorithm_id: algorithm defined in enum tpm2_algorithms > > > > > > > + * Return: algorithm name string or "" > > > > > > > + */ > > > > > > > +const char *tpm2_algorithm_name(enum tpm2_algorithms); > > > > > > > + > > > > > > > #endif /* __TPM_V2_H */ > > > > > > > diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c > > > > > > > index b07e0099c27e..cf5d8de018a7 100644 > > > > > > > --- a/lib/efi_loader/efi_tcg2.c > > > > > > > +++ b/lib/efi_loader/efi_tcg2.c > > > > > > > @@ -414,8 +414,8 @@ static efi_status_t tcg2_hash_pe_image(void *efi, u64 efi_size, > > > > > > > } > > > > > > > > > > > > > > digest_list->count = 0; > > > > > > > - for (i = 0; i < ARRAY_SIZE(tpm2_supported_algorithms); i++) { > > > > > > > - u16 hash_alg = tpm2_supported_algorithms[i]; > > > > > > > + for (i = 0; i < ARRAY_SIZE(hash_algo_list); i++) { > > > > > > > + u16 hash_alg = hash_algo_list[i].hash_alg; > > > > > > > > > > > > > > if (!(active & tpm2_algorithm_to_mask(hash_alg))) > > > > > > > continue; > > > > > > > diff --git a/lib/tpm-v2.c b/lib/tpm-v2.c > > > > > > > index 68eaaa639f89..83f490c82541 100644 > > > > > > > --- a/lib/tpm-v2.c > > > > > > > +++ b/lib/tpm-v2.c > > > > > > > @@ -22,13 +22,6 @@ > > > > > > > > > > > > > > #include "tpm-utils.h" > > > > > > > > > > > > > > -const enum tpm2_algorithms tpm2_supported_algorithms[4] = { > > > > > > > - TPM2_ALG_SHA1, > > > > > > > - , > > > > > > > - TPM2_ALG_SHA384, > > > > > > > - TPM2_ALG_SHA512, > > > > > > > -}; > > > > > > The current tpm2_algorithm_to_mask() operates on those values and bits > > > shifts based on the enum. Since you remove the enum above, you must > > > also change the function implementation and return > > > hash_algo_list.hash_mask > > > > > > > That's already taken care of as the calls to tpm2_algorithm_to_mask() > > in my patch were adjusted from: > > tpm2_algorithm_to_mask(tpm2_supported_algorithms[i]) > > to > > tpm2_algorithm_to_mask(hash_algo_list[i].hash_alg) > > > > The hash_alg is the value from the previous enum. > > The previous enums were defining TPM2_ALG_SHA1=1, TPM2_ALG_SHA256=2 etc > The current values are different. On top of that the .hash_mask entry > of digest_info is never used. So you need to change > tpm2_algorithm_to_mask() and directly return the hash_mask > Hi Ilias, Just getting back to this. I don't agree with your assessment. The previous enum: -const enum tpm2_algorithms tpm2_supported_algorithms[4] = { - TPM2_ALG_SHA1, - TPM2_ALG_SHA256, - TPM2_ALG_SHA384, - TPM2_ALG_SHA512, -}; is defining 4 values which equate to 0x04, 0x0B, 0x0C, 0x0D (not 1, 2, 3, 4 as you say) which is what I'm putting in the hash_alg field. The tpm2_algorithm_to_mask macro shifts those values (not the index into the enum). So with the current code: for (i = 0; i < ARRAY_SIZE(tpm2_supported_algorithms); i++) { int alg = tpm2_supported_algorithms[i]; printf("alg:0x%02x mask=0x%02x\n", alg, tpm2_algorithm_to_mask(alg)); } alg:0x04 mask=0x10 alg:0x0b mask=0x800 alg:0x0c mask=0x1000 alg:0x0d mask=0x2000 With this proposed patch: for (i = 0; i < ARRAY_SIZE(hash_algo_list); i++) { int alg = hash_algo_list[i].hash_alg; printf("alg:0x%02x mask=0x%02x %s\n", alg, tpm2_algorithm_to_mask(alg), tpm2_algorithm_name(alg)); } alg:0x04 mask=0x10 sha1 alg:0x0b mask=0x800 sha256 alg:0x0c mask=0x1000 sha384 alg:0x0d mask=0x2000 sha512 Am I misunderstanding something else you are pointing out? Maybe it would be easier to compare if I named the new struct tpm2_supported_algorithms? Best Regards, Tim > Cheers > /Ilias > > > > Best Regards, > > > > Tim > > > > > Cheers > > > /Ilias > > > > > > > > > > > > > > > > - > > > > > > > int tcg2_get_active_pcr_banks(struct udevice *dev, u32 *active_pcr_banks) > > > > > > > { > > > > > > > u32 supported = 0; > > > > > > > @@ -82,14 +75,14 @@ int tcg2_create_digest(struct udevice *dev, const u8 *input, u32 length, > > > > > > > return rc; > > > > > > > > > > > > > > digest_list->count = 0; > > > > > > > - for (i = 0; i < ARRAY_SIZE(tpm2_supported_algorithms); ++i) { > > > > > > > + for (i = 0; i < ARRAY_SIZE(hash_algo_list); ++i) { > > > > > > > u32 mask = > > > > > > > - tpm2_algorithm_to_mask(tpm2_supported_algorithms[i]); > > > > > > > + tpm2_algorithm_to_mask(hash_algo_list[i].hash_alg); > > > > > > > > > > > > > > if (!(active & mask))
Hi Tim, Apologies for the late reply. I was attending a conference. On Thu, 16 May 2024 at 03:28, Tim Harvey <tharvey@gateworks.com> wrote: > > On Fri, Apr 19, 2024 at 1:04 PM Ilias Apalodimas > <ilias.apalodimas@linaro.org> wrote: > > > > On Fri, 19 Apr 2024 at 20:52, Tim Harvey <tharvey@gateworks.com> wrote: > > > > > > On Fri, Apr 19, 2024 at 10:37 AM Ilias Apalodimas > > > <ilias.apalodimas@linaro.org> wrote: > > > > > > > > Also quickly looking at this, you need a new function for > > > > tpm2_algorithm_to_mask() (look below) > > > > > > > > On Fri, 19 Apr 2024 at 20:20, Ilias Apalodimas > > > > <ilias.apalodimas@linaro.org> wrote: > > > > > > > > > > Hi Tim, > > > > > > > > > > On Fri, 19 Apr 2024 at 20:13, Tim Harvey <tharvey@gateworks.com> wrote: > > > > > > > > > > > > On Sat, Apr 6, 2024 at 9:33 AM Ilias Apalodimas > > > > > > <ilias.apalodimas@linaro.org> wrote: > > > > > > > > > > > > > > Hi Tim, > > > > > > > > > > > > > > Thanks for the patch > > > > > > > > > > > > > > I'll be away next week, I'll try to find time and take a closer look. > > > > > > > The pipeline [0] shows some TPM related failures > > > > > > > > > > > > > > [0] https://source.denx.de/u-boot/custodians/u-boot-tpm/-/commit/9b4be64e41454e17269a968397933eeff300c380 > > > > > > > > > > > > > > > > > > > Hi Ilias, > > > > > > > > > > > > I changed the output of 'tpm pcr_read' so that it shows the algo and > > > > > > size causing the test in test/py/tests/test_tpm2.py to fail: > > > > > > @@ -151,11 +171,12 @@ static int do_tpm_pcr_read(struct cmd_tbl > > > > > > *cmdtp, int flag, int argc, > > > > > > > > > > > > data = map_sysmem(simple_strtoul(argv[2], NULL, 0), 0); > > > > > > > > > > > > - rc = tpm2_pcr_read(dev, index, priv->pcr_select_min, TPM2_ALG_SHA256, > > > > > > - data, TPM2_DIGEST_LEN, &updates); > > > > > > + rc = tpm2_pcr_read(dev, index, priv->pcr_select_min, algo, > > > > > > + data, algo_len, &updates); > > > > > > if (!rc) { > > > > > > - printf("PCR #%u content (%u known updates):\n", index, updates); > > > > > > - print_byte_string(data, TPM2_DIGEST_LEN); > > > > > > + printf("PCR #%u %s %d byte content (%u known > > > > > > updates):\n", index, > > > > > > + tpm2_algorithm_name(algo), algo_len, updates); > > > > > > + print_byte_string(data, algo_len); > > > > > > } > > > > > > > > > > > > failure: > > > > > > E AssertionError: assert 'PCR #10 content' in 'PCR #10 sha256 32 > > > > > > byte content (723 known updates):\r\r\n 00 00 00 00 00 00 00 00 00 00 > > > > > > 00 00 00 00 00 00\r\r\n 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > > > > > > 00' > > > > > > > > > > > > So I suppose I need to update test/py/tests/test_tpm2.py as well. > > > > > > > > > > Yes > > > > > > > > > > > > > > > > > Would I update test/py/tests/test_tpm2.py in the same patch as the one > > > > > > that causes the failure? > > > > > > > > > > Yes please, I'd like patches merged that won't break the CI > > > > > > > > > > > > > > > > > How do I go about running the tests manually to make sure I've addressed it? > > > > > > > > > > You can send a PR against U-Boots repo (in github) > > > > > > > > > > Cheers > > > > > /Ilias > > > > > > > > > > > > Best Regards, > > > > > > > > > > > > Tim > > > > > > > > > > > > > Cheers > > > > > > > /Ilias > > > > > > > > > > > > > > On Fri, 5 Apr 2024 at 03:17, Tim Harvey <tharvey@gateworks.com> wrote: > > > > > > > > > > > > > > > > For pcr_read and pcr_extend commands allow the digest algorithm to be > > > > > > > > specified by an additional argument. If not specified it will default to > > > > > > > > SHA256 for backwards compatibility. > > > > > > > > > > > > > > > > A follow-on to this could be to extend all PCR banks with the detected > > > > > > > > algo when the <digest_algo> argument is 'auto'. > > > > > > > > > > > > > > > > Signed-off-by: Tim Harvey <tharvey@gateworks.com> > > > > > > > > --- > > > > > > > > v3: > > > > > > > > - replace tpm2_supported_algorithms with struct and use this to relate hash algoirthm > > > > > > > > details > > > > > > > > v2: > > > > > > > > - use tpm2_algorithm_to_len > > > > > > > > - use enum tpm2_algorithms > > > > > > > > - make function names and parameter names more consistent with existing > > > > > > > > tpm-v2 functions > > > > > > > > - fix various spelling errors > > > > > > > > --- > > > > > > > > cmd/tpm-v2.c | 49 ++++++++++++++++++++-------- > > > > > > > > include/tpm-v2.h | 67 ++++++++++++++++++++++++++++++++++++++- > > > > > > > > lib/efi_loader/efi_tcg2.c | 4 +-- > > > > > > > > lib/tpm-v2.c | 62 +++++++++++++++++++++++------------- > > > > > > > > 4 files changed, 143 insertions(+), 39 deletions(-) > > > > > > > > > > > > > > > > diff --git a/cmd/tpm-v2.c b/cmd/tpm-v2.c > > > > > > > > index 7e479b9dfe36..2343b4d9cb9e 100644 > > > > > > > > --- a/cmd/tpm-v2.c > > > > > > > > +++ b/cmd/tpm-v2.c > > > > > > > > @@ -99,11 +99,19 @@ static int do_tpm2_pcr_extend(struct cmd_tbl *cmdtp, int flag, int argc, > > > > > > > > struct tpm_chip_priv *priv; > > > > > > > > u32 index = simple_strtoul(argv[1], NULL, 0); > > > > > > > > void *digest = map_sysmem(simple_strtoul(argv[2], NULL, 0), 0); > > > > > > > > + int algo = TPM2_ALG_SHA256; > > > > > > > > + int algo_len; > > > > > > > > int ret; > > > > > > > > u32 rc; > > > > > > > > > > > > > > > > - if (argc != 3) > > > > > > > > + if (argc < 3 || argc > 4) > > > > > > > > return CMD_RET_USAGE; > > > > > > > > + if (argc == 4) { > > > > > > > > + algo = tpm2_name_to_algorithm(argv[3]); > > > > > > > > + if (algo < 0) > > > > > > > > + return CMD_RET_FAILURE; > > > > > > > > + } > > > > > > > > + algo_len = tpm2_algorithm_to_len(algo); > > > > > > > > > > > > > > > > ret = get_tpm(&dev); > > > > > > > > if (ret) > > > > > > > > @@ -116,8 +124,12 @@ static int do_tpm2_pcr_extend(struct cmd_tbl *cmdtp, int flag, int argc, > > > > > > > > if (index >= priv->pcr_count) > > > > > > > > return -EINVAL; > > > > > > > > > > > > > > > > - rc = tpm2_pcr_extend(dev, index, TPM2_ALG_SHA256, digest, > > > > > > > > - TPM2_DIGEST_LEN); > > > > > > > > + rc = tpm2_pcr_extend(dev, index, algo, digest, algo_len); > > > > > > > > + if (!rc) { > > > > > > > > + printf("PCR #%u extended with %d byte %s digest\n", index, > > > > > > > > + algo_len, tpm2_algorithm_name(algo)); > > > > > > > > + print_byte_string(digest, algo_len); > > > > > > > > + } > > > > > > > > > > > > > > > > unmap_sysmem(digest); > > > > > > > > > > > > > > > > @@ -127,15 +139,23 @@ static int do_tpm2_pcr_extend(struct cmd_tbl *cmdtp, int flag, int argc, > > > > > > > > static int do_tpm_pcr_read(struct cmd_tbl *cmdtp, int flag, int argc, > > > > > > > > char *const argv[]) > > > > > > > > { > > > > > > > > + enum tpm2_algorithms algo = TPM2_ALG_SHA256; > > > > > > > > struct udevice *dev; > > > > > > > > struct tpm_chip_priv *priv; > > > > > > > > u32 index, rc; > > > > > > > > + int algo_len; > > > > > > > > unsigned int updates; > > > > > > > > void *data; > > > > > > > > int ret; > > > > > > > > > > > > > > > > - if (argc != 3) > > > > > > > > + if (argc < 3 || argc > 4) > > > > > > > > return CMD_RET_USAGE; > > > > > > > > + if (argc == 4) { > > > > > > > > + algo = tpm2_name_to_algorithm(argv[3]); > > > > > > > > + if (algo < 0) > > > > > > > > + return CMD_RET_FAILURE; > > > > > > > > + } > > > > > > > > + algo_len = tpm2_algorithm_to_len(algo); > > > > > > > > > > > > > > > > ret = get_tpm(&dev); > > > > > > > > if (ret) > > > > > > > > @@ -151,11 +171,12 @@ static int do_tpm_pcr_read(struct cmd_tbl *cmdtp, int flag, int argc, > > > > > > > > > > > > > > > > data = map_sysmem(simple_strtoul(argv[2], NULL, 0), 0); > > > > > > > > > > > > > > > > - rc = tpm2_pcr_read(dev, index, priv->pcr_select_min, TPM2_ALG_SHA256, > > > > > > > > - data, TPM2_DIGEST_LEN, &updates); > > > > > > > > + rc = tpm2_pcr_read(dev, index, priv->pcr_select_min, algo, > > > > > > > > + data, algo_len, &updates); > > > > > > > > if (!rc) { > > > > > > > > - printf("PCR #%u content (%u known updates):\n", index, updates); > > > > > > > > - print_byte_string(data, TPM2_DIGEST_LEN); > > > > > > > > + printf("PCR #%u %s %d byte content (%u known updates):\n", index, > > > > > > > > + tpm2_algorithm_name(algo), algo_len, updates); > > > > > > > > + print_byte_string(data, algo_len); > > > > > > > > } > > > > > > > > > > > > > > > > unmap_sysmem(data); > > > > > > > > @@ -415,14 +436,14 @@ U_BOOT_CMD(tpm2, CONFIG_SYS_MAXARGS, 1, do_tpm, "Issue a TPMv2.x command", > > > > > > > > " <hierarchy> is one of:\n" > > > > > > > > " * TPM2_RH_LOCKOUT\n" > > > > > > > > " * TPM2_RH_PLATFORM\n" > > > > > > > > -"pcr_extend <pcr> <digest_addr>\n" > > > > > > > > -" Extend PCR #<pcr> with digest at <digest_addr>.\n" > > > > > > > > +"pcr_extend <pcr> <digest_addr> [<digest_algo>]\n" > > > > > > > > +" Extend PCR #<pcr> with digest at <digest_addr> with digest_algo.\n" > > > > > > > > " <pcr>: index of the PCR\n" > > > > > > > > -" <digest_addr>: address of a 32-byte SHA256 digest\n" > > > > > > > > -"pcr_read <pcr> <digest_addr>\n" > > > > > > > > -" Read PCR #<pcr> to memory address <digest_addr>.\n" > > > > > > > > +" <digest_addr>: address of digest of digest_algo type (defaults to SHA256)\n" > > > > > > > > +"pcr_read <pcr> <digest_addr> [<digest_algo>]\n" > > > > > > > > +" Read PCR #<pcr> to memory address <digest_addr> with <digest_algo>.\n" > > > > > > > > " <pcr>: index of the PCR\n" > > > > > > > > -" <digest_addr>: address to store the a 32-byte SHA256 digest\n" > > > > > > > > +" <digest_addr>: address of digest of digest_algo type (defaults to SHA256)\n" > > > > > > > > "get_capability <capability> <property> <addr> <count>\n" > > > > > > > > " Read and display <count> entries indexed by <capability>/<property>.\n" > > > > > > > > " Values are 4 bytes long and are written at <addr>.\n" > > > > > > > > diff --git a/include/tpm-v2.h b/include/tpm-v2.h > > > > > > > > index 33dd103767c4..263f9529e55d 100644 > > > > > > > > --- a/include/tpm-v2.h > > > > > > > > +++ b/include/tpm-v2.h > > > > > > > > @@ -386,7 +386,54 @@ enum tpm2_algorithms { > > > > > > > > TPM2_ALG_SM3_256 = 0x12, > > > > > > > > }; > > > > > > > > > > > > > > > > -extern const enum tpm2_algorithms tpm2_supported_algorithms[4]; > > > > > > > > +/** > > > > > > > > + * struct digest_info - details of supported digests > > > > > > > > + * > > > > > > > > + * @hash_name: hash name > > > > > > > > + * @hash_alg: hash algorithm id > > > > > > > > + * @hash_mask: hash registry mask > > > > > > > > + * @hash_len: hash digest length > > > > > > > > + */ > > > > > > > > +struct digest_info { > > > > > > > > + const char *hash_name; > > > > > > > > + u16 hash_alg; > > > > > > > > + u32 hash_mask; > > > > > > > > + u16 hash_len; > > > > > > > > +}; > > > > > > > > + > > > > > > > > +/* Algorithm Registry */ > > > > > > > > +#define TCG2_BOOT_HASH_ALG_SHA1 0x00000001 > > > > > > > > +#define TCG2_BOOT_HASH_ALG_SHA256 0x00000002 > > > > > > > > +#define TCG2_BOOT_HASH_ALG_SHA384 0x00000004 > > > > > > > > +#define TCG2_BOOT_HASH_ALG_SHA512 0x00000008 > > > > > > > > +#define TCG2_BOOT_HASH_ALG_SM3_256 0x00000010 > > > > > > > > + > > > > > > > > +static const struct digest_info hash_algo_list[] = { > > > > > > > > + { > > > > > > > > + "sha1", > > > > > > > > + TPM2_ALG_SHA1, > > > > > > > > + TCG2_BOOT_HASH_ALG_SHA1, > > > > > > > > + TPM2_SHA1_DIGEST_SIZE, > > > > > > > > + }, > > > > > > > > + { > > > > > > > > + "sha256", > > > > > > > > + TPM2_ALG_SHA256, > > > > > > > > + TCG2_BOOT_HASH_ALG_SHA256, > > > > > > > > + TPM2_SHA256_DIGEST_SIZE, > > > > > > > > + }, > > > > > > > > + { > > > > > > > > + "sha384", > > > > > > > > + TPM2_ALG_SHA384, > > > > > > > > + TCG2_BOOT_HASH_ALG_SHA384, > > > > > > > > + TPM2_SHA384_DIGEST_SIZE, > > > > > > > > + }, > > > > > > > > + { > > > > > > > > + "sha512", > > > > > > > > + TPM2_ALG_SHA512, > > > > > > > > + TCG2_BOOT_HASH_ALG_SHA512, > > > > > > > > + TPM2_SHA512_DIGEST_SIZE, > > > > > > > > + }, > > > > > > > > +}; > > > > > > > > > > > > > > > > static inline u16 tpm2_algorithm_to_len(enum tpm2_algorithms a) > > > > > > > > { > > > > > > > > @@ -965,4 +1012,22 @@ u32 tpm2_enable_nvcommits(struct udevice *dev, uint vendor_cmd, > > > > > > > > */ > > > > > > > > u32 tpm2_auto_start(struct udevice *dev); > > > > > > > > > > > > > > > > +/** > > > > > > > > + * tpm2_name_to_algorithm() - Return an algorithm id given a supported > > > > > > > > + * algorithm name > > > > > > > > + * > > > > > > > > + * @name: algorithm name > > > > > > > > + * Return: enum tpm2_algorithms or -EINVAL > > > > > > > > + */ > > > > > > > > +enum tpm2_algorithms tpm2_name_to_algorithm(const char *name); > > > > > > > > + > > > > > > > > +/** > > > > > > > > + * tpm2_algorithm_name() - Return an algorithm name string for a > > > > > > > > + * supported algorithm id > > > > > > > > + * > > > > > > > > + * @algorithm_id: algorithm defined in enum tpm2_algorithms > > > > > > > > + * Return: algorithm name string or "" > > > > > > > > + */ > > > > > > > > +const char *tpm2_algorithm_name(enum tpm2_algorithms); > > > > > > > > + > > > > > > > > #endif /* __TPM_V2_H */ > > > > > > > > diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c > > > > > > > > index b07e0099c27e..cf5d8de018a7 100644 > > > > > > > > --- a/lib/efi_loader/efi_tcg2.c > > > > > > > > +++ b/lib/efi_loader/efi_tcg2.c > > > > > > > > @@ -414,8 +414,8 @@ static efi_status_t tcg2_hash_pe_image(void *efi, u64 efi_size, > > > > > > > > } > > > > > > > > > > > > > > > > digest_list->count = 0; > > > > > > > > - for (i = 0; i < ARRAY_SIZE(tpm2_supported_algorithms); i++) { > > > > > > > > - u16 hash_alg = tpm2_supported_algorithms[i]; > > > > > > > > + for (i = 0; i < ARRAY_SIZE(hash_algo_list); i++) { > > > > > > > > + u16 hash_alg = hash_algo_list[i].hash_alg; > > > > > > > > > > > > > > > > if (!(active & tpm2_algorithm_to_mask(hash_alg))) > > > > > > > > continue; > > > > > > > > diff --git a/lib/tpm-v2.c b/lib/tpm-v2.c > > > > > > > > index 68eaaa639f89..83f490c82541 100644 > > > > > > > > --- a/lib/tpm-v2.c > > > > > > > > +++ b/lib/tpm-v2.c > > > > > > > > @@ -22,13 +22,6 @@ > > > > > > > > > > > > > > > > #include "tpm-utils.h" > > > > > > > > > > > > > > > > -const enum tpm2_algorithms tpm2_supported_algorithms[4] = { > > > > > > > > - TPM2_ALG_SHA1, > > > > > > > > - , > > > > > > > > - TPM2_ALG_SHA384, > > > > > > > > - TPM2_ALG_SHA512, > > > > > > > > -}; > > > > > > > > The current tpm2_algorithm_to_mask() operates on those values and bits > > > > shifts based on the enum. Since you remove the enum above, you must > > > > also change the function implementation and return > > > > hash_algo_list.hash_mask > > > > > > > > > > That's already taken care of as the calls to tpm2_algorithm_to_mask() > > > in my patch were adjusted from: > > > tpm2_algorithm_to_mask(tpm2_supported_algorithms[i]) > > > to > > > tpm2_algorithm_to_mask(hash_algo_list[i].hash_alg) > > > > > > The hash_alg is the value from the previous enum. > > > > The previous enums were defining TPM2_ALG_SHA1=1, TPM2_ALG_SHA256=2 etc > > The current values are different. On top of that the .hash_mask entry > > of digest_info is never used. So you need to change > > tpm2_algorithm_to_mask() and directly return the hash_mask > > > > Hi Ilias, > > Just getting back to this. I don't agree with your assessment. > > The previous enum: > -const enum tpm2_algorithms tpm2_supported_algorithms[4] = { > - TPM2_ALG_SHA1, > - TPM2_ALG_SHA256, > - TPM2_ALG_SHA384, > - TPM2_ALG_SHA512, > -}; > > is defining 4 values which equate to 0x04, 0x0B, 0x0C, 0x0D (not 1, 2, > 3, 4 as you say) which is what I'm putting in the hash_alg field. The > tpm2_algorithm_to_mask macro shifts those values (not the index into > the enum). > > So with the current code: > for (i = 0; i < ARRAY_SIZE(tpm2_supported_algorithms); i++) { > int alg = tpm2_supported_algorithms[i]; > printf("alg:0x%02x mask=0x%02x\n", alg, tpm2_algorithm_to_mask(alg)); > } > alg:0x04 mask=0x10 > alg:0x0b mask=0x800 > alg:0x0c mask=0x1000 > alg:0x0d mask=0x2000 > > With this proposed patch: > for (i = 0; i < ARRAY_SIZE(hash_algo_list); i++) { > int alg = hash_algo_list[i].hash_alg; > printf("alg:0x%02x mask=0x%02x %s\n", alg, > tpm2_algorithm_to_mask(alg), tpm2_algorithm_name(alg)); > } > alg:0x04 mask=0x10 sha1 > alg:0x0b mask=0x800 sha256 > alg:0x0c mask=0x1000 sha384 > alg:0x0d mask=0x2000 sha512 > > Am I misunderstanding something else you are pointing out? > > Maybe it would be easier to compare if I named the new struct > tpm2_supported_algorithms? Ok, I think we are looking at a preexisting bug, because the prints above make no sense to me. There was an enum in the current code const enum tpm2_algorithms tpm2_supported_algorithms[4] = { TPM2_ALG_SHA1, TPM2_ALG_SHA256, TPM2_ALG_SHA384, TPM2_ALG_SHA512, } But those values are defined in include/tpm-v2.h hence the enum values were not starting from 0... Reading at the TCG spec [0] we should be adding any of the values defined below to HashAlgorithmBitMap. #define EFI_TCG2_BOOT_HASH_ALG_SHA1 0x00000001 #define EFI_TCG2_BOOT_HASH_ALG_SHA256 0x00000002 #define EFI_TCG2_BOOT_HASH_ALG_SHA384 0x00000004 #define EFI_TCG2_BOOT_HASH_ALG_SHA512 0x00000008 #define EFI_TCG2_BOOT_HASH_ALG_SM3_256 0x00000010 We used to do that in v2023.01. The patch below is on top of 2023.01 diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c index a525ebf75b58..7371c9e94b5c 100644 --- a/lib/efi_loader/efi_tcg2.c +++ b/lib/efi_loader/efi_tcg2.c @@ -1628,6 +1628,7 @@ static efi_status_t create_specid_event(struct udevice *dev, void *buffer, u16 hash_alg = hash_algo_list[i].hash_alg; u16 hash_len = hash_algo_list[i].hash_len; + printf("Trying to add %08x and %08x\n", hash_alg, alg_to_mask(hash_alg)); if (active & alg_to_mask(hash_alg)) { put_unaligned_le16(hash_alg, &spec_event->digest_sizes[alg_count].algorithm_id); prints Trying to add 00000004 and 00000001 Trying to add 0000000b and 00000002 Trying to add 0000000c and 00000004 Trying to add 0000000d and 00000008 The values of 0x00000001, 0x00000002 etc are what's added to the eventlog But since 97707f12fdabf we are putting different values (which I think is wrong and not what the spec expects....) I also think Eddie intended to make tpm2_supported_algorithms an enum that starts from 0, but the values he used were already defined and that's how we missed it ... Eddie any idea if that's what happened? Tim, we will need to fix all this regardless. The fix should be relatively simple, just return the newly added values instead of the algorithm_to_mask value, when adding it on the eventlog. Can you take a look at the spec and verify what I am seeing? At some point, we also need to add the value checking in self-tests, rather than only looking at the EFI return code. [0] 6.4.3 Related Definitions https://trustedcomputinggroup.org/wp-content/uploads/EFI-Protocol-Specification-rev13-160330final.pdf Thanks /Ilias > > Best Regards, > > Tim > > > Cheers > > /Ilias > > > > > > Best Regards, > > > > > > Tim > > > > > > > Cheers > > > > /Ilias > > > > > > > > > > > > > > > > > > > > - > > > > > > > > int tcg2_get_active_pcr_banks(struct udevice *dev, u32 *active_pcr_banks) > > > > > > > > { > > > > > > > > u32 supported = 0; > > > > > > > > @@ -82,14 +75,14 @@ int tcg2_create_digest(struct udevice *dev, const u8 *input, u32 length, > > > > > > > > return rc; > > > > > > > > > > > > > > > > digest_list->count = 0; > > > > > > > > - for (i = 0; i < ARRAY_SIZE(tpm2_supported_algorithms); ++i) { > > > > > > > > + for (i = 0; i < ARRAY_SIZE(hash_algo_list); ++i) { > > > > > > > > u32 mask = > > > > > > > > - tpm2_algorithm_to_mask(tpm2_supported_algorithms[i]); > > > > > > > > + tpm2_algorithm_to_mask(hash_algo_list[i].hash_alg); > > > > > > > > > > > > > > > > if (!(active & mask))
On Tue, 21 May 2024 at 13:17, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Hi Tim, > > Apologies for the late reply. I was attending a conference. > > > On Thu, 16 May 2024 at 03:28, Tim Harvey <tharvey@gateworks.com> wrote: > > > > On Fri, Apr 19, 2024 at 1:04 PM Ilias Apalodimas > > <ilias.apalodimas@linaro.org> wrote: > > > > > > On Fri, 19 Apr 2024 at 20:52, Tim Harvey <tharvey@gateworks.com> wrote: > > > > > > > > On Fri, Apr 19, 2024 at 10:37 AM Ilias Apalodimas > > > > <ilias.apalodimas@linaro.org> wrote: > > > > > > > > > > Also quickly looking at this, you need a new function for > > > > > tpm2_algorithm_to_mask() (look below) > > > > > > > > > > On Fri, 19 Apr 2024 at 20:20, Ilias Apalodimas > > > > > <ilias.apalodimas@linaro.org> wrote: > > > > > > > > > > > > Hi Tim, > > > > > > > > > > > > On Fri, 19 Apr 2024 at 20:13, Tim Harvey <tharvey@gateworks.com> wrote: > > > > > > > > > > > > > > On Sat, Apr 6, 2024 at 9:33 AM Ilias Apalodimas > > > > > > > <ilias.apalodimas@linaro.org> wrote: > > > > > > > > > > > > > > > > Hi Tim, > > > > > > > > > > > > > > > > Thanks for the patch > > > > > > > > > > > > > > > > I'll be away next week, I'll try to find time and take a closer look. > > > > > > > > The pipeline [0] shows some TPM related failures > > > > > > > > > > > > > > > > [0] https://source.denx.de/u-boot/custodians/u-boot-tpm/-/commit/9b4be64e41454e17269a968397933eeff300c380 > > > > > > > > > > > > > > > > > > > > > > Hi Ilias, > > > > > > > > > > > > > > I changed the output of 'tpm pcr_read' so that it shows the algo and > > > > > > > size causing the test in test/py/tests/test_tpm2.py to fail: > > > > > > > @@ -151,11 +171,12 @@ static int do_tpm_pcr_read(struct cmd_tbl > > > > > > > *cmdtp, int flag, int argc, > > > > > > > > > > > > > > data = map_sysmem(simple_strtoul(argv[2], NULL, 0), 0); > > > > > > > > > > > > > > - rc = tpm2_pcr_read(dev, index, priv->pcr_select_min, TPM2_ALG_SHA256, > > > > > > > - data, TPM2_DIGEST_LEN, &updates); > > > > > > > + rc = tpm2_pcr_read(dev, index, priv->pcr_select_min, algo, > > > > > > > + data, algo_len, &updates); > > > > > > > if (!rc) { > > > > > > > - printf("PCR #%u content (%u known updates):\n", index, updates); > > > > > > > - print_byte_string(data, TPM2_DIGEST_LEN); > > > > > > > + printf("PCR #%u %s %d byte content (%u known > > > > > > > updates):\n", index, > > > > > > > + tpm2_algorithm_name(algo), algo_len, updates); > > > > > > > + print_byte_string(data, algo_len); > > > > > > > } > > > > > > > > > > > > > > failure: > > > > > > > E AssertionError: assert 'PCR #10 content' in 'PCR #10 sha256 32 > > > > > > > byte content (723 known updates):\r\r\n 00 00 00 00 00 00 00 00 00 00 > > > > > > > 00 00 00 00 00 00\r\r\n 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > > > > > > > 00' > > > > > > > > > > > > > > So I suppose I need to update test/py/tests/test_tpm2.py as well. > > > > > > > > > > > > Yes > > > > > > > > > > > > > > > > > > > > Would I update test/py/tests/test_tpm2.py in the same patch as the one > > > > > > > that causes the failure? > > > > > > > > > > > > Yes please, I'd like patches merged that won't break the CI > > > > > > > > > > > > > > > > > > > > How do I go about running the tests manually to make sure I've addressed it? > > > > > > > > > > > > You can send a PR against U-Boots repo (in github) > > > > > > > > > > > > Cheers > > > > > > /Ilias > > > > > > > > > > > > > > Best Regards, > > > > > > > > > > > > > > Tim > > > > > > > > > > > > > > > Cheers > > > > > > > > /Ilias > > > > > > > > > > > > > > > > On Fri, 5 Apr 2024 at 03:17, Tim Harvey <tharvey@gateworks.com> wrote: > > > > > > > > > > > > > > > > > > For pcr_read and pcr_extend commands allow the digest algorithm to be > > > > > > > > > specified by an additional argument. If not specified it will default to > > > > > > > > > SHA256 for backwards compatibility. > > > > > > > > > > > > > > > > > > A follow-on to this could be to extend all PCR banks with the detected > > > > > > > > > algo when the <digest_algo> argument is 'auto'. > > > > > > > > > > > > > > > > > > Signed-off-by: Tim Harvey <tharvey@gateworks.com> > > > > > > > > > --- > > > > > > > > > v3: > > > > > > > > > - replace tpm2_supported_algorithms with struct and use this to relate hash algoirthm > > > > > > > > > details > > > > > > > > > v2: > > > > > > > > > - use tpm2_algorithm_to_len > > > > > > > > > - use enum tpm2_algorithms > > > > > > > > > - make function names and parameter names more consistent with existing > > > > > > > > > tpm-v2 functions > > > > > > > > > - fix various spelling errors > > > > > > > > > --- > > > > > > > > > cmd/tpm-v2.c | 49 ++++++++++++++++++++-------- > > > > > > > > > include/tpm-v2.h | 67 ++++++++++++++++++++++++++++++++++++++- > > > > > > > > > lib/efi_loader/efi_tcg2.c | 4 +-- > > > > > > > > > lib/tpm-v2.c | 62 +++++++++++++++++++++++------------- > > > > > > > > > 4 files changed, 143 insertions(+), 39 deletions(-) > > > > > > > > > > > > > > > > > > diff --git a/cmd/tpm-v2.c b/cmd/tpm-v2.c > > > > > > > > > index 7e479b9dfe36..2343b4d9cb9e 100644 > > > > > > > > > --- a/cmd/tpm-v2.c > > > > > > > > > +++ b/cmd/tpm-v2.c > > > > > > > > > @@ -99,11 +99,19 @@ static int do_tpm2_pcr_extend(struct cmd_tbl *cmdtp, int flag, int argc, > > > > > > > > > struct tpm_chip_priv *priv; > > > > > > > > > u32 index = simple_strtoul(argv[1], NULL, 0); > > > > > > > > > void *digest = map_sysmem(simple_strtoul(argv[2], NULL, 0), 0); > > > > > > > > > + int algo = TPM2_ALG_SHA256; > > > > > > > > > + int algo_len; > > > > > > > > > int ret; > > > > > > > > > u32 rc; > > > > > > > > > > > > > > > > > > - if (argc != 3) > > > > > > > > > + if (argc < 3 || argc > 4) > > > > > > > > > return CMD_RET_USAGE; > > > > > > > > > + if (argc == 4) { > > > > > > > > > + algo = tpm2_name_to_algorithm(argv[3]); > > > > > > > > > + if (algo < 0) > > > > > > > > > + return CMD_RET_FAILURE; > > > > > > > > > + } > > > > > > > > > + algo_len = tpm2_algorithm_to_len(algo); > > > > > > > > > > > > > > > > > > ret = get_tpm(&dev); > > > > > > > > > if (ret) > > > > > > > > > @@ -116,8 +124,12 @@ static int do_tpm2_pcr_extend(struct cmd_tbl *cmdtp, int flag, int argc, > > > > > > > > > if (index >= priv->pcr_count) > > > > > > > > > return -EINVAL; > > > > > > > > > > > > > > > > > > - rc = tpm2_pcr_extend(dev, index, TPM2_ALG_SHA256, digest, > > > > > > > > > - TPM2_DIGEST_LEN); > > > > > > > > > + rc = tpm2_pcr_extend(dev, index, algo, digest, algo_len); > > > > > > > > > + if (!rc) { > > > > > > > > > + printf("PCR #%u extended with %d byte %s digest\n", index, > > > > > > > > > + algo_len, tpm2_algorithm_name(algo)); > > > > > > > > > + print_byte_string(digest, algo_len); > > > > > > > > > + } > > > > > > > > > > > > > > > > > > unmap_sysmem(digest); > > > > > > > > > > > > > > > > > > @@ -127,15 +139,23 @@ static int do_tpm2_pcr_extend(struct cmd_tbl *cmdtp, int flag, int argc, > > > > > > > > > static int do_tpm_pcr_read(struct cmd_tbl *cmdtp, int flag, int argc, > > > > > > > > > char *const argv[]) > > > > > > > > > { > > > > > > > > > + enum tpm2_algorithms algo = TPM2_ALG_SHA256; > > > > > > > > > struct udevice *dev; > > > > > > > > > struct tpm_chip_priv *priv; > > > > > > > > > u32 index, rc; > > > > > > > > > + int algo_len; > > > > > > > > > unsigned int updates; > > > > > > > > > void *data; > > > > > > > > > int ret; > > > > > > > > > > > > > > > > > > - if (argc != 3) > > > > > > > > > + if (argc < 3 || argc > 4) > > > > > > > > > return CMD_RET_USAGE; > > > > > > > > > + if (argc == 4) { > > > > > > > > > + algo = tpm2_name_to_algorithm(argv[3]); > > > > > > > > > + if (algo < 0) > > > > > > > > > + return CMD_RET_FAILURE; > > > > > > > > > + } > > > > > > > > > + algo_len = tpm2_algorithm_to_len(algo); > > > > > > > > > > > > > > > > > > ret = get_tpm(&dev); > > > > > > > > > if (ret) > > > > > > > > > @@ -151,11 +171,12 @@ static int do_tpm_pcr_read(struct cmd_tbl *cmdtp, int flag, int argc, > > > > > > > > > > > > > > > > > > data = map_sysmem(simple_strtoul(argv[2], NULL, 0), 0); > > > > > > > > > > > > > > > > > > - rc = tpm2_pcr_read(dev, index, priv->pcr_select_min, TPM2_ALG_SHA256, > > > > > > > > > - data, TPM2_DIGEST_LEN, &updates); > > > > > > > > > + rc = tpm2_pcr_read(dev, index, priv->pcr_select_min, algo, > > > > > > > > > + data, algo_len, &updates); > > > > > > > > > if (!rc) { > > > > > > > > > - printf("PCR #%u content (%u known updates):\n", index, updates); > > > > > > > > > - print_byte_string(data, TPM2_DIGEST_LEN); > > > > > > > > > + printf("PCR #%u %s %d byte content (%u known updates):\n", index, > > > > > > > > > + tpm2_algorithm_name(algo), algo_len, updates); > > > > > > > > > + print_byte_string(data, algo_len); > > > > > > > > > } > > > > > > > > > > > > > > > > > > unmap_sysmem(data); > > > > > > > > > @@ -415,14 +436,14 @@ U_BOOT_CMD(tpm2, CONFIG_SYS_MAXARGS, 1, do_tpm, "Issue a TPMv2.x command", > > > > > > > > > " <hierarchy> is one of:\n" > > > > > > > > > " * TPM2_RH_LOCKOUT\n" > > > > > > > > > " * TPM2_RH_PLATFORM\n" > > > > > > > > > -"pcr_extend <pcr> <digest_addr>\n" > > > > > > > > > -" Extend PCR #<pcr> with digest at <digest_addr>.\n" > > > > > > > > > +"pcr_extend <pcr> <digest_addr> [<digest_algo>]\n" > > > > > > > > > +" Extend PCR #<pcr> with digest at <digest_addr> with digest_algo.\n" > > > > > > > > > " <pcr>: index of the PCR\n" > > > > > > > > > -" <digest_addr>: address of a 32-byte SHA256 digest\n" > > > > > > > > > -"pcr_read <pcr> <digest_addr>\n" > > > > > > > > > -" Read PCR #<pcr> to memory address <digest_addr>.\n" > > > > > > > > > +" <digest_addr>: address of digest of digest_algo type (defaults to SHA256)\n" > > > > > > > > > +"pcr_read <pcr> <digest_addr> [<digest_algo>]\n" > > > > > > > > > +" Read PCR #<pcr> to memory address <digest_addr> with <digest_algo>.\n" > > > > > > > > > " <pcr>: index of the PCR\n" > > > > > > > > > -" <digest_addr>: address to store the a 32-byte SHA256 digest\n" > > > > > > > > > +" <digest_addr>: address of digest of digest_algo type (defaults to SHA256)\n" > > > > > > > > > "get_capability <capability> <property> <addr> <count>\n" > > > > > > > > > " Read and display <count> entries indexed by <capability>/<property>.\n" > > > > > > > > > " Values are 4 bytes long and are written at <addr>.\n" > > > > > > > > > diff --git a/include/tpm-v2.h b/include/tpm-v2.h > > > > > > > > > index 33dd103767c4..263f9529e55d 100644 > > > > > > > > > --- a/include/tpm-v2.h > > > > > > > > > +++ b/include/tpm-v2.h > > > > > > > > > @@ -386,7 +386,54 @@ enum tpm2_algorithms { > > > > > > > > > TPM2_ALG_SM3_256 = 0x12, > > > > > > > > > }; > > > > > > > > > > > > > > > > > > -extern const enum tpm2_algorithms tpm2_supported_algorithms[4]; > > > > > > > > > +/** > > > > > > > > > + * struct digest_info - details of supported digests > > > > > > > > > + * > > > > > > > > > + * @hash_name: hash name > > > > > > > > > + * @hash_alg: hash algorithm id > > > > > > > > > + * @hash_mask: hash registry mask > > > > > > > > > + * @hash_len: hash digest length > > > > > > > > > + */ > > > > > > > > > +struct digest_info { > > > > > > > > > + const char *hash_name; > > > > > > > > > + u16 hash_alg; > > > > > > > > > + u32 hash_mask; > > > > > > > > > + u16 hash_len; > > > > > > > > > +}; > > > > > > > > > + > > > > > > > > > +/* Algorithm Registry */ > > > > > > > > > +#define TCG2_BOOT_HASH_ALG_SHA1 0x00000001 > > > > > > > > > +#define TCG2_BOOT_HASH_ALG_SHA256 0x00000002 > > > > > > > > > +#define TCG2_BOOT_HASH_ALG_SHA384 0x00000004 > > > > > > > > > +#define TCG2_BOOT_HASH_ALG_SHA512 0x00000008 > > > > > > > > > +#define TCG2_BOOT_HASH_ALG_SM3_256 0x00000010 > > > > > > > > > + > > > > > > > > > +static const struct digest_info hash_algo_list[] = { > > > > > > > > > + { > > > > > > > > > + "sha1", > > > > > > > > > + TPM2_ALG_SHA1, > > > > > > > > > + TCG2_BOOT_HASH_ALG_SHA1, > > > > > > > > > + TPM2_SHA1_DIGEST_SIZE, > > > > > > > > > + }, > > > > > > > > > + { > > > > > > > > > + "sha256", > > > > > > > > > + TPM2_ALG_SHA256, > > > > > > > > > + TCG2_BOOT_HASH_ALG_SHA256, > > > > > > > > > + TPM2_SHA256_DIGEST_SIZE, > > > > > > > > > + }, > > > > > > > > > + { > > > > > > > > > + "sha384", > > > > > > > > > + TPM2_ALG_SHA384, > > > > > > > > > + TCG2_BOOT_HASH_ALG_SHA384, > > > > > > > > > + TPM2_SHA384_DIGEST_SIZE, > > > > > > > > > + }, > > > > > > > > > + { > > > > > > > > > + "sha512", > > > > > > > > > + TPM2_ALG_SHA512, > > > > > > > > > + TCG2_BOOT_HASH_ALG_SHA512, > > > > > > > > > + TPM2_SHA512_DIGEST_SIZE, > > > > > > > > > + }, > > > > > > > > > +}; > > > > > > > > > > > > > > > > > > static inline u16 tpm2_algorithm_to_len(enum tpm2_algorithms a) > > > > > > > > > { > > > > > > > > > @@ -965,4 +1012,22 @@ u32 tpm2_enable_nvcommits(struct udevice *dev, uint vendor_cmd, > > > > > > > > > */ > > > > > > > > > u32 tpm2_auto_start(struct udevice *dev); > > > > > > > > > > > > > > > > > > +/** > > > > > > > > > + * tpm2_name_to_algorithm() - Return an algorithm id given a supported > > > > > > > > > + * algorithm name > > > > > > > > > + * > > > > > > > > > + * @name: algorithm name > > > > > > > > > + * Return: enum tpm2_algorithms or -EINVAL > > > > > > > > > + */ > > > > > > > > > +enum tpm2_algorithms tpm2_name_to_algorithm(const char *name); > > > > > > > > > + > > > > > > > > > +/** > > > > > > > > > + * tpm2_algorithm_name() - Return an algorithm name string for a > > > > > > > > > + * supported algorithm id > > > > > > > > > + * > > > > > > > > > + * @algorithm_id: algorithm defined in enum tpm2_algorithms > > > > > > > > > + * Return: algorithm name string or "" > > > > > > > > > + */ > > > > > > > > > +const char *tpm2_algorithm_name(enum tpm2_algorithms); > > > > > > > > > + > > > > > > > > > #endif /* __TPM_V2_H */ > > > > > > > > > diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c > > > > > > > > > index b07e0099c27e..cf5d8de018a7 100644 > > > > > > > > > --- a/lib/efi_loader/efi_tcg2.c > > > > > > > > > +++ b/lib/efi_loader/efi_tcg2.c > > > > > > > > > @@ -414,8 +414,8 @@ static efi_status_t tcg2_hash_pe_image(void *efi, u64 efi_size, > > > > > > > > > } > > > > > > > > > > > > > > > > > > digest_list->count = 0; > > > > > > > > > - for (i = 0; i < ARRAY_SIZE(tpm2_supported_algorithms); i++) { > > > > > > > > > - u16 hash_alg = tpm2_supported_algorithms[i]; > > > > > > > > > + for (i = 0; i < ARRAY_SIZE(hash_algo_list); i++) { > > > > > > > > > + u16 hash_alg = hash_algo_list[i].hash_alg; > > > > > > > > > > > > > > > > > > if (!(active & tpm2_algorithm_to_mask(hash_alg))) > > > > > > > > > continue; > > > > > > > > > diff --git a/lib/tpm-v2.c b/lib/tpm-v2.c > > > > > > > > > index 68eaaa639f89..83f490c82541 100644 > > > > > > > > > --- a/lib/tpm-v2.c > > > > > > > > > +++ b/lib/tpm-v2.c > > > > > > > > > @@ -22,13 +22,6 @@ > > > > > > > > > > > > > > > > > > #include "tpm-utils.h" > > > > > > > > > > > > > > > > > > -const enum tpm2_algorithms tpm2_supported_algorithms[4] = { > > > > > > > > > - TPM2_ALG_SHA1, > > > > > > > > > - , > > > > > > > > > - TPM2_ALG_SHA384, > > > > > > > > > - TPM2_ALG_SHA512, > > > > > > > > > -}; > > > > > > > > > > The current tpm2_algorithm_to_mask() operates on those values and bits > > > > > shifts based on the enum. Since you remove the enum above, you must > > > > > also change the function implementation and return > > > > > hash_algo_list.hash_mask > > > > > > > > > > > > > That's already taken care of as the calls to tpm2_algorithm_to_mask() > > > > in my patch were adjusted from: > > > > tpm2_algorithm_to_mask(tpm2_supported_algorithms[i]) > > > > to > > > > tpm2_algorithm_to_mask(hash_algo_list[i].hash_alg) > > > > > > > > The hash_alg is the value from the previous enum. > > > > > > The previous enums were defining TPM2_ALG_SHA1=1, TPM2_ALG_SHA256=2 etc > > > The current values are different. On top of that the .hash_mask entry > > > of digest_info is never used. So you need to change > > > tpm2_algorithm_to_mask() and directly return the hash_mask > > > > > > > Hi Ilias, > > > > Just getting back to this. I don't agree with your assessment. > > > > The previous enum: > > -const enum tpm2_algorithms tpm2_supported_algorithms[4] = { > > - TPM2_ALG_SHA1, > > - TPM2_ALG_SHA256, > > - TPM2_ALG_SHA384, > > - TPM2_ALG_SHA512, > > -}; > > > > is defining 4 values which equate to 0x04, 0x0B, 0x0C, 0x0D (not 1, 2, > > 3, 4 as you say) which is what I'm putting in the hash_alg field. The > > tpm2_algorithm_to_mask macro shifts those values (not the index into > > the enum). > > > > So with the current code: > > for (i = 0; i < ARRAY_SIZE(tpm2_supported_algorithms); i++) { > > int alg = tpm2_supported_algorithms[i]; > > printf("alg:0x%02x mask=0x%02x\n", alg, tpm2_algorithm_to_mask(alg)); > > } > > alg:0x04 mask=0x10 > > alg:0x0b mask=0x800 > > alg:0x0c mask=0x1000 > > alg:0x0d mask=0x2000 > > > > With this proposed patch: > > for (i = 0; i < ARRAY_SIZE(hash_algo_list); i++) { > > int alg = hash_algo_list[i].hash_alg; > > printf("alg:0x%02x mask=0x%02x %s\n", alg, > > tpm2_algorithm_to_mask(alg), tpm2_algorithm_name(alg)); > > } > > alg:0x04 mask=0x10 sha1 > > alg:0x0b mask=0x800 sha256 > > alg:0x0c mask=0x1000 sha384 > > alg:0x0d mask=0x2000 sha512 > > > > Am I misunderstanding something else you are pointing out? > > > > Maybe it would be easier to compare if I named the new struct > > tpm2_supported_algorithms? > > Ok, I think we are looking at a preexisting bug, because the prints > above make no sense to me. > > There was an enum in the current code > const enum tpm2_algorithms tpm2_supported_algorithms[4] = { > TPM2_ALG_SHA1, > TPM2_ALG_SHA256, > TPM2_ALG_SHA384, > TPM2_ALG_SHA512, > } > But those values are defined in include/tpm-v2.h hence the enum values > were not starting from 0... > > Reading at the TCG spec [0] we should be adding any of the values > defined below to HashAlgorithmBitMap. > #define EFI_TCG2_BOOT_HASH_ALG_SHA1 0x00000001 > #define EFI_TCG2_BOOT_HASH_ALG_SHA256 0x00000002 > #define EFI_TCG2_BOOT_HASH_ALG_SHA384 0x00000004 > #define EFI_TCG2_BOOT_HASH_ALG_SHA512 0x00000008 > #define EFI_TCG2_BOOT_HASH_ALG_SM3_256 0x00000010 > > We used to do that in v2023.01. The patch below is on top of 2023.01 > > diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c > index a525ebf75b58..7371c9e94b5c 100644 > --- a/lib/efi_loader/efi_tcg2.c > +++ b/lib/efi_loader/efi_tcg2.c > @@ -1628,6 +1628,7 @@ static efi_status_t create_specid_event(struct > udevice *dev, void *buffer, > u16 hash_alg = hash_algo_list[i].hash_alg; > u16 hash_len = hash_algo_list[i].hash_len; > > + printf("Trying to add %08x and %08x\n", hash_alg, > alg_to_mask(hash_alg)); > if (active & alg_to_mask(hash_alg)) { > put_unaligned_le16(hash_alg, > > &spec_event->digest_sizes[alg_count].algorithm_id); > > prints > > Trying to add 00000004 and 00000001 > Trying to add 0000000b and 00000002 > Trying to add 0000000c and 00000004 > Trying to add 0000000d and 00000008 > > The values of 0x00000001, 0x00000002 etc are what's added to the eventlog > > But since 97707f12fdabf we are putting different values (which I think > is wrong and not what the spec expects....) > I also think Eddie intended to make tpm2_supported_algorithms an enum > that starts from 0, but the values he used were already defined and > that's how we missed it ... > > Eddie any idea if that's what happened? > > Tim, we will need to fix all this regardless. The fix should be > relatively simple, just return the newly added values instead of the > algorithm_to_mask value, when adding it on the eventlog. > Can you take a look at the spec and verify what I am seeing? At some > point, we also need to add the value checking in self-tests, rather > than only looking at the EFI return code. Apologies for the noise, I wasn't remembering the spec properly. The hash values are only supposed to be used on getting the capability and active PCR banks. Let me read this patch again Thanks /Ilias > > > [0] 6.4.3 Related Definitions > https://trustedcomputinggroup.org/wp-content/uploads/EFI-Protocol-Specification-rev13-160330final.pdf > > Thanks > /Ilias > > > > > > > Best Regards, > > > > Tim > > > > > Cheers > > > /Ilias > > > > > > > > Best Regards, > > > > > > > > Tim > > > > > > > > > Cheers > > > > > /Ilias > > > > > > > > > > > > > > > > > > > > > > > > - > > > > > > > > > int tcg2_get_active_pcr_banks(struct udevice *dev, u32 *active_pcr_banks) > > > > > > > > > { > > > > > > > > > u32 supported = 0; > > > > > > > > > @@ -82,14 +75,14 @@ int tcg2_create_digest(struct udevice *dev, const u8 *input, u32 length, > > > > > > > > > return rc; > > > > > > > > > > > > > > > > > > digest_list->count = 0; > > > > > > > > > - for (i = 0; i < ARRAY_SIZE(tpm2_supported_algorithms); ++i) { > > > > > > > > > + for (i = 0; i < ARRAY_SIZE(hash_algo_list); ++i) { > > > > > > > > > u32 mask = > > > > > > > > > - tpm2_algorithm_to_mask(tpm2_supported_algorithms[i]); > > > > > > > > > + tpm2_algorithm_to_mask(hash_algo_list[i].hash_alg); > > > > > > > > > > > > > > > > > > if (!(active & mask))
Hi Tim, > > > > > > > > > > > > > > > > > > > > -const enum tpm2_algorithms tpm2_supported_algorithms[4] = { > > > > > > > > > > - TPM2_ALG_SHA1, > > > > > > > > > > - , > > > > > > > > > > - TPM2_ALG_SHA384, > > > > > > > > > > - TPM2_ALG_SHA512, > > > > > > > > > > -}; > > > > > > > > > > > > The current tpm2_algorithm_to_mask() operates on those values and bits > > > > > > shifts based on the enum. Since you remove the enum above, you must > > > > > > also change the function implementation and return > > > > > > hash_algo_list.hash_mask > > > > > > > > > > > > > > > > That's already taken care of as the calls to tpm2_algorithm_to_mask() > > > > > in my patch were adjusted from: > > > > > tpm2_algorithm_to_mask(tpm2_supported_algorithms[i]) > > > > > to > > > > > tpm2_algorithm_to_mask(hash_algo_list[i].hash_alg) > > > > > > > > > > The hash_alg is the value from the previous enum. > > > > > > > > The previous enums were defining TPM2_ALG_SHA1=1, TPM2_ALG_SHA256=2 etc > > > > The current values are different. On top of that the .hash_mask entry > > > > of digest_info is never used. So you need to change > > > > tpm2_algorithm_to_mask() and directly return the hash_mask > > > > > > > > > > Hi Ilias, > > > > > > Just getting back to this. I don't agree with your assessment. > > > > > > The previous enum: > > > -const enum tpm2_algorithms tpm2_supported_algorithms[4] = { > > > - TPM2_ALG_SHA1, > > > - TPM2_ALG_SHA256, > > > - TPM2_ALG_SHA384, > > > - TPM2_ALG_SHA512, > > > -}; > > > > > > is defining 4 values which equate to 0x04, 0x0B, 0x0C, 0x0D (not 1, 2, > > > 3, 4 as you say) which is what I'm putting in the hash_alg field. The > > > tpm2_algorithm_to_mask macro shifts those values (not the index into > > > the enum). > > > > > > So with the current code: > > > for (i = 0; i < ARRAY_SIZE(tpm2_supported_algorithms); i++) { > > > int alg = tpm2_supported_algorithms[i]; > > > printf("alg:0x%02x mask=0x%02x\n", alg, tpm2_algorithm_to_mask(alg)); > > > } > > > alg:0x04 mask=0x10 > > > alg:0x0b mask=0x800 > > > alg:0x0c mask=0x1000 > > > alg:0x0d mask=0x2000 > > > > > > With this proposed patch: > > > for (i = 0; i < ARRAY_SIZE(hash_algo_list); i++) { > > > int alg = hash_algo_list[i].hash_alg; > > > printf("alg:0x%02x mask=0x%02x %s\n", alg, > > > tpm2_algorithm_to_mask(alg), tpm2_algorithm_name(alg)); > > > } > > > alg:0x04 mask=0x10 sha1 > > > alg:0x0b mask=0x800 sha256 > > > alg:0x0c mask=0x1000 sha384 > > > alg:0x0d mask=0x2000 sha512 > > > > > > Am I misunderstanding something else you are pointing out? > > > > > > Maybe it would be easier to compare if I named the new struct > > > tpm2_supported_algorithms? > > > > Ok, I think we are looking at a preexisting bug, because the prints > > above make no sense to me. > > > > There was an enum in the current code > > const enum tpm2_algorithms tpm2_supported_algorithms[4] = { > > TPM2_ALG_SHA1, > > TPM2_ALG_SHA256, > > TPM2_ALG_SHA384, > > TPM2_ALG_SHA512, > > } > > But those values are defined in include/tpm-v2.h hence the enum values > > were not starting from 0... > > > > Reading at the TCG spec [0] we should be adding any of the values > > defined below to HashAlgorithmBitMap. > > #define EFI_TCG2_BOOT_HASH_ALG_SHA1 0x00000001 > > #define EFI_TCG2_BOOT_HASH_ALG_SHA256 0x00000002 > > #define EFI_TCG2_BOOT_HASH_ALG_SHA384 0x00000004 > > #define EFI_TCG2_BOOT_HASH_ALG_SHA512 0x00000008 > > #define EFI_TCG2_BOOT_HASH_ALG_SM3_256 0x00000010 > > > > We used to do that in v2023.01. The patch below is on top of 2023.01 > > > > diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c > > index a525ebf75b58..7371c9e94b5c 100644 > > --- a/lib/efi_loader/efi_tcg2.c > > +++ b/lib/efi_loader/efi_tcg2.c > > @@ -1628,6 +1628,7 @@ static efi_status_t create_specid_event(struct > > udevice *dev, void *buffer, > > u16 hash_alg = hash_algo_list[i].hash_alg; > > u16 hash_len = hash_algo_list[i].hash_len; > > > > + printf("Trying to add %08x and %08x\n", hash_alg, > > alg_to_mask(hash_alg)); > > if (active & alg_to_mask(hash_alg)) { > > put_unaligned_le16(hash_alg, > > > > &spec_event->digest_sizes[alg_count].algorithm_id); > > > > prints > > > > Trying to add 00000004 and 00000001 > > Trying to add 0000000b and 00000002 > > Trying to add 0000000c and 00000004 > > Trying to add 0000000d and 00000008 > > > > The values of 0x00000001, 0x00000002 etc are what's added to the eventlog > > > > But since 97707f12fdabf we are putting different values (which I think > > is wrong and not what the spec expects....) > > I also think Eddie intended to make tpm2_supported_algorithms an enum > > that starts from 0, but the values he used were already defined and > > that's how we missed it ... > > > > Eddie any idea if that's what happened? > > > > Tim, we will need to fix all this regardless. The fix should be > > relatively simple, just return the newly added values instead of the > > algorithm_to_mask value, when adding it on the eventlog. > > Can you take a look at the spec and verify what I am seeing? At some > > point, we also need to add the value checking in self-tests, rather > > than only looking at the EFI return code. > > Apologies for the noise, I wasn't remembering the spec properly. > The hash values are only supposed to be used on getting the capability > and active PCR banks. > > Let me read this patch again Ok, I figured it out. The bug is indeed there, but it's while reading and interpreting the TPM capabilities, instead of writing those. What I wrote above is still relevant and I still think Eddie wanted to define the enum as 0,1,2, etc instead of the values that are there. A simple way to reproduce the bug is checkout v2023.01 and add this patch: diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c index a525ebf75b58..b06dd47575d3 100644 --- a/lib/efi_loader/efi_tcg2.c +++ b/lib/efi_loader/efi_tcg2.c @@ -617,6 +617,7 @@ static int tpm2_get_pcr_info(struct udevice *dev, u32 *supported_pcr, } *pcr_banks = pcrs.count; + printf("ACTIVE BANKS 0x%08x\n", *active_pcr); return 0; out: If you load u-boot and initialize the TPM (e.g do a printenv -e), you'll get prints looking like this: ACTIVE BANKS 0x00000003 when sha1 and sha256 banks are enabled If you apply a similar patch to -master the print looks like this (which doesn't match the TCG spec) ACTIVE BANKS 0x00000810 Now I understand the bug is completely irrelevant to your patches, but I can't pick it up before we fix it properly. I can either send a fix here, but it will take me some time to find free cycles, or you can send it as a prerequisite for your patches. That new patch will use the new TCG2_BOOT_HASH_ALG_SHA1/XXX you added and fix the algorithm checking before adding any capabilites on the cmd line. Let me know what you prefer Thanks /Ilias > > Thanks > /Ilias > > > > > > [0] 6.4.3 Related Definitions > > https://trustedcomputinggroup.org/wp-content/uploads/EFI-Protocol-Specification-rev13-160330final.pdf > > > > Thanks > > /Ilias > > > > > > > > > > > > Best Regards, > > > > > > Tim > > > > > > > Cheers > > > > /Ilias > > > > > > > > > > Best Regards, > > > > > > > > > > Tim > > > > > > > > > > > Cheers > > > > > > /Ilias > > > > > > > > > > > > > > > > > > > > > > > > > > > > - > > > > > > > > > > int tcg2_get_active_pcr_banks(struct udevice *dev, u32 *active_pcr_banks) > > > > > > > > > > { > > > > > > > > > > u32 supported = 0; > > > > > > > > > > @@ -82,14 +75,14 @@ int tcg2_create_digest(struct udevice *dev, const u8 *input, u32 length, > > > > > > > > > > return rc; > > > > > > > > > > > > > > > > > > > > digest_list->count = 0; > > > > > > > > > > - for (i = 0; i < ARRAY_SIZE(tpm2_supported_algorithms); ++i) { > > > > > > > > > > + for (i = 0; i < ARRAY_SIZE(hash_algo_list); ++i) { > > > > > > > > > > u32 mask = > > > > > > > > > > - tpm2_algorithm_to_mask(tpm2_supported_algorithms[i]); > > > > > > > > > > + tpm2_algorithm_to_mask(hash_algo_list[i].hash_alg); > > > > > > > > > > > > > > > > > > > > if (!(active & mask))
top posting but if it makes your life easier this would probably work
on top of your patch (compile tested only).
We ofc need to make proper patches and merge this first, but it will
probably help you understand what's wrong with the current code
diff --git a/include/tpm-v2.h b/include/tpm-v2.h
index 263f9529e55d..a1f8303523b9 100644
--- a/include/tpm-v2.h
+++ b/include/tpm-v2.h
@@ -434,6 +434,9 @@ static const struct digest_info hash_algo_list[] = {
TPM2_SHA512_DIGEST_SIZE,
},
};
+#define MAX_HASH_COUNT ARRAY_SIZE(hash_algo_list)
+
+u32 tpm2_algorithm_to_mask(u16 hash_alg);
static inline u16 tpm2_algorithm_to_len(enum tpm2_algorithms a)
{
@@ -451,8 +454,6 @@ static inline u16 tpm2_algorithm_to_len(enum
tpm2_algorithms a)
}
}
-#define tpm2_algorithm_to_mask(a) (1 << (a))
-
/* NV index attributes */
enum tpm_index_attrs {
TPMA_NV_PPWRITE = 1UL << 0,
diff --git a/lib/tpm-v2.c b/lib/tpm-v2.c
index 83f490c82541..0b27d70a4144 100644
--- a/lib/tpm-v2.c
+++ b/lib/tpm-v2.c
@@ -22,6 +22,26 @@
#include "tpm-utils.h"
+
+/**
+ * tpm2_algorithm_to_mask - Get a TCG hash mask for algorithms
+ *
+ * @hash_alg: TCG defined algorithm
+ *
+ * @Return: TCG hashing algorithm bitmaps, 0 if the algorithm is not supported
+ */
+u32 tpm2_algorithm_to_mask(u16 hash_alg)
+{
+ size_t i;
+
+ for (i = 0; i < MAX_HASH_COUNT; i++) {
+ if (hash_algo_list[i].hash_alg == hash_alg)
+ return hash_algo_list[i].hash_mask;
+ }
+
+ return 0;
+}
+
int tcg2_get_active_pcr_banks(struct udevice *dev, u32 *active_pcr_banks)
{
u32 supported = 0;
On Tue, 21 May 2024 at 15:31, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Tim,
>
> > > > > > > > > > >
> > > > > > > > > > > -const enum tpm2_algorithms tpm2_supported_algorithms[4] = {
> > > > > > > > > > > - TPM2_ALG_SHA1,
> > > > > > > > > > > - ,
> > > > > > > > > > > - TPM2_ALG_SHA384,
> > > > > > > > > > > - TPM2_ALG_SHA512,
> > > > > > > > > > > -};
> > > > > > >
> > > > > > > The current tpm2_algorithm_to_mask() operates on those values and bits
> > > > > > > shifts based on the enum. Since you remove the enum above, you must
> > > > > > > also change the function implementation and return
> > > > > > > hash_algo_list.hash_mask
> > > > > > >
> > > > > >
> > > > > > That's already taken care of as the calls to tpm2_algorithm_to_mask()
> > > > > > in my patch were adjusted from:
> > > > > > tpm2_algorithm_to_mask(tpm2_supported_algorithms[i])
> > > > > > to
> > > > > > tpm2_algorithm_to_mask(hash_algo_list[i].hash_alg)
> > > > > >
> > > > > > The hash_alg is the value from the previous enum.
> > > > >
> > > > > The previous enums were defining TPM2_ALG_SHA1=1, TPM2_ALG_SHA256=2 etc
> > > > > The current values are different. On top of that the .hash_mask entry
> > > > > of digest_info is never used. So you need to change
> > > > > tpm2_algorithm_to_mask() and directly return the hash_mask
> > > > >
> > > >
> > > > Hi Ilias,
> > > >
> > > > Just getting back to this. I don't agree with your assessment.
> > > >
> > > > The previous enum:
> > > > -const enum tpm2_algorithms tpm2_supported_algorithms[4] = {
> > > > - TPM2_ALG_SHA1,
> > > > - TPM2_ALG_SHA256,
> > > > - TPM2_ALG_SHA384,
> > > > - TPM2_ALG_SHA512,
> > > > -};
> > > >
> > > > is defining 4 values which equate to 0x04, 0x0B, 0x0C, 0x0D (not 1, 2,
> > > > 3, 4 as you say) which is what I'm putting in the hash_alg field. The
> > > > tpm2_algorithm_to_mask macro shifts those values (not the index into
> > > > the enum).
> > > >
> > > > So with the current code:
> > > > for (i = 0; i < ARRAY_SIZE(tpm2_supported_algorithms); i++) {
> > > > int alg = tpm2_supported_algorithms[i];
> > > > printf("alg:0x%02x mask=0x%02x\n", alg, tpm2_algorithm_to_mask(alg));
> > > > }
> > > > alg:0x04 mask=0x10
> > > > alg:0x0b mask=0x800
> > > > alg:0x0c mask=0x1000
> > > > alg:0x0d mask=0x2000
> > > >
> > > > With this proposed patch:
> > > > for (i = 0; i < ARRAY_SIZE(hash_algo_list); i++) {
> > > > int alg = hash_algo_list[i].hash_alg;
> > > > printf("alg:0x%02x mask=0x%02x %s\n", alg,
> > > > tpm2_algorithm_to_mask(alg), tpm2_algorithm_name(alg));
> > > > }
> > > > alg:0x04 mask=0x10 sha1
> > > > alg:0x0b mask=0x800 sha256
> > > > alg:0x0c mask=0x1000 sha384
> > > > alg:0x0d mask=0x2000 sha512
> > > >
> > > > Am I misunderstanding something else you are pointing out?
> > > >
> > > > Maybe it would be easier to compare if I named the new struct
> > > > tpm2_supported_algorithms?
> > >
> > > Ok, I think we are looking at a preexisting bug, because the prints
> > > above make no sense to me.
> > >
> > > There was an enum in the current code
> > > const enum tpm2_algorithms tpm2_supported_algorithms[4] = {
> > > TPM2_ALG_SHA1,
> > > TPM2_ALG_SHA256,
> > > TPM2_ALG_SHA384,
> > > TPM2_ALG_SHA512,
> > > }
> > > But those values are defined in include/tpm-v2.h hence the enum values
> > > were not starting from 0...
> > >
> > > Reading at the TCG spec [0] we should be adding any of the values
> > > defined below to HashAlgorithmBitMap.
> > > #define EFI_TCG2_BOOT_HASH_ALG_SHA1 0x00000001
> > > #define EFI_TCG2_BOOT_HASH_ALG_SHA256 0x00000002
> > > #define EFI_TCG2_BOOT_HASH_ALG_SHA384 0x00000004
> > > #define EFI_TCG2_BOOT_HASH_ALG_SHA512 0x00000008
> > > #define EFI_TCG2_BOOT_HASH_ALG_SM3_256 0x00000010
> > >
> > > We used to do that in v2023.01. The patch below is on top of 2023.01
> > >
> > > diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
> > > index a525ebf75b58..7371c9e94b5c 100644
> > > --- a/lib/efi_loader/efi_tcg2.c
> > > +++ b/lib/efi_loader/efi_tcg2.c
> > > @@ -1628,6 +1628,7 @@ static efi_status_t create_specid_event(struct
> > > udevice *dev, void *buffer,
> > > u16 hash_alg = hash_algo_list[i].hash_alg;
> > > u16 hash_len = hash_algo_list[i].hash_len;
> > >
> > > + printf("Trying to add %08x and %08x\n", hash_alg,
> > > alg_to_mask(hash_alg));
> > > if (active & alg_to_mask(hash_alg)) {
> > > put_unaligned_le16(hash_alg,
> > >
> > > &spec_event->digest_sizes[alg_count].algorithm_id);
> > >
> > > prints
> > >
> > > Trying to add 00000004 and 00000001
> > > Trying to add 0000000b and 00000002
> > > Trying to add 0000000c and 00000004
> > > Trying to add 0000000d and 00000008
> > >
> > > The values of 0x00000001, 0x00000002 etc are what's added to the eventlog
> > >
> > > But since 97707f12fdabf we are putting different values (which I think
> > > is wrong and not what the spec expects....)
> > > I also think Eddie intended to make tpm2_supported_algorithms an enum
> > > that starts from 0, but the values he used were already defined and
> > > that's how we missed it ...
> > >
> > > Eddie any idea if that's what happened?
> > >
> > > Tim, we will need to fix all this regardless. The fix should be
> > > relatively simple, just return the newly added values instead of the
> > > algorithm_to_mask value, when adding it on the eventlog.
> > > Can you take a look at the spec and verify what I am seeing? At some
> > > point, we also need to add the value checking in self-tests, rather
> > > than only looking at the EFI return code.
> >
> > Apologies for the noise, I wasn't remembering the spec properly.
> > The hash values are only supposed to be used on getting the capability
> > and active PCR banks.
> >
> > Let me read this patch again
>
> Ok, I figured it out. The bug is indeed there, but it's while reading
> and interpreting the TPM capabilities, instead of writing those.
> What I wrote above is still relevant and I still think Eddie wanted to
> define the enum as 0,1,2, etc instead of the values that are there.
>
> A simple way to reproduce the bug is checkout v2023.01 and add this patch:
>
> diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
> index a525ebf75b58..b06dd47575d3 100644
> --- a/lib/efi_loader/efi_tcg2.c
> +++ b/lib/efi_loader/efi_tcg2.c
> @@ -617,6 +617,7 @@ static int tpm2_get_pcr_info(struct udevice *dev,
> u32 *supported_pcr,
> }
>
> *pcr_banks = pcrs.count;
> + printf("ACTIVE BANKS 0x%08x\n", *active_pcr);
>
> return 0;
> out:
>
> If you load u-boot and initialize the TPM (e.g do a printenv -e),
> you'll get prints looking like this:
> ACTIVE BANKS 0x00000003
> when sha1 and sha256 banks are enabled
>
> If you apply a similar patch to -master the print looks like this
> (which doesn't match the TCG spec)
> ACTIVE BANKS 0x00000810
>
> Now I understand the bug is completely irrelevant to your patches, but
> I can't pick it up before we fix it properly.
> I can either send a fix here, but it will take me some time to find
> free cycles, or you can send it as a prerequisite for your patches.
> That new patch will use the new TCG2_BOOT_HASH_ALG_SHA1/XXX you added
> and fix the algorithm checking before adding any capabilites on the
> cmd line.
>
> Let me know what you prefer
>
> Thanks
> /Ilias
>
> >
> > Thanks
> > /Ilias
> > >
> > >
> > > [0] 6.4.3 Related Definitions
> > > https://trustedcomputinggroup.org/wp-content/uploads/EFI-Protocol-Specification-rev13-160330final.pdf
> > >
> > > Thanks
> > > /Ilias
> > >
> > >
> > >
> > > >
> > > > Best Regards,
> > > >
> > > > Tim
> > > >
> > > > > Cheers
> > > > > /Ilias
> > > > > >
> > > > > > Best Regards,
> > > > > >
> > > > > > Tim
> > > > > >
> > > > > > > Cheers
> > > > > > > /Ilias
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > > > > > -
> > > > > > > > > > > int tcg2_get_active_pcr_banks(struct udevice *dev, u32 *active_pcr_banks)
> > > > > > > > > > > {
> > > > > > > > > > > u32 supported = 0;
> > > > > > > > > > > @@ -82,14 +75,14 @@ int tcg2_create_digest(struct udevice *dev, const u8 *input, u32 length,
> > > > > > > > > > > return rc;
> > > > > > > > > > >
> > > > > > > > > > > digest_list->count = 0;
> > > > > > > > > > > - for (i = 0; i < ARRAY_SIZE(tpm2_supported_algorithms); ++i) {
> > > > > > > > > > > + for (i = 0; i < ARRAY_SIZE(hash_algo_list); ++i) {
> > > > > > > > > > > u32 mask =
> > > > > > > > > > > - tpm2_algorithm_to_mask(tpm2_supported_algorithms[i]);
> > > > > > > > > > > + tpm2_algorithm_to_mask(hash_algo_list[i].hash_alg);
> > > > > > > > > > >
> > > > > > > > > > > if (!(active & mask))
diff --git a/cmd/tpm-v2.c b/cmd/tpm-v2.c index 7e479b9dfe36..2343b4d9cb9e 100644 --- a/cmd/tpm-v2.c +++ b/cmd/tpm-v2.c @@ -99,11 +99,19 @@ static int do_tpm2_pcr_extend(struct cmd_tbl *cmdtp, int flag, int argc, struct tpm_chip_priv *priv; u32 index = simple_strtoul(argv[1], NULL, 0); void *digest = map_sysmem(simple_strtoul(argv[2], NULL, 0), 0); + int algo = TPM2_ALG_SHA256; + int algo_len; int ret; u32 rc; - if (argc != 3) + if (argc < 3 || argc > 4) return CMD_RET_USAGE; + if (argc == 4) { + algo = tpm2_name_to_algorithm(argv[3]); + if (algo < 0) + return CMD_RET_FAILURE; + } + algo_len = tpm2_algorithm_to_len(algo); ret = get_tpm(&dev); if (ret) @@ -116,8 +124,12 @@ static int do_tpm2_pcr_extend(struct cmd_tbl *cmdtp, int flag, int argc, if (index >= priv->pcr_count) return -EINVAL; - rc = tpm2_pcr_extend(dev, index, TPM2_ALG_SHA256, digest, - TPM2_DIGEST_LEN); + rc = tpm2_pcr_extend(dev, index, algo, digest, algo_len); + if (!rc) { + printf("PCR #%u extended with %d byte %s digest\n", index, + algo_len, tpm2_algorithm_name(algo)); + print_byte_string(digest, algo_len); + } unmap_sysmem(digest); @@ -127,15 +139,23 @@ static int do_tpm2_pcr_extend(struct cmd_tbl *cmdtp, int flag, int argc, static int do_tpm_pcr_read(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) { + enum tpm2_algorithms algo = TPM2_ALG_SHA256; struct udevice *dev; struct tpm_chip_priv *priv; u32 index, rc; + int algo_len; unsigned int updates; void *data; int ret; - if (argc != 3) + if (argc < 3 || argc > 4) return CMD_RET_USAGE; + if (argc == 4) { + algo = tpm2_name_to_algorithm(argv[3]); + if (algo < 0) + return CMD_RET_FAILURE; + } + algo_len = tpm2_algorithm_to_len(algo); ret = get_tpm(&dev); if (ret) @@ -151,11 +171,12 @@ static int do_tpm_pcr_read(struct cmd_tbl *cmdtp, int flag, int argc, data = map_sysmem(simple_strtoul(argv[2], NULL, 0), 0); - rc = tpm2_pcr_read(dev, index, priv->pcr_select_min, TPM2_ALG_SHA256, - data, TPM2_DIGEST_LEN, &updates); + rc = tpm2_pcr_read(dev, index, priv->pcr_select_min, algo, + data, algo_len, &updates); if (!rc) { - printf("PCR #%u content (%u known updates):\n", index, updates); - print_byte_string(data, TPM2_DIGEST_LEN); + printf("PCR #%u %s %d byte content (%u known updates):\n", index, + tpm2_algorithm_name(algo), algo_len, updates); + print_byte_string(data, algo_len); } unmap_sysmem(data); @@ -415,14 +436,14 @@ U_BOOT_CMD(tpm2, CONFIG_SYS_MAXARGS, 1, do_tpm, "Issue a TPMv2.x command", " <hierarchy> is one of:\n" " * TPM2_RH_LOCKOUT\n" " * TPM2_RH_PLATFORM\n" -"pcr_extend <pcr> <digest_addr>\n" -" Extend PCR #<pcr> with digest at <digest_addr>.\n" +"pcr_extend <pcr> <digest_addr> [<digest_algo>]\n" +" Extend PCR #<pcr> with digest at <digest_addr> with digest_algo.\n" " <pcr>: index of the PCR\n" -" <digest_addr>: address of a 32-byte SHA256 digest\n" -"pcr_read <pcr> <digest_addr>\n" -" Read PCR #<pcr> to memory address <digest_addr>.\n" +" <digest_addr>: address of digest of digest_algo type (defaults to SHA256)\n" +"pcr_read <pcr> <digest_addr> [<digest_algo>]\n" +" Read PCR #<pcr> to memory address <digest_addr> with <digest_algo>.\n" " <pcr>: index of the PCR\n" -" <digest_addr>: address to store the a 32-byte SHA256 digest\n" +" <digest_addr>: address of digest of digest_algo type (defaults to SHA256)\n" "get_capability <capability> <property> <addr> <count>\n" " Read and display <count> entries indexed by <capability>/<property>.\n" " Values are 4 bytes long and are written at <addr>.\n" diff --git a/include/tpm-v2.h b/include/tpm-v2.h index 33dd103767c4..263f9529e55d 100644 --- a/include/tpm-v2.h +++ b/include/tpm-v2.h @@ -386,7 +386,54 @@ enum tpm2_algorithms { TPM2_ALG_SM3_256 = 0x12, }; -extern const enum tpm2_algorithms tpm2_supported_algorithms[4]; +/** + * struct digest_info - details of supported digests + * + * @hash_name: hash name + * @hash_alg: hash algorithm id + * @hash_mask: hash registry mask + * @hash_len: hash digest length + */ +struct digest_info { + const char *hash_name; + u16 hash_alg; + u32 hash_mask; + u16 hash_len; +}; + +/* Algorithm Registry */ +#define TCG2_BOOT_HASH_ALG_SHA1 0x00000001 +#define TCG2_BOOT_HASH_ALG_SHA256 0x00000002 +#define TCG2_BOOT_HASH_ALG_SHA384 0x00000004 +#define TCG2_BOOT_HASH_ALG_SHA512 0x00000008 +#define TCG2_BOOT_HASH_ALG_SM3_256 0x00000010 + +static const struct digest_info hash_algo_list[] = { + { + "sha1", + TPM2_ALG_SHA1, + TCG2_BOOT_HASH_ALG_SHA1, + TPM2_SHA1_DIGEST_SIZE, + }, + { + "sha256", + TPM2_ALG_SHA256, + TCG2_BOOT_HASH_ALG_SHA256, + TPM2_SHA256_DIGEST_SIZE, + }, + { + "sha384", + TPM2_ALG_SHA384, + TCG2_BOOT_HASH_ALG_SHA384, + TPM2_SHA384_DIGEST_SIZE, + }, + { + "sha512", + TPM2_ALG_SHA512, + TCG2_BOOT_HASH_ALG_SHA512, + TPM2_SHA512_DIGEST_SIZE, + }, +}; static inline u16 tpm2_algorithm_to_len(enum tpm2_algorithms a) { @@ -965,4 +1012,22 @@ u32 tpm2_enable_nvcommits(struct udevice *dev, uint vendor_cmd, */ u32 tpm2_auto_start(struct udevice *dev); +/** + * tpm2_name_to_algorithm() - Return an algorithm id given a supported + * algorithm name + * + * @name: algorithm name + * Return: enum tpm2_algorithms or -EINVAL + */ +enum tpm2_algorithms tpm2_name_to_algorithm(const char *name); + +/** + * tpm2_algorithm_name() - Return an algorithm name string for a + * supported algorithm id + * + * @algorithm_id: algorithm defined in enum tpm2_algorithms + * Return: algorithm name string or "" + */ +const char *tpm2_algorithm_name(enum tpm2_algorithms); + #endif /* __TPM_V2_H */ diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c index b07e0099c27e..cf5d8de018a7 100644 --- a/lib/efi_loader/efi_tcg2.c +++ b/lib/efi_loader/efi_tcg2.c @@ -414,8 +414,8 @@ static efi_status_t tcg2_hash_pe_image(void *efi, u64 efi_size, } digest_list->count = 0; - for (i = 0; i < ARRAY_SIZE(tpm2_supported_algorithms); i++) { - u16 hash_alg = tpm2_supported_algorithms[i]; + for (i = 0; i < ARRAY_SIZE(hash_algo_list); i++) { + u16 hash_alg = hash_algo_list[i].hash_alg; if (!(active & tpm2_algorithm_to_mask(hash_alg))) continue; diff --git a/lib/tpm-v2.c b/lib/tpm-v2.c index 68eaaa639f89..83f490c82541 100644 --- a/lib/tpm-v2.c +++ b/lib/tpm-v2.c @@ -22,13 +22,6 @@ #include "tpm-utils.h" -const enum tpm2_algorithms tpm2_supported_algorithms[4] = { - TPM2_ALG_SHA1, - TPM2_ALG_SHA256, - TPM2_ALG_SHA384, - TPM2_ALG_SHA512, -}; - int tcg2_get_active_pcr_banks(struct udevice *dev, u32 *active_pcr_banks) { u32 supported = 0; @@ -82,14 +75,14 @@ int tcg2_create_digest(struct udevice *dev, const u8 *input, u32 length, return rc; digest_list->count = 0; - for (i = 0; i < ARRAY_SIZE(tpm2_supported_algorithms); ++i) { + for (i = 0; i < ARRAY_SIZE(hash_algo_list); ++i) { u32 mask = - tpm2_algorithm_to_mask(tpm2_supported_algorithms[i]); + tpm2_algorithm_to_mask(hash_algo_list[i].hash_alg); if (!(active & mask)) continue; - switch (tpm2_supported_algorithms[i]) { + switch (hash_algo_list[i].hash_alg) { case TPM2_ALG_SHA1: sha1_starts(&ctx); sha1_update(&ctx, input, length); @@ -116,12 +109,12 @@ int tcg2_create_digest(struct udevice *dev, const u8 *input, u32 length, break; default: printf("%s: unsupported algorithm %x\n", __func__, - tpm2_supported_algorithms[i]); + hash_algo_list[i].hash_alg); continue; } digest_list->digests[digest_list->count].hash_alg = - tpm2_supported_algorithms[i]; + hash_algo_list[i].hash_alg; memcpy(&digest_list->digests[digest_list->count].digest, final, len); digest_list->count++; @@ -208,13 +201,13 @@ static int tcg2_log_init(struct udevice *dev, struct tcg2_event_log *elog) return rc; event_size = offsetof(struct tcg_efi_spec_id_event, digest_sizes); - for (i = 0; i < ARRAY_SIZE(tpm2_supported_algorithms); ++i) { - mask = tpm2_algorithm_to_mask(tpm2_supported_algorithms[i]); + for (i = 0; i < ARRAY_SIZE(hash_algo_list); ++i) { + mask = tpm2_algorithm_to_mask(hash_algo_list[i].hash_alg); if (!(active & mask)) continue; - switch (tpm2_supported_algorithms[i]) { + switch (hash_algo_list[i].hash_alg) { case TPM2_ALG_SHA1: case TPM2_ALG_SHA256: case TPM2_ALG_SHA384: @@ -253,17 +246,17 @@ static int tcg2_log_init(struct udevice *dev, struct tcg2_event_log *elog) put_unaligned_le32(count, &ev->number_of_algorithms); count = 0; - for (i = 0; i < ARRAY_SIZE(tpm2_supported_algorithms); ++i) { - mask = tpm2_algorithm_to_mask(tpm2_supported_algorithms[i]); + for (i = 0; i < ARRAY_SIZE(hash_algo_list); ++i) { + mask = tpm2_algorithm_to_mask(hash_algo_list[i].hash_alg); if (!(active & mask)) continue; - len = tpm2_algorithm_to_len(tpm2_supported_algorithms[i]); + len = hash_algo_list[i].hash_len; if (!len) continue; - put_unaligned_le16(tpm2_supported_algorithms[i], + put_unaligned_le16(hash_algo_list[i].hash_alg, &ev->digest_sizes[count].algorithm_id); put_unaligned_le16(len, &ev->digest_sizes[count].digest_size); count++; @@ -304,7 +297,7 @@ static int tcg2_replay_eventlog(struct tcg2_event_log *elog, pos = offsetof(struct tcg_pcr_event2, digests) + offsetof(struct tpml_digest_values, count); count = get_unaligned_le32(log + pos); - if (count > ARRAY_SIZE(tpm2_supported_algorithms) || + if (count > ARRAY_SIZE(hash_algo_list) || (digest_list->count && digest_list->count != count)) return 0; @@ -407,7 +400,7 @@ static int tcg2_log_parse(struct udevice *dev, struct tcg2_event_log *elog) return 0; count = get_unaligned_le32(&event->number_of_algorithms); - if (count > ARRAY_SIZE(tpm2_supported_algorithms)) + if (count > ARRAY_SIZE(hash_algo_list)) return 0; calc_size = offsetof(struct tcg_efi_spec_id_event, digest_sizes) + @@ -1110,7 +1103,7 @@ int tpm2_get_pcr_info(struct udevice *dev, u32 *supported_pcr, u32 *active_pcr, * We only support 5 algorithms for now so check against that * instead of TPM2_NUM_PCR_BANKS */ - if (pcrs.count > ARRAY_SIZE(tpm2_supported_algorithms) || + if (pcrs.count > ARRAY_SIZE(hash_algo_list) || pcrs.count < 1) { printf("%s: too many pcrs: %u\n", __func__, pcrs.count); return -EMSGSIZE; @@ -1555,3 +1548,28 @@ u32 tpm2_enable_nvcommits(struct udevice *dev, uint vendor_cmd, return 0; } + +enum tpm2_algorithms tpm2_name_to_algorithm(const char *name) +{ + size_t i; + + for (i = 0; i < ARRAY_SIZE(hash_algo_list); ++i) { + if (!strcasecmp(name, hash_algo_list[i].hash_name)) + return hash_algo_list[i].hash_alg; + } + printf("%s: unsupported algorithm %s\n", __func__, name); + + return -EINVAL; +} + +const char *tpm2_algorithm_name(enum tpm2_algorithms algo) +{ + size_t i; + + for (i = 0; i < ARRAY_SIZE(hash_algo_list); ++i) { + if (hash_algo_list[i].hash_alg == algo) + return hash_algo_list[i].hash_name; + } + + return ""; +}
For pcr_read and pcr_extend commands allow the digest algorithm to be specified by an additional argument. If not specified it will default to SHA256 for backwards compatibility. A follow-on to this could be to extend all PCR banks with the detected algo when the <digest_algo> argument is 'auto'. Signed-off-by: Tim Harvey <tharvey@gateworks.com> --- v3: - replace tpm2_supported_algorithms with struct and use this to relate hash algoirthm details v2: - use tpm2_algorithm_to_len - use enum tpm2_algorithms - make function names and parameter names more consistent with existing tpm-v2 functions - fix various spelling errors --- cmd/tpm-v2.c | 49 ++++++++++++++++++++-------- include/tpm-v2.h | 67 ++++++++++++++++++++++++++++++++++++++- lib/efi_loader/efi_tcg2.c | 4 +-- lib/tpm-v2.c | 62 +++++++++++++++++++++++------------- 4 files changed, 143 insertions(+), 39 deletions(-)