[coreboot-gerrit] Change in coreboot[master]: security/vboot: Removed vboot_prepare from vboot_locator

[Aaron Durbin (Code Review)]

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.