Change in ...coreboot[master]: vboot: relocate code to log and clear recovery mode switch
Joel Kitching has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33534
Change subject: vboot: relocate code to log and clear recovery mode switch ......................................................................
vboot: relocate code to log and clear recovery mode switch
Logging and clearing the recovery mode switch doesn't have anything to do with vboot_handoff. Move it to the main verstage logic file.
BUG=b:124141368, b:124192753 TEST=make clean && make test-abuild BRANCH=none
Change-Id: I2e74f3893463e43fe5fad4a8df8036560f34e0db Signed-off-by: Joel Kitching kitching@google.com --- M src/security/vboot/Makefile.inc M src/security/vboot/vboot_handoff.c M src/security/vboot/vboot_logic.c 3 files changed, 27 insertions(+), 15 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/33534/1
diff --git a/src/security/vboot/Makefile.inc b/src/security/vboot/Makefile.inc index 9ce724e..3306f41 100644 --- a/src/security/vboot/Makefile.inc +++ b/src/security/vboot/Makefile.inc @@ -88,6 +88,7 @@ verstage-y += secdata_tpm.c romstage-$(CONFIG_VBOOT_SEPARATE_VERSTAGE) += secdata_tpm.c endif +romstage-y += vboot_logic.c romstage-y += vboot_handoff.c common.c
ramstage-y += common.c diff --git a/src/security/vboot/vboot_handoff.c b/src/security/vboot/vboot_handoff.c index 8a6b3d6..19773c54 100644 --- a/src/security/vboot/vboot_handoff.c +++ b/src/security/vboot/vboot_handoff.c @@ -122,21 +122,6 @@
/* needed until we finish transtion to vboot2 for kernel verification */ fill_vboot_handoff(vh, sd); - - - /* Log the recovery mode switches if required, before clearing them. */ - log_recovery_mode_switch(); - - /* - * The recovery mode switch is cleared (typically backed by EC) here - * to allow multiple queries to get_recovery_mode_switch() and have - * them return consistent results during the verified boot path as well - * as dram initialization. x86 systems ignore the saved dram settings - * in the recovery path in order to start from a clean slate. Therefore - * clear the state here since this function is called when memory - * is known to be up. - */ - clear_recovery_mode_switch(); }
/* diff --git a/src/security/vboot/vboot_logic.c b/src/security/vboot/vboot_logic.c index 7b98be2..62e033a 100644 --- a/src/security/vboot/vboot_logic.c +++ b/src/security/vboot/vboot_logic.c @@ -16,6 +16,7 @@ #include <arch/exception.h> #include <assert.h> #include <bootmode.h> +#include <cbmem.h> #include <console/console.h> #include <console/vtxprintf.h> #include <string.h> @@ -281,6 +282,26 @@ vboot_extend_pcr(ctx, 1, HWID_DIGEST_PCR); }
+static void vboot_log_and_clear_recovery_mode_switch(int unused) +{ + /* Log the recovery mode switches if required, before clearing them. */ + log_recovery_mode_switch(); + + /* + * The recovery mode switch is cleared (typically backed by EC) here + * to allow multiple queries to get_recovery_mode_switch() and have + * them return consistent results during the verified boot path as well + * as dram initialization. x86 systems ignore the saved dram settings + * in the recovery path in order to start from a clean slate. Therefore + * clear the state here since this function is called when memory + * is known to be up. + */ + clear_recovery_mode_switch(); +} +#if !CONFIG(VBOOT_STARTS_IN_ROMSTAGE) +ROMSTAGE_CBMEM_INIT_HOOK(vboot_log_and_clear_recovery_mode_switch) +#endif + /** * Verify and select the firmware in the RW image * @@ -448,6 +469,11 @@ vboot_set_selected_region(region_device_region(&fw_main));
verstage_main_exit: + /* If CBMEM is not up yet, let the ROMSTAGE_CBMEM_INIT_HOOK take care + of running this function. */ + if (ENV_ROMSTAGE && CONFIG(VBOOT_STARTS_IN_ROMSTAGE)) + vboot_log_and_clear_recovery_mode_switch(0); + vboot_finalize_work_context(&ctx); timestamp_add_now(TS_END_VBOOT); }
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33534 )
Change subject: vboot: relocate code to log and clear recovery mode switch ......................................................................
Patch Set 1: Code-Review+2
(1 comment)
https://review.coreboot.org/#/c/33534/1/src/security/vboot/vboot_logic.c File src/security/vboot/vboot_logic.c:
https://review.coreboot.org/#/c/33534/1/src/security/vboot/vboot_logic.c@285 PS1, Line 285: vboot_log_and_clear_recovery_mode_switch Just thinking out loud: This should have the same effect as before because - 1. !VBOOT_STARTS_IN_ROMSTAGE --> Earlier it was done in vboot_fill_handoff which was a cbmem init hook and now vboot_log_and_clear_recovery_mode_switch is run in cbmem init hook of its own.
2. VBOOT_STARTS_IN_ROMSTAGE --> vboot_fill_handoff was called after verstage_main returns and recovery reason is saved to VBNV. Now, vboot_log_and_clear_recovery_mode_switch is called at the end of verstage main and before recovery reason is saved to VBNV. However, saving of VBNV should be independent of logging/clearing recovery switches.
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33534 )
Change subject: vboot: relocate code to log and clear recovery mode switch ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/33534/1/src/security/vboot/vboot_logic.c File src/security/vboot/vboot_logic.c:
https://review.coreboot.org/#/c/33534/1/src/security/vboot/vboot_logic.c@285 PS1, Line 285: vboot_log_and_clear_recovery_mode_switch
Just thinking out loud: This should have the same effect as before because - […]
Two questions about saving recovery reason to VBNV:
(1) In vboot_prepare, vboot_save_recovery_reason_vbnv is only called in the verification_should_run() clause, and not in the verstage_should_load() clause. Why is that?
(2) Depending on the answer to (1), should we be calling vboot_save_recovery_reason_vbnv from verstage_main instead?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33534 )
Change subject: vboot: relocate code to log and clear recovery mode switch ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/33534/1/src/security/vboot/vboot_logic.c File src/security/vboot/vboot_logic.c:
https://review.coreboot.org/#/c/33534/1/src/security/vboot/vboot_logic.c@285 PS1, Line 285: vboot_log_and_clear_recovery_mode_switch
Two questions about saving recovery reason to VBNV: […]
From the history of how vboot_save_recovery_reason_vbnv() was added, it looks like it was done to address an issue on x86 platforms specifically. These platforms never return from prog_run() in verification_should_load() clause and so the vboot_save_recovery_reason_vbnv() ended up being added to verification_should_run() clause only. vboot_save_recovery_reason_vbnv truly does something only if VBOOT_SAVE_RECOVERY_REASON_ON_REBOOT is selected which is currently done only on x86 platforms. But I agree that in order to keep both paths consistent, it would make sense to take the action of saving recovery reason to vbnv in both clauses.
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33534 )
Change subject: vboot: relocate code to log and clear recovery mode switch ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/33534/1/src/security/vboot/vboot_logic.c File src/security/vboot/vboot_logic.c:
https://review.coreboot.org/#/c/33534/1/src/security/vboot/vboot_logic.c@285 PS1, Line 285: vboot_log_and_clear_recovery_mode_switch
From the history of how vboot_save_recovery_reason_vbnv() was added, it looks like it was done to ad […]
Added https://review.coreboot.org/c/coreboot/+/33551
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33534 )
Change subject: vboot: relocate code to log and clear recovery mode switch ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/#/c/33534/3/src/security/vboot/Makefile.inc File src/security/vboot/Makefile.inc:
https://review.coreboot.org/#/c/33534/3/src/security/vboot/Makefile.inc@91 PS3, Line 91: romstage-y += vboot_logic.c Everything else in vboot_logic is the "main" verification code that runs in verstage, so it seems odd to me to add something from a different stage there. I think bootmode.c or vboot_common.c would be better places for this.
https://review.coreboot.org/#/c/33534/3/src/security/vboot/vboot_logic.c File src/security/vboot/vboot_logic.c:
https://review.coreboot.org/#/c/33534/3/src/security/vboot/vboot_logic.c@302 PS3, Line 302: ROMSTAGE_CBMEM_INIT_HOOK(vboot_log_and_clear_recovery_mode_switch) Only x86 devices have elog available in romstage. This needs to be done in ramstage. (And it should be done for the VBOOT_STARTS_IN_ROMSTAGE case as well, even if we didn't use to do that.)
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33534 )
Change subject: vboot: relocate code to log and clear recovery mode switch ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/33534/3/src/security/vboot/vboot_logic.c File src/security/vboot/vboot_logic.c:
https://review.coreboot.org/#/c/33534/3/src/security/vboot/vboot_logic.c@302 PS3, Line 302: ROMSTAGE_CBMEM_INIT_HOOK(vboot_log_and_clear_recovery_mode_switch)
Only x86 devices have elog available in romstage. This needs to be done in ramstage. […]
Hmm... I just re-read how this actually works and that it has always been in romstage. But the whole thing seems like a pretty horrible design with having to plumb it through CBMEM first. Why are we doing all of this in the first place? Is it really just for get_recovery_mode_switch() like that comment says? Nobody other than verstage_main() should ever be calling get_recovery_mode_switch() in the first place! Code running in romstage should just check working data and nothing else.
I guess we can submit this as a clean move without changing behavior for now, but it would be great if you could clean the whole thing up a bit in a follow-up patch.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33534 )
Change subject: vboot: relocate code to log and clear recovery mode switch ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/33534/3/src/security/vboot/vboot_logic.c File src/security/vboot/vboot_logic.c:
https://review.coreboot.org/#/c/33534/3/src/security/vboot/vboot_logic.c@302 PS3, Line 302: ROMSTAGE_CBMEM_INIT_HOOK(vboot_log_and_clear_recovery_mode_switch)
Why are we doing all of this in the first place?
Do you mean clearing of recovery request here instead of some place else like when the switches are actually read from the EC? I think it is probably because the get recovery mode function has changed a lot in past few years and at the time this was added probably that was the source of truth for identifying recovery mode. I agree that get_recovery-mode_switch() should not be used by anything other than verstage_main(). It might be good to clean this up in a follow up CL and test it out to ensure no flows break.
Julius Werner has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33534 )
Change subject: vboot: relocate code to log and clear recovery mode switch ......................................................................
vboot: relocate code to log and clear recovery mode switch
Logging and clearing the recovery mode switch doesn't have anything to do with vboot_handoff. Move it to the main verstage logic file.
BUG=b:124141368, b:124192753 TEST=make clean && make test-abuild BRANCH=none
Change-Id: I2e74f3893463e43fe5fad4a8df8036560f34e0db Signed-off-by: Joel Kitching kitching@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/33534 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Furquan Shaikh furquan@google.com --- M src/security/vboot/Makefile.inc M src/security/vboot/vboot_handoff.c M src/security/vboot/vboot_logic.c 3 files changed, 27 insertions(+), 15 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved
diff --git a/src/security/vboot/Makefile.inc b/src/security/vboot/Makefile.inc index 9ce724e..3306f41 100644 --- a/src/security/vboot/Makefile.inc +++ b/src/security/vboot/Makefile.inc @@ -88,6 +88,7 @@ verstage-y += secdata_tpm.c romstage-$(CONFIG_VBOOT_SEPARATE_VERSTAGE) += secdata_tpm.c endif +romstage-y += vboot_logic.c romstage-y += vboot_handoff.c common.c
ramstage-y += common.c diff --git a/src/security/vboot/vboot_handoff.c b/src/security/vboot/vboot_handoff.c index 8a6b3d6..19773c54 100644 --- a/src/security/vboot/vboot_handoff.c +++ b/src/security/vboot/vboot_handoff.c @@ -122,21 +122,6 @@
/* needed until we finish transtion to vboot2 for kernel verification */ fill_vboot_handoff(vh, sd); - - - /* Log the recovery mode switches if required, before clearing them. */ - log_recovery_mode_switch(); - - /* - * The recovery mode switch is cleared (typically backed by EC) here - * to allow multiple queries to get_recovery_mode_switch() and have - * them return consistent results during the verified boot path as well - * as dram initialization. x86 systems ignore the saved dram settings - * in the recovery path in order to start from a clean slate. Therefore - * clear the state here since this function is called when memory - * is known to be up. - */ - clear_recovery_mode_switch(); }
/* diff --git a/src/security/vboot/vboot_logic.c b/src/security/vboot/vboot_logic.c index 7b98be2..62e033a 100644 --- a/src/security/vboot/vboot_logic.c +++ b/src/security/vboot/vboot_logic.c @@ -16,6 +16,7 @@ #include <arch/exception.h> #include <assert.h> #include <bootmode.h> +#include <cbmem.h> #include <console/console.h> #include <console/vtxprintf.h> #include <string.h> @@ -281,6 +282,26 @@ vboot_extend_pcr(ctx, 1, HWID_DIGEST_PCR); }
+static void vboot_log_and_clear_recovery_mode_switch(int unused) +{ + /* Log the recovery mode switches if required, before clearing them. */ + log_recovery_mode_switch(); + + /* + * The recovery mode switch is cleared (typically backed by EC) here + * to allow multiple queries to get_recovery_mode_switch() and have + * them return consistent results during the verified boot path as well + * as dram initialization. x86 systems ignore the saved dram settings + * in the recovery path in order to start from a clean slate. Therefore + * clear the state here since this function is called when memory + * is known to be up. + */ + clear_recovery_mode_switch(); +} +#if !CONFIG(VBOOT_STARTS_IN_ROMSTAGE) +ROMSTAGE_CBMEM_INIT_HOOK(vboot_log_and_clear_recovery_mode_switch) +#endif + /** * Verify and select the firmware in the RW image * @@ -448,6 +469,11 @@ vboot_set_selected_region(region_device_region(&fw_main));
verstage_main_exit: + /* If CBMEM is not up yet, let the ROMSTAGE_CBMEM_INIT_HOOK take care + of running this function. */ + if (ENV_ROMSTAGE && CONFIG(VBOOT_STARTS_IN_ROMSTAGE)) + vboot_log_and_clear_recovery_mode_switch(0); + vboot_finalize_work_context(&ctx); timestamp_add_now(TS_END_VBOOT); }
-
Furquan Shaikh (Code Review) -
Joel Kitching (Code Review)
-
Julius Werner (Code Review)