Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/29563 )
Change subject: security/tpm: Fix TCPA log feature ......................................................................
Patch Set 32:
(8 comments)
https://review.coreboot.org/#/c/29563/32/src/commonlib/include/commonlib/tcp... File src/commonlib/include/commonlib/tcpa_log_serialized.h:
https://review.coreboot.org/#/c/29563/32/src/commonlib/include/commonlib/tcp... PS32, Line 24: TCPA_PCR_HASH_TYPE This should be called ...HASH_TYPE_LENGTH or something like that.
Also, do we realy need to store this in ASCII? Wouldn't a uint32_t enum be enough (ideally reusing enum vb2_hash_algorithm)? And aren't we always going to use the same hash for all measurements, so why put it into every entry rather than once into tcpa_table?
https://review.coreboot.org/#/c/29563/26/src/security/tpm/tspi/log.c File src/security/tpm/tspi/log.c:
https://review.coreboot.org/#/c/29563/26/src/security/tpm/tspi/log.c@32 PS26, Line 32: struct tcpa_table *tclt; Why is the static gone here, again? (Sorry if we talked about that already, I forgot everything about this CL since I last reviewed it.)
https://review.coreboot.org/#/c/29563/32/src/security/tpm/tspi/log.c File src/security/tpm/tspi/log.c:
https://review.coreboot.org/#/c/29563/32/src/security/tpm/tspi/log.c@75 PS32, Line 75: " %u ", No need to put every element on a new line, I think?
https://review.coreboot.org/#/c/29563/32/src/security/tpm/tspi/log.c@83 PS32, Line 83: tce->digest_type, If you store an enum this could look up the conversion to ASCII in a table. (There are existing tables and helpers for this in vboot_reference/host/lib21/host_key.c, maybe those could be moved into a place that coreboot can link.)
https://review.coreboot.org/#/c/29563/32/src/security/tpm/tspi/log.c@92 PS32, Line 92: if (ENV_POSTCAR) You realize this means you won't be measuring the ramstage, right? Are you planning to fix that later?
https://review.coreboot.org/#/c/29563/32/src/security/tpm/tspi/log.c@104 PS32, Line 104: if (!name || !digest_type) Should this be an assert()?
https://review.coreboot.org/#/c/29563/32/src/security/tpm/tspi/log.c@117 PS32, Line 117: MIN(digest_length, TCPA_DIGEST_MAX_LENGTH) This can still just be digest_length
https://review.coreboot.org/#/c/29563/32/src/security/tpm/tspi/log.c@140 PS32, Line 140: tclt->num_entries = 0; Now that this hook is only called once you shouldn't need this.