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 55:
(10 comments)
Please fix the line length issues, too.
https://review.coreboot.org/#/c/29563/55/src/commonlib/include/commonlib/tcp... File src/commonlib/include/commonlib/tcpa_log_serialized.h:
https://review.coreboot.org/#/c/29563/55/src/commonlib/include/commonlib/tcp... PS55, Line 26: PREMEM nit: "PRERAM" would be more consistent with other coreboot terminology usage
https://review.coreboot.org/#/c/29563/55/src/include/memlayout.h File src/include/memlayout.h:
https://review.coreboot.org/#/c/29563/55/src/include/memlayout.h@169 PS55, Line 169: 2K nit: Considering that this is such a pain to fit on some SoCs, do you think it might make sense to reduce it to 1K? Can you think of a situation where you'd need to measure more than 8 pre-RAM files? (Alternatively you could just not define a minimum and calculate the available space for entries dynamically, so that SoCs with less space but also less things to measure could use less SRAM for it.)
https://review.coreboot.org/#/c/29563/55/src/security/tpm/tspi.h File src/security/tpm/tspi.h:
https://review.coreboot.org/#/c/29563/55/src/security/tpm/tspi.h@56 PS55, Line 56: diget_type typo
https://review.coreboot.org/#/c/29563/55/src/security/tpm/tspi/log.c File src/security/tpm/tspi/log.c:
https://review.coreboot.org/#/c/29563/55/src/security/tpm/tspi/log.c@40 PS55, Line 40: memset(tclt, 0, tcpa_log_len); unnecesary?
https://review.coreboot.org/#/c/29563/55/src/security/tpm/tspi/log.c@124 PS55, Line 124: MIN( MIN() is unnecessary
https://review.coreboot.org/#/c/29563/55/src/security/tpm/tspi/log.c@131 PS55, Line 131: && IS_ENABLED(CONFIG_VBOOT_TCPA_LOG_RECOVERY)) { Not sure you need all these checks here? This should only be called in cases where that is all true anyway (except for LOG_RECOVERY, maybe), and for everything else it should be garbage-collected out harmlessly.
Note that ENV_CACHE_AS_RAM is actually always 0 on non-x86 systems, so that will make it not work on there.
https://review.coreboot.org/#/c/29563/55/src/security/tpm/tspi/log.c@135 PS55, Line 135: printk(BIOS_ERR, "TCPA: Couldn't clear coreboot TCPA log\n"); nit: this is pretty pointless to check, either the symbol exists or you get a build-time linker error.
https://review.coreboot.org/#/c/29563/55/src/security/tpm/tspi/tspi.c File src/security/tpm/tspi/tspi.c:
https://review.coreboot.org/#/c/29563/55/src/security/tpm/tspi/tspi.c@241 PS55, Line 241: if (IS_ENABLED(CONFIG_TPM1)) { Why not just move this part into tcpa_log_add_table_entry()? As long as we hardcode which hash to use for which TPM type anyway, no need to do this so high up.
https://review.coreboot.org/#/c/29563/55/src/security/vboot/secdata_tpm.c File src/security/vboot/secdata_tpm.c:
https://review.coreboot.org/#/c/29563/55/src/security/vboot/secdata_tpm.c@83 PS55, Line 83: "SHA1" I don't think this belongs here, right? Which algorithm is used is hardcoded in tlcl_extend(), it's not decided here.
https://review.coreboot.org/#/c/29563/55/src/security/vboot/symbols.h File src/security/vboot/symbols.h:
https://review.coreboot.org/#/c/29563/55/src/security/vboot/symbols.h@25 PS55, Line 25: #define _vboot2_tpm_log_size (_evboot2_tpm_log - _vboot2_tpm_log) Note that this got changed (CB:31539).