Shelley Chen has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/46509 )
Change subject: mrc_cache: Add config MRC_SAVE_HASH_IN_TPM ......................................................................
mrc_cache: Add config MRC_SAVE_HASH_IN_TPM
Use this config to specify whether we want to save a hash of the MRC_CACHE in the TPM NVRAM space. Replace all uses of FSP2_0_USES_TPM_MRC_HASH with MRC_SAVE_HASH_IN_TPM and remove the FSP2_0_USES_TPM_MRC_HASH config.
BUG=b:150502246 BRANCH=None TEST=emerge-nami coreboot chromeos-bootimage
Change-Id: Ic5ffcdba27cb1f09c39c3835029c8d9cc3453af1 Signed-off-by: Shelley Chen shchen@google.com --- M src/drivers/intel/fsp2_0/Kconfig M src/drivers/intel/fsp2_0/memory_init.c M src/drivers/mrc_cache/Kconfig M src/security/vboot/Kconfig M src/security/vboot/Makefile.inc 5 files changed, 15 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/09/46509/1
diff --git a/src/drivers/intel/fsp2_0/Kconfig b/src/drivers/intel/fsp2_0/Kconfig index 1a1da6c..59bb4e4 100644 --- a/src/drivers/intel/fsp2_0/Kconfig +++ b/src/drivers/intel/fsp2_0/Kconfig @@ -3,6 +3,7 @@ config PLATFORM_USES_FSP2_0 bool default n + select MRC_SAVE_HASH_IN_TPM if HAS_RECOVERY_MRC_CACHE help Include FSP 2.0 wrappers and functionality
@@ -141,23 +142,6 @@ own stack that will be placed in DRAM and not in CAR, this is the amount of memory the FSP needs for its stack and heap.
-config FSP2_0_USES_TPM_MRC_HASH - bool - depends on TPM1 || TPM2 - depends on VBOOT && VBOOT_STARTS_IN_BOOTBLOCK - default y if HAS_RECOVERY_MRC_CACHE - default n - select VBOOT_HAS_REC_HASH_SPACE - help - Store hash of trained recovery MRC cache in NVRAM space in TPM. - Use the hash to validate recovery MRC cache before using it. - This hash needs to be updated every time recovery mode training - is recomputed, or if the hash does not match recovery MRC cache. - Selecting this option requires that TPM already be setup by this - point in time. Thus it is only compatible when the option - VBOOT_STARTS_IN_BOOTBLOCK is selected, which causes verstage and - TPM setup to occur prior to memory initialization. - config FSP_PLATFORM_MEMORY_SETTINGS_VERSIONS bool help diff --git a/src/drivers/intel/fsp2_0/memory_init.c b/src/drivers/intel/fsp2_0/memory_init.c index 14aec98..09aad6b 100644 --- a/src/drivers/intel/fsp2_0/memory_init.c +++ b/src/drivers/intel/fsp2_0/memory_init.c @@ -19,15 +19,15 @@ #include <symbols.h> #include <timestamp.h> #include <security/vboot/vboot_common.h> -#include <security/tpm/tspi.h> #include <security/vboot/mrc_cache_hash_tpm.h> +#include <security/tpm/tspi.h> #include <vb2_api.h> #include <types.h>
static uint8_t temp_ram[CONFIG_FSP_TEMP_RAM_SIZE] __aligned(sizeof(uint64_t));
/* TPM MRC hash functionality depends on vboot starting before memory init. */ -_Static_assert(!CONFIG(FSP2_0_USES_TPM_MRC_HASH) || +_Static_assert(!CONFIG(MRC_SAVE_HASH_IN_TPM) || CONFIG(VBOOT_STARTS_IN_BOOTBLOCK), "for TPM MRC hash functionality, vboot must start in bootblock");
@@ -55,7 +55,7 @@ mrc_data_size) < 0) printk(BIOS_ERR, "Failed to stash MRC data\n");
- if (CONFIG(FSP2_0_USES_TPM_MRC_HASH)) + if (CONFIG(MRC_SAVE_HASH_IN_TPM)) mrc_cache_update_hash(mrc_data, mrc_data_size); }
@@ -121,7 +121,7 @@ if (data == NULL) return;
- if (CONFIG(FSP2_0_USES_TPM_MRC_HASH) && + if (CONFIG(MRC_SAVE_HASH_IN_TPM) && !mrc_cache_verify_hash(data, mrc_size)) return;
diff --git a/src/drivers/mrc_cache/Kconfig b/src/drivers/mrc_cache/Kconfig index e09c5d8..bb97398 100644 --- a/src/drivers/mrc_cache/Kconfig +++ b/src/drivers/mrc_cache/Kconfig @@ -49,4 +49,12 @@ that need to write back the MRC data in late ramstage boot states (MRC_WRITE_NV_LATE).
+config MRC_SAVE_HASH_IN_TPM + bool + depends on VBOOT && TPM2 && !TPM1 + default n + help + Store a hash of the MRC_CACHE training data in a TPM NVRAM + space to ensure that it cannot be tampered with. + endif # CACHE_MRC_SETTINGS diff --git a/src/security/vboot/Kconfig b/src/security/vboot/Kconfig index ee8d36a..094cbb9 100644 --- a/src/security/vboot/Kconfig +++ b/src/security/vboot/Kconfig @@ -159,6 +159,7 @@
config VBOOT_HAS_REC_HASH_SPACE bool + default y if MRC_SAVE_HASH_IN_TPM && HAS_RECOVERY_MRC_CACHE default n help Set this option to indicate to vboot that recovery data hash space diff --git a/src/security/vboot/Makefile.inc b/src/security/vboot/Makefile.inc index e92396d..d4dabe2 100644 --- a/src/security/vboot/Makefile.inc +++ b/src/security/vboot/Makefile.inc @@ -118,7 +118,7 @@ ramstage-y += common.c postcar-y += common.c
-romstage-$(CONFIG_FSP2_0_USES_TPM_MRC_HASH) += mrc_cache_hash_tpm.c +romstage-$(CONFIG_MRC_SAVE_HASH_IN_TPM) += mrc_cache_hash_tpm.c
ifeq ($(CONFIG_VBOOT_SEPARATE_VERSTAGE),y)
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Andrey Petrov, Patrick Rudolph, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46509
to look at the new patch set (#2).
Change subject: mrc_cache: Add config MRC_SAVE_HASH_IN_TPM ......................................................................
mrc_cache: Add config MRC_SAVE_HASH_IN_TPM
Use this config to specify whether we want to save a hash of the MRC_CACHE in the TPM NVRAM space. Replace all uses of FSP2_0_USES_TPM_MRC_HASH with MRC_SAVE_HASH_IN_TPM and remove the FSP2_0_USES_TPM_MRC_HASH config.
BUG=b:150502246 BRANCH=None TEST=emerge-nami coreboot chromeos-bootimage
Change-Id: Ic5ffcdba27cb1f09c39c3835029c8d9cc3453af1 Signed-off-by: Shelley Chen shchen@google.com --- M src/drivers/intel/fsp2_0/Kconfig M src/drivers/intel/fsp2_0/memory_init.c M src/drivers/mrc_cache/Kconfig M src/security/vboot/Kconfig M src/security/vboot/Makefile.inc 5 files changed, 15 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/09/46509/2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46509 )
Change subject: mrc_cache: Add config MRC_SAVE_HASH_IN_TPM ......................................................................
Patch Set 2: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/46509/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46509/2//COMMIT_MSG@13 PS2, Line 13: Can you please add here that this is dropping the selection of MRC_SAVE_HASH_IN_TPM Kconfig for TPM1 as none of the TPM1 platforms use FSP2.0 and have recovery MRC cache.
Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth, Andrey Petrov, Patrick Rudolph, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46509
to look at the new patch set (#3).
Change subject: mrc_cache: Add config MRC_SAVE_HASH_IN_TPM ......................................................................
mrc_cache: Add config MRC_SAVE_HASH_IN_TPM
Use this config to specify whether we want to save a hash of the MRC_CACHE in the TPM NVRAM space. Replace all uses of FSP2_0_USES_TPM_MRC_HASH with MRC_SAVE_HASH_IN_TPM and remove the FSP2_0_USES_TPM_MRC_HASH config. Note that TPM1 platforms will not select MRC_SAVE_HASH_IN_TPM as none of them use FSP2.0 and have recovery MRC_CACHE.
BUG=b:150502246 BRANCH=None TEST=emerge-nami coreboot chromeos-bootimage
Change-Id: Ic5ffcdba27cb1f09c39c3835029c8d9cc3453af1 Signed-off-by: Shelley Chen shchen@google.com --- M src/drivers/intel/fsp2_0/Kconfig M src/drivers/intel/fsp2_0/memory_init.c M src/drivers/mrc_cache/Kconfig M src/security/vboot/Kconfig M src/security/vboot/Makefile.inc 5 files changed, 15 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/09/46509/3
Shelley Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46509 )
Change subject: mrc_cache: Add config MRC_SAVE_HASH_IN_TPM ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46509/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46509/2//COMMIT_MSG@13 PS2, Line 13:
Can you please add here that this is dropping the selection of MRC_SAVE_HASH_IN_TPM Kconfig for TPM1 […]
Done
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46509 )
Change subject: mrc_cache: Add config MRC_SAVE_HASH_IN_TPM ......................................................................
Patch Set 3:
(4 comments)
https://review.coreboot.org/c/coreboot/+/46509/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46509/2//COMMIT_MSG@13 PS2, Line 13:
Done
Well, technically there are a couple (e.g. Lenovo t530) which do. But I don't think anyone is using vboot with "real" recovery mode on there so I doubt they care.
https://review.coreboot.org/c/coreboot/+/46509/2/src/drivers/intel/fsp2_0/Kc... File src/drivers/intel/fsp2_0/Kconfig:
https://review.coreboot.org/c/coreboot/+/46509/2/src/drivers/intel/fsp2_0/Kc... PS2, Line 6: select MRC_SAVE_HASH_IN_TPM if HAS_RECOVERY_MRC_CACHE && TPM2 Does this need to have a select here? (The previous version also just used default y, not select, so it was overrideable.)
I think you should just make the new option default y too and then you shouldn't need this line here.
https://review.coreboot.org/c/coreboot/+/46509/2/src/drivers/mrc_cache/Kconf... File src/drivers/mrc_cache/Kconfig:
https://review.coreboot.org/c/coreboot/+/46509/2/src/drivers/mrc_cache/Kconf... PS2, Line 54: VBOOT This should be VBOOT_STARTS_IN_BOOTBLOCK because the mrc_cache.c code is enforcing that.
https://review.coreboot.org/c/coreboot/+/46509/2/src/drivers/mrc_cache/Kconf... PS2, Line 55: n Why not just make this default y? I think we'll probably want this on all future devices.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46509 )
Change subject: mrc_cache: Add config MRC_SAVE_HASH_IN_TPM ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46509/2/src/drivers/mrc_cache/Kconf... File src/drivers/mrc_cache/Kconfig:
https://review.coreboot.org/c/coreboot/+/46509/2/src/drivers/mrc_cache/Kconf... PS2, Line 54: VBOOT
This should be VBOOT_STARTS_IN_BOOTBLOCK because the mrc_cache.c code is enforcing that.
Oh I guess you removed that in the next patch? I agree that I'm not sure why that restriction was there... but if you want to remove that, you should do it in this patch already to keep things in sync.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46509 )
Change subject: mrc_cache: Add config MRC_SAVE_HASH_IN_TPM ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46509/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46509/2//COMMIT_MSG@13 PS2, Line 13:
Well, technically there are a couple (e.g. Lenovo t530) which do. […]
t530 is INTEL_SANDYBRIDGE which doesn't really use FSP2.0.
https://review.coreboot.org/c/coreboot/+/46509/2/src/drivers/mrc_cache/Kconf... File src/drivers/mrc_cache/Kconfig:
https://review.coreboot.org/c/coreboot/+/46509/2/src/drivers/mrc_cache/Kconf... PS2, Line 55: n
Why not just make this default y? I think we'll probably want this on all future devices.
Probably do it only if CHROMEOS is selected? I don't think we want to change the default for any non-Chrome OS FSP2.0 platforms.
Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth, Andrey Petrov, Patrick Rudolph, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46509
to look at the new patch set (#4).
Change subject: mrc_cache: Add config MRC_SAVE_HASH_IN_TPM ......................................................................
mrc_cache: Add config MRC_SAVE_HASH_IN_TPM
Use this config to specify whether we want to save a hash of the MRC_CACHE in the TPM NVRAM space. Replace all uses of FSP2_0_USES_TPM_MRC_HASH with MRC_SAVE_HASH_IN_TPM and remove the FSP2_0_USES_TPM_MRC_HASH config. Note that TPM1 platforms will not select MRC_SAVE_HASH_IN_TPM as none of them use FSP2.0 and have recovery MRC_CACHE.
BUG=b:150502246 BRANCH=None TEST=emerge-nami coreboot chromeos-bootimage
Change-Id: Ic5ffcdba27cb1f09c39c3835029c8d9cc3453af1 Signed-off-by: Shelley Chen shchen@google.com --- M src/drivers/intel/fsp2_0/Kconfig M src/drivers/intel/fsp2_0/memory_init.c M src/drivers/mrc_cache/Kconfig M src/security/vboot/Kconfig M src/security/vboot/Makefile.inc 5 files changed, 15 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/09/46509/4
Shelley Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46509 )
Change subject: mrc_cache: Add config MRC_SAVE_HASH_IN_TPM ......................................................................
Patch Set 4:
(3 comments)
https://review.coreboot.org/c/coreboot/+/46509/2/src/drivers/intel/fsp2_0/Kc... File src/drivers/intel/fsp2_0/Kconfig:
https://review.coreboot.org/c/coreboot/+/46509/2/src/drivers/intel/fsp2_0/Kc... PS2, Line 6: select MRC_SAVE_HASH_IN_TPM if HAS_RECOVERY_MRC_CACHE && TPM2
Does this need to have a select here? (The previous version also just used default y, not select, so […]
Removed.
https://review.coreboot.org/c/coreboot/+/46509/2/src/drivers/mrc_cache/Kconf... File src/drivers/mrc_cache/Kconfig:
https://review.coreboot.org/c/coreboot/+/46509/2/src/drivers/mrc_cache/Kconf... PS2, Line 54: VBOOT
Oh I guess you removed that in the next patch? I agree that I'm not sure why that restriction was th […]
No I didn't? Changed to VBOOT_STARTS_IN_BOOTBLOCK now though.
https://review.coreboot.org/c/coreboot/+/46509/2/src/drivers/mrc_cache/Kconf... PS2, Line 55: n
Probably do it only if CHROMEOS is selected? I don't think we want to change the default for any non […]
Agree we should default y the future boards. Changed to reflect that for ChromeOS boards.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46509 )
Change subject: mrc_cache: Add config MRC_SAVE_HASH_IN_TPM ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46509/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46509/2//COMMIT_MSG@13 PS2, Line 13:
t530 is INTEL_SANDYBRIDGE which doesn't really use FSP2.0.
Whoops sorry, must have confused some things as I was digging through Kconfigs there. Looks like you're right, there's no device using this with TPM1.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46509 )
Change subject: mrc_cache: Add config MRC_SAVE_HASH_IN_TPM ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46509/2/src/drivers/mrc_cache/Kconf... File src/drivers/mrc_cache/Kconfig:
https://review.coreboot.org/c/coreboot/+/46509/2/src/drivers/mrc_cache/Kconf... PS2, Line 54: VBOOT
No I didn't? Changed to VBOOT_STARTS_IN_BOOTBLOCK now though.
You are removing the _Static_assert() for that from https://review.coreboot.org/c/coreboot/+/46510/6/src/drivers/intel/fsp2_0/me... and I don't see it reappear anywhere else. If you're not intentionally removing that, you should move it to mrc_cache.c (but like I said I'm not really sure why we're mandating that).
https://review.coreboot.org/c/coreboot/+/46509/2/src/drivers/mrc_cache/Kconf... PS2, Line 55: n
Agree we should default y the future boards. Changed to reflect that for ChromeOS boards.
Not sure about the Chrome OS only part to be honest, I don't think this question has anything to do with Chrome OS. If we think this is a good thing to have for our boards, why shouldn't it be a good thing to have for other boards (that match the requirements)? Of course anyone can override it as they want (actually might be worth making this user-selectable in menuconfig... need to put a name string behind the 'bool' to do that), but by default I think if it's useful then it should be default-on everywhere. (Like you said it currently only affects TPM2 devices which are all Chrome OS right now, so we're not immediately affecting anyone else anyway.)
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46509 )
Change subject: mrc_cache: Add config MRC_SAVE_HASH_IN_TPM ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46509/2/src/drivers/mrc_cache/Kconf... File src/drivers/mrc_cache/Kconfig:
https://review.coreboot.org/c/coreboot/+/46509/2/src/drivers/mrc_cache/Kconf... PS2, Line 55: n I agree that this has nothing to do with Chrome OS only. The suggestion was to ensure that we do not change the behavior for any of the boards without the owners deciding/understanding this. Yes, it is a good thing to have for all boards. At minimum, I think we should send out a PSA on the mailing list to ensure that the community knows that this change is coming.
Like you said it currently only affects TPM2 devices which are all Chrome OS right now, so we're not immediately affecting anyone else anyway.
That is not really true? I see the following boards using TPM2: ``` git grep "MAINBOARD_HAS_TPM2" src/mainboard/ | grep -v google src/mainboard/clevo/cml-u/Kconfig: select MAINBOARD_HAS_TPM2 src/mainboard/facebook/fbg1701/Kconfig: select MAINBOARD_HAS_TPM2 src/mainboard/facebook/monolith/Kconfig: select MAINBOARD_HAS_TPM2 src/mainboard/intel/galileo/Kconfig: select MAINBOARD_HAS_TPM2 src/mainboard/intel/jasperlake_rvp/Kconfig: select VBOOT_MOCK_SECDATA if !MAINBOARD_HAS_TPM2 src/mainboard/ocp/deltalake/Kconfig: select MAINBOARD_HAS_TPM2 src/mainboard/prodrive/hermes/Kconfig: select MAINBOARD_HAS_TPM2 src/mainboard/razer/blade_stealth_kbl/Kconfig: select MAINBOARD_HAS_TPM2 src/mainboard/siemens/mc_apl1/variants/mc_apl2/Kconfig: select MAINBOARD_HAS_TPM2 src/mainboard/siemens/mc_apl1/variants/mc_apl4/Kconfig: select MAINBOARD_HAS_TPM2 src/mainboard/siemens/mc_apl1/variants/mc_apl5/Kconfig: select MAINBOARD_HAS_TPM2 src/mainboard/siemens/mc_apl1/variants/mc_apl6/Kconfig: select MAINBOARD_HAS_TPM2 src/mainboard/system76/lemp9/Kconfig: select MAINBOARD_HAS_TPM2 ```
Shelley Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46509 )
Change subject: mrc_cache: Add config MRC_SAVE_HASH_IN_TPM ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46509/2/src/drivers/mrc_cache/Kconf... File src/drivers/mrc_cache/Kconfig:
https://review.coreboot.org/c/coreboot/+/46509/2/src/drivers/mrc_cache/Kconf... PS2, Line 54: VBOOT
You are removing the _Static_assert() for that from https://review.coreboot. […]
Oops. That was unintentional. Added it back into https://review.coreboot.org/c/coreboot/+/46432
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46509 )
Change subject: mrc_cache: Add config MRC_SAVE_HASH_IN_TPM ......................................................................
Patch Set 4: Code-Review+2
(2 comments)
LGTM once the two comments are resolved: 1. Making option user visible 2. Do we want to drop the "if CHROMEOS" condition for the config. If yes, can you please send out a PSA to coreboot mailing list?
https://review.coreboot.org/c/coreboot/+/46509/2/src/drivers/mrc_cache/Kconf... File src/drivers/mrc_cache/Kconfig:
https://review.coreboot.org/c/coreboot/+/46509/2/src/drivers/mrc_cache/Kconf... PS2, Line 55: n
I agree that this has nothing to do with Chrome OS only. […]
I am looking at all TPM2 boards and not just the ones using FSP2.0+ because by the end of this patch series, we would have this Kconfig enabling hash spaces for both recovery and non-recovery cases and the calls moved to mrc_cache driver. So, it will add this new space for all the current TPM2 boards.
https://review.coreboot.org/c/coreboot/+/46509/4/src/drivers/mrc_cache/Kconf... File src/drivers/mrc_cache/Kconfig:
https://review.coreboot.org/c/coreboot/+/46509/4/src/drivers/mrc_cache/Kconf... PS4, Line 53: bool Julius had a comment on earlier patchset that this option should be made user visible.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46509 )
Change subject: mrc_cache: Add config MRC_SAVE_HASH_IN_TPM ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46509/4/src/drivers/intel/fsp2_0/me... File src/drivers/intel/fsp2_0/memory_init.c:
https://review.coreboot.org/c/coreboot/+/46509/4/src/drivers/intel/fsp2_0/me... PS4, Line 30: _Static_assert(!CONFIG(MRC_SAVE_HASH_IN_TPM) || This shouldn't remain in this file now though, because it's not specific to FSP 2.0. If you want to keep it, it should be moved to one of the MRC cache files.
https://review.coreboot.org/c/coreboot/+/46509/4/src/drivers/mrc_cache/Kconf... File src/drivers/mrc_cache/Kconfig:
https://review.coreboot.org/c/coreboot/+/46509/4/src/drivers/mrc_cache/Kconf... PS4, Line 55: default y if CHROMEOS Yes, please let's remove the CHROMEOS. Feel free to send out a PSA on the mailing list (although generally we make changes to vboot all the time, I'm not sure if this is a particularly big deal).
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46509 )
Change subject: mrc_cache: Add config MRC_SAVE_HASH_IN_TPM ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46509/4/src/drivers/intel/fsp2_0/me... File src/drivers/intel/fsp2_0/memory_init.c:
https://review.coreboot.org/c/coreboot/+/46509/4/src/drivers/intel/fsp2_0/me... PS4, Line 30: _Static_assert(!CONFIG(MRC_SAVE_HASH_IN_TPM) ||
This shouldn't remain in this file now though, because it's not specific to FSP 2.0. […]
Oh I see you're moving it next patch. Sorry for the noise.
Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth, Andrey Petrov, Patrick Rudolph, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46509
to look at the new patch set (#5).
Change subject: mrc_cache: Add config MRC_SAVE_HASH_IN_TPM ......................................................................
mrc_cache: Add config MRC_SAVE_HASH_IN_TPM
Use this config to specify whether we want to save a hash of the MRC_CACHE in the TPM NVRAM space. Replace all uses of FSP2_0_USES_TPM_MRC_HASH with MRC_SAVE_HASH_IN_TPM and remove the FSP2_0_USES_TPM_MRC_HASH config. Note that TPM1 platforms will not select MRC_SAVE_HASH_IN_TPM as none of them use FSP2.0 and have recovery MRC_CACHE.
BUG=b:150502246 BRANCH=None TEST=emerge-nami coreboot chromeos-bootimage
Change-Id: Ic5ffcdba27cb1f09c39c3835029c8d9cc3453af1 Signed-off-by: Shelley Chen shchen@google.com --- M src/drivers/intel/fsp2_0/Kconfig M src/drivers/intel/fsp2_0/memory_init.c M src/drivers/mrc_cache/Kconfig M src/security/vboot/Kconfig M src/security/vboot/Makefile.inc 5 files changed, 14 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/09/46509/5
Shelley Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46509 )
Change subject: mrc_cache: Add config MRC_SAVE_HASH_IN_TPM ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46509/4/src/drivers/mrc_cache/Kconf... File src/drivers/mrc_cache/Kconfig:
https://review.coreboot.org/c/coreboot/+/46509/4/src/drivers/mrc_cache/Kconf... PS4, Line 53: bool
Julius had a comment on earlier patchset that this option should be made user visible.
Done
https://review.coreboot.org/c/coreboot/+/46509/4/src/drivers/mrc_cache/Kconf... PS4, Line 55: default y if CHROMEOS
Yes, please let's remove the CHROMEOS. […]
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46509 )
Change subject: mrc_cache: Add config MRC_SAVE_HASH_IN_TPM ......................................................................
Patch Set 5: Code-Review+2
Julius Werner has submitted this change. ( https://review.coreboot.org/c/coreboot/+/46509 )
Change subject: mrc_cache: Add config MRC_SAVE_HASH_IN_TPM ......................................................................
mrc_cache: Add config MRC_SAVE_HASH_IN_TPM
Use this config to specify whether we want to save a hash of the MRC_CACHE in the TPM NVRAM space. Replace all uses of FSP2_0_USES_TPM_MRC_HASH with MRC_SAVE_HASH_IN_TPM and remove the FSP2_0_USES_TPM_MRC_HASH config. Note that TPM1 platforms will not select MRC_SAVE_HASH_IN_TPM as none of them use FSP2.0 and have recovery MRC_CACHE.
BUG=b:150502246 BRANCH=None TEST=emerge-nami coreboot chromeos-bootimage
Change-Id: Ic5ffcdba27cb1f09c39c3835029c8d9cc3453af1 Signed-off-by: Shelley Chen shchen@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/46509 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Furquan Shaikh furquan@google.com --- M src/drivers/intel/fsp2_0/Kconfig M src/drivers/intel/fsp2_0/memory_init.c M src/drivers/mrc_cache/Kconfig M src/security/vboot/Kconfig M src/security/vboot/Makefile.inc 5 files changed, 14 insertions(+), 22 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved
diff --git a/src/drivers/intel/fsp2_0/Kconfig b/src/drivers/intel/fsp2_0/Kconfig index 1a1da6c..ad7afd8 100644 --- a/src/drivers/intel/fsp2_0/Kconfig +++ b/src/drivers/intel/fsp2_0/Kconfig @@ -141,23 +141,6 @@ own stack that will be placed in DRAM and not in CAR, this is the amount of memory the FSP needs for its stack and heap.
-config FSP2_0_USES_TPM_MRC_HASH - bool - depends on TPM1 || TPM2 - depends on VBOOT && VBOOT_STARTS_IN_BOOTBLOCK - default y if HAS_RECOVERY_MRC_CACHE - default n - select VBOOT_HAS_REC_HASH_SPACE - help - Store hash of trained recovery MRC cache in NVRAM space in TPM. - Use the hash to validate recovery MRC cache before using it. - This hash needs to be updated every time recovery mode training - is recomputed, or if the hash does not match recovery MRC cache. - Selecting this option requires that TPM already be setup by this - point in time. Thus it is only compatible when the option - VBOOT_STARTS_IN_BOOTBLOCK is selected, which causes verstage and - TPM setup to occur prior to memory initialization. - config FSP_PLATFORM_MEMORY_SETTINGS_VERSIONS bool help diff --git a/src/drivers/intel/fsp2_0/memory_init.c b/src/drivers/intel/fsp2_0/memory_init.c index 14aec98..09aad6b 100644 --- a/src/drivers/intel/fsp2_0/memory_init.c +++ b/src/drivers/intel/fsp2_0/memory_init.c @@ -19,15 +19,15 @@ #include <symbols.h> #include <timestamp.h> #include <security/vboot/vboot_common.h> -#include <security/tpm/tspi.h> #include <security/vboot/mrc_cache_hash_tpm.h> +#include <security/tpm/tspi.h> #include <vb2_api.h> #include <types.h>
static uint8_t temp_ram[CONFIG_FSP_TEMP_RAM_SIZE] __aligned(sizeof(uint64_t));
/* TPM MRC hash functionality depends on vboot starting before memory init. */ -_Static_assert(!CONFIG(FSP2_0_USES_TPM_MRC_HASH) || +_Static_assert(!CONFIG(MRC_SAVE_HASH_IN_TPM) || CONFIG(VBOOT_STARTS_IN_BOOTBLOCK), "for TPM MRC hash functionality, vboot must start in bootblock");
@@ -55,7 +55,7 @@ mrc_data_size) < 0) printk(BIOS_ERR, "Failed to stash MRC data\n");
- if (CONFIG(FSP2_0_USES_TPM_MRC_HASH)) + if (CONFIG(MRC_SAVE_HASH_IN_TPM)) mrc_cache_update_hash(mrc_data, mrc_data_size); }
@@ -121,7 +121,7 @@ if (data == NULL) return;
- if (CONFIG(FSP2_0_USES_TPM_MRC_HASH) && + if (CONFIG(MRC_SAVE_HASH_IN_TPM) && !mrc_cache_verify_hash(data, mrc_size)) return;
diff --git a/src/drivers/mrc_cache/Kconfig b/src/drivers/mrc_cache/Kconfig index e09c5d8..b09c196 100644 --- a/src/drivers/mrc_cache/Kconfig +++ b/src/drivers/mrc_cache/Kconfig @@ -49,4 +49,12 @@ that need to write back the MRC data in late ramstage boot states (MRC_WRITE_NV_LATE).
+config MRC_SAVE_HASH_IN_TPM + bool "Save a hash of the MRC_CACHE data in TPM NVRAM" + depends on VBOOT_STARTS_IN_BOOTBLOCK && TPM2 && !TPM1 + default y + help + Store a hash of the MRC_CACHE training data in a TPM NVRAM + space to ensure that it cannot be tampered with. + endif # CACHE_MRC_SETTINGS diff --git a/src/security/vboot/Kconfig b/src/security/vboot/Kconfig index ee8d36a..094cbb9 100644 --- a/src/security/vboot/Kconfig +++ b/src/security/vboot/Kconfig @@ -159,6 +159,7 @@
config VBOOT_HAS_REC_HASH_SPACE bool + default y if MRC_SAVE_HASH_IN_TPM && HAS_RECOVERY_MRC_CACHE default n help Set this option to indicate to vboot that recovery data hash space diff --git a/src/security/vboot/Makefile.inc b/src/security/vboot/Makefile.inc index e92396d..d4dabe2 100644 --- a/src/security/vboot/Makefile.inc +++ b/src/security/vboot/Makefile.inc @@ -118,7 +118,7 @@ ramstage-y += common.c postcar-y += common.c
-romstage-$(CONFIG_FSP2_0_USES_TPM_MRC_HASH) += mrc_cache_hash_tpm.c +romstage-$(CONFIG_MRC_SAVE_HASH_IN_TPM) += mrc_cache_hash_tpm.c
ifeq ($(CONFIG_VBOOT_SEPARATE_VERSTAGE),y)
-
Furquan Shaikh (Code Review) -
Julius Werner (Code Review)
-
Shelley Chen (Code Review)