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>
diff --git a/firmware/2lib/2misc.c b/firmware/2lib/2misc.c
index bab4b21..43d39f8 100644
--- a/firmware/2lib/2misc.c
+++ b/firmware/2lib/2misc.c
@@ -146,10 +146,6 @@
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 5139ba3..d6fab61 100644
--- a/firmware/lib/vboot_api_kernel.c
+++ b/firmware/lib/vboot_api_kernel.c
@@ -262,15 +262,6 @@
*/
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 @@
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 @@
/* 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 c2a52db..9cb2f19 100644
--- a/tests/vb2_api_tests.c
+++ b/tests/vb2_api_tests.c
@@ -393,7 +393,7 @@
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 @@
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 @@
" 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 @@
" 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 22de3ae..2a3f88d 100644
--- a/tests/vb2_misc_tests.c
+++ b/tests/vb2_misc_tests.c
@@ -414,7 +414,7 @@
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 @@
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 cbae595..97a6b1d 100644
--- a/tests/vboot_api_kernel4_tests.c
+++ b/tests/vboot_api_kernel4_tests.c
@@ -301,6 +301,16 @@
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");
