Julius Werner has posted comments on this change. ( https://review.coreboot.org/27769 )
Change subject: security/tpm: TCPA log follow up ......................................................................
Patch Set 1:
(4 comments)
https://review.coreboot.org/#/c/27769/1/src/security/tpm/tspi/log.c File src/security/tpm/tspi/log.c:
https://review.coreboot.org/#/c/27769/1/src/security/tpm/tspi/log.c@24 PS1, Line 24: static Does this build correctly for x86 verstage? I guess it might get optimized out. Still, I think it's safer to use MAYBE_STATIC (and explicitly initialize to NULL) so correctness is not dependent on such details.
https://review.coreboot.org/#/c/27769/1/src/security/tpm/tspi/log.c@31 PS1, Line 31: NULL Doesn't this need to be 'return ce'?
https://review.coreboot.org/#/c/27769/1/src/security/tpm/tspi/log.c@57 PS1, Line 57: printk(BIOS_ERR, "ERROR: No TCPA log table found\n"); Now you'll have the problem Furquan fixed again. I think you probably just want to move this error message back into tcpa_log_init() and only print it for the cbmem_add() failure (which is the only real error, all other code paths are WAI).
https://review.coreboot.org/#/c/27769/1/src/security/tpm/tspi/log.c@68 PS1, Line 68: TCPA_PCR_HASH_NAME You need to subtract 1 here or you might not write the null terminator and get weird output in cbmem. (Or you can fix the cbmem implementation to not read more than TCPA_PCR_HASH_NAME chars from this, which I don't think it currently checks for.)
-
Julius Werner (Code Review)