Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/29563 )
Change subject: security/tpm: Fix TCPA log feature ......................................................................
Patch Set 13:
(13 comments)
https://review.coreboot.org/#/c/29563/13/src/commonlib/include/commonlib/tcp... File src/commonlib/include/commonlib/tcpa_log_serialized.h:
https://review.coreboot.org/#/c/29563/13/src/commonlib/include/commonlib/tcp... PS13, Line 24: 128
This should be (TCPA_DIGEST_MAX_LENGTH * 2 + 1)
Done
https://review.coreboot.org/#/c/29563/13/src/commonlib/include/commonlib/tcp... PS13, Line 32: char name[TCPA_PCR_HASH_NAME];
If you are directly exposing this structure how is the tooling supposed to know where the fields end […]
TCPA_FORMAT_HASH_LENGTH and TCPA_LOG_STRING_LENGTH are only used from cbmem tool
https://review.coreboot.org/#/c/29563/13/src/security/tpm/tspi/log.c File src/security/tpm/tspi/log.c:
https://review.coreboot.org/#/c/29563/13/src/security/tpm/tspi/log.c@30 PS13, Line 30: if (!cbmem_possibly_online() &&
You should add a comment here about what case you are dealing with.
Done
https://review.coreboot.org/#/c/29563/13/src/security/tpm/tspi/log.c@43 PS13, Line 43: sizeof(struct tcpa_entry));
You should memset() to 0 because cbmem is not guarantee to be initialized.
Done
https://review.coreboot.org/#/c/29563/13/src/security/tpm/tspi/log.c@73 PS13, Line 73: (char *)
hash is already char type. No need to cast.
Done
https://review.coreboot.org/#/c/29563/13/src/security/tpm/tspi/log.c@74 PS13, Line 74: tce->digest_length,
This length calculation is wrong. You are overwriting the buffer on the last 2 nibbles.
Done
https://review.coreboot.org/#/c/29563/13/src/security/tpm/tspi/log.c@82 PS13, Line 82: hash
This string is not NUL terminated. It's exactly 2x max hash size. It is not a valid C string.
Done
https://review.coreboot.org/#/c/29563/13/src/security/tpm/tspi/log.c@102 PS13, Line 102: if (tce)
This would always evaluate to non-NULL unless we were wrapping arithmetic or entries was first field […]
Done
https://review.coreboot.org/#/c/29563/13/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/#/c/29563/13/src/security/vboot/Kconfig@258 PS13, Line 258: default y
If it's default y how is anyone supposed to unselect it? If it's driven by VBOOT_MEASURED_BOOT then […]
Done
https://review.coreboot.org/#/c/29563/13/src/security/vboot/vboot_crtm.c File src/security/vboot/vboot_crtm.c:
https://review.coreboot.org/#/c/29563/13/src/security/vboot/vboot_crtm.c@165 PS13, Line 165: tcpa_log_dump()
We can just call this when we're exiting BS_LOAD_PAYLOAD state. […]
Done
https://review.coreboot.org/#/c/29563/13/src/security/vboot/vboot_crtm.c@170 PS13, Line 170: static void recover_tcpa_log(int is_recovery)
Why isn't this function just in the other file where you handled the rest of the log stuff?
It's only needed for measurements in the vboot case
https://review.coreboot.org/#/c/29563/13/src/security/vboot/vboot_crtm.c@173 PS13, Line 173: if (tclt)
When would this be NULL?
Done
https://review.coreboot.org/#/c/29563/13/src/security/vboot/vboot_crtm.c@188 PS13, Line 188: ROMSTAGE_CBMEM_INIT_HOOK(recover_tcpa_log);
Do you want to handle other conditions of cbmem coming online later? If so you need the other CBMEM_ […]
mhh I guess you are right. Only ramstage and postcar are leftovers ?
-
Philipp Deppenwiese (Code Review)