Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46432 )
Change subject: security/vboot: Make mrc_cache hash functions generic ......................................................................
Patch Set 1:
(9 comments)
https://review.coreboot.org/c/coreboot/+/46432/1/src/security/vboot/antiroll... File src/security/vboot/antirollback.h:
https://review.coreboot.org/c/coreboot/+/46432/1/src/security/vboot/antiroll... PS1, Line 27: NORM_HASH_NV_INDEX I'd call this MRC_HASH_NV_INDEX (or something like MRC_RW_HASH_NV_INDEX if you want to be specific) because otherwise there's no indication that this has anything to do with MRC cache. (The other one should've probably been called MRC_REC_HASH_NV_INDEX or something like that...)
https://review.coreboot.org/c/coreboot/+/46432/1/src/security/vboot/mrc_cach... File src/security/vboot/mrc_cache_hash_tpm.c:
https://review.coreboot.org/c/coreboot/+/46432/1/src/security/vboot/mrc_cach... PS1, Line 28: if (!vboot_recovery_mode_enabled()) Removing this will immediately enable this on the FSP 2.0 platforms that already use the recovery cache hash. Generally I don't see a problem with using this everywhere, but those platforms also already use platform-specific flash protection for MRC cache so maybe they'd consider it redundant and unnecessary. +Furquan should decide whether this is okay. (Also because of the TPM 1.2 problem I mentioned that exists for the RW hash but not for the recovery hash, maybe we should still keep both hashes separate in Kconfig so that boards with TPM 1.2 have the option to enable only one but not the other.)
https://review.coreboot.org/c/coreboot/+/46432/1/src/security/vboot/mrc_cach... PS1, Line 36: if (vb2_digest_buffer(data, size, VB2_HASH_SHA256, data_hash, nit: In practice this function can't fail unless the arguments are wrong, so if you ask me this could also just be an assert() and we wouldn't need that whole dead_hash stuff. But somewhat off-topic.
https://review.coreboot.org/c/coreboot/+/46432/1/src/security/vboot/mrc_cach... PS1, Line 57: printk(BIOS_INFO, "MRC: TPM MRC hash updated successfully.\n"); nit: might be nice to also print the index here now
https://review.coreboot.org/c/coreboot/+/46432/1/src/security/vboot/mrc_cach... PS1, Line 93: "Using data from MRC_CACHE\n"); nit: here too (and I'm not sure what "Using data from MRC_CACHE" is really supposed to tell us here, isn't that implied when you talk about "MRC: Hash comparison"?)
https://review.coreboot.org/c/coreboot/+/46432/1/src/security/vboot/secdata_... File src/security/vboot/secdata_tpm.c:
https://review.coreboot.org/c/coreboot/+/46432/1/src/security/vboot/secdata_... PS1, Line 169: ro_space_attributes This is the tricky part: for the RW hash, this needs to be rw_space_attributes and no policy (similar to what set_kernel_space() above does).
https://review.coreboot.org/c/coreboot/+/46432/1/src/security/vboot/secdata_... PS1, Line 242: static uint32_t set_hash_space(uint32_t index, const uint8_t *data) For complicated reasons, having an RW MRC cache hash isn't really possible for TPM 1.2 (at least not without making things way more complicated). So we should just prevent that with Kconfig dependencies and assert() here that it can't happen or something.
https://review.coreboot.org/c/coreboot/+/46432/1/src/security/vboot/secdata_... PS1, Line 423: rec nit: remove
https://review.coreboot.org/c/coreboot/+/46432/1/src/security/vboot/secdata_... PS1, Line 437: rec here too