Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33252 )
Change subject: security/tpm/tss/tcg-2.0: Add multi digits support to tlcl_extend() ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33252/8/src/security/tpm/tspi/tspi.... File src/security/tpm/tspi/tspi.c:
https://review.coreboot.org/c/coreboot/+/33252/8/src/security/tpm/tspi/tspi.... PS8, Line 206: enum vb2_hash_algorithm digest_algo We just discovered another subtle but serious problem with this CL.
Quick recap of how PCR extensions work to explain:
1. Machine boots, PCRs are initialized to 0. 2. CPU wants to measure a blob of data. 3. CPU hashes blob of data with a hash algorithm of its choice (e.g. SHA1). 4. CPU sends this hash to TPM with TPM2_Extend command. 5. TPM sets PCR to (roughly): HASH(PCR + <TPM2_Extend data>)
The problem is that the hash algorithm used in 3 and the one used in 5 are not necessarily the same thing! The algorithm used in 3 (which is the one denoted by the digest_algo parameter here) is completely hidden within the CPU, it doesn't even need to be an algorithm supported by the TPM. From the TPM's point of view, the data sent with TPM2_Extend is really just a (small) random blob of data that could be anything. The algorithm used on the TPM side (in step 5) is selected via the hashAlg parameter in the TPM2_Extend command and must be supported by the TPM. On top of that, the TPM actually maintains a completely separate PCR for every algorithm type it supports (for each PCR number)... so when you send a TPM2_Extend(PCR0, data, SHA1) and a TPM2_Extend(PCR0, data, SHA256), those two command actually extend two completely separate PCRs.
Frans, can you explain *why* you need to use the SHA1 bank of your TPM at all? It generally doesn't matter much which PCR bank you use as long you make sure you always use the same one. SHA256 is generally considered secure (and SHA1 isn't), so unless there's a real need to allow coreboot code to access different PCR banks, I think the current policy of hardcoding a single PCR hashAlg per TPM version makes a lot of sense and avoids confusing issues like this. (You can still measure your data with SHA1 hashes and extend those into the SHA256 PCR bank if you want, that's perfectly fine!)
If you really need to use a different PCR bank for some reason, I'd suggest we create a Kconfig for that (e.g. CONFIG_TPM2_PCR_BANK_ALG). Then you can use SHA1 on your platform and we can keep SHA256 as the default, and we still make sure that you can only use one PCR bank at a time. Using multiple PCR banks at once is really just a huge potential for confusion and I see no realistic use case why we should need to support that.