vboot: only clear recovery requests at kernel verification

Instead of clearing recovery requests early on in firmware
verification, defer this task until kernel verification has
begun.

If the system is rebooted for any non-vboot-related reason when
entering recovery mode (e.g. FSP initialization), the recovery
request will still be available in nvdata.

Additionally, relocate the reboot triggered by memory training
into VbSelectAndLoadKernel.

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

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

5 files changed, 40 insertions, 21 deletions

diff --git a/firmware/2lib/2misc.c b/firmware/2lib/2misc.c
index bab4b211..43d39f8b 100644
--- a/firmware/2lib/2misc.c
+++ b/firmware/2lib/2misc.c
@@ -146,10 +146,6 @@ void vb2_check_recovery(struct vb2_context *ctx)
 	if (!sd->recovery_reason)
 		sd->recovery_reason = reason;
 
-	/* Clear request and subcode so we don't get stuck in recovery mode */
-	vb2_nv_set(ctx, VB2_NV_RECOVERY_REQUEST, VB2_RECOVERY_NOT_REQUESTED);
-	vb2_nv_set(ctx, VB2_NV_RECOVERY_SUBCODE, VB2_RECOVERY_NOT_REQUESTED);
-
 	if (ctx->flags & VB2_CONTEXT_FORCE_RECOVERY_MODE) {
 		VB2_DEBUG("Recovery was requested manually\n");
 		if (subcode && !sd->recovery_reason)
diff --git a/firmware/lib/vboot_api_kernel.c b/firmware/lib/vboot_api_kernel.c
index 5139ba33..d6fab619 100644
--- a/firmware/lib/vboot_api_kernel.c
+++ b/firmware/lib/vboot_api_kernel.c
@@ -262,15 +262,6 @@ static vb2_error_t vb2_kernel_setup(struct vb2_context *ctx,
 	 */
 	sd->vbsd = shared;
 
-	/*
-	 * 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");
-		return VBERROR_REBOOT_REQUIRED;
-	}
-
 	/* Fill in params for calls to LoadKernel() */
 	memset(&lkp, 0, sizeof(lkp));
 	lkp.kernel_buffer = kparams->kernel_buffer;
@@ -373,6 +364,7 @@ vb2_error_t VbSelectAndLoadKernel(struct vb2_context *ctx,
 				  VbSharedDataHeader *shared,
 				  VbSelectAndLoadKernelParams *kparams)
 {
+	struct vb2_shared_data *sd = vb2_get_sd(ctx);
 	vb2_error_t rv, call_rv;
 
 	rv = vb2_kernel_setup(ctx, shared, kparams);
@@ -401,6 +393,27 @@ vb2_error_t VbSelectAndLoadKernel(struct vb2_context *ctx,
 
 	/* Select boot path */
 	if (ctx->flags & VB2_CONTEXT_RECOVERY_MODE) {
+		/*
+		 * 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).
+		 */
+		vb2_nv_set(ctx, VB2_NV_RECOVERY_REQUEST,
+			   VB2_RECOVERY_NOT_REQUESTED);
+		vb2_nv_set(ctx, VB2_NV_RECOVERY_SUBCODE,
+			   VB2_RECOVERY_NOT_REQUESTED);
+
+		/* 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");
+			rv = VBERROR_REBOOT_REQUIRED;
+			goto VbSelectAndLoadKernel_exit;
+		}
+
 		/* Recovery boot.  This has UI. */
 		if (LEGACY_MENU_UI)
 			rv = VbBootRecoveryMenu(ctx);
diff --git a/tests/vb2_api_tests.c b/tests/vb2_api_tests.c
index c2a52dbb..9cb2f195 100644
--- a/tests/vb2_api_tests.c
+++ b/tests/vb2_api_tests.c
@@ -393,7 +393,7 @@ static void phase1_tests(void)
 	TEST_EQ(vb2_nv_get(ctx, VB2_NV_TPM_REQUESTED_REBOOT),
 		1, "  tpm reboot request");
 	TEST_EQ(vb2_nv_get(ctx, VB2_NV_RECOVERY_REQUEST),
-		0, "  recovery request");
+		VB2_RECOVERY_RO_TPM_REBOOT, "  recovery request");
 
 	reset_common_data(FOR_MISC);
 	ctx->flags |= VB2_CONTEXT_SECDATA_WANTS_REBOOT;
@@ -405,7 +405,7 @@ static void phase1_tests(void)
 	TEST_EQ(vb2_nv_get(ctx, VB2_NV_TPM_REQUESTED_REBOOT),
 		1, "  tpm reboot request");
 	TEST_EQ(vb2_nv_get(ctx, VB2_NV_RECOVERY_REQUEST),
-		VB2_RECOVERY_RO_UNSPECIFIED, "  recovery request not cleared");
+		VB2_RECOVERY_RO_UNSPECIFIED, "  recovery request");
 
 	reset_common_data(FOR_MISC);
 	vb2_nv_set(ctx, VB2_NV_TPM_REQUESTED_REBOOT, 1);
@@ -416,8 +416,8 @@ static void phase1_tests(void)
 		"  recovery reason");
 	TEST_EQ(vb2_nv_get(ctx, VB2_NV_TPM_REQUESTED_REBOOT),
 		0, "  tpm reboot request");
-	TEST_EQ(vb2_nv_get(ctx, VB2_NV_RECOVERY_REQUEST), 0,
-		"  recovery request cleared");
+	TEST_EQ(vb2_nv_get(ctx, VB2_NV_RECOVERY_REQUEST),
+		VB2_RECOVERY_RO_UNSPECIFIED, "  recovery request");
 
 	reset_common_data(FOR_MISC);
 	ctx->flags |= VB2_CONTEXT_SECDATA_WANTS_REBOOT;
@@ -429,8 +429,8 @@ static void phase1_tests(void)
 		"  recovery reason");
 	TEST_EQ(vb2_nv_get(ctx, VB2_NV_TPM_REQUESTED_REBOOT),
 		1, "  tpm reboot request");
-	TEST_EQ(vb2_nv_get(ctx, VB2_NV_RECOVERY_REQUEST), 0,
-		"  recovery request cleared");
+	TEST_EQ(vb2_nv_get(ctx, VB2_NV_RECOVERY_REQUEST),
+		VB2_RECOVERY_RO_UNSPECIFIED, "  recovery request");
 
 	/* Cases for checking DISPLAY_INIT and DISPLAY_AVAILABLE. */
 	reset_common_data(FOR_MISC);
diff --git a/tests/vb2_misc_tests.c b/tests/vb2_misc_tests.c
index 22de3ae2..2a3f88dc 100644
--- a/tests/vb2_misc_tests.c
+++ b/tests/vb2_misc_tests.c
@@ -414,7 +414,7 @@ static void recovery_tests(void)
 	vb2_nv_set(ctx, VB2_NV_RECOVERY_REQUEST, 3);
 	vb2_check_recovery(ctx);
 	TEST_EQ(sd->recovery_reason, 3, "Recovery reason from request");
-	TEST_EQ(vb2_nv_get(ctx, VB2_NV_RECOVERY_REQUEST), 0, "NV cleared");
+	TEST_EQ(vb2_nv_get(ctx, VB2_NV_RECOVERY_REQUEST), 3, "NV not cleared");
 	TEST_EQ(sd->flags & VB2_SD_FLAG_MANUAL_RECOVERY,
 		0, "Not manual recovery");
 	TEST_NEQ(ctx->flags & VB2_CONTEXT_RECOVERY_MODE,
@@ -427,7 +427,7 @@ static void recovery_tests(void)
 	vb2_check_recovery(ctx);
 	TEST_EQ(sd->recovery_reason, 5, "Recovery reason already failed");
 	TEST_EQ(vb2_nv_get(ctx, VB2_NV_RECOVERY_REQUEST),
-		0, "NV still cleared");
+		4, "NV not cleared");
 
 	/* Override */
 	reset_common_data();
diff --git a/tests/vboot_api_kernel4_tests.c b/tests/vboot_api_kernel4_tests.c
index cbae595c..97a6b1d2 100644
--- a/tests/vboot_api_kernel4_tests.c
+++ b/tests/vboot_api_kernel4_tests.c
@@ -301,6 +301,16 @@ static void VbSlkTest(void)
 	commit_data_retval = VB2_ERROR_UNKNOWN;
 	test_slk(0, 0, "Recovery return unknown write error");
 
+	/* Boot recovery - nvstorage cleared */
+	ResetMocks();
+	sd->recovery_reason = 123;
+	vb2_nv_set(ctx, VB2_NV_RECOVERY_REQUEST, 5);
+	vb2_nv_set(ctx, VB2_NV_RECOVERY_SUBCODE, 13);
+	test_slk(0, 0, "Recovery with nvstorage");
+	TEST_EQ(vb2_nv_get(ctx, VB2_NV_RECOVERY_SUBCODE),
+		0, "  recovery subcode cleared");
+
+	/* Boot recovery - memory retraining */
 	ResetMocks();
 	sd->recovery_reason = VB2_RECOVERY_TRAIN_AND_REBOOT;
 	test_slk(VBERROR_REBOOT_REQUIRED, 0, "Recovery train and reboot");