vboot: integrate BROKEN screen recovery request logic into VBSLK

CL:1940398 brought us towards the goal of deferring clearing
recovery requests until kernel verification stage.  However,
now we are modifying recovery requests from multiple locations
in kernel verification code -- namely, also on the BROKEN screen
in UI code.

Integrate the logic into a function called vb2_clear_recovery to
be called from VbSelectAndLoadKernel.

Add tests to ensure that recovery requests get properly updated
*before* entering the UI.

BUG=b:124141368, b:35576380
TEST=make clean && make runtests
BRANCH=none

Change-Id: I5b0f4f7556c045ccc0d0739acc2668905a2a93e9
Signed-off-by: Joel Kitching <kitching@google.com>
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/2044954
Tested-by: Joel Kitching <kitching@chromium.org>
Reviewed-by: Julius Werner <jwerner@chromium.org>
Commit-Queue: Joel Kitching <kitching@chromium.org>

5 files changed, 46 insertions, 43 deletions

diff --git a/firmware/2lib/2misc.c b/firmware/2lib/2misc.c
index 43d39f8b..b289f315 100644
--- a/firmware/2lib/2misc.c
+++ b/firmware/2lib/2misc.c
@@ -416,6 +416,24 @@ int vb2_allow_recovery(struct vb2_context *ctx)
 	return !!(vb2_get_sd(ctx)->flags & VB2_SD_FLAG_MANUAL_RECOVERY);
 }
 
+void vb2_clear_recovery(struct vb2_context *ctx)
+{
+	struct vb2_shared_data *sd = vb2_get_sd(ctx);
+
+	VB2_DEBUG("Clearing recovery request: %#x / %#x\n",
+		  vb2_nv_get(ctx, VB2_NV_RECOVERY_REQUEST),
+		  vb2_nv_get(ctx, VB2_NV_RECOVERY_SUBCODE));
+
+	/* Clear recovery request for both cases. */
+	vb2_nv_set(ctx, VB2_NV_RECOVERY_REQUEST, VB2_RECOVERY_NOT_REQUESTED);
+
+	if (!vb2_allow_recovery(ctx)) {
+		VB2_DEBUG("Stow recovery reason as subcode (%#x)\n",
+			  sd->recovery_reason);
+		vb2_nv_set(ctx, VB2_NV_RECOVERY_SUBCODE, sd->recovery_reason);
+	}
+}
+
 int vb2api_need_reboot_for_display(struct vb2_context *ctx)
 {
 	if (!(vb2_get_sd(ctx)->flags & VB2_SD_FLAG_DISPLAY_AVAILABLE)) {
diff --git a/firmware/2lib/include/2misc.h b/firmware/2lib/include/2misc.h
index 6a61f9e5..b80f6906 100644
--- a/firmware/2lib/include/2misc.h
+++ b/firmware/2lib/include/2misc.h
@@ -188,4 +188,26 @@ vb2_error_t vb2_enable_developer_mode(struct vb2_context *ctx);
  */
 int vb2_allow_recovery(struct vb2_context *ctx);
 
+/**
+ * Clear recovery request appropriately.
+ *
+ * To avoid the recovery request "sticking" and the user being in a permanent
+ * recovery loop, the recovery request must be cleared and committed to nvdata.
+ * Note that this should be done at some point after we are certain the system
+ * does not require any reboots for non-vboot-related reasons (e.g. FSP
+ * initialization), and before triggering a reboot to exit a transient recovery
+ * mode (e.g. memory retraining request).
+ *
+ * In BROKEN cases, the recovery reason will be stowed away as subcode, to be
+ * retrieved after the user reboots in manual recovery.  In manual recovery,
+ * subcode will be left alone to keep available for subsequent manual recovery
+ * requests, or for accessing from userspace on the next boot.
+ *
+ * This function modifies nvdata in vb2_context, but the caller is still
+ * expected to call vb2_commit_data.
+ *
+ * @param ctx		Vboot context
+ */
+void vb2_clear_recovery(struct vb2_context *ctx);
+
 #endif  /* VBOOT_REFERENCE_2MISC_H_ */
diff --git a/firmware/lib/vboot_api_kernel.c b/firmware/lib/vboot_api_kernel.c
index 57a827cb..36bac6ed 100644
--- a/firmware/lib/vboot_api_kernel.c
+++ b/firmware/lib/vboot_api_kernel.c
@@ -374,23 +374,18 @@ vb2_error_t VbSelectAndLoadKernel(struct vb2_context *ctx,
 
 	/* Select boot path */
 	if (ctx->flags & VB2_CONTEXT_RECOVERY_MODE) {
+		vb2_clear_recovery(ctx);
+
 		/*
-		 * Clear recovery request and subcode from nvdata, so that we
-		 * don't get stuck in recovery mode after reboot.  Should be
-		 * called at some point after we are certain the system does
-		 * not require any reboots for non-vboot-related reasons (e.g.
-		 * FSP initialization), and before triggering a reboot to exit
-		 * transient recovery mode (e.g. memory retraining request).
+		 * Need to commit nvdata changes immediately, since we will be
+		 * entering either manual recovery UI or BROKEN screen shortly.
 		 */
-		vb2_nv_set(ctx, VB2_NV_RECOVERY_REQUEST,
-			   VB2_RECOVERY_NOT_REQUESTED);
-		vb2_nv_set(ctx, VB2_NV_RECOVERY_SUBCODE,
-			   VB2_RECOVERY_NOT_REQUESTED);
+		vb2_commit_data(ctx);
 
 		/* If we're in recovery mode just to do memory retraining, all
 		   we need to do is reboot. */
 		if (sd->recovery_reason == VB2_RECOVERY_TRAIN_AND_REBOOT) {
-			VB2_DEBUG("Reboot after retraining in recovery.\n");
+			VB2_DEBUG("Reboot after retraining in recovery\n");
 			rv = VBERROR_REBOOT_REQUIRED;
 			goto VbSelectAndLoadKernel_exit;
 		}
diff --git a/firmware/lib/vboot_ui_legacy_clamshell.c b/firmware/lib/vboot_ui_legacy_clamshell.c
index cc0c4175..8efcfe9e 100644
--- a/firmware/lib/vboot_ui_legacy_clamshell.c
+++ b/firmware/lib/vboot_ui_legacy_clamshell.c
@@ -440,25 +440,6 @@ static vb2_error_t recovery_ui(struct vb2_context *ctx)
 	VB2_DEBUG("recovery UI - start\n");
 
 	if (!vb2_allow_recovery(ctx)) {
-		/*
-		 * We have to save the reason here so that it will survive
-		 * coming up three-finger-salute. We're saving it in
-		 * VB2_RECOVERY_SUBCODE to avoid a recovery loop.
-		 * If we save the reason in VB2_RECOVERY_REQUEST, we will come
-		 * back here, thus, we won't be able to give a user a chance to
-		 * reboot to workaround a boot hiccup.
-		 */
-		VB2_DEBUG("recovery UI - saving recovery reason (%#x)\n",
-			  sd->recovery_reason);
-		vb2_nv_set(ctx, VB2_NV_RECOVERY_SUBCODE, sd->recovery_reason);
-
-		/*
-		 * Non-manual recovery mode is meant to be left via three-finger
-		 * salute (into manual recovery mode). Need to commit nvdata
-		 * changes immediately.  Ignore commit errors in recovery mode.
-		 */
-		vb2_commit_data(ctx);
-
 		VbDisplayScreen(ctx, VB_SCREEN_OS_BROKEN, 0, NULL);
 		VB2_DEBUG("recovery UI - waiting for manual recovery\n");
 		while (1) {
diff --git a/firmware/lib/vboot_ui_legacy_menu.c b/firmware/lib/vboot_ui_legacy_menu.c
index 2635a7f3..f441b410 100644
--- a/firmware/lib/vboot_ui_legacy_menu.c
+++ b/firmware/lib/vboot_ui_legacy_menu.c
@@ -837,19 +837,6 @@ vb2_error_t VbBootDeveloperLegacyMenu(struct vb2_context *ctx)
 /* Main function that handles non-manual recovery (BROKEN) menu functionality */
 static vb2_error_t broken_ui(struct vb2_context *ctx)
 {
-	VbSharedDataHeader *vbsd = vb2_get_sd(ctx)->vbsd;
-
-	/*
-	 * Temporarily stash recovery reason in subcode so we'll still know what
-	 * to display if the user reboots into manual recovery from here. Commit
-	 * immediately since the user may hard-reset out of here.
-	 */
-	VB2_DEBUG("saving recovery reason (%#x)\n", vbsd->recovery_reason);
-	vb2_nv_set(ctx, VB2_NV_RECOVERY_SUBCODE, vbsd->recovery_reason);
-
-	/* Ignore commit errors in recovery mode. */
-	vb2_commit_data(ctx);
-
 	enter_recovery_base_screen(ctx);
 
 	/* Loop and wait for the user to reset or shut down. */