vboot: always return after calling vb2_ui_screen_change
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>
diff --git a/firmware/2lib/2ui_screens.c b/firmware/2lib/2ui_screens.c
index 3026f76..adebc53 100644
--- a/firmware/2lib/2ui_screens.c
+++ b/firmware/2lib/2ui_screens.c
@@ -690,7 +690,7 @@
/* 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 @@
/* 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 c5d1331..c8e2928 100644
--- a/firmware/2lib/include/2ui.h
+++ b/firmware/2lib/include/2ui.h
@@ -138,7 +138,7 @@
/**
* 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 @@
/**
* 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 @@
/**
* 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 @@
/**
* 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);
