[PATCH v3] tpm-v2: allow algoirthm name to be configured for pcr_read and pcr_extend
Ilias Apalodimas
ilias.apalodimas at linaro.org
Tue May 21 14:31:21 CEST 2024
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))
More information about the U-Boot
mailing list