firmware: Do not set recovery reason directly in LoadKernel()

LoadKernel() currently contains code that sets the recovery reason
directly (via direct nvdata access, bypassing the usual
VbSetRecoveryReason() helper) whenever it has a problem loading a
kernel. This seems to be an ancient vestige from the time when
LoadKernel() (and not VbSelectAndLoadKernel()) was still the external
API. In our current use, VbTryLoadKernel() will always immediately
override any recovery reason set this way.

This patch removes this pointless code to avoid confusion. Instead,
TryLoadKernel() is expanded to be able to tell the difference between
LoadKernel() return codes and set a more precise recovery reason based
on that.

BRANCH=None
BUG=chromium:692715
TEST=make runtests

Change-Id: Idd8bd6e16d5ef1472aa3b2b66468248726d5c889
Signed-off-by: Julius Werner <jwerner@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/1859686
(cherry picked from commit ddcec12ff1a033dfc533212ca2012e406a58f458)
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/1876588
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>

5 files changed, 103 insertions, 56 deletions

diff --git a/firmware/lib/include/load_kernel_fw.h b/firmware/lib/include/load_kernel_fw.h
index f76d7535..b2841e17 100644
--- a/firmware/lib/include/load_kernel_fw.h
+++ b/firmware/lib/include/load_kernel_fw.h
@@ -62,8 +62,7 @@ typedef struct LoadKernelParams {
  * @param ctx		Vboot context
  * @param params	Params specific to loading the kernel
  *
- * Returns VB2_SUCCESS if successful.  If unsuccessful, sets a recovery
- * reason via VbNvStorage and returns an error code.
+ * Returns VB2_SUCCESS if successful.  If unsuccessful, returns an error code.
  */
 vb2_error_t LoadKernel(struct vb2_context *ctx, LoadKernelParams *params);
 
diff --git a/firmware/lib/vboot_api_kernel.c b/firmware/lib/vboot_api_kernel.c
index 906341dd..094cc3f3 100644
--- a/firmware/lib/vboot_api_kernel.c
+++ b/firmware/lib/vboot_api_kernel.c
@@ -57,7 +57,7 @@ uint32_t vb2_get_fwmp_flags(void)
 
 vb2_error_t VbTryLoadKernel(struct vb2_context *ctx, uint32_t get_info_flags)
 {
-	vb2_error_t rv;
+	vb2_error_t rv = VBERROR_NO_DISK_FOUND;
 	VbDiskInfo* disk_info = NULL;
 	uint32_t disk_count = 0;
 	uint32_t i;
@@ -69,19 +69,15 @@ vb2_error_t VbTryLoadKernel(struct vb2_context *ctx, uint32_t get_info_flags)
 	lkp.disk_handle = NULL;
 
 	/* Find disks */
-	rv = VbExDiskGetInfo(&disk_info, &disk_count, get_info_flags);
-	if (VB2_SUCCESS != rv)
+	if (VB2_SUCCESS != VbExDiskGetInfo(&disk_info, &disk_count,
+					   get_info_flags))
 		disk_count = 0;
 
 	VB2_DEBUG("VbTryLoadKernel() found %d disks\n", (int)disk_count);
-	if (0 == disk_count) {
-		vb2api_fail(ctx, VB2_RECOVERY_RW_NO_DISK, rv);
-		return VBERROR_NO_DISK_FOUND;
-	}
 
 	/* Loop over disks */
 	for (i = 0; i < disk_count; i++) {
-		VB2_DEBUG("VbTryLoadKernel() trying disk %d\n", (int)i);
+		VB2_DEBUG("trying disk %d\n", (int)i);
 		/*
 		 * Sanity-check what we can. FWIW, VbTryLoadKernel() is always
 		 * called with only a single bit set in get_info_flags.
@@ -109,33 +105,40 @@ vb2_error_t VbTryLoadKernel(struct vb2_context *ctx, uint32_t get_info_flags)
 						?: lkp.gpt_lba_count;
 		lkp.boot_flags |= disk_info[i].flags & VB_DISK_FLAG_EXTERNAL_GPT
 				? BOOT_FLAG_EXTERNAL_GPT : 0;
-		rv = LoadKernel(ctx, &lkp);
 
-		VB2_DEBUG("VbTryLoadKernel() LoadKernel() = %d\n", rv);
+		vb2_error_t new_rv = LoadKernel(ctx, &lkp);
+		VB2_DEBUG("LoadKernel() = %#x\n", new_rv);
 
-		/*
-		 * Stop now if we found a kernel.
-		 *
-		 * TODO: If recovery requested, should track the farthest we
-		 * get, instead of just returning the value from the last disk
-		 * attempted.
-		 */
-		if (VB2_SUCCESS == rv)
-			break;
+		/* Stop now if we found a kernel. */
+		if (VB2_SUCCESS == new_rv) {
+			VbExDiskFreeInfo(disk_info, lkp.disk_handle);
+			return VB2_SUCCESS;
+		}
+
+		/* Don't update error if we already have a more specific one. */
+		if (VBERROR_INVALID_KERNEL_FOUND != rv)
+			rv = new_rv;
 	}
 
-	/* If we didn't find any good kernels, don't return a disk handle. */
-	if (VB2_SUCCESS != rv) {
+	/* 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);
-		lkp.disk_handle = NULL;
+		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;
 	}
 
-	VbExDiskFreeInfo(disk_info, lkp.disk_handle);
+	/* If we didn't find any good kernels, don't return a disk handle. */
+	VbExDiskFreeInfo(disk_info, NULL);
 
-	/*
-	 * Pass through return code.  Recovery reason (if any) has already been
-	 * set by LoadKernel().
-	 */
 	return rv;
 }
 
diff --git a/firmware/lib/vboot_kernel.c b/firmware/lib/vboot_kernel.c
index 18661160..ce4828fd 100644
--- a/firmware/lib/vboot_kernel.c
+++ b/firmware/lib/vboot_kernel.c
@@ -436,7 +436,6 @@ vb2_error_t LoadKernel(struct vb2_context *ctx, LoadKernelParams *params)
 	int found_partitions = 0;
 	uint32_t lowest_version = LOWEST_TPM_VERSION;
 	vb2_error_t retval = VB2_ERROR_UNKNOWN;
-	int recovery = VB2_RECOVERY_LK_UNSPECIFIED;
 	vb2_error_t rv;
 
 	vb2_workbuf_from_ctx(ctx, &wb);
@@ -647,20 +646,13 @@ gpt_done:
 		retval = VB2_SUCCESS;
 	} else if (found_partitions > 0) {
 		shcall->check_result = VBSD_LKC_CHECK_INVALID_PARTITIONS;
-		recovery = VB2_RECOVERY_RW_INVALID_OS;
 		retval = VBERROR_INVALID_KERNEL_FOUND;
 	} else {
 		shcall->check_result = VBSD_LKC_CHECK_NO_PARTITIONS;
-		recovery = VB2_RECOVERY_RW_NO_KERNEL;
 		retval = VBERROR_NO_KERNEL_FOUND;
 	}
 
 load_kernel_exit:
-	/* Store recovery request, if any */
-	vb2_nv_set(ctx, VB2_NV_RECOVERY_REQUEST,
-		   VB2_SUCCESS != retval ?
-		   recovery : VB2_RECOVERY_NOT_REQUESTED);
-
 	shcall->return_code = (uint8_t)retval;
 	return retval;
 }
diff --git a/tests/vboot_api_kernel_tests.c b/tests/vboot_api_kernel_tests.c
index 69128730..1734c386 100644
--- a/tests/vboot_api_kernel_tests.c
+++ b/tests/vboot_api_kernel_tests.c
@@ -75,8 +75,8 @@ test_case_t test[] = {
 		},
 		.disk_count_to_return = DEFAULT_COUNT,
 		.diskgetinfo_return_val = VB2_SUCCESS,
-		.loadkernel_return_val = {0, 1, 1, 1, 1, 1, 1, 1, 1, 1,},
-		.external_expected = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0,},
+		.loadkernel_return_val = {0},
+		.external_expected = {0},
 
 		.expected_recovery_request_val = VB2_RECOVERY_NOT_REQUESTED,
 		.expected_to_find_disk = pickme,
@@ -84,7 +84,7 @@ test_case_t test[] = {
 		.expected_return_val = VB2_SUCCESS
 	},
 	{
-		.name = "first removable drive",
+		.name = "first removable drive (skip external GPT)",
 		.want_flags = VB_DISK_FLAG_REMOVABLE,
 		.disks_to_provide = {
 			/* too small */
@@ -107,8 +107,8 @@ test_case_t test[] = {
 		},
 		.disk_count_to_return = DEFAULT_COUNT,
 		.diskgetinfo_return_val = VB2_SUCCESS,
-		.loadkernel_return_val = {0, 1, 1, 1, 1, 1, 1, 1, 1, 1,},
-		.external_expected = {1, 0, 0, 0, 0, 0, 0, 0, 0, 0,},
+		.loadkernel_return_val = {0, 0},
+		.external_expected = {1, 0},
 
 		.expected_recovery_request_val = VB2_RECOVERY_NOT_REQUESTED,
 		.expected_to_find_disk = pickme,
@@ -126,7 +126,7 @@ test_case_t test[] = {
 		},
 		.disk_count_to_return = DEFAULT_COUNT,
 		.diskgetinfo_return_val = VB2_SUCCESS,
-		.loadkernel_return_val = {1, 0, 1, 1, 1, 1, 1, 1, 1, 1,},
+		.loadkernel_return_val = {VBERROR_INVALID_KERNEL_FOUND, 0},
 
 		.expected_recovery_request_val = VB2_RECOVERY_NOT_REQUESTED,
 		.expected_to_find_disk = pickme,
@@ -158,7 +158,7 @@ test_case_t test[] = {
 		},
 		.disk_count_to_return = DEFAULT_COUNT,
 		.diskgetinfo_return_val = VB2_SUCCESS,
-		.loadkernel_return_val = {0, 1, 1, 1, 1, 1, 1, 1, 1, 1,},
+		.loadkernel_return_val = {0},
 
 		.expected_recovery_request_val = VB2_RECOVERY_NOT_REQUESTED,
 		.expected_to_find_disk = pickme,
@@ -171,7 +171,6 @@ test_case_t test[] = {
 		.disks_to_provide = {},
 		.disk_count_to_return = DEFAULT_COUNT,
 		.diskgetinfo_return_val = VB2_SUCCESS,
-		.loadkernel_return_val = {1, 1, 1, 1, 1, 1, 1, 1, 1, 1,},
 
 		.expected_recovery_request_val = VB2_RECOVERY_RW_NO_DISK,
 		.expected_to_find_disk = 0,
@@ -179,7 +178,22 @@ test_case_t test[] = {
 		.expected_return_val = VBERROR_NO_DISK_FOUND
 	},
 	{
-		.name = "no valid drives",
+		.name = "VbExDiskGetInfo() error",
+		.want_flags = VB_DISK_FLAG_FIXED,
+		.disks_to_provide = {
+			{512,  10, VB_DISK_FLAG_REMOVABLE, 0},
+			{512, 100, VB_DISK_FLAG_FIXED, 0},
+		},
+		.disk_count_to_return = DEFAULT_COUNT,
+		.diskgetinfo_return_val = VB2_ERROR_UNKNOWN,
+
+		.expected_recovery_request_val = VB2_RECOVERY_RW_NO_DISK,
+		.expected_to_find_disk = 0,
+		.expected_to_load_disk = 0,
+		.expected_return_val = VBERROR_NO_DISK_FOUND,
+	},
+	{
+		.name = "invalid kernel",
 		.want_flags = VB_DISK_FLAG_FIXED,
 		.disks_to_provide = {
 			/* too small */
@@ -195,18 +209,53 @@ test_case_t test[] = {
 			/* still wrong flags */
 			{512,  100,  -1, 0},
 			/* doesn't load */
-			{512,  100,  VB_DISK_FLAG_FIXED, "bad1"},
+			{512,  100,  VB_DISK_FLAG_FIXED, "corrupted kernel"},
 			/* doesn't load */
-			{512,  100,  VB_DISK_FLAG_FIXED, "bad2"},
+			{512,  100,  VB_DISK_FLAG_FIXED, "stateful partition"},
+		},
+		.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_RW_INVALID_OS,
+		.expected_to_find_disk = DONT_CARE,
+		.expected_to_load_disk = 0,
+		.expected_return_val = VBERROR_INVALID_KERNEL_FOUND,
+	},
+	{
+		.name = "invalid kernel, order flipped",
+		.want_flags = VB_DISK_FLAG_FIXED,
+		.disks_to_provide = {
+			{512, 1000, VB_DISK_FLAG_FIXED, "stateful partition"},
+			{512, 1000, VB_DISK_FLAG_FIXED, "corrupted kernel"},
+		},
+		.disk_count_to_return = DEFAULT_COUNT,
+		.diskgetinfo_return_val = VB2_SUCCESS,
+		.loadkernel_return_val = {VBERROR_NO_KERNEL_FOUND,
+					  VBERROR_INVALID_KERNEL_FOUND},
+
+		.expected_recovery_request_val = VB2_RECOVERY_RW_INVALID_OS,
+		.expected_to_find_disk = DONT_CARE,
+		.expected_to_load_disk = 0,
+		.expected_return_val = VBERROR_INVALID_KERNEL_FOUND,
+	},
+	{
+		.name = "no Chrome OS partitions",
+		.want_flags = VB_DISK_FLAG_FIXED,
+		.disks_to_provide = {
+			{512, 100, VB_DISK_FLAG_FIXED, "stateful partition"},
+			{512, 1000, VB_DISK_FLAG_FIXED, "Chrubuntu"},
 		},
 		.disk_count_to_return = DEFAULT_COUNT,
 		.diskgetinfo_return_val = VB2_SUCCESS,
-		.loadkernel_return_val = {1, 1, 1, 1, 1, 1, 1, 1, 1, 1,},
+		.loadkernel_return_val = {VBERROR_NO_KERNEL_FOUND,
+					  VBERROR_NO_KERNEL_FOUND},
 
 		.expected_recovery_request_val = VB2_RECOVERY_RW_NO_KERNEL,
 		.expected_to_find_disk = DONT_CARE,
 		.expected_to_load_disk = 0,
-		.expected_return_val = 1
+		.expected_return_val = VBERROR_NO_KERNEL_FOUND,
 	},
 };
 
diff --git a/tests/vboot_kernel_tests.c b/tests/vboot_kernel_tests.c
index b43d68b4..9c80eed7 100644
--- a/tests/vboot_kernel_tests.c
+++ b/tests/vboot_kernel_tests.c
@@ -335,6 +335,12 @@ vb2_error_t vb2_digest_buffer(const uint8_t *buf, uint32_t size,
 	return VB2_SUCCESS;
 }
 
+/* Make sure nothing tested here ever calls this directly. */
+void vb2api_fail(struct vb2_context *c, uint8_t reason, uint8_t subcode)
+{
+	TEST_TRUE(0, "  called vb2api_fail()");
+}
+
 /**
  * Test reading/writing GPT
  */
@@ -589,6 +595,10 @@ static void ReadWriteGptTest(void)
 static void TestLoadKernel(int expect_retval, const char *test_name)
 {
 	TEST_EQ(LoadKernel(&ctx, &lkp), expect_retval, test_name);
+
+	/* LoadKernel() should never request recovery directly. */
+	TEST_EQ(vb2_nv_get(&ctx, VB2_NV_RECOVERY_REQUEST),
+		0, "  recovery request");
 }
 
 /**
@@ -616,8 +626,6 @@ static void LoadKernelTest(void)
 	TEST_EQ(lkp.bootloader_size, 0x1234, "  bootloader size");
 	TEST_STR_EQ((char *)lkp.partition_guid, "FakeGuid", "  guid");
 	TEST_EQ(gpt_flag_external, 0, "GPT was internal");
-	TEST_EQ(vb2_nv_get(&ctx, VB2_NV_RECOVERY_REQUEST),
-		0, "  recovery request");
 
 	ResetMocks();
 	mock_parts[1].start = 300;
@@ -630,15 +638,11 @@ static void LoadKernelTest(void)
 	ResetMocks();
 	mock_parts[0].size = 0;
 	TestLoadKernel(VBERROR_NO_KERNEL_FOUND, "No kernels");
-	TEST_EQ(vb2_nv_get(&ctx, VB2_NV_RECOVERY_REQUEST),
-		VB2_RECOVERY_RW_NO_KERNEL, "  recovery request");
 
 	/* Skip kernels which are too small */
 	ResetMocks();
 	mock_parts[0].size = 10;
 	TestLoadKernel(VBERROR_INVALID_KERNEL_FOUND, "Too small");
-	TEST_EQ(vb2_nv_get(&ctx, VB2_NV_RECOVERY_REQUEST),
-		VB2_RECOVERY_RW_INVALID_OS, "  recovery request");
 
 	ResetMocks();
 	disk_read_to_fail = 100;