vboot: clear recovery request in all boot modes

Previously, recovery requests are only cleared when user
initiates a manual recovery.  This causes problems with
two cases specifically:

  * Transient failures - The recovery request remains in the
    subcode field for some unknown period of time, and then
    erroneously gets promoted to the "recovery reason" the
    next time the user initiates a manual recovery request.

  * TRAIN_AND_REBOOT - The recovery request remains in the
    subcode field after training has completed.  The next
    time a manual recovery request is initiated, the subcode
    is promoted and training occurs yet again.  When finished,
    a reboot occurs and the user ends up back in the OS.

Make two changes to deal with these cases:

* Clear recovery request (including subcode) unconditionally
  for non-recovery boot modes.

* Stop promoting TRAIN_AND_REBOOT subcodes.

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

Change-Id: I79f8fbed72a9d052b5ed5f70e9a2515136b6ef10
Signed-off-by: Joel Kitching <kitching@google.com>
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/2139335
Tested-by: Joel Kitching <kitching@chromium.org>
Tested-by: Frank Wu <frank_wu@compal.corp-partner.google.com>
Reviewed-by: Julius Werner <jwerner@chromium.org>
Commit-Queue: Joel Kitching <kitching@chromium.org>

3 files changed, 33 insertions, 11 deletions

diff --git a/firmware/2lib/2misc.c b/firmware/2lib/2misc.c
index 30d6011c..de4b4241 100644
--- a/firmware/2lib/2misc.c
+++ b/firmware/2lib/2misc.c
@@ -144,7 +144,8 @@ void vb2_check_recovery(struct vb2_context *ctx)
 
 	if (ctx->flags & VB2_CONTEXT_FORCE_RECOVERY_MODE) {
 		VB2_DEBUG("Recovery was requested manually\n");
-		if (subcode && !sd->recovery_reason)
+		if (subcode && !sd->recovery_reason &&
+		    subcode != VB2_RECOVERY_TRAIN_AND_REBOOT)
 			/*
 			 * Recovery was requested at 'broken' screen.
 			 * Promote subcode to reason.
@@ -416,16 +417,20 @@ int vb2_allow_recovery(struct vb2_context *ctx)
 void vb2_clear_recovery(struct vb2_context *ctx)
 {
 	struct vb2_shared_data *sd = vb2_get_sd(ctx);
+	uint32_t reason = vb2_nv_get(ctx, VB2_NV_RECOVERY_REQUEST);
+	uint32_t subcode = vb2_nv_get(ctx, VB2_NV_RECOVERY_SUBCODE);
 
-	VB2_DEBUG("Clearing recovery request: %#x / %#x\n",
-		  vb2_nv_get(ctx, VB2_NV_RECOVERY_REQUEST),
-		  vb2_nv_get(ctx, VB2_NV_RECOVERY_SUBCODE));
+	if (reason || subcode)
+		VB2_DEBUG("Clearing recovery request: %#x / %#x\n",
+			  reason, subcode);
 
-	/* Clear recovery request for both cases. */
+	/* Clear recovery request for both manual and non-manual. */
 	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_nv_set(ctx, VB2_NV_RECOVERY_SUBCODE, 0);
 
-	if (!vb2_allow_recovery(ctx)) {
+	/* But stow recovery reason as subcode for non-manual recovery. */
+	if ((ctx->flags & VB2_CONTEXT_RECOVERY_MODE) &&
+	    !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);
diff --git a/firmware/lib/vboot_api_kernel.c b/firmware/lib/vboot_api_kernel.c
index ff13a256..14f70c71 100644
--- a/firmware/lib/vboot_api_kernel.c
+++ b/firmware/lib/vboot_api_kernel.c
@@ -198,10 +198,14 @@ vb2_error_t VbSelectAndLoadKernel(struct vb2_context *ctx,
 		VB2_TRY(handle_battery_cutoff(ctx));
 	}
 
+	/*
+	 * If in non-manual recovery mode, save the recovery reason as subcode.
+	 * Otherwise, clear any leftover recovery requests or subcodes.
+	 */
+	vb2_clear_recovery(ctx);
+
 	/* Select boot path */
 	if (ctx->flags & VB2_CONTEXT_RECOVERY_MODE) {
-		vb2_clear_recovery(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) {
@@ -214,6 +218,7 @@ vb2_error_t VbSelectAndLoadKernel(struct vb2_context *ctx,
 		 * entering either manual recovery UI or BROKEN screen shortly.
 		 */
 		vb2ex_commit_data(ctx);
+
 		/*
 		 * In EFS2, recovery mode can be entered even when battery is
 		 * drained or damaged. EC-RO sets NO_BOOT flag in such case and
diff --git a/tests/vb2_misc_tests.c b/tests/vb2_misc_tests.c
index 1d8b42ed..493aab45 100644
--- a/tests/vb2_misc_tests.c
+++ b/tests/vb2_misc_tests.c
@@ -458,13 +458,23 @@ static void recovery_tests(void)
 	TEST_NEQ(sd->flags & VB2_SD_FLAG_MANUAL_RECOVERY,
 		 0, "SD flag set");
 
-	/* Override at broken screen */
+	/* Override subcode TRAIN_AND_REBOOT */
+	reset_common_data();
+	vb2_nv_set(ctx, VB2_NV_RECOVERY_SUBCODE, VB2_RECOVERY_TRAIN_AND_REBOOT);
+	ctx->flags |= VB2_CONTEXT_FORCE_RECOVERY_MODE;
+	vb2_check_recovery(ctx);
+	TEST_EQ(sd->recovery_reason, VB2_RECOVERY_RO_MANUAL,
+		"Recovery reason forced");
+	TEST_NEQ(sd->flags & VB2_SD_FLAG_MANUAL_RECOVERY,
+		 0, "SD flag set");
+
+	/* Promote subcode from BROKEN screen*/
 	reset_common_data();
 	vb2_nv_set(ctx, VB2_NV_RECOVERY_SUBCODE, VB2_RECOVERY_US_TEST);
 	ctx->flags |= VB2_CONTEXT_FORCE_RECOVERY_MODE;
 	vb2_check_recovery(ctx);
 	TEST_EQ(sd->recovery_reason, VB2_RECOVERY_US_TEST,
-		"Recovery reason forced from broken");
+		"Recovery reason forced from BROKEN");
 	TEST_NEQ(sd->flags & VB2_SD_FLAG_MANUAL_RECOVERY,
 		 0, "SD flag set");
 }
@@ -768,6 +778,7 @@ static void clear_recovery_tests(void)
 	reset_common_data();
 	allow_recovery_retval = 1;
 	sd->recovery_reason = 4;
+	ctx->flags |= VB2_CONTEXT_RECOVERY_MODE;
 	vb2_nv_set(ctx, VB2_NV_RECOVERY_REQUEST, 5);
 	vb2_nv_set(ctx, VB2_NV_RECOVERY_SUBCODE, 13);
 	vb2_clear_recovery(ctx);
@@ -780,6 +791,7 @@ static void clear_recovery_tests(void)
 	reset_common_data();
 	allow_recovery_retval = 0;
 	sd->recovery_reason = 4;
+	ctx->flags |= VB2_CONTEXT_RECOVERY_MODE;
 	vb2_nv_set(ctx, VB2_NV_RECOVERY_REQUEST, 5);
 	vb2_nv_set(ctx, VB2_NV_RECOVERY_SUBCODE, 13);
 	vb2_clear_recovery(ctx);