vboot: always return after calling vb2_ui_screen_changestabilize-rust-13836.Bstabilize-13836.B

Without returning, subsequent code may operate under the
assumption that the screen has *not* changed, leading to
unexpected behaviour.  The user may also be able to select
otherwise disallowed menu items.

BUG=b:181087237, chromium:1181484
TEST=make clean && make runtests
BRANCH=none

Signed-off-by: Joel Kitching <kitching@google.com>
Change-Id: I820e387417ad39e2f7bd47f65d08c387cf66d6e5
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/2717449
Tested-by: Joel Kitching <kitching@chromium.org>
Reviewed-by: Yu-Ping Wu <yupingso@chromium.org>
Commit-Queue: Joel Kitching <kitching@chromium.org>

2 files changed, 20 insertions, 4 deletions

diff --git a/firmware/2lib/2ui_screens.c b/firmware/2lib/2ui_screens.c
index 3026f76e..adebc53e 100644
--- a/firmware/2lib/2ui_screens.c
+++ b/firmware/2lib/2ui_screens.c
@@ -690,7 +690,7 @@ vb2_error_t developer_mode_init(struct vb2_ui_context *ui)
 
 	/* TODO(b/159579189): Split this case into a separate root screen */
 	if (!vb2_dev_boot_allowed(ui->ctx))
-		vb2_ui_screen_change(ui, VB2_SCREEN_DEVELOPER_TO_NORM);
+		return vb2_ui_screen_change(ui, VB2_SCREEN_DEVELOPER_TO_NORM);
 
 	/* Don't show "Return to secure mode" button if GBB forces dev mode. */
 	if (vb2_get_gbb(ui->ctx)->flags & VB2_GBB_FLAG_FORCE_DEV_SWITCH_ON)
@@ -783,7 +783,7 @@ vb2_error_t developer_mode_action(struct vb2_ui_context *ui)
 
 	/* TODO(b/159579189): Split this case into a separate root screen */
 	if (!vb2_dev_boot_allowed(ui->ctx))
-		vb2_ui_screen_change(ui, VB2_SCREEN_DEVELOPER_TO_NORM);
+		return vb2_ui_screen_change(ui, VB2_SCREEN_DEVELOPER_TO_NORM);
 
 	/* Once any user interaction occurs, stop the timer. */
 	if (ui->key)
diff --git a/firmware/2lib/include/2ui.h b/firmware/2lib/include/2ui.h
index c5d13311..c8e2928b 100644
--- a/firmware/2lib/include/2ui.h
+++ b/firmware/2lib/include/2ui.h
@@ -138,7 +138,7 @@ vb2_error_t vb2_ui_developer_mode_boot_altfw_action(
 /**
  * Get info struct of a screen.
  *
- * @param screen	Screen from enum vb2_screen
+ * @param id		Screen from enum vb2_screen
  *
  * @return screen info struct on success, NULL on error.
  */
@@ -174,6 +174,9 @@ vb2_error_t vb2_ui_menu_next(struct vb2_ui_context *ui);
 /**
  * Select the current menu item.
  *
+ * The caller should take care of returning after this function, and should not
+ * continue executing under the assumption that the screen has *not* changed.
+ *
  * If the current menu item has an action associated with it, run the action.
  * Otherwise, navigate to the target screen.  If neither of these are set, then
  * selecting the menu item is a no-op.
@@ -189,7 +192,13 @@ vb2_error_t vb2_ui_menu_select(struct vb2_ui_context *ui);
 /**
  * Return back to the previous screen.
  *
- * If the current screen is already the root scren, the request is ignored.
+ * The caller should take care of returning after this function, and should not
+ * continue executing under the assumption that the screen has *not* changed.
+ *
+ * If the current screen is already the root screen, the request is ignored.
+ *
+ * TODO(b/157625765): Consider falling into recovery mode (BROKEN screen) when
+ * the current screen is already the root screen.
  *
  * @param ui		UI context pointer
  * @return VB2_REQUEST_UI_CONTINUE, or error code on error.
@@ -199,9 +208,16 @@ vb2_error_t vb2_ui_screen_back(struct vb2_ui_context *ui);
 /**
  * Change to the given screen.
  *
+ * The caller should take care of returning after this function, and should not
+ * continue executing under the assumption that the screen has *not* changed.
+ *
  * If the screen is not found, the request is ignored.
  *
+ * TODO(b/157625765): Consider falling into recovery mode (BROKEN screen) when
+ * the target screen is not found.
+ *
  * @param ui		UI context pointer
+ * @param id		Screen from enum vb2_screen
  * @return VB2_REQUEST_UI_CONTINUE, or error code on error.
  */
 vb2_error_t vb2_ui_screen_change(struct vb2_ui_context *ui, enum vb2_screen id);