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:
(5 comments)
Sorry, not sure why I didn't catch this patch before it went in. But now it triggers a load of Coverity warnings from buffer overflows, and some of the design choices also look very wrong. Please provide a fixup quickly, or revert the patch until you have time to do that.
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 217: tpml_digests.count = 1; Why do you need to pass a TPML_DIGEST_VALUES struct here when you only ever have a single digest? Just pass the raw byte buffer as before and add a new digest_type argument to tlcl_extend(). If we wanted to be able to extend multiple algorithms with one command we'd need that, but that's not what you're doing here.
Using TPML_DIGEST_VALUES here break the whole purpose of separating TSPI and TSS layers. The whole point of the TSPI layer is that it does *not* know about the implementation details of the specific TPM protocols, so even if you wanted to do something like this you should be using a generic interface rather than a TPM 2.0 specific protocol structure in the TSPI<->TSS interface.
https://review.coreboot.org/c/coreboot/+/33252/8/src/security/tpm/tspi/tspi.... PS8, Line 222: sizeof(TPMU_HA) This was flagged by Coverity, you're using the wrong size. This should be SHA1_DIGEST_SIZE, not the size of the whole union. You're overrunning the input buffer.
https://review.coreboot.org/c/coreboot/+/33252/8/src/security/tpm/tspi/tspi.... PS8, Line 232: result = tlcl_extend(pcr, (uint8_t *)&tpml_digests, NULL); You also shouldn't completely change the meaning of the arguments to a TSS function based on the TPM version (again, the TSPI layer should ideally not care about the TPM version at all). If you want to pass a TPML_DIGEST_VALUE pointer the argument type should be TPML_DIGEST_VALUE*, not u8* (but again, you shouldn't be doing that in the first place).
I think you should keep the existing arguments to tlcl_extend() the way they are and just add a new digest_type argument (for tss-1.2 it's fine to just return an error if the digest_type is not SHA1).
https://review.coreboot.org/c/coreboot/+/33252/8/src/security/tpm/tss/tcg-2.... File src/security/tpm/tss/tcg-2.0/tss.c:
https://review.coreboot.org/c/coreboot/+/33252/8/src/security/tpm/tss/tcg-2.... PS8, Line 151: sizeof(TPMU_HA) These memcpy()s also use the wrong size.
https://review.coreboot.org/c/coreboot/+/33252/8/src/security/tpm/tss/tcg-2.... PS8, Line 183: (tpml_digests->digests[0].hashAlg == TPM_ALG_ERROR) && I don't understand what this is. Why would you put ALG_ERROR in there? That use doesn't look valid according to the spec.