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.