Furquan Shaikh 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 4:
(8 comments)
https://review.coreboot.org/c/coreboot/+/46432/4/src/security/vboot/antiroll... File src/security/vboot/antirollback.h:
https://review.coreboot.org/c/coreboot/+/46432/4/src/security/vboot/antiroll... PS4, Line 28: normal nit: non-recovery
https://review.coreboot.org/c/coreboot/+/46432/4/src/security/vboot/antiroll... PS4, Line 62: recovery hash comment needs update here and for the functions below.
https://review.coreboot.org/c/coreboot/+/46432/4/src/security/vboot/antiroll... PS4, Line 63: antirollback_read_space_hash Probably add _mrc_ in the name to make it clear that these functions deal with hash of MRC data.
Also, can you please add a comment indicating that index can be either MRC_REC_HASH_NV_INDEX or MRC_RW_HASH_NV_INDEX depending upon the mode for which the hash of the MRC data is calculated.
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())
What do you think Furquan?
I think it's okay to have the RW MRC hash enabled for FSP2.0 platforms that were using recovery hash space already. But rather than silently changing it here, we should do this in proper sequence of CLs and documenting the impact in commit message.
I am thinking the CL train should look something like:
CL-1: Header clean up - Move drivers/intel/fsp2_0/include/fsp/memory_init.h into security/vboot/mrc_cache_hash_tpm.h - Include mrc_cache_hash_tpm.h instead of memory_init.h.
CL-2: Kconfig variable clean up - Add new Kconfig in drivers/mrc_cache/Kconfig: `MRC_SAVE_HASH_IN_TPM` which depends on VBOOT && TPM2 && !TPM1. I don't think this should be set to 'y' by default. It should be left to the individual platform to decide this. - In drivers/intel/fsp2_0/Kconfig, drop `FSP2_0_USES_TPM_MRC_HASH` and instead auto-select `MRC_SAVE_HASH_IN_TPM` for `PLATFORM_USES_FSP2_0` if `HAS_RECOVERY_MRC_CACHE` is selected. - Replace all uses of `FSP2_0_USES_TPM_MRC_HASH` with `MRC_SAVE_HASH_IN_TPM`. `VBOOT_HAS_REC_HASH_SPACE` should be set to default 'y' when `MRC_SAVE_HASH_IN_TPM` and `HAS_RECOVERY_MRC_CACHE` is selected.
CL-3: - Move the calls to `mrc_cache_{update,verify}_hash` from drivers/intel/fsp2_0/memory_init.c to drivers/mrc_cache/mrc_cache.c (This change would still be saving the hash only in recovery mode).
CL-4: - Add support for index parameter in all required functions (antirollback_*, mrc_cache_update_hash, mrc_cache_verify_hash). - In drivers/mrc_cache/mrc_cache.c, call `mrc_cache_{update,verify}_hash()` functions with `MRC_REC_HASH_NV_INDEX` if boot is recovery mode. Drop the check for recovery mode from `mrc_cache_{update,verify}_hash()` functions. (This change would still be saving the hash only in recovery mode, but the decision is moved to mrc_cache.c from mrc_cache_hash_tpm.c).
CL-5: - Add new index for `MRC_RW_HASH_NV_INDEX` and update the `antirollback_*()` functions to handle this index.
CL-6: - Add a new member to `struct cache_region` in mrc_cache.c - `uint32_t tpm_hash_index;` which indicates what index to pass to `mrc_cache_{update,verify}_hash()` functions. - In `normal_training` structure, set `tpm_hash_index` to `MRC_RW_HASH_NV_INDEX` if `MRC_SAVE_HASH_IN_TPM` is selected. - In `recovery_training` structure, set `tpm_hash_index` to `MRC_REC_HASH_NV_INDEX` if `MRC_SAVE_HASH_IN_TPM` and `HAS_RECOVERY_MRC_CACHE` are selected. - In `variable_data` structure, set `tpm_hash_index` to 0. - Call `mrc_cache_{update,verify}_hash()` functions only if tpm_hash_index is non-zero. - Change the selection of `MRC_SAVE_HASH_IN_TPM` by `PLATFORM_USES_FSP2_0` if `CHROMEOS` instead of `HAS_RECOVERY_MRC_CACHE`. (This CL actually uses both recovery and non-recovery hash spaces when a platform is using FSP2.0 and CHROMEOS. This change might also need a little refactoring to avoid calling `lookup_region_type()` multiple times.)
CL-7: - We can optimize by skipping the checksum calculation when tpm_hash_index is non-zero and we are making call to `mrc_cache_verify_hash()`.
CL-8: - Drop support in TPM1.2 for reading/writing/locking recovery mrc hash space in TPM.
I know it's breaking down into a large number of smaller CLs, but I think that will help in also identifying any regression if we miss out on some platform quirks. I can help push some CLs as well if you need some help.
https://review.coreboot.org/c/coreboot/+/46432/4/src/security/vboot/mrc_cach... File src/security/vboot/mrc_cache_hash_tpm.c:
https://review.coreboot.org/c/coreboot/+/46432/4/src/security/vboot/mrc_cach... PS4, Line 26: uint32_t hash_idx = vboot_recovery_mode_enabled() ? : MRC_REC_HASH_NV_INDEX : MRC_RW_HASH_NV_INDEX; Instead of identifying what hash_idx to use here, I think it would be better to let the caller implement that policy and pass in the index.
And we can use `struct cache_region` to encode the index that needs to be used for a particular type of cache. It allows the caller flexibility to decide when/if to use the hash functionality for a particular mode and cache type. You can also get rid of the dependency on !MRC_SETTINGS_VARIABLE_DATA in the following CL. I think that is just artificially restricting platforms that have variable data from being able to use the hash in TPM functionality.
https://review.coreboot.org/c/coreboot/+/46432/4/src/security/vboot/mrc_cach... PS4, Line 41: recovery Comment will need update.
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 242: static uint32_t set_hash_space(uint32_t index, const uint8_t *data)
Hmmm... okay so we're just completely forbidding it for TPM 1. […]
I think that should be okay. All the platforms that are using RECOVERY_MRC_CACHE and the recovery mrc hash space in TPM are TPM2.0 platforms. The lenovo platforms don't use FSP2.0, so the hash in TPM Kconfig never gets enabled for these.
I think we can drop the support of hash in TPM for TPM1.2 completely. (We should clean up the functions in secdata_tpm.c to print out error and not really do anything with the TPM space).
https://review.coreboot.org/c/coreboot/+/46432/4/src/security/vboot/secdata_... File src/security/vboot/secdata_tpm.c:
https://review.coreboot.org/c/coreboot/+/46432/4/src/security/vboot/secdata_... PS4, Line 199: Is the initialization of MRC_RW_HASH_NV_INDEX intentionally skipped here?