firmware: Don't set recovery reason for removable boot in TryLoadKernel

Right now TryLoadKernel() always sets a recovery reason when it did not
manage to load a kernel for any reason. In many cases (e.g. we're
already in recovery mode, or we're trying to boot off some random USB
stick in dev mode) we don't actually want that to happen, so there are
four different instances of code unconditionally clearing the recovery
reason again right after calling TryLoadKernel().

This is confusing and there's a far simpler solution: only set the
recovery reason when we're booting off a fixed disk. We never want to
set it when trying to boot a removable disk anyway, so centralizing this
distinction right in TryLoadKernel() makes the logic easier to follow.

BRANCH=None
BUG=None
TEST=make runtests, played around with a Kevin

Change-Id: I9d56356b0f3547b3690be2c24cf6936e57e4cf1f
Signed-off-by: Julius Werner <jwerner@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/1859687
(cherry picked from commit 0320617a0b9abf44eba822e1cb043a87a6e808fe)
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/1902829
Reviewed-by: Hung-Te Lin <hungte@chromium.org>
Commit-Queue: Hung-Te Lin <hungte@chromium.org>
Tested-by: Hung-Te Lin <hungte@chromium.org>
Auto-Submit: Hung-Te Lin <hungte@chromium.org>

4 files changed, 47 insertions, 41 deletions

diff --git a/firmware/lib/vboot_api_kernel.c b/firmware/lib/vboot_api_kernel.c
index 094cc3f3..659cb07c 100644
--- a/firmware/lib/vboot_api_kernel.c
+++ b/firmware/lib/vboot_api_kernel.c
@@ -121,19 +121,21 @@ vb2_error_t VbTryLoadKernel(struct vb2_context *ctx, uint32_t get_info_flags)
 	}
 
 	/* If we drop out of the loop, we didn't find any usable kernel. */
-	switch (rv) {
-	case VBERROR_INVALID_KERNEL_FOUND:
-		vb2api_fail(ctx, VB2_RECOVERY_RW_INVALID_OS, rv);
-		break;
-	case VBERROR_NO_KERNEL_FOUND:
-		vb2api_fail(ctx, VB2_RECOVERY_RW_NO_KERNEL, rv);
-		break;
-	case VBERROR_NO_DISK_FOUND:
-		vb2api_fail(ctx, VB2_RECOVERY_RW_NO_DISK, rv);
-		break;
-	default:
-		vb2api_fail(ctx, VB2_RECOVERY_LK_UNSPECIFIED, rv);
-		break;
+	if (get_info_flags & VB_DISK_FLAG_FIXED) {
+		switch (rv) {
+		case VBERROR_INVALID_KERNEL_FOUND:
+			vb2api_fail(ctx, VB2_RECOVERY_RW_INVALID_OS, rv);
+			break;
+		case VBERROR_NO_KERNEL_FOUND:
+			vb2api_fail(ctx, VB2_RECOVERY_RW_NO_KERNEL, rv);
+			break;
+		case VBERROR_NO_DISK_FOUND:
+			vb2api_fail(ctx, VB2_RECOVERY_RW_NO_DISK, rv);
+			break;
+		default:
+			vb2api_fail(ctx, VB2_RECOVERY_LK_UNSPECIFIED, rv);
+			break;
+		}
 	}
 
 	/* If we didn't find any good kernels, don't return a disk handle. */
diff --git a/firmware/lib/vboot_ui.c b/firmware/lib/vboot_ui.c
index a2cdab46..cae29ec8 100644
--- a/firmware/lib/vboot_ui.c
+++ b/firmware/lib/vboot_ui.c
@@ -91,14 +91,6 @@ static vb2_error_t VbTryUsb(struct vb2_context *ctx)
 		vb2_error_notify("Could not boot from USB\n",
 				 "VbBootDeveloper() - no kernel found on USB\n",
 				 VB_BEEP_FAILED);
-		/*
-		 * Clear recovery requests from failed
-		 * kernel loading, so that powering off
-		 * at this point doesn't put us into
-		 * recovery mode.
-		 */
-		vb2_nv_set(ctx, VB2_NV_RECOVERY_REQUEST,
-			   VB2_RECOVERY_NOT_REQUESTED);
 	}
 	return retval;
 }
@@ -855,15 +847,6 @@ static vb2_error_t recovery_ui(struct vb2_context *ctx)
 		VB2_DEBUG("VbBootRecovery() attempting to load kernel2\n");
 		retval = VbTryLoadKernel(ctx, VB_DISK_FLAG_REMOVABLE);
 
-		/*
-		 * Clear recovery requests from failed kernel loading, since
-		 * we're already in recovery mode.  Do this now, so that
-		 * powering off after inserting an invalid disk doesn't leave
-		 * us stuck in recovery mode.
-		 */
-		vb2_nv_set(ctx, VB2_NV_RECOVERY_REQUEST,
-			   VB2_RECOVERY_NOT_REQUESTED);
-
 		if (VB2_SUCCESS == retval)
 			break; /* Found a recovery kernel */
 
diff --git a/firmware/lib/vboot_ui_menu.c b/firmware/lib/vboot_ui_menu.c
index 58349ca3..d1808b90 100644
--- a/firmware/lib/vboot_ui_menu.c
+++ b/firmware/lib/vboot_ui_menu.c
@@ -198,8 +198,6 @@ static vb2_error_t boot_usb_action(struct vb2_context *ctx)
 		return VB2_SUCCESS;
 	}
 
-	/* Loading kernel failed. Clear recovery request from that. */
-	vb2_nv_set(ctx, VB2_NV_RECOVERY_REQUEST, VB2_RECOVERY_NOT_REQUESTED);
 	vb2_flash_screen(ctx);
 	vb2_error_notify(no_kernel, NULL, VB_BEEP_FAILED);
 	return VBERROR_KEEP_LOOPING;
@@ -894,15 +892,6 @@ static vb2_error_t recovery_ui(struct vb2_context *ctx)
 		VB2_DEBUG("attempting to load kernel2\n");
 		ret = VbTryLoadKernel(ctx, VB_DISK_FLAG_REMOVABLE);
 
-		/*
-		 * Clear recovery requests from failed kernel loading, since
-		 * we're already in recovery mode.  Do this now, so that
-		 * powering off after inserting an invalid disk doesn't leave
-		 * us stuck in recovery mode.
-		 */
-		vb2_nv_set(ctx, VB2_NV_RECOVERY_REQUEST,
-			   VB2_RECOVERY_NOT_REQUESTED);
-
 		if (VB2_SUCCESS == ret)
 			return ret; /* Found a recovery kernel */
 
diff --git a/tests/vboot_api_kernel_tests.c b/tests/vboot_api_kernel_tests.c
index 1734c386..8c57dee6 100644
--- a/tests/vboot_api_kernel_tests.c
+++ b/tests/vboot_api_kernel_tests.c
@@ -257,6 +257,38 @@ test_case_t test[] = {
 		.expected_to_load_disk = 0,
 		.expected_return_val = VBERROR_NO_KERNEL_FOUND,
 	},
+	{
+		.name = "invalid kernel (removable)",
+		.want_flags = VB_DISK_FLAG_REMOVABLE,
+		.disks_to_provide = {
+			{512,  100,  VB_DISK_FLAG_REMOVABLE, "corrupted"},
+			{512,  100,  VB_DISK_FLAG_REMOVABLE, "data"},
+		},
+		.disk_count_to_return = DEFAULT_COUNT,
+		.diskgetinfo_return_val = VB2_SUCCESS,
+		.loadkernel_return_val = {VBERROR_INVALID_KERNEL_FOUND,
+					  VBERROR_NO_KERNEL_FOUND},
+
+		.expected_recovery_request_val = VB2_RECOVERY_NOT_REQUESTED,
+		.expected_to_find_disk = DONT_CARE,
+		.expected_to_load_disk = 0,
+		.expected_return_val = VBERROR_INVALID_KERNEL_FOUND,
+	},
+	{
+		.name = "no kernel (removable)",
+		.want_flags = VB_DISK_FLAG_REMOVABLE,
+		.disks_to_provide = {
+			{512,  100,  VB_DISK_FLAG_REMOVABLE, "data"},
+		},
+		.disk_count_to_return = DEFAULT_COUNT,
+		.diskgetinfo_return_val = VB2_SUCCESS,
+		.loadkernel_return_val = {VBERROR_NO_KERNEL_FOUND},
+
+		.expected_recovery_request_val = VB2_RECOVERY_NOT_REQUESTED,
+		.expected_to_find_disk = DONT_CARE,
+		.expected_to_load_disk = 0,
+		.expected_return_val = VBERROR_NO_KERNEL_FOUND,
+	},
 };
 
 /****************************************************************************/