Change in coreboot[master]: [WIP] security/vboot: Remove selected_region from struct vboot_workin...
Yu-Ping Wu has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36847 )
Change subject: [WIP] security/vboot: Remove selected_region from struct vboot_working_data ......................................................................
[WIP] security/vboot: Remove selected_region from struct vboot_working_data
vboot_is_slot_selected() implies vboot_set_selected_region() is called, which implies (rv != VB2_ERROR_API_PHASE1_RECOVERY) in verstage_main(), which implies ctx->flags does not contain VB2_CONTEXT_RECOVERY_MODE in vb2api_fw_phase1(), which implies sd->recovery_reason is 0 in vb2_check_recovery().
As a result, we can safely remove the vboot_is_slot_selected() check.
BRANCH=none BUG=chromium:1021452 TEST=emerge-kukui coreboot
Change-Id: I501110582669ec4f5c1a009f3fd0c0e310083496 Signed-off-by: Yu-Ping Wu yupingso@chromium.org --- M src/security/vboot/bootmode.c 1 file changed, 1 insertion(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/36847/1
diff --git a/src/security/vboot/bootmode.c b/src/security/vboot/bootmode.c index bc89e73..30d0241 100644 --- a/src/security/vboot/bootmode.c +++ b/src/security/vboot/bootmode.c @@ -113,8 +113,7 @@ * Identify if vboot verification is already complete and no slot * was selected i.e. recovery path was requested. */ - if (vboot_possibly_executed() && vboot_logic_executed() && - !vboot_is_slot_selected()) + if (vboot_possibly_executed() && vboot_logic_executed()) return vboot_get_recovery_reason_shared_data();
return 0;
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36847 )
Change subject: [WIP] security/vboot: Remove selected_region from struct vboot_working_data ......................................................................
Patch Set 1:
(1 comment)
Maybe just merge this into the previous CL and get rid of the whole selected_region field right away?
https://review.coreboot.org/c/coreboot/+/36847/1/src/security/vboot/bootmode... File src/security/vboot/bootmode.c:
https://review.coreboot.org/c/coreboot/+/36847/1/src/security/vboot/bootmode... PS1, Line 114: * was selected i.e. recovery path was requested. nit: comment needs update
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36847 )
Change subject: [WIP] security/vboot: Remove selected_region from struct vboot_working_data ......................................................................
Patch Set 1:
I think your logic makes sense. We're basically removing vboot_is_slot_selected() and directly reading recovery_reason instead, since we know that a slot will be selected iff a recovery reason is set.
I agree with Julius that we may want to handle all of the vboot_{set,get}_selected_region and vboot_is_slot_selected refactoring in one single CL.
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36847 )
Change subject: [WIP] security/vboot: Remove selected_region from struct vboot_working_data ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36847/1/src/security/vboot/bootmode... File src/security/vboot/bootmode.c:
https://review.coreboot.org/c/coreboot/+/36847/1/src/security/vboot/bootmode... PS1, Line 27: vboot_get_recovery_reason_shared_data We should also start thinking about how to clean up this recovery_reason business. Why do we need to independently read the recovery reason stored in nvdata and the one from vb2_shared_data? After introducing persistent context, can we just rely on the latter?
What kind of API do we want to use to expose recovery_reason from vboot? Do we need a vb2api_get_recovery_reason()? And a vb2api_clear_recovery_reason()?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36847 )
Change subject: [WIP] security/vboot: Remove selected_region from struct vboot_working_data ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36847/1/src/security/vboot/bootmode... File src/security/vboot/bootmode.c:
https://review.coreboot.org/c/coreboot/+/36847/1/src/security/vboot/bootmode... PS1, Line 27: vboot_get_recovery_reason_shared_data
We should also start thinking about how to clean up this recovery_reason business. […]
First of all, we should fix these ugly x86 hacks of saving/restoring the recovery reason in various cases. coreboot shouldn't have to deal with any of that stuff. We should just clear the recovery reason at a later point (e.g. right before entering the recovery UI) so that when early platform init needs to reboot for whatever reason, it doesn't affect vboot state.
After that, we should evaluate if we really need anything outside of vboot to know the recovery reason at all anymore. The one more thing I'm aware of is the MEMORY_RETRAIN stuff, but that might be better served by a context flag instead. If we do decide we need it, then yeah, just a vb2_get_recovery_reason() is probably best.
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36847 )
Change subject: [WIP] security/vboot: Remove selected_region from struct vboot_working_data ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36847/1/src/security/vboot/bootmode... File src/security/vboot/bootmode.c:
https://review.coreboot.org/c/coreboot/+/36847/1/src/security/vboot/bootmode... PS1, Line 114: * was selected i.e. recovery path was requested.
nit: comment needs update
Done
Yu-Ping Wu has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/36847 )
Change subject: [WIP] security/vboot: Remove selected_region from struct vboot_working_data ......................................................................
Abandoned
Merged into CB:36845
-
Joel Kitching (Code Review) -
Julius Werner (Code Review)
-
Yu-Ping Wu (Code Review)