Yu-Ping Wu has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36845 )
Change subject: security/vboot: Remove vboot_set_selected_region() ......................................................................
security/vboot: Remove vboot_set_selected_region()
Since we already have pre-RAM cache for FMAP (CB:36657), calling load_firmware() multiple times is no longer a problem. This patch replaces vboot_get_selected_region() usage with vboot_locate_firmware(), which locates the firmware by reading from the CBMEM cache.
BRANCH=none BUG=chromium:1021452 TEST=emerge-kukui coreboot
Change-Id: I27cb1a2175beb189053fc3e44b17b60aba474bb0 Signed-off-by: Yu-Ping Wu yupingso@chromium.org --- M src/security/vboot/common.c M src/security/vboot/misc.h M src/security/vboot/vboot_loader.c M src/security/vboot/vboot_logic.c 4 files changed, 32 insertions(+), 45 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/36845/1
diff --git a/src/security/vboot/common.c b/src/security/vboot/common.c index ea22bdf..68960c9 100644 --- a/src/security/vboot/common.c +++ b/src/security/vboot/common.c @@ -83,23 +83,6 @@ return *vboot_ctx_ptr; }
-int vboot_get_selected_region(struct region *region) -{ - const struct selected_region *reg = - &vboot_get_working_data()->selected_region; - - if (reg == NULL) - return -1; - - if (reg->offset == 0 && reg->size == 0) - return -1; - - region->offset = reg->offset; - region->size = reg->size; - - return 0; -} - void vboot_set_selected_region(const struct region *region) { struct selected_region *reg = @@ -121,6 +104,24 @@ return reg->size > 0; }
+int vboot_is_slot_a(const struct vb2_context *ctx) +{ + return !(ctx->flags & VB2_CONTEXT_FW_SLOT_B); +} + +int vboot_locate_firmware(const struct vb2_context *ctx, + struct region_device *fw) +{ + const char *name; + + if (vboot_is_slot_a(ctx)) + name = "FW_MAIN_A"; + else + name = "FW_MAIN_B"; + + return vboot_named_region_device(name, fw); +} + #if CONFIG(VBOOT_STARTS_IN_BOOTBLOCK) /* * For platforms that do not employ VBOOT_STARTS_IN_ROMSTAGE, vboot diff --git a/src/security/vboot/misc.h b/src/security/vboot/misc.h index 6141674..6d40f16 100644 --- a/src/security/vboot/misc.h +++ b/src/security/vboot/misc.h @@ -47,12 +47,13 @@ struct vboot_working_data *vboot_get_working_data(void); struct vb2_context *vboot_get_context(void);
-/* Returns 0 on success. < 0 on failure. */ -int vboot_get_selected_region(struct region *region); - void vboot_set_selected_region(const struct region *region); int vboot_is_slot_selected(void);
+int vboot_is_slot_a(const struct vb2_context *ctx); +int vboot_locate_firmware(const struct vb2_context *ctx, + struct region_device *fw); + /* * Source: security/vboot/vboot_handoff.c */ diff --git a/src/security/vboot/vboot_loader.c b/src/security/vboot/vboot_loader.c index 3aac48d..016e9a5 100644 --- a/src/security/vboot/vboot_loader.c +++ b/src/security/vboot/vboot_loader.c @@ -73,17 +73,20 @@
static int vboot_locate(struct cbfs_props *props) { - struct region selected_region; + const struct vb2_context *ctx = vboot_get_context(); + struct region_device fw_main; + const struct region *selected_region;
/* Don't honor vboot results until the vboot logic has run. */ if (!vboot_logic_executed()) return -1;
- if (vboot_get_selected_region(&selected_region)) + if (!vboot_locate_firmware(ctx, &fw_main)) return -1;
- props->offset = region_offset(&selected_region); - props->size = region_sz(&selected_region); + selected_region = region_device_region(&fw_main); + props->offset = region_offset(selected_region); + props->size = region_sz(selected_region);
return 0; } diff --git a/src/security/vboot/vboot_logic.c b/src/security/vboot/vboot_logic.c index c4389a9..42771e4 100644 --- a/src/security/vboot/vboot_logic.c +++ b/src/security/vboot/vboot_logic.c @@ -34,11 +34,6 @@
#define TODO_BLOCK_SIZE 1024
-static int is_slot_a(struct vb2_context *ctx) -{ - return !(ctx->flags & VB2_CONTEXT_FW_SLOT_B); -} - /* exports */
void vb2ex_printf(const char *func, const char *fmt, ...) @@ -69,7 +64,7 @@ name = "GBB"; break; case VB2_RES_FW_VBLOCK: - if (is_slot_a(ctx)) + if (vboot_is_slot_a(ctx)) name = "VBLOCK_A"; else name = "VBLOCK_B"; @@ -255,19 +250,6 @@ return VB2_SUCCESS; }
-static int locate_firmware(struct vb2_context *ctx, - struct region_device *fw_main) -{ - const char *name; - - if (is_slot_a(ctx)) - name = "FW_MAIN_A"; - else - name = "FW_MAIN_B"; - - return vboot_named_region_device(name, fw_main); -} - /** * Save non-volatile and/or secure data if needed. */ @@ -416,7 +398,7 @@ }
printk(BIOS_INFO, "Phase 4\n"); - rv = locate_firmware(ctx, &fw_main); + rv = vboot_locate_firmware(ctx, &fw_main); if (rv) die_with_post_code(POST_INVALID_ROM, "Failed to read FMAP to locate firmware"); @@ -467,7 +449,7 @@ } }
- printk(BIOS_INFO, "Slot %c is selected\n", is_slot_a(ctx) ? 'A' : 'B'); + printk(BIOS_INFO, "Slot %c is selected\n", vboot_is_slot_a(ctx) ? 'A' : 'B'); vboot_set_selected_region(region_device_region(&fw_main));
verstage_main_exit:
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36845 )
Change subject: security/vboot: Remove vboot_set_selected_region() ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/coreboot/+/36845/1/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/c/coreboot/+/36845/1/src/security/vboot/common.c... PS1, Line 122: vboot_named_region_device This would be a separate CL if we decide to fix it but -- I wonder why we have both security/vboot/common.c and security/vboot/vboot_common.c? There's only a few functions in vboot_common.c -- perhaps we should merge them into this file.
https://review.coreboot.org/c/coreboot/+/36845/1/src/security/vboot/misc.h File src/security/vboot/misc.h:
https://review.coreboot.org/c/coreboot/+/36845/1/src/security/vboot/misc.h@5... PS1, Line 53: const struct vb2_context *ctx I wonder if we really need to pass the context object around in coreboot (same for the function below). Perhaps we should just call vboot_get_context() whenever it is needed locally. Julius, what is your preference?
https://review.coreboot.org/c/coreboot/+/36845/1/src/security/vboot/misc.h@5... PS1, Line 54: vboot_locate_firmware I wonder if we should still call this new function "vboot_get_selected_region"?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36845 )
Change subject: security/vboot: Remove vboot_set_selected_region() ......................................................................
Patch Set 1:
(5 comments)
https://review.coreboot.org/c/coreboot/+/36845/1/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/c/coreboot/+/36845/1/src/security/vboot/common.c... PS1, Line 114: { It does not appear we are properly handling the recovery path in vboot_locate_firmware(). There was a provision that would check if the selected region was not filled in. I do not see equivalent semantics in this function. This check is needed for vboot_locate() to work correctly and fall back to RO.
https://review.coreboot.org/c/coreboot/+/36845/1/src/security/vboot/misc.h File src/security/vboot/misc.h:
https://review.coreboot.org/c/coreboot/+/36845/1/src/security/vboot/misc.h@5... PS1, Line 53: int vboot_is_slot_a(const struct vb2_context *ctx); We should name this better as it's exposed.
https://review.coreboot.org/c/coreboot/+/36845/1/src/security/vboot/misc.h@5... PS1, Line 55: struct region_device *fw); Please add a comment as to what this function does and the return values' meaning. Same for vboot_is_slot_a() above.
https://review.coreboot.org/c/coreboot/+/36845/1/src/security/vboot/vboot_lo... File src/security/vboot/vboot_loader.c:
https://review.coreboot.org/c/coreboot/+/36845/1/src/security/vboot/vboot_lo... PS1, Line 87: selected_region = region_device_region(&fw_main); There's no need for selected_region variable. Just use the below:
props->offset = region_device_offset(&fw_main); props->size = region_device_sz(&fw_main);
https://review.coreboot.org/c/coreboot/+/36845/1/src/security/vboot/vboot_lo... File src/security/vboot/vboot_logic.c:
https://review.coreboot.org/c/coreboot/+/36845/1/src/security/vboot/vboot_lo... PS1, Line 453: vboot_set_selected_region(region_device_region(&fw_main)); What does this do now if one isn't utilizing information? I think it's actually harder to try to harmonize the calls because during the vboot phases they need different semantics.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36845 )
Change subject: security/vboot: Remove vboot_set_selected_region() ......................................................................
Patch Set 1:
(4 comments)
https://review.coreboot.org/c/coreboot/+/36845/1/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/c/coreboot/+/36845/1/src/security/vboot/common.c... PS1, Line 107: int vboot_is_slot_a(const struct vb2_context *ctx) nit: should probably be a static inline
https://review.coreboot.org/c/coreboot/+/36845/1/src/security/vboot/common.c... PS1, Line 122: vboot_named_region_device
This would be a separate CL if we decide to fix it but -- I wonder why we have both security/vboot/c […]
Agreed, merging sounds fine to me. Also we should get rid of vboot_named_region_device(_rw) and just use fmap_locate_area_as_rdev(_rw) directly.
https://review.coreboot.org/c/coreboot/+/36845/1/src/security/vboot/misc.h File src/security/vboot/misc.h:
https://review.coreboot.org/c/coreboot/+/36845/1/src/security/vboot/misc.h@5... PS1, Line 53: const struct vb2_context *ctx
I wonder if we really need to pass the context object around in coreboot (same for the function belo […]
I'd say it depends on how it's called. If every caller already has a context anyway, I think doing it this way is fine. (Honestly, I'm not sure why we need this function at all, I don't think checking a single flag needs to be wrapped.)
https://review.coreboot.org/c/coreboot/+/36845/1/src/security/vboot/vboot_lo... File src/security/vboot/vboot_loader.c:
https://review.coreboot.org/c/coreboot/+/36845/1/src/security/vboot/vboot_lo... PS1, Line 76: const struct vb2_context *ctx = vboot_get_context(); This is only legal to call after vboot_logic_executed() returns true, so you have to move it below that check.
Hello Aaron Durbin, Julius Werner, build bot (Jenkins), Joel Kitching,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36845
to look at the new patch set (#2).
Change subject: security/vboot: Remove vboot_set_selected_region() ......................................................................
security/vboot: Remove vboot_set_selected_region()
Since we already have pre-RAM cache for FMAP (CB:36657), calling load_firmware() multiple times is no longer a problem. This patch replaces vboot_get_selected_region() usage with vboot_locate_firmware(), which locates the firmware by reading from the CBMEM cache.
BRANCH=none BUG=chromium:1021452 TEST=emerge-kukui coreboot
Change-Id: I27cb1a2175beb189053fc3e44b17b60aba474bb0 Signed-off-by: Yu-Ping Wu yupingso@chromium.org --- M src/security/vboot/common.c M src/security/vboot/misc.h M src/security/vboot/vboot_loader.c M src/security/vboot/vboot_logic.c 4 files changed, 42 insertions(+), 45 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/36845/2
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36845 )
Change subject: security/vboot: Remove vboot_set_selected_region() ......................................................................
Patch Set 2:
(10 comments)
https://review.coreboot.org/c/coreboot/+/36845/1/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/c/coreboot/+/36845/1/src/security/vboot/common.c... PS1, Line 107: int vboot_is_slot_a(const struct vb2_context *ctx)
nit: should probably be a static inline
Done
https://review.coreboot.org/c/coreboot/+/36845/1/src/security/vboot/common.c... PS1, Line 114: {
It does not appear we are properly handling the recovery path in vboot_locate_firmware(). […]
Added a vboot_is_slot_selected() check in vboot_locate(). Does that resolve the problem?
https://review.coreboot.org/c/coreboot/+/36845/1/src/security/vboot/common.c... PS1, Line 122: vboot_named_region_device
Agreed, merging sounds fine to me. […]
Ack
https://review.coreboot.org/c/coreboot/+/36845/1/src/security/vboot/misc.h File src/security/vboot/misc.h:
https://review.coreboot.org/c/coreboot/+/36845/1/src/security/vboot/misc.h@5... PS1, Line 53: const struct vb2_context *ctx
I'd say it depends on how it's called. […]
Since most callers already have a context, let's keep it this way.
https://review.coreboot.org/c/coreboot/+/36845/1/src/security/vboot/misc.h@5... PS1, Line 53: int vboot_is_slot_a(const struct vb2_context *ctx);
We should name this better as it's exposed.
Renamed to vboot_is_firmware_slot_a.
https://review.coreboot.org/c/coreboot/+/36845/1/src/security/vboot/misc.h@5... PS1, Line 54: vboot_locate_firmware
I wonder if we should still call this new function "vboot_get_selected_region"?
I think this naming is better given that it returns struct region_device instead of struct region.
https://review.coreboot.org/c/coreboot/+/36845/1/src/security/vboot/misc.h@5... PS1, Line 55: struct region_device *fw);
Please add a comment as to what this function does and the return values' meaning. […]
Done
https://review.coreboot.org/c/coreboot/+/36845/1/src/security/vboot/vboot_lo... File src/security/vboot/vboot_loader.c:
https://review.coreboot.org/c/coreboot/+/36845/1/src/security/vboot/vboot_lo... PS1, Line 76: const struct vb2_context *ctx = vboot_get_context();
This is only legal to call after vboot_logic_executed() returns true, so you have to move it below t […]
Done. But why is there a call to vboot_logic_executed() inside vboot_get_context()?
https://review.coreboot.org/c/coreboot/+/36845/1/src/security/vboot/vboot_lo... PS1, Line 87: selected_region = region_device_region(&fw_main);
There's no need for selected_region variable. Just use the below: […]
Done
https://review.coreboot.org/c/coreboot/+/36845/1/src/security/vboot/vboot_lo... File src/security/vboot/vboot_logic.c:
https://review.coreboot.org/c/coreboot/+/36845/1/src/security/vboot/vboot_lo... PS1, Line 453: vboot_set_selected_region(region_device_region(&fw_main));
What does this do now if one isn't utilizing information? I think it's actually harder to try to har […]
Honestly I'm not sure how to deal with this now.
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36845 )
Change subject: security/vboot: Remove vboot_set_selected_region() ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36845/1/src/security/vboot/vboot_lo... File src/security/vboot/vboot_loader.c:
https://review.coreboot.org/c/coreboot/+/36845/1/src/security/vboot/vboot_lo... PS1, Line 76: const struct vb2_context *ctx = vboot_get_context();
Done. […]
Our current assumption is that vboot_get_context() will only ever be called by verstage for the first time.
vboot_get_context() calls vboot_logic_executed() to figure out whether it should create a new context object, or restore the one from a previous stage.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36845 )
Change subject: security/vboot: Remove vboot_set_selected_region() ......................................................................
Patch Set 2:
(4 comments)
https://review.coreboot.org/c/coreboot/+/36845/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36845/2//COMMIT_MSG@7 PS2, Line 7: Remove vboot_set_selected_region() s/set/get/
https://review.coreboot.org/c/coreboot/+/36845/1/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/c/coreboot/+/36845/1/src/security/vboot/common.c... PS1, Line 114: {
Added a vboot_is_slot_selected() check in vboot_locate(). […]
Yes, that will resolve that problem.
https://review.coreboot.org/c/coreboot/+/36845/1/src/security/vboot/common.c... PS1, Line 122: vboot_named_region_device
Ack
There's still vbnv_flash() using vboot_named_region_device_rw().
https://review.coreboot.org/c/coreboot/+/36845/1/src/security/vboot/vboot_lo... File src/security/vboot/vboot_logic.c:
https://review.coreboot.org/c/coreboot/+/36845/1/src/security/vboot/vboot_lo... PS1, Line 453: vboot_set_selected_region(region_device_region(&fw_main));
Honestly I'm not sure how to deal with this now.
You are still relying on it for vboot_is_slot_selected().
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36845 )
Change subject: security/vboot: Remove vboot_set_selected_region() ......................................................................
Patch Set 2:
(3 comments)
I agree with Aaron's points, I think it would be cleaner to just remove the selected_region field and everything related to it right away, rather than leaving things in this half-state where some code still looks at that and some doesn't anymore.
https://review.coreboot.org/c/coreboot/+/36845/1/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/c/coreboot/+/36845/1/src/security/vboot/common.c... PS1, Line 122: vboot_named_region_device
There's still vbnv_flash() using vboot_named_region_device_rw().
Okay, we should clean that up too then. (I think those functions were written before the more generic fmap_locate_area_as_rdev() existed, but now that it does we should just use that directly.)
https://review.coreboot.org/c/coreboot/+/36845/1/src/security/vboot/vboot_lo... File src/security/vboot/vboot_loader.c:
https://review.coreboot.org/c/coreboot/+/36845/1/src/security/vboot/vboot_lo... PS1, Line 76: const struct vb2_context *ctx = vboot_get_context();
Our current assumption is that vboot_get_context() will only ever be called by verstage for the firs […]
Yeah, so more precisely, it's only legal to call it in verstage or later. But for the purposes of this function, vboot_logic_executed() is a sufficient proxy for that.
https://review.coreboot.org/c/coreboot/+/36845/2/src/security/vboot/vboot_lo... File src/security/vboot/vboot_loader.c:
https://review.coreboot.org/c/coreboot/+/36845/2/src/security/vboot/vboot_lo... PS2, Line 83: if (!vboot_is_slot_selected()) Just check directly for (ctx->flags & RECOVERY) here instead?
Hello Aaron Durbin, Julius Werner, build bot (Jenkins), Joel Kitching,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36845
to look at the new patch set (#3).
Change subject: security/vboot: Remove selected_region from struct vboot_working_data ......................................................................
security/vboot: Remove selected_region from struct vboot_working_data
Since we already have pre-RAM cache for FMAP (CB:36657), calling load_firmware() multiple times is no longer a problem. This patch replaces vboot_get_selected_region() usage with vboot_locate_firmware(), which locates the firmware by reading from the CBMEM cache.
In addition, returning false from vboot_is_slot_selected() implies the recovery path was requested, i.e., vb2_shared_data.recovery_reason was set. Therefore, we simply remove the vboot_is_slot_selected() check from vboot_check_recovery_request().
BRANCH=none BUG=chromium:1021452 TEST=emerge-kukui coreboot
Change-Id: I27cb1a2175beb189053fc3e44b17b60aba474bb0 Signed-off-by: Yu-Ping Wu yupingso@chromium.org --- M src/security/vboot/bootmode.c M src/security/vboot/common.c M src/security/vboot/misc.h M src/security/vboot/vboot_loader.c M src/security/vboot/vboot_logic.c 5 files changed, 39 insertions(+), 72 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/36845/3
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36845 )
Change subject: security/vboot: Remove selected_region from struct vboot_working_data ......................................................................
Patch Set 3:
(4 comments)
https://review.coreboot.org/c/coreboot/+/36845/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36845/2//COMMIT_MSG@7 PS2, Line 7: Remove vboot_set_selected_region()
s/set/get/
Ack
https://review.coreboot.org/c/coreboot/+/36845/1/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/c/coreboot/+/36845/1/src/security/vboot/common.c... PS1, Line 114: {
Yes, that will resolve that problem.
Ack
https://review.coreboot.org/c/coreboot/+/36845/2/src/security/vboot/vboot_lo... File src/security/vboot/vboot_loader.c:
https://review.coreboot.org/c/coreboot/+/36845/2/src/security/vboot/vboot_lo... PS2, Line 83: if (!vboot_is_slot_selected())
Just check directly for (ctx->flags & RECOVERY) here instead?
Done
https://review.coreboot.org/c/coreboot/+/36845/1/src/security/vboot/vboot_lo... File src/security/vboot/vboot_logic.c:
https://review.coreboot.org/c/coreboot/+/36845/1/src/security/vboot/vboot_lo... PS1, Line 453: vboot_set_selected_region(region_device_region(&fw_main));
You are still relying on it for vboot_is_slot_selected().
Merged with CB:36847.
Hello Aaron Durbin, Julius Werner, build bot (Jenkins), Joel Kitching,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36845
to look at the new patch set (#4).
Change subject: security/vboot: Remove selected_region from struct vboot_working_data ......................................................................
security/vboot: Remove selected_region from struct vboot_working_data
Since we already have pre-RAM cache for FMAP (CB:36657), calling load_firmware() multiple times is no longer a problem. This patch replaces vboot_get_selected_region() usage with vboot_locate_firmware(), which locates the firmware by reading from the CBMEM cache.
In addition, returning false from vboot_is_slot_selected() implies the recovery path was requested, i.e., vb2_shared_data.recovery_reason was set. Therefore, we simply remove the vboot_is_slot_selected() check from vboot_check_recovery_request().
BRANCH=none BUG=chromium:1021452 TEST=emerge-kukui coreboot
Change-Id: I27cb1a2175beb189053fc3e44b17b60aba474bb0 Signed-off-by: Yu-Ping Wu yupingso@chromium.org --- M src/security/vboot/bootmode.c M src/security/vboot/common.c M src/security/vboot/misc.h M src/security/vboot/vboot_loader.c M src/security/vboot/vboot_logic.c 5 files changed, 39 insertions(+), 77 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/36845/4
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36845 )
Change subject: security/vboot: Remove selected_region from struct vboot_working_data ......................................................................
Patch Set 4:
(1 comment)
Patch Set 2:
(3 comments)
I agree with Aaron's points, I think it would be cleaner to just remove the selected_region field and everything related to it right away, rather than leaving things in this half-state where some code still looks at that and some doesn't anymore.
https://review.coreboot.org/c/coreboot/+/36845/1/src/security/vboot/vboot_lo... File src/security/vboot/vboot_logic.c:
https://review.coreboot.org/c/coreboot/+/36845/1/src/security/vboot/vboot_lo... PS1, Line 453: vboot_set_selected_region(region_device_region(&fw_main));
Merged with CB:36847.
Ack
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36845 )
Change subject: security/vboot: Remove selected_region from struct vboot_working_data ......................................................................
Patch Set 4: Code-Review+2
Looks good! Now you can go and race Aaron to see who merges first (CB:36939). ;)
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36845 )
Change subject: security/vboot: Remove selected_region from struct vboot_working_data ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36845/4/src/security/vboot/vboot_lo... File src/security/vboot/vboot_loader.c:
https://review.coreboot.org/c/coreboot/+/36845/4/src/security/vboot/vboot_lo... PS4, Line 88: if (!vboot_locate_firmware(ctx, &fw_main)) This return value check is wrong. 0 is successful. I'll fix this up in the rebase.
Aaron Durbin has uploaded a new patch set (#5) to the change originally created by Yu-Ping Wu. ( https://review.coreboot.org/c/coreboot/+/36845 )
Change subject: security/vboot: Remove selected_region from struct vboot_working_data ......................................................................
security/vboot: Remove selected_region from struct vboot_working_data
Since we already have pre-RAM cache for FMAP (CB:36657), calling load_firmware() multiple times is no longer a problem. This patch replaces vboot_get_selected_region() usage with vboot_locate_firmware(), which locates the firmware by reading from the CBMEM cache.
In addition, returning false from vboot_is_slot_selected() implies the recovery path was requested, i.e., vb2_shared_data.recovery_reason was set. Therefore, we simply remove the vboot_is_slot_selected() check from vboot_check_recovery_request().
BRANCH=none BUG=chromium:1021452 TEST=emerge-kukui coreboot
Change-Id: I27cb1a2175beb189053fc3e44b17b60aba474bb0 Signed-off-by: Yu-Ping Wu yupingso@chromium.org --- M src/security/vboot/bootmode.c M src/security/vboot/common.c M src/security/vboot/misc.h M src/security/vboot/vboot_loader.c M src/security/vboot/vboot_logic.c 5 files changed, 39 insertions(+), 77 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/36845/5
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36845 )
Change subject: security/vboot: Remove selected_region from struct vboot_working_data ......................................................................
Patch Set 5: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/36845/4/src/security/vboot/vboot_lo... File src/security/vboot/vboot_loader.c:
https://review.coreboot.org/c/coreboot/+/36845/4/src/security/vboot/vboot_lo... PS4, Line 88: if (!vboot_locate_firmware(ctx, &fw_main))
This return value check is wrong. 0 is successful. I'll fix this up in the rebase.
Whoops...
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/36845 )
Change subject: security/vboot: Remove selected_region from struct vboot_working_data ......................................................................
security/vboot: Remove selected_region from struct vboot_working_data
Since we already have pre-RAM cache for FMAP (CB:36657), calling load_firmware() multiple times is no longer a problem. This patch replaces vboot_get_selected_region() usage with vboot_locate_firmware(), which locates the firmware by reading from the CBMEM cache.
In addition, returning false from vboot_is_slot_selected() implies the recovery path was requested, i.e., vb2_shared_data.recovery_reason was set. Therefore, we simply remove the vboot_is_slot_selected() check from vboot_check_recovery_request().
BRANCH=none BUG=chromium:1021452 TEST=emerge-kukui coreboot
Change-Id: I27cb1a2175beb189053fc3e44b17b60aba474bb0 Signed-off-by: Yu-Ping Wu yupingso@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/36845 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Julius Werner jwerner@chromium.org --- M src/security/vboot/bootmode.c M src/security/vboot/common.c M src/security/vboot/misc.h M src/security/vboot/vboot_loader.c M src/security/vboot/vboot_logic.c 5 files changed, 39 insertions(+), 77 deletions(-)
Approvals: build bot (Jenkins): Verified Julius Werner: Looks good to me, approved
diff --git a/src/security/vboot/bootmode.c b/src/security/vboot/bootmode.c index 0cab0c8..83baa81 100644 --- a/src/security/vboot/bootmode.c +++ b/src/security/vboot/bootmode.c @@ -71,9 +71,8 @@ * VB2_RECOVERY_RO_MANUAL. * 2. Checks if recovery request is present in VBNV and returns the code read * from it. - * 3. Checks if vboot verification is done and looks up selected region - * to identify if vboot_reference library has requested recovery path. - * If yes, return the reason code from shared data. + * 3. Checks if vboot verification is done. If yes, return the reason code from + * shared data. * 4. If nothing applies, return 0 indicating no recovery request. */ int vboot_check_recovery_request(void) @@ -88,11 +87,8 @@ if ((reason = get_recovery_mode_from_vbnv()) != 0) return reason;
- /* - * Identify if vboot verification is already complete and no slot - * was selected i.e. recovery path was requested. - */ - if (vboot_logic_executed() && !vboot_is_slot_selected()) + /* Identify if vboot verification is already complete. */ + if (vboot_logic_executed()) return vboot_get_recovery_reason_shared_data();
return 0; diff --git a/src/security/vboot/common.c b/src/security/vboot/common.c index 3f57602..bad01ff 100644 --- a/src/security/vboot/common.c +++ b/src/security/vboot/common.c @@ -16,6 +16,7 @@ #include <assert.h> #include <cbmem.h> #include <console/console.h> +#include <fmap.h> #include <stdint.h> #include <string.h> #include <symbols.h> @@ -83,42 +84,17 @@ return *vboot_ctx_ptr; }
-int vboot_get_selected_region(struct region *region) +int vboot_locate_firmware(const struct vb2_context *ctx, + struct region_device *fw) { - const struct selected_region *reg = - &vboot_get_working_data()->selected_region; + const char *name;
- if (reg == NULL) - return -1; + if (vboot_is_firmware_slot_a(ctx)) + name = "FW_MAIN_A"; + else + name = "FW_MAIN_B";
- if (reg->offset == 0 && reg->size == 0) - return -1; - - region->offset = reg->offset; - region->size = reg->size; - - return 0; -} - -void vboot_set_selected_region(const struct region *region) -{ - struct selected_region *reg = - &vboot_get_working_data()->selected_region; - - assert(reg != NULL); - - reg->offset = region_offset(region); - reg->size = region_sz(region); -} - -int vboot_is_slot_selected(void) -{ - struct selected_region *reg = - &vboot_get_working_data()->selected_region; - - assert(reg != NULL); - - return reg->size > 0; + return fmap_locate_area_as_rdev(name, fw); }
#if CONFIG(VBOOT_STARTS_IN_BOOTBLOCK) diff --git a/src/security/vboot/misc.h b/src/security/vboot/misc.h index e438848..1b14799 100644 --- a/src/security/vboot/misc.h +++ b/src/security/vboot/misc.h @@ -23,11 +23,6 @@ struct vb2_context; struct vb2_shared_data;
-struct selected_region { - uint32_t offset; - uint32_t size; -}; - /* * Stores vboot-related information. selected_region is used by verstage to * store the location of the selected slot. buffer is used by vboot to store @@ -36,7 +31,6 @@ * Keep the struct CPU architecture agnostic as it crosses stage boundaries. */ struct vboot_working_data { - struct selected_region selected_region; /* offset of the buffer from the start of this struct */ uint16_t buffer_offset; }; @@ -47,11 +41,19 @@ struct vboot_working_data *vboot_get_working_data(void); struct vb2_context *vboot_get_context(void);
-/* Returns 0 on success. < 0 on failure. */ -int vboot_get_selected_region(struct region *region); +/* + * Returns 1 if firmware slot A is used, 0 if slot B is used. + */ +static inline int vboot_is_firmware_slot_a(const struct vb2_context *ctx) +{ + return !(ctx->flags & VB2_CONTEXT_FW_SLOT_B); +}
-void vboot_set_selected_region(const struct region *region); -int vboot_is_slot_selected(void); +/* + * Locates firmware as a region device. Returns 0 on success, -1 on failure. + */ +int vboot_locate_firmware(const struct vb2_context *ctx, + struct region_device *fw);
/* * Source: security/vboot/vboot_handoff.c diff --git a/src/security/vboot/vboot_loader.c b/src/security/vboot/vboot_loader.c index 2b7ba83..3903f18 100644 --- a/src/security/vboot/vboot_loader.c +++ b/src/security/vboot/vboot_loader.c @@ -73,17 +73,23 @@
static int vboot_locate(struct cbfs_props *props) { - struct region selected_region; + const struct vb2_context *ctx; + struct region_device fw_main;
/* Don't honor vboot results until the vboot logic has run. */ if (!vboot_logic_executed()) return -1;
- if (vboot_get_selected_region(&selected_region)) + ctx = vboot_get_context(); + + if (ctx->flags & VB2_CONTEXT_RECOVERY_MODE) return -1;
- props->offset = region_offset(&selected_region); - props->size = region_sz(&selected_region); + if (vboot_locate_firmware(ctx, &fw_main)) + return -1; + + props->offset = region_device_offset(&fw_main); + props->size = region_device_sz(&fw_main);
return 0; } diff --git a/src/security/vboot/vboot_logic.c b/src/security/vboot/vboot_logic.c index 71371cd..6ee4d94 100644 --- a/src/security/vboot/vboot_logic.c +++ b/src/security/vboot/vboot_logic.c @@ -35,11 +35,6 @@
#define TODO_BLOCK_SIZE 1024
-static int is_slot_a(struct vb2_context *ctx) -{ - return !(ctx->flags & VB2_CONTEXT_FW_SLOT_B); -} - /* exports */
void vb2ex_printf(const char *func, const char *fmt, ...) @@ -70,7 +65,7 @@ name = "GBB"; break; case VB2_RES_FW_VBLOCK: - if (is_slot_a(ctx)) + if (vboot_is_firmware_slot_a(ctx)) name = "VBLOCK_A"; else name = "VBLOCK_B"; @@ -256,19 +251,6 @@ return VB2_SUCCESS; }
-static int locate_firmware(struct vb2_context *ctx, - struct region_device *fw_main) -{ - const char *name; - - if (is_slot_a(ctx)) - name = "FW_MAIN_A"; - else - name = "FW_MAIN_B"; - - return fmap_locate_area_as_rdev(name, fw_main); -} - /** * Save non-volatile and/or secure data if needed. */ @@ -417,7 +399,7 @@ }
printk(BIOS_INFO, "Phase 4\n"); - rv = locate_firmware(ctx, &fw_main); + rv = vboot_locate_firmware(ctx, &fw_main); if (rv) die_with_post_code(POST_INVALID_ROM, "Failed to read FMAP to locate firmware"); @@ -468,8 +450,8 @@ } }
- printk(BIOS_INFO, "Slot %c is selected\n", is_slot_a(ctx) ? 'A' : 'B'); - vboot_set_selected_region(region_device_region(&fw_main)); + printk(BIOS_INFO, "Slot %c is selected\n", + vboot_is_firmware_slot_a(ctx) ? 'A' : 'B');
verstage_main_exit: /* If CBMEM is not up yet, let the ROMSTAGE_CBMEM_INIT_HOOK take care
-
Aaron Durbin (Code Review) -
Joel Kitching (Code Review)
-
Julius Werner (Code Review)
-
Patrick Georgi (Code Review)
-
Yu-Ping Wu (Code Review)