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 5:
(7 comments)
File src/commonlib/include/commonlib/tpm_log_defs.h:
https://review.coreboot.org/c/coreboot/+/68747/comment/cf0905e8_71058367 PS5, Line 1: /* SPDX-License-Identifier: GPL-2.0-only */ Note that licensing this as GPLv2 would make it hard to share with libpayload in the future. So if you think having a payload display the log table or add further entries from files it loaded could become useful in the future, I'd maybe consider moving this to commonlib/bsd (and licensing as BSD). (Of course this is assuming you have the rights to all of this code, if you copied it from another GPL source then you cannot do that.)
File src/security/tpm/Kconfig:
https://review.coreboot.org/c/coreboot/+/68747/comment/734f2d0a_8cf33b3e PS5, Line 107: TPM_LOG_TPM12 We generally use "TPM1", not "TPM12" in the code, so I would suggest calling this TPM_LOG_TPM1 as well (and the same in all other identifiers).
https://review.coreboot.org/c/coreboot/+/68747/comment/64d39a2f_b66101a4 PS5, Line 108: bool "TPM 1.2 format" Can this log format be used with 2.0 TPMs? Otherwise, this should probably have a `depends on TPM1`.
File src/security/tpm/tspi.h:
https://review.coreboot.org/c/coreboot/+/68747/comment/4c36d568_1148f85d PS5, Line 11: #include "tpm12_log_serialized.h" Please use angled brackets notation (i.e. <security/tpm/tpm1_log_serialized.h>) and alphabetize with the other headers above (well, I guess they aren't alphabetized in the first place in this case, but... idk just group with the other security/tpm #include).
File src/security/tpm/tspi/log-tpm12.c:
https://review.coreboot.org/c/coreboot/+/68747/comment/b47ebaea_b76607aa PS5, Line 115: strncpy((char *)tce->data, name, sizeof(tce->data) - 1); This needs to guarantee that tce->data gets null-terminated (your other functions rely on that), so you should have an explicit `tce->data[sizeof(tce->data) - 1] = '\0'` here. (In fact, it looks like the old coreboot format doesn't do that correctly either, would be good to fix that.)
https://review.coreboot.org/c/coreboot/+/68747/comment/7720e959_d29b220e PS5, Line 123: tclt->spec_id.num_entries = 0; So this means that the pre-RAM version of the log table will never be fully initialized (e.g. fields like spec_version_xxx will not be filled out). I guess that's fine since it will be migrated to CBMEM eventually anyway, but it is a bit weird. Maybe explain that in a comment somewhere (if it is intentional)?
https://review.coreboot.org/c/coreboot/+/68747/comment/553a2a2e_d0c940b5 PS5, Line 161: struct tpm_12_log_entry *tce = &to_log->entries[to_log->spec_id.num_entries++]; Why not just memcpy() the whole entry?