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.