Attention is currently required from: Michał Żygowski, Maciej Pijanowski, Christian Walter, Krystian Hebel, Sergii Dmytruk.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/68747 )
Change subject: security/tpm: add TPM log format as per 1.2 spec ......................................................................
Patch Set 4:
(3 comments)
File src/security/tpm/Kconfig:
https://review.coreboot.org/c/coreboot/+/68747/comment/50d93059_8848b6b0 PS4, Line 105: config USE_TPM_LOG_TPM12 I don't get the point of these USE_xxx options, and it doesn't seem like you're adding any code that actually uses them? I think this could just be left as a simple menuconfig choice for now. (In general I think it's a bad idea if mainboards just randomly change the defaults for other Kconfigs, we may occasionally do that to ensure backwards compatibility but we should try not to propagate it going forward.)
File src/security/tpm/tspi.h:
https://review.coreboot.org/c/coreboot/+/68747/comment/31181cea_7d237fbe PS4, Line 13: #include "tpm12_log_serialized.h" I think conditional inclusion of different headers is generally a bad pattern that we should try to avoid where possible. It makes it very confusing to understand where which definition actually comes from (especially if you then start defining the same named constant with different values in different places). I know we did it with the TSS stuff as well but it was probably already a bad idea there.
I think it would be better if you just include all the headers and namespaced the definitions properly so there are no collisions. (Or in the few rare cases where you actually want the same constant to be defined to different values based on Kconfig, it should be a very explicit #if block only around that one definition like for TPM_MEASURE_ALGO, not buried deep in conditionally included headers that have a few definitions which are intentionally overlapping like that and a ton that are not.)
File src/security/tpm/tspi/log-tpm12.c:
https://review.coreboot.org/c/coreboot/+/68747/comment/16aec382_3c7d6930 PS4, Line 59: struct tcpa_table *tcpa_log_init(void) We should avoid duplicating so much code between log implementations. I think you should put this function and the common parts of recover_tcpa_log() and tcpa_cbmem_init() into a shared file, and then have them call out to functions that are implemented by each individual log format and only concerned with the formats themselves (so that when things like how the log is migrated from SRAM to CBMEM change, we don't have to modify 3 different copies of that code).
-
Julius Werner (Code Review)