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.apalodi...@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))
