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
diff --git a/firmware/lib/include/load_kernel_fw.h b/firmware/lib/include/load_kernel_fw.h
index f76d753..b2841e1 100644
--- a/firmware/lib/include/load_kernel_fw.h
+++ b/firmware/lib/include/load_kernel_fw.h
@@ -62,8 +62,7 @@
  * @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 31c0793..ff4cbf4 100644
--- a/firmware/lib/vboot_api_kernel.c
+++ b/firmware/lib/vboot_api_kernel.c
@@ -57,7 +57,7 @@
 
 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 @@
 	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 @@
 						?: 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 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 we didn't find any good kernels, don't return a disk handle. */
-	if (VB2_SUCCESS != rv) {
-		vb2api_fail(ctx, VB2_RECOVERY_RW_NO_KERNEL, rv);
-		lkp.disk_handle = NULL;
-	}
+	VbExDiskFreeInfo(disk_info, NULL);
 
-	VbExDiskFreeInfo(disk_info, lkp.disk_handle);
-
-	/*
-	 * 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 1866116..ce4828f 100644
--- a/firmware/lib/vboot_kernel.c
+++ b/firmware/lib/vboot_kernel.c
@@ -436,7 +436,6 @@
 	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 @@
 		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 6912873..1734c38 100644
--- a/tests/vboot_api_kernel_tests.c
+++ b/tests/vboot_api_kernel_tests.c
@@ -75,8 +75,8 @@
 		},
 		.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 @@
 		.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 @@
 		},
 		.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 @@
 		},
 		.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 @@
 		},
 		.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 @@
 		.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 @@
 		.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 @@
 			/* 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 = {1, 1, 1, 1, 1, 1, 1, 1, 1, 1,},
+		.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 = {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 b43d68b..9c80eed 100644
--- a/tests/vboot_kernel_tests.c
+++ b/tests/vboot_kernel_tests.c
@@ -335,6 +335,12 @@
 	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 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 @@
 	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 @@
 	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;