Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46434 )
Change subject: mrc_cache: Add TPM Hash verification ......................................................................
Patch Set 8:
(5 comments)
https://review.coreboot.org/c/coreboot/+/46434/8/src/drivers/mrc_cache/Kconf... File src/drivers/mrc_cache/Kconfig:
https://review.coreboot.org/c/coreboot/+/46434/8/src/drivers/mrc_cache/Kconf... PS8, Line 52: MRC_SAVE_MRC_HASH MRC twice? (I would consider MRC_SAVE_TPM_HASH or MRC_SAVE_HASH_IN_TPM, otherwise it's not very clear where it's saving that hash)
https://review.coreboot.org/c/coreboot/+/46434/8/src/drivers/mrc_cache/Kconf... PS8, Line 57: object When I said "missing object", I meant "this sentence is missing its (grammatical) object" (i.e. *what* does it ensure?), I didn't mean you you should literally insert the word "object" here. ;)
I assume what you wanted to say was something like "Store a hash of the MRC cache training data in a TPM NVRAM space to ensure it cannot be tampered with."
https://review.coreboot.org/c/coreboot/+/46434/5/src/drivers/mrc_cache/mrc_c... File src/drivers/mrc_cache/mrc_cache.c:
https://review.coreboot.org/c/coreboot/+/46434/5/src/drivers/mrc_cache/mrc_c... PS5, Line 181: checksum = compute_ip_checksum(data, data_size);
Do we want to replace this w/ the mrc_cache_verify_hash when MRC_SAVE_MRC_HASH is enabled? Seems lik […]
You mean just skip the data_checksum check when the TPM hash is enabled, right? Basically
if (CONFIG(MRC_SAVE_MRC_HASH)) { if (!mrc_cache_verify_hash(data, data_size))) return -1; } else { ...all the data checksum stuff... }
I agree that would make sense, although the actual time spent on this is probably in the microsecond range.
https://review.coreboot.org/c/coreboot/+/46434/5/src/security/vboot/mrc_cach... File src/security/vboot/mrc_cache_hash_tpm.c:
https://review.coreboot.org/c/coreboot/+/46434/5/src/security/vboot/mrc_cach... PS5, Line 13: void mrc_cache_update_hash(const uint8_t *data, size_t size)
I feel like we should pass in hash objects that are calculated outside of these functions. […]
I mean... I think this file is just an extension of the MRC cache code anyway, it should maybe just be moved to src/drivers/mrc_cache. I don't think it wants to be a generic hash storing library, it just separates out the MRC cache code that is guarded by that Kconfig. So I don't really see a problem with having some policy defined here.
Unfortunately we're kinda running up against a deadline with this, so if you want a larger-scale refactoring would you mind proposing a concrete API so we converge on something quickly? Or maybe we can keep using this for now (it has been used for years already, after all) and postpone wider refactoring discussions to later?
https://review.coreboot.org/c/coreboot/+/46434/5/src/security/vboot/mrc_cach... PS5, Line 58: /* Lock TPM space */
Removed locking of TPM as Julius said that it'll get locked when jumping to depthcharge.
Clarification: when depthcharge jumps to the kernel (by virtue of disabling platform hierarchy).