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

3 files changed, 31 insertions, 37 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 31c07938..ff4cbf4b 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;
 }