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>

7 files changed, 150 insertions, 93 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. */
diff --git a/tests/vb2_misc_tests.c b/tests/vb2_misc_tests.c
index b3da6454..ac7671e2 100644
--- a/tests/vb2_misc_tests.c
+++ b/tests/vb2_misc_tests.c
@@ -23,12 +23,12 @@ static struct vb2_shared_data *sd;
 static struct vb2_gbb_header gbb;
 
 /* Mocked function data */
-enum vb2_resource_index mock_resource_index;
-void *mock_resource_ptr;
-uint32_t mock_resource_size;
-int mock_tpm_clear_called;
-int mock_tpm_clear_retval;
-
+static enum vb2_resource_index mock_resource_index;
+static void *mock_resource_ptr;
+static uint32_t mock_resource_size;
+static int mock_tpm_clear_called;
+static int mock_tpm_clear_retval;
+static int allow_recovery_retval;
 
 static void reset_common_data(void)
 {
@@ -49,9 +49,16 @@ static void reset_common_data(void)
 
 	mock_tpm_clear_called = 0;
 	mock_tpm_clear_retval = VB2_SUCCESS;
+	allow_recovery_retval = 0;
 };
 
 /* Mocked functions */
+
+int vb2_allow_recovery(struct vb2_context *c)
+{
+	return allow_recovery_retval;
+}
+
 struct vb2_gbb_header *vb2_get_gbb(struct vb2_context *c)
 {
 	return &gbb;
@@ -750,6 +757,34 @@ static void need_reboot_for_display_tests(void)
 		"  not set display request");
 }
 
+static void clear_recovery_tests(void)
+{
+
+	/* Manual recovery */
+	reset_common_data();
+	allow_recovery_retval = 1;
+	sd->recovery_reason = 4;
+	vb2_nv_set(ctx, VB2_NV_RECOVERY_REQUEST, 5);
+	vb2_nv_set(ctx, VB2_NV_RECOVERY_SUBCODE, 13);
+	vb2_clear_recovery(ctx);
+	TEST_EQ(vb2_nv_get(ctx, VB2_NV_RECOVERY_REQUEST),
+		0, "  request cleared");
+	TEST_EQ(vb2_nv_get(ctx, VB2_NV_RECOVERY_SUBCODE),
+		13, "  subcode retained");
+
+	/* BROKEN recovery */
+	reset_common_data();
+	allow_recovery_retval = 0;
+	sd->recovery_reason = 4;
+	vb2_nv_set(ctx, VB2_NV_RECOVERY_REQUEST, 5);
+	vb2_nv_set(ctx, VB2_NV_RECOVERY_SUBCODE, 13);
+	vb2_clear_recovery(ctx);
+	TEST_EQ(vb2_nv_get(ctx, VB2_NV_RECOVERY_REQUEST),
+		0, "  request cleared");
+	TEST_EQ(vb2_nv_get(ctx, VB2_NV_RECOVERY_SUBCODE),
+		4, "  subcode shifted");
+}
+
 int main(int argc, char* argv[])
 {
 	init_workbuf_tests();
@@ -761,6 +796,7 @@ int main(int argc, char* argv[])
 	tpm_clear_tests();
 	select_slot_tests();
 	need_reboot_for_display_tests();
+	clear_recovery_tests();
 
 	return gTestSuccess ? 0 : 255;
 }
diff --git a/tests/vboot_api_kernel4_tests.c b/tests/vboot_api_kernel4_tests.c
index 91ba5198..6ac25bd8 100644
--- a/tests/vboot_api_kernel4_tests.c
+++ b/tests/vboot_api_kernel4_tests.c
@@ -22,6 +22,7 @@
 #include "vboot_test.h"
 
 /* Mock data */
+
 static uint8_t workbuf[VB2_KERNEL_WORKBUF_RECOMMENDED_SIZE]
 	__attribute__((aligned(VB2_WORKBUF_ALIGN)));
 static struct vb2_context *ctx;
@@ -39,13 +40,14 @@ static int commit_data_called;
 static vb2_error_t secdata_kernel_init_retval;
 static vb2_error_t secdata_fwmp_init_retval;
 static vb2_error_t kernel_phase1_retval;
+static uint32_t current_recovery_reason;
+static uint32_t expected_recovery_reason;
 
 static uint32_t mock_switches[8];
 static uint32_t mock_switches_count;
 static int mock_switches_are_stuck;
 
-/* Reset mock data (for use before each test) */
-static void ResetMocks(void)
+static void reset_common_data(void)
 {
 	memset(&kparams, 0, sizeof(kparams));
 
@@ -73,12 +75,23 @@ static void ResetMocks(void)
 	secdata_kernel_init_retval = VB2_SUCCESS;
 	secdata_fwmp_init_retval = VB2_SUCCESS;
 	kernel_phase1_retval = VB2_SUCCESS;
+	current_recovery_reason = 0;
+	expected_recovery_reason = 0;
 
 	memset(mock_switches, 0, sizeof(mock_switches));
 	mock_switches_count = 0;
 	mock_switches_are_stuck = 0;
 }
 
+static void test_slk(vb2_error_t retval, int recovery_reason, const char *desc)
+{
+	expected_recovery_reason = recovery_reason;
+	TEST_EQ(VbSelectAndLoadKernel(ctx, shared, &kparams), retval, desc);
+	TEST_EQ(current_recovery_reason, expected_recovery_reason,
+		"  recovery reason");
+	TEST_TRUE(commit_data_called, "  commit nvdata");
+}
+
 /* Mock functions */
 
 struct vb2_gbb_header *vb2_get_gbb(struct vb2_context *c)
@@ -96,6 +109,7 @@ vb2_error_t vb2api_kernel_phase1(struct vb2_context *c)
 
 vb2_error_t vb2ex_commit_data(struct vb2_context *c)
 {
+	current_recovery_reason = vb2_nv_get(ctx, VB2_NV_RECOVERY_REQUEST);
 	commit_data_called = 1;
 	return commit_data_retval;
 }
@@ -139,6 +153,10 @@ vb2_error_t VbBootDeveloperLegacyClamshell(struct vb2_context *c)
 
 vb2_error_t VbBootRecoveryLegacyClamshell(struct vb2_context *c)
 {
+	TEST_EQ(current_recovery_reason, expected_recovery_reason,
+		"  recovery reason");
+	TEST_TRUE(commit_data_called, "  commit nvdata");
+
 	shared->kernel_version_tpm = new_version;
 
 	if (vbboot_retval == -3)
@@ -155,15 +173,6 @@ vb2_error_t VbBootDiagnosticLegacyClamshell(struct vb2_context *c)
 	return vbboot_retval;
 }
 
-static void test_slk(vb2_error_t retval, int recovery_reason, const char *desc)
-{
-	TEST_EQ(VbSelectAndLoadKernel(ctx, shared, &kparams), retval, desc);
-	TEST_EQ(vb2_nv_get(ctx, VB2_NV_RECOVERY_REQUEST),
-		recovery_reason, "  recovery reason");
-	if (recovery_reason)
-		TEST_TRUE(commit_data_called, "  didn't commit nvdata");
-}
-
 uint32_t VbExGetSwitches(uint32_t request_mask)
 {
 	if (mock_switches_are_stuck)
@@ -181,67 +190,67 @@ vb2_error_t vb2ex_tpm_set_mode(enum vb2_tpm_mode mode_val)
 
 /* Tests */
 
-static void VbSlkTest(void)
+static void select_and_load_kernel_tests(void)
 {
 	/* Normal boot */
-	ResetMocks();
+	reset_common_data();
 	test_slk(0, 0, "Normal");
 	TEST_EQ(kernel_version, 0x10002, "  version");
 	TEST_NEQ(sd->flags & VB2_SD_STATUS_EC_SYNC_COMPLETE, 0,
 		 "  EC sync complete");
 
 	/* Check EC sync toggling */
-	ResetMocks();
+	reset_common_data();
 	ctx->flags |= VB2_CONTEXT_EC_SYNC_SUPPORTED;
 	gbb.flags |= VB2_GBB_FLAG_DISABLE_EC_SOFTWARE_SYNC;
 	test_slk(0, 0, "EC sync disabled by GBB");
 	TEST_NEQ(sd->flags & VB2_SD_STATUS_EC_SYNC_COMPLETE, 0,
 		 "  EC sync complete");
 
-	ResetMocks();
+	reset_common_data();
 	ctx->flags |= VB2_CONTEXT_EC_SYNC_SUPPORTED;
 	test_slk(0, 0, "Normal with EC sync");
 	TEST_NEQ(sd->flags & VB2_SD_STATUS_EC_SYNC_COMPLETE, 0,
 		 "  EC sync complete");
 
-	ResetMocks();
+	reset_common_data();
 	new_version = 0x20003;
 	test_slk(0, 0, "Roll forward");
 	TEST_EQ(kernel_version, 0x20003, "  version");
 
-	ResetMocks();
+	reset_common_data();
 	vb2_nv_set(ctx, VB2_NV_FW_RESULT, VB2_FW_RESULT_TRYING);
 	new_version = 0x20003;
 	test_slk(0, 0, "Don't roll forward kernel when trying new FW");
 	TEST_EQ(kernel_version, 0x10002, "  version");
 
-	ResetMocks();
+	reset_common_data();
 	vb2_nv_set(ctx, VB2_NV_KERNEL_MAX_ROLLFORWARD, 0x30005);
 	new_version = 0x40006;
 	test_slk(0, 0, "Limit max roll forward");
 	TEST_EQ(kernel_version, 0x30005, "  version");
 
-	ResetMocks();
+	reset_common_data();
 	vb2_nv_set(ctx, VB2_NV_KERNEL_MAX_ROLLFORWARD, 0x10001);
 	new_version = 0x40006;
 	test_slk(0, 0, "Max roll forward can't rollback");
 	TEST_EQ(kernel_version, 0x10002, "  version");
 
 
-	ResetMocks();
+	reset_common_data();
 	new_version = 0x20003;
 	commit_data_retval = VB2_ERROR_SECDATA_KERNEL_WRITE;
 	test_slk(VB2_ERROR_SECDATA_KERNEL_WRITE,
 		 VB2_RECOVERY_RW_TPM_W_ERROR, "Write kernel rollback");
 
 	/* Boot normal */
-	ResetMocks();
+	reset_common_data();
 	vbboot_retval = -1;
 	test_slk(VB2_ERROR_MOCK, 0, "Normal boot bad");
 
 	/* Check that NV_DIAG_REQUEST triggers diagnostic UI */
 	if (DIAGNOSTIC_UI) {
-		ResetMocks();
+		reset_common_data();
 		mock_switches[1] = VB_SWITCH_FLAG_PHYS_PRESENCE_PRESSED;
 		vb2_nv_set(ctx, VB2_NV_DIAG_REQUEST, 1);
 		vbboot_retval = -4;
@@ -254,12 +263,12 @@ static void VbSlkTest(void)
 	}
 
 	/* Boot normal - phase1 failure */
-	ResetMocks();
+	reset_common_data();
 	kernel_phase1_retval = VB2_ERROR_MOCK;
 	test_slk(VB2_ERROR_MOCK, 0, "Normal phase1 failure");
 
 	/* Boot normal - commit data failures */
-	ResetMocks();
+	reset_common_data();
 	commit_data_retval = VB2_ERROR_SECDATA_FIRMWARE_WRITE;
 	test_slk(commit_data_retval, VB2_RECOVERY_RW_TPM_W_ERROR,
 		 "Normal secdata_firmware write error triggers recovery");
@@ -274,75 +283,84 @@ static void VbSlkTest(void)
 		   "Normal unknown commit error aborts");
 
 	/* Boot dev */
-	ResetMocks();
+	reset_common_data();
 	sd->flags |= VB2_SD_FLAG_DEV_MODE_ENABLED;
 	vbboot_retval = -2;
 	test_slk(VB2_ERROR_MOCK, 0, "Dev boot bad");
 
-	ResetMocks();
+	reset_common_data();
 	sd->flags |= VB2_SD_FLAG_DEV_MODE_ENABLED;
 	new_version = 0x20003;
 	test_slk(0, 0, "Dev doesn't roll forward");
 	TEST_EQ(kernel_version, 0x10002, "  version");
 
 	/* Boot dev - phase1 failure */
-	ResetMocks();
+	reset_common_data();
 	sd->flags |= VB2_SD_FLAG_DEV_MODE_ENABLED;
 	kernel_phase1_retval = VB2_ERROR_MOCK;
 	test_slk(VB2_ERROR_MOCK, 0, "Dev phase1 failure");
 
 	/* Boot recovery */
-	ResetMocks();
+	reset_common_data();
 	sd->recovery_reason = 123;
 	vbboot_retval = -3;
 	test_slk(VB2_ERROR_MOCK, 0, "Recovery boot bad");
 
-	ResetMocks();
+	reset_common_data();
 	sd->recovery_reason = 123;
 	new_version = 0x20003;
 	test_slk(0, 0, "Recovery doesn't roll forward");
 	TEST_EQ(kernel_version, 0x10002, "  version");
 
 	/* Boot recovery - phase1 failure */
-	ResetMocks();
+	reset_common_data();
 	sd->recovery_reason = 123;
 	kernel_phase1_retval = VB2_ERROR_MOCK;
 	test_slk(VB2_ERROR_MOCK, 0, "Recovery phase1 failure");
 
 	/* Boot recovery - commit data failures */
-	ResetMocks();
+	reset_common_data();
 	sd->recovery_reason = 123;
 	commit_data_retval = VB2_ERROR_SECDATA_FIRMWARE_WRITE;
 	test_slk(0, 0, "Recovery ignore secdata_firmware write error");
+
+	reset_common_data();
+	sd->recovery_reason = 123;
 	commit_data_retval = VB2_ERROR_SECDATA_KERNEL_WRITE;
 	test_slk(0, 0, "Recovery ignore secdata_kernel write error");
+
+	reset_common_data();
+	sd->recovery_reason = 123;
 	commit_data_retval = VB2_ERROR_NV_WRITE;
 	test_slk(0, 0, "Recovery return nvdata write error");
-	commit_data_retval = VB2_ERROR_UNKNOWN;
-	test_slk(0, 0, "Recovery return unknown write error");
 
-	/* Boot recovery - nvstorage cleared */
-	ResetMocks();
+	reset_common_data();
 	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");
+	commit_data_retval = VB2_ERROR_UNKNOWN;
+	test_slk(0, 0, "Recovery return unknown write error");
 
 	/* Boot recovery - memory retraining */
-	ResetMocks();
+	reset_common_data();
 	sd->recovery_reason = VB2_RECOVERY_TRAIN_AND_REBOOT;
 	test_slk(VBERROR_REBOOT_REQUIRED, 0, "Recovery train and reboot");
 
-	// todo: rkr/w/l fail ignored if recovery
-
+	/* Boot BROKEN recovery */
+	reset_common_data();
+	sd->recovery_reason = 123;
+	vb2_nv_set(ctx, VB2_NV_RECOVERY_SUBCODE, 13);
+	test_slk(0, 0, "BROKEN recovery");
 
+	/* Boot manual recovery */
+	reset_common_data();
+	sd->recovery_reason = VB2_RECOVERY_RO_MANUAL;
+	vb2_nv_set(ctx, VB2_NV_RECOVERY_SUBCODE, 13);
+	sd->flags |= VB2_SD_FLAG_MANUAL_RECOVERY;
+	test_slk(0, 0, "Manual recovery");
 }
 
 int main(void)
 {
-	VbSlkTest();
+	select_and_load_kernel_tests();
 
 	return gTestSuccess ? 0 : 255;
 }