Attention is currently required from: Maciej Pijanowski, Christian Walter, Julius Werner, Krystian Hebel, Sergii Dmytruk.
Michał Żygowski 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 7:
(6 comments)
File src/commonlib/bsd/include/commonlib/bsd/cbmem_id.h:
https://review.coreboot.org/c/coreboot/+/68747/comment/31f27de5_b519a71b PS7, Line 143: { CBMEM_ID_TPM_LOG, "TPM STD LOG" }, \ Why do we create a new cbmem ID? We already have IDs TPM logs: CBMEM_ID_TCPA_TCG_LOG and CBMEM_ID_TPM2_TCG_LOG
In case of TPM1.2 we should use CBMEM_ID_TCPA_TCG_LOG and for TPM2.0 CBMEM_ID_TPM2_TCG_LOG
File src/security/tpm/Kconfig:
https://review.coreboot.org/c/coreboot/+/68747/comment/68b94a16_cf831f1e PS5, Line 108: bool "TPM 1.2 format"
Look at 10.1 section in https://trustedcomputinggroup. […]
Exactly, TPM2 should have different format. Furthermore certain TPMs like Intel PTT (fTPM) on modern platforms enable only SH256 banks (SHA1 is disabled by default and has to be explicitly enabled by issuing require commands), so the old format would not work at all for them.
File src/security/tpm/Kconfig:
https://review.coreboot.org/c/coreboot/+/68747/comment/cbfd1fd6_db1c9692 PS7, Line 100: default TPM_LOG_TPM1 if TPM1 I thought we will make this choice much simpler: 1. Provide option to choose coreboot log format or TCG compliant format. 2. If TCG compliant format is selected, coreboto should use the lgo format applicable for detected TPM.
So I expect he option name called TPM_LOG_TCG without any dependencies on TPM1 or TPM2.
File src/security/tpm/tpm1_log_serialized.h:
https://review.coreboot.org/c/coreboot/+/68747/comment/43cb31d4_dc5bc13f PS7, Line 1: /* SPDX-License-Identifier: GPL-2.0-only */ This file could be commonlib with BSD license isn't it? Also I would make this file be included in commonlib/tpm_log_serialized.h so that this one file would include all formats supported by coreboot.
https://review.coreboot.org/c/coreboot/+/68747/comment/602929a2_4d8efa2a PS7, Line 13: #define TPM_1_LOG_VI_MAGIC 0x31544243 /* "CBT1" in LE */ For ACPI table we use "COREv4" as OEM ID and "CORE" as ASL compiler. Maybe we could use "CORE" here in LE as the magic number? At least we will have some consistence
File src/security/tpm/tspi/log-tpm1.c:
https://review.coreboot.org/c/coreboot/+/68747/comment/f37485c8_6c5becfd PS7, Line 16: void *tpm_log_cbmem_init(void) If it is source file for TPM1.2 log only, shouldn't all function names have a tpm1_ or tpm12_ or tcpa_ prefix? It could be later used in common source file for TPM TCG log creation which would detect TPM and create appropriate log for it by calling singe function from this file. For source file with TPM2 event log all functions should have a prefix of tpm2_.
-
Michał Żygowski (Code Review)