Wim Vervoorn has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36543 )
Change subject: drivers/intel/fsp1_1: Do not use prog_locate if VBOOT_STARTS_IN_ROMSTAGE ......................................................................
drivers/intel/fsp1_1: Do not use prog_locate if VBOOT_STARTS_IN_ROMSTAGE
If VBOOT_STARTS_IN_ROMSTAGE is enabled prog_locate should be not be used before memory is available. Doing so will cause the vboot_locate to be called too early.
As an alternative the cbfs routines are used to provide the same result.
BUG=N/A TEST=tested on facebook fbg1701
Change-Id: Id5e8fd78458c09dd3896bfd142bd49c2c3d686df Signed-off-by: Wim Vervoorn wvervoorn@eltan.com --- M src/drivers/intel/fsp1_1/car.c 1 file changed, 13 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/36543/1
diff --git a/src/drivers/intel/fsp1_1/car.c b/src/drivers/intel/fsp1_1/car.c index 2039c9c..dc54782 100644 --- a/src/drivers/intel/fsp1_1/car.c +++ b/src/drivers/intel/fsp1_1/car.c @@ -13,6 +13,7 @@
#include <arch/romstage.h> #include <arch/symbols.h> +#include <cbfs.h> #include <cbmem.h> #include <console/console.h> #include <commonlib/helpers.h> @@ -41,7 +42,18 @@ * is still enabled. We can directly access work buffer here. */ struct prog fsp = PROG_INIT(PROG_REFCODE, "fsp.bin");
- if (prog_locate(&fsp)) + if (CONFIG(VBOOT_STARTS_IN_ROMSTAGE)) { + + struct cbfsf file; + + if (cbfs_boot_locate(&file, prog_name(&fsp), NULL)) + die_with_post_code(POST_INVALID_CBFS, "Unable to locate fsp.bin"); + + cbfsf_file_type(&file, &fsp.cbfs_type); + + cbfs_file_data(prog_rdev(&fsp), &file); + + } else if (prog_locate(&fsp)) die_with_post_code(POST_INVALID_CBFS, "Unable to locate fsp.bin");
/* This leaks a mapping which this code assumes is benign as
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36543 )
Change subject: drivers/intel/fsp1_1: Do not use prog_locate if VBOOT_STARTS_IN_ROMSTAGE ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36543/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36543/1//COMMIT_MSG@9 PS1, Line 9: If VBOOT_STARTS_IN_ROMSTAGE is enabled prog_locate should be not be used that should be documented as inline comment in code.
Hello Aaron Durbin, Patrick Rudolph, Julius Werner, Hung-Te Lin, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Werner Zeh, Huang Jin, Daisuke Nojiri, Lee Leahy, Philipp Deppenwiese, Joel Kitching,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36543
to look at the new patch set (#2).
Change subject: drivers/intel/fsp1_1: Do not use prog_locate if VBOOT_STARTS_IN_ROMSTAGE ......................................................................
drivers/intel/fsp1_1: Do not use prog_locate if VBOOT_STARTS_IN_ROMSTAGE
If VBOOT_STARTS_IN_ROMSTAGE is enabled prog_locate should be not be used before memory is available. Doing so will cause the vboot_locate to be called too early.
As an alternative the cbfs routines are used to provide the same result.
BUG=N/A TEST=tested on facebook fbg1701
Change-Id: Id5e8fd78458c09dd3896bfd142bd49c2c3d686df Signed-off-by: Wim Vervoorn wvervoorn@eltan.com --- M src/drivers/intel/fsp1_1/car.c 1 file changed, 16 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/36543/2
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36543 )
Change subject: drivers/intel/fsp1_1: Do not use prog_locate if VBOOT_STARTS_IN_ROMSTAGE ......................................................................
Patch Set 2:
(7 comments)
https://review.coreboot.org/c/coreboot/+/36543/2/src/drivers/intel/fsp1_1/ca... File src/drivers/intel/fsp1_1/car.c:
https://review.coreboot.org/c/coreboot/+/36543/2/src/drivers/intel/fsp1_1/ca... PS2, Line 46: trailing whitespace
https://review.coreboot.org/c/coreboot/+/36543/2/src/drivers/intel/fsp1_1/ca... PS2, Line 46: code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/36543/2/src/drivers/intel/fsp1_1/ca... PS2, Line 46: please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/36543/2/src/drivers/intel/fsp1_1/ca... PS2, Line 47: /* If VBOOT_STARTS_IN_ROMSTAGE is enabled prog_locate should be not be used before trailing whitespace
https://review.coreboot.org/c/coreboot/+/36543/2/src/drivers/intel/fsp1_1/ca... PS2, Line 47: /* If VBOOT_STARTS_IN_ROMSTAGE is enabled prog_locate should be not be used before line over 96 characters
https://review.coreboot.org/c/coreboot/+/36543/2/src/drivers/intel/fsp1_1/ca... PS2, Line 48: * memory is available. Doing so will cause the vboot_locate to be called too early. trailing whitespace
https://review.coreboot.org/c/coreboot/+/36543/2/src/drivers/intel/fsp1_1/ca... PS2, Line 48: * memory is available. Doing so will cause the vboot_locate to be called too early. line over 96 characters
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36543 )
Change subject: drivers/intel/fsp1_1: Do not use prog_locate if VBOOT_STARTS_IN_ROMSTAGE ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36543/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36543/1//COMMIT_MSG@9 PS1, Line 9: If VBOOT_STARTS_IN_ROMSTAGE is enabled prog_locate should be not be used
that should be documented as inline comment in code.
I added the description as a comment in the code and left it in the commit message as well. So the aim of the patch is still clear.
Hello Aaron Durbin, Patrick Rudolph, Julius Werner, Hung-Te Lin, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Werner Zeh, Huang Jin, Daisuke Nojiri, Lee Leahy, Philipp Deppenwiese, Joel Kitching,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36543
to look at the new patch set (#3).
Change subject: drivers/intel/fsp1_1: Do not use prog_locate if VBOOT_STARTS_IN_ROMSTAGE ......................................................................
drivers/intel/fsp1_1: Do not use prog_locate if VBOOT_STARTS_IN_ROMSTAGE
If VBOOT_STARTS_IN_ROMSTAGE is enabled prog_locate should be not be used before memory is available. Doing so will cause the vboot_locate to be called too early.
As an alternative the cbfs routines are used to provide the same result.
BUG=N/A TEST=tested on facebook fbg1701
Change-Id: Id5e8fd78458c09dd3896bfd142bd49c2c3d686df Signed-off-by: Wim Vervoorn wvervoorn@eltan.com --- M src/drivers/intel/fsp1_1/car.c 1 file changed, 17 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/36543/3
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36543 )
Change subject: drivers/intel/fsp1_1: Do not use prog_locate if VBOOT_STARTS_IN_ROMSTAGE ......................................................................
Patch Set 3:
(7 comments)
https://review.coreboot.org/c/coreboot/+/36543/2/src/drivers/intel/fsp1_1/ca... File src/drivers/intel/fsp1_1/car.c:
https://review.coreboot.org/c/coreboot/+/36543/2/src/drivers/intel/fsp1_1/ca... PS2, Line 46:
please, no spaces at the start of a line
Done
https://review.coreboot.org/c/coreboot/+/36543/2/src/drivers/intel/fsp1_1/ca... PS2, Line 46:
code indent should use tabs where possible
Done
https://review.coreboot.org/c/coreboot/+/36543/2/src/drivers/intel/fsp1_1/ca... PS2, Line 46:
trailing whitespace
Done
https://review.coreboot.org/c/coreboot/+/36543/2/src/drivers/intel/fsp1_1/ca... PS2, Line 47: /* If VBOOT_STARTS_IN_ROMSTAGE is enabled prog_locate should be not be used before
trailing whitespace
Done
https://review.coreboot.org/c/coreboot/+/36543/2/src/drivers/intel/fsp1_1/ca... PS2, Line 47: /* If VBOOT_STARTS_IN_ROMSTAGE is enabled prog_locate should be not be used before
line over 96 characters
Done
https://review.coreboot.org/c/coreboot/+/36543/2/src/drivers/intel/fsp1_1/ca... PS2, Line 48: * memory is available. Doing so will cause the vboot_locate to be called too early.
trailing whitespace
Done
https://review.coreboot.org/c/coreboot/+/36543/2/src/drivers/intel/fsp1_1/ca... PS2, Line 48: * memory is available. Doing so will cause the vboot_locate to be called too early.
line over 96 characters
Done
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36543 )
Change subject: drivers/intel/fsp1_1: Do not use prog_locate if VBOOT_STARTS_IN_ROMSTAGE ......................................................................
Patch Set 3: Code-Review+2
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36543 )
Change subject: drivers/intel/fsp1_1: Do not use prog_locate if VBOOT_STARTS_IN_ROMSTAGE ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36543/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36543/3//COMMIT_MSG@11 PS3, Line 11: called too early. Why? What is broken? Because vboot selection starts when trying to look up fsp.bin and you actually want to delay it? If so, then we should put together an API to vboot to signal when it should start.
Please provide more details as to the 1. the problem and 2. why the proposed solution is the correct one.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36543 )
Change subject: drivers/intel/fsp1_1: Do not use prog_locate if VBOOT_STARTS_IN_ROMSTAGE ......................................................................
Patch Set 3: Code-Review-1
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36543 )
Change subject: drivers/intel/fsp1_1: Do not use prog_locate if VBOOT_STARTS_IN_ROMSTAGE ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36543/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36543/3//COMMIT_MSG@11 PS3, Line 11: called too early.
Why? What is broken? Because vboot selection starts when trying to look up fsp. […]
Effectively the following:
diff --git a/src/lib/prog_loaders.c b/src/lib/prog_loaders.c index 183a22bff0..015660e620 100644 --- a/src/lib/prog_loaders.c +++ b/src/lib/prog_loaders.c @@ -135,6 +135,9 @@ void run_ramstage(void) !CONFIG(NO_STAGE_CACHE)) run_ramstage_from_resume(&ramstage);
+ if (ENV_ROMSTAGE && CONFIG(VBOOT_STARTS_IN_ROMSTAGE)) + vboot_run_logic(); + if (prog_locate(&ramstage)) goto fail;
Now, we can remove the prepare() API from CBFS and add all the necessary code paths like above to trigger the vboot logic such that subsequent lookups will honor vboot. The proper places are effectively at the end of the subsequent stage that corresponds to VBOOT_STARTS_IN_* (ROMSTAGE or BOOTLOCK which would be in run_romstage() and run_ramstage()).
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36543 )
Change subject: drivers/intel/fsp1_1: Do not use prog_locate if VBOOT_STARTS_IN_ROMSTAGE ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36543/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36543/3//COMMIT_MSG@11 PS3, Line 11: called too early.
Effectively the following: […]
You nailed it. The issue is in the stage where vboot becomes active. Now the prog_locate handles all requests during this stage as equal while, in fact, it should only do this for the "next" stage. The issue shows when vboot_prepare() is called by the prog_locate.
In the transition stage this is done too early in some cases.
I have circumvented the issue by using the boot_locate call (as in this case locating the fsp is the only item that happens too early). This works for the case I implemented with but indeed is not the generic solution you may want to have.
What you are proposing is more generic but has a bigger impact on the overall code of coreboot. I will give it a try and remove the .prepare item in vboot locate while adding a call to vboot_prepare() or verstage_main() where appropriate.
Removing the prepare member in the cbfs driver is not a good idea as this is also used by our verified boot solution.
Hello Aaron Durbin, Patrick Rudolph, Julius Werner, Frans Hendriks, Hung-Te Lin, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Werner Zeh, Huang Jin, Daisuke Nojiri, Lee Leahy, Philipp Deppenwiese, Joel Kitching,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36543
to look at the new patch set (#4).
Change subject: security/vboot: Removed vboot_prepare from vboot_locator ......................................................................
security/vboot: Removed vboot_prepare from vboot_locator
When prog_locate() is called in the stage VBOOT is starting from and the image to be loaded is not the target image vboot_prepare() may be called too early.
To prevent this vboot_prepare() is removed from the vboot_locator structure. This allows more control over the start of the vboot logic.
To clarify the change the vboot_prepare() has been renamed to vboot_run_logic() and calls to initialize vboot have been added at the following places:
postcar_loader: when VBOOT starts in ROMSTAGE romstage_loader: when VBOOT starts in BOOTBLOCK ramstage_loader: when VBOOT starts in ROMSTAGE
BUG=N/A TEST=tested on facebook fbg1701
Change-Id: Id5e8fd78458c09dd3896bfd142bd49c2c3d686df Signed-off-by: Wim Vervoorn wvervoorn@eltan.com --- M src/arch/x86/postcar_loader.c M src/lib/prog_loaders.c M src/security/vboot/vboot_common.h M src/security/vboot/vboot_loader.c 4 files changed, 14 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/36543/4
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36543 )
Change subject: security/vboot: Removed vboot_prepare from vboot_locator ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36543/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36543/3//COMMIT_MSG@11 PS3, Line 11: called too early.
You nailed it. The issue is in the stage where vboot becomes active. […]
Changed the solution according to Aaron's suggestion. This has been tested on the facebook fbg1701 system.
The thing we overlooked was the fact that romstage is loading POSTCAR instead of RAMSTAGE (on x86 that is).
So now the vboot will be initialized when either ROMSTAGE, POSTCAR or RAMSTAGE will be loaded. Of course depending on the VBOOT_STARTS_IN* setting. I assume the cases included in the commit are covering all possible situations.
The vboot_prepare has been renamed to vboot_run_logic() this has been done to highlight the change from a static function to one that is called from various places.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36543 )
Change subject: security/vboot: Removed vboot_prepare from vboot_locator ......................................................................
Patch Set 4:
(3 comments)
https://review.coreboot.org/c/coreboot/+/36543/4/src/arch/x86/postcar_loader... File src/arch/x86/postcar_loader.c:
https://review.coreboot.org/c/coreboot/+/36543/4/src/arch/x86/postcar_loader... PS4, Line 175: ENV_ROMSTAGE nit: would it ever not be?
https://review.coreboot.org/c/coreboot/+/36543/4/src/lib/prog_loaders.c File src/lib/prog_loaders.c:
https://review.coreboot.org/c/coreboot/+/36543/4/src/lib/prog_loaders.c@63 PS4, Line 63: (ENV_BOOTBLOCK || ENV_VERSTAGE) ENV_VERSTAGE is wrong here, that's when you explicitly *don't* want to run it (because then it already ran). However, vboot_run_logic() already checks for that (with the verification_should_run()/verification_should_load() checks). So I think it might be best to just remove all the checks here (including the VBOOT_STARTS_IN checks), call the function vboot_may_run(), and let the function itself figure out when it actually needs to run (like vboot_prepare() already does today).
https://review.coreboot.org/c/coreboot/+/36543/4/src/security/vboot/vboot_co... File src/security/vboot/vboot_common.h:
https://review.coreboot.org/c/coreboot/+/36543/4/src/security/vboot/vboot_co... PS4, Line 73: void vboot_run_logic(void); nit: If you remove the checks like I suggest, you should move this under the #if CONFIG(VBOOT) below and add an empty stub for it when vboot is not configured in.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36543 )
Change subject: security/vboot: Removed vboot_prepare from vboot_locator ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36543/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36543/3//COMMIT_MSG@11 PS3, Line 11: called too early.
Changed the solution according to Aaron's suggestion. […]
I was thinking the function would just set a variable to indicate vboot should run. Not directly call through into the logic. Something like this:
diff --git a/src/security/vboot/misc.h b/src/security/vboot/misc.h index 1458354ffc..7d26b91b74 100644 --- a/src/security/vboot/misc.h +++ b/src/security/vboot/misc.h @@ -83,16 +83,30 @@ void vboot_save_recovery_reason_vbnv(void);
static inline int verification_should_run(void) { + extern int vboot_should_execute; /* should not be globally accessible */ + if (CONFIG(VBOOT_SEPARATE_VERSTAGE)) - return ENV_VERSTAGE; + return ENV_VERSTAGE && car_get_var(vboot_should_execute); else if (CONFIG(VBOOT_STARTS_IN_ROMSTAGE)) - return ENV_ROMSTAGE; + return ENV_ROMSTAGE && car_get_var(vboot_should_execute); else if (CONFIG(VBOOT_STARTS_IN_BOOTBLOCK)) - return ENV_BOOTBLOCK; + return ENV_BOOTBLOCK && car_get_var(vboot_should_execute); else dead_code(); }
+static inline void vboot_run_logic(void) +{ + extern int vboot_should_execute; /* should not be globally accessible */ + + if (CONFIG(VBOOT_SEPARATE_VERSTAGE) && ENV_VERSTAGE) + car_set_var(vboot_should_execute, 1); + else if (CONFIG(VBOOT_STARTS_IN_ROMSTAGE) && ENV_ROMSTAGE) + car_set_var(vboot_should_execute, 1); + else if (CONFIG(VBOOT_STARTS_IN_BOOTBLOCK) && ENV_BOOTBLOCK) + car_set_var(vboot_should_execute, 1); +} + static inline int verstage_should_load(void) { if (CONFIG(VBOOT_SEPARATE_VERSTAGE)) diff --git a/src/security/vboot/vboot_loader.c b/src/security/vboot/vboot_loader.c index 3aac48d174..f521b117be 100644 --- a/src/security/vboot/vboot_loader.c +++ b/src/security/vboot/vboot_loader.c @@ -34,6 +34,7 @@ _Static_assert(!CONFIG(VBOOT_RETURN_FROM_VERSTAGE) || "return from verstage only makes sense for separate verstages");
int vboot_executed CAR_GLOBAL; +int vboot_should_execute CAR_GLOBAL;
static void vboot_prepare(void) {
It is more indirect, though. And I guess we could decorate the call sites where we know we should start running verified vboot as I think those are more well defined:
src/lib/prog_loaders.c (run_romstage, run_ramstage). src/arch/x86/postcar_loader.c (run_postcar_phase).
I'm not sure about other platforms/arch off hand.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36543 )
Change subject: security/vboot: Removed vboot_prepare from vboot_locator ......................................................................
Patch Set 4:
I think Julius's recommendations are the bets approach.
Hello Aaron Durbin, Patrick Rudolph, Julius Werner, Frans Hendriks, Hung-Te Lin, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Werner Zeh, Huang Jin, Daisuke Nojiri, Lee Leahy, Philipp Deppenwiese, Joel Kitching,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36543
to look at the new patch set (#5).
Change subject: security/vboot: Removed vboot_prepare from vboot_locator ......................................................................
security/vboot: Removed vboot_prepare from vboot_locator
When prog_locate() is called in the stage VBOOT is starting from and the image to be loaded is not the target image vboot_prepare() may be called too early.
To prevent this vboot_prepare() is removed from the vboot_locator structure. This allows more control over the start of the vboot logic.
To clarify the change the vboot_prepare() has been renamed to vboot_run_logic() and calls to initialize vboot have been added at the following places:
postcar_loader: when VBOOT starts in ROMSTAGE romstage_loader: when VBOOT starts in BOOTBLOCK ramstage_loader: when VBOOT starts in ROMSTAGE
BUG=N/A TEST=tested on facebook fbg1701
Change-Id: Id5e8fd78458c09dd3896bfd142bd49c2c3d686df Signed-off-by: Wim Vervoorn wvervoorn@eltan.com --- M src/arch/x86/postcar_loader.c M src/lib/prog_loaders.c M src/security/vboot/vboot_common.h M src/security/vboot/vboot_loader.c 4 files changed, 11 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/36543/5
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36543 )
Change subject: security/vboot: Removed vboot_prepare from vboot_locator ......................................................................
Patch Set 5:
Patch Set 4:
(3 comments)
Updated the commit using your input. This is indeed cleaner and sufficient.
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36543 )
Change subject: security/vboot: Removed vboot_prepare from vboot_locator ......................................................................
Patch Set 5: Code-Review+1
(3 comments)
Patch Set 4:
I think Julius's recommendations are the bets approach.
Updated with these recommendations.
https://review.coreboot.org/c/coreboot/+/36543/4/src/arch/x86/postcar_loader... File src/arch/x86/postcar_loader.c:
https://review.coreboot.org/c/coreboot/+/36543/4/src/arch/x86/postcar_loader... PS4, Line 175: ENV_ROMSTAGE
nit: would it ever not be?
Now called unconditionally.
https://review.coreboot.org/c/coreboot/+/36543/4/src/lib/prog_loaders.c File src/lib/prog_loaders.c:
https://review.coreboot.org/c/coreboot/+/36543/4/src/lib/prog_loaders.c@63 PS4, Line 63: (ENV_BOOTBLOCK || ENV_VERSTAGE)
ENV_VERSTAGE is wrong here, that's when you explicitly *don't* want to run it (because then it alrea […]
Now called unconditionally.
https://review.coreboot.org/c/coreboot/+/36543/4/src/security/vboot/vboot_co... File src/security/vboot/vboot_common.h:
https://review.coreboot.org/c/coreboot/+/36543/4/src/security/vboot/vboot_co... PS4, Line 73: void vboot_run_logic(void);
nit: If you remove the checks like I suggest, you should move this under the #if CONFIG(VBOOT) below […]
Now called unconditionally.
Hello Aaron Durbin, Patrick Rudolph, Julius Werner, Frans Hendriks, Hung-Te Lin, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Werner Zeh, Huang Jin, Daisuke Nojiri, Lee Leahy, Philipp Deppenwiese, Joel Kitching,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36543
to look at the new patch set (#6).
Change subject: security/vboot: Removed vboot_prepare from vboot_locator ......................................................................
security/vboot: Removed vboot_prepare from vboot_locator
When prog_locate() is called in the stage VBOOT is starting from and the image to be loaded is not the target image vboot_prepare() may be called too early.
To prevent this vboot_prepare() is removed from the vboot_locator structure. This allows more control over the start of the vboot logic.
To clarify the change the vboot_prepare() has been renamed to vboot_run_logic() and calls to initialize vboot have been added at the following places:
postcar_loader: when VBOOT starts in ROMSTAGE romstage_loader: when VBOOT starts in BOOTBLOCK ramstage_loader: when VBOOT starts in ROMSTAGE
BUG=N/A TEST=tested on facebook fbg1701
Change-Id: Id5e8fd78458c09dd3896bfd142bd49c2c3d686df Signed-off-by: Wim Vervoorn wvervoorn@eltan.com --- M src/arch/x86/postcar_loader.c M src/lib/prog_loaders.c M src/security/vboot/vboot_common.h M src/security/vboot/vboot_loader.c 4 files changed, 11 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/36543/6
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36543 )
Change subject: security/vboot: Removed vboot_prepare from vboot_locator ......................................................................
Patch Set 6: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/36543 )
Change subject: security/vboot: Removed vboot_prepare from vboot_locator ......................................................................
security/vboot: Removed vboot_prepare from vboot_locator
When prog_locate() is called in the stage VBOOT is starting from and the image to be loaded is not the target image vboot_prepare() may be called too early.
To prevent this vboot_prepare() is removed from the vboot_locator structure. This allows more control over the start of the vboot logic.
To clarify the change the vboot_prepare() has been renamed to vboot_run_logic() and calls to initialize vboot have been added at the following places:
postcar_loader: when VBOOT starts in ROMSTAGE romstage_loader: when VBOOT starts in BOOTBLOCK ramstage_loader: when VBOOT starts in ROMSTAGE
BUG=N/A TEST=tested on facebook fbg1701
Change-Id: Id5e8fd78458c09dd3896bfd142bd49c2c3d686df Signed-off-by: Wim Vervoorn wvervoorn@eltan.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/36543 Reviewed-by: Frans Hendriks fhendriks@eltan.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/arch/x86/postcar_loader.c M src/lib/prog_loaders.c M src/security/vboot/vboot_common.h M src/security/vboot/vboot_loader.c 4 files changed, 11 insertions(+), 2 deletions(-)
Approvals: build bot (Jenkins): Verified Frans Hendriks: Looks good to me, approved
diff --git a/src/arch/x86/postcar_loader.c b/src/arch/x86/postcar_loader.c index 0a5d50c..868b770 100644 --- a/src/arch/x86/postcar_loader.c +++ b/src/arch/x86/postcar_loader.c @@ -23,6 +23,7 @@ #include <romstage_handoff.h> #include <stage_cache.h> #include <timestamp.h> +#include <security/vboot/vboot_common.h>
static inline void stack_push(struct postcar_frame *pcf, uint32_t val) { @@ -171,6 +172,8 @@ .prog = prog, };
+ vboot_run_logic(); + if (prog_locate(prog)) die_with_post_code(POST_INVALID_ROM, "Failed to locate after CAR program.\n"); diff --git a/src/lib/prog_loaders.c b/src/lib/prog_loaders.c index 183a22b..72c1de1 100644 --- a/src/lib/prog_loaders.c +++ b/src/lib/prog_loaders.c @@ -30,6 +30,7 @@ #include <symbols.h> #include <timestamp.h> #include <fit_payload.h> +#include <security/vboot/vboot_common.h>
/* Only can represent up to 1 byte less than size_t. */ const struct mem_region_device addrspace_32bit = @@ -59,6 +60,8 @@ struct prog romstage = PROG_INIT(PROG_ROMSTAGE, CONFIG_CBFS_PREFIX "/romstage");
+ vboot_run_logic(); + if (prog_locate(&romstage)) goto fail;
@@ -135,6 +138,8 @@ !CONFIG(NO_STAGE_CACHE)) run_ramstage_from_resume(&ramstage);
+ vboot_run_logic(); + if (prog_locate(&ramstage)) goto fail;
diff --git a/src/security/vboot/vboot_common.h b/src/security/vboot/vboot_common.h index 8aadf9e..42b4a6b 100644 --- a/src/security/vboot/vboot_common.h +++ b/src/security/vboot/vboot_common.h @@ -76,12 +76,14 @@ int vboot_recovery_mode_enabled(void); int vboot_recovery_mode_memory_retrain(void); int vboot_can_enable_udc(void); +void vboot_run_logic(void); #else /* !CONFIG_VBOOT */ static inline int vboot_developer_mode_enabled(void) { return 0; } static inline int vboot_recovery_mode_enabled(void) { return 0; } static inline int vboot_recovery_mode_memory_retrain(void) { return 0; } /* If VBOOT is not enabled, we are okay enabling USB device controller (UDC). */ static inline int vboot_can_enable_udc(void) { return 1; } +static inline void vboot_run_logic(void) {} #endif
#endif /* __VBOOT_VBOOT_COMMON_H__ */ diff --git a/src/security/vboot/vboot_loader.c b/src/security/vboot/vboot_loader.c index 3aac48d..2b7ba83 100644 --- a/src/security/vboot/vboot_loader.c +++ b/src/security/vboot/vboot_loader.c @@ -35,7 +35,7 @@
int vboot_executed CAR_GLOBAL;
-static void vboot_prepare(void) +void vboot_run_logic(void) { if (verification_should_run()) { /* Note: this path is not used for VBOOT_RETURN_FROM_VERSTAGE */ @@ -90,6 +90,5 @@
const struct cbfs_locator vboot_locator = { .name = "VBOOT", - .prepare = vboot_prepare, .locate = vboot_locate, };
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36543 )
Change subject: security/vboot: Removed vboot_prepare from vboot_locator ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36543/7/src/security/vboot/vboot_lo... File src/security/vboot/vboot_loader.c:
https://review.coreboot.org/c/coreboot/+/36543/7/src/security/vboot/vboot_lo... PS7, Line 93: .prepare = vboot_prepare, Can you please also dismantle the whole infrastructure for the .prepare() callback now (i.e. remove it from struct cbfs_locator and get rid of cbfs_prepare_program_locate())? It's not needed anymore.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36543 )
Change subject: security/vboot: Removed vboot_prepare from vboot_locator ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36543/7/src/security/vboot/vboot_lo... File src/security/vboot/vboot_loader.c:
https://review.coreboot.org/c/coreboot/+/36543/7/src/security/vboot/vboot_lo... PS7, Line 93: .prepare = vboot_prepare,
Can you please also dismantle the whole infrastructure for the .prepare() callback now (i.e. […]
+1
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36543 )
Change subject: security/vboot: Removed vboot_prepare from vboot_locator ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36543/7/src/security/vboot/vboot_lo... File src/security/vboot/vboot_loader.c:
https://review.coreboot.org/c/coreboot/+/36543/7/src/security/vboot/vboot_lo... PS7, Line 93: .prepare = vboot_prepare,
+1
https://review.coreboot.org/c/coreboot/+/36690
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36543 )
Change subject: security/vboot: Removed vboot_prepare from vboot_locator ......................................................................
Patch Set 7:
Patch Set 7:
(1 comment)
The prepare mechanism is used by the eltan verified boot mechanism. So removing this completely would break that.