Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/29563 )
Change subject: security/tpm: Fix TCPA log feature ......................................................................
Patch Set 43:
(10 comments)
https://review.coreboot.org/#/c/29563/43/src/arch/x86/car.ld File src/arch/x86/car.ld:
https://review.coreboot.org/#/c/29563/43/src/arch/x86/car.ld@35 PS43, Line 35: N No need for capital letter here.
https://review.coreboot.org/#/c/29563/43/src/commonlib/include/commonlib/tcp... File src/commonlib/include/commonlib/tcpa_log_serialized.h:
https://review.coreboot.org/#/c/29563/43/src/commonlib/include/commonlib/tcp... PS43, Line 24: TCPA_PCR_HASH_LEN 9 So far what I have seen you need to store either "SHA1", "SHA256" or "SHA512". 7 characters would be enough for that. Any extensions in mind where 9 chars will be needed?
https://review.coreboot.org/#/c/29563/43/src/commonlib/include/commonlib/tcp... PS43, Line 25: // Use a propper comment style. Or is it something clang-format has introduced and I am just no aware of it?
https://review.coreboot.org/#/c/29563/43/src/security/tpm/tspi.h File src/security/tpm/tspi.h:
https://review.coreboot.org/#/c/29563/43/src/security/tpm/tspi.h@50 PS43, Line 50: const char *digest_type Mind to update the doxygen comment with this new argument?
https://review.coreboot.org/#/c/29563/43/src/security/tpm/tspi/log.c File src/security/tpm/tspi/log.c:
https://review.coreboot.org/#/c/29563/43/src/security/tpm/tspi/log.c@32 PS43, Line 32: /* We are dealing here with pre CBMEM environment Add a "." at the end as your sentence is finished.
https://review.coreboot.org/#/c/29563/43/src/security/tpm/tspi/log.c@50 PS43, Line 50: if (!cbmem_possibly_online() && IS_ENABLED(CONFIG_VBOOT_TCPA_LOG_RECOVERY)) { Maybe just "else if (IS_ENABLED(CONFIG_VBOOT_TCPA_LOG_RECOVERY))" here as cbmem_possibly_online() has been processed already and you are checking for !cbmem_possibly_online() here again.
https://review.coreboot.org/#/c/29563/43/src/security/tpm/tspi/log.c@68 PS43, Line 68: tcpa_log_init(); Do you really want to call this init_code when the tables are dumped? Aren't you just need to get the pointer to the table without modifying the contents? Or do I oversee something here?
https://review.coreboot.org/#/c/29563/43/src/security/tpm/tspi/tspi.c File src/security/tpm/tspi/tspi.c:
https://review.coreboot.org/#/c/29563/43/src/security/tpm/tspi/tspi.c@243 PS43, Line 243: strncpy(hash_name, "SHA1", TCPA_PCR_HASH_LEN - 1); Maybe provide a const array of strings and then index into this array with hash_alg to get the right string?
https://review.coreboot.org/#/c/29563/43/util/cbmem/cbmem.c File util/cbmem/cbmem.c:
https://review.coreboot.org/#/c/29563/43/util/cbmem/cbmem.c@687 PS43, Line 687: static void dump_tcpa_log(void) Ther is already a function which is very similar: tcpa_log_dump() in tspi/log.c
Can't you use it to dump the log?
https://review.coreboot.org/#/c/29563/43/util/cbmem/cbmem.c@694 PS43, Line 694: tcpa_log Make sure tcpa_log is not NULL?