vboot/vboot_kernel: break disk check out to separate functionstabilize-14031.B

Move disk validity check to static function is_valid_disk().

If multiple disk types are selected (e.g. REMOVABLE | FIXED),
is_valid_disk() will now check that exactly *one* of those flags
is selected by VbDiskInfo.flags.

Also, split disk flags into two 16-bit sections:
 - Disk selection in the lower 16 bits (where the disk lives)
 - Disk attributes in the higher 16 bits (extra information about
   the disk needed to access it correctly)

This CL is part of a series to merge vboot1 and vboot2.0
kernel verification code; see b/181739551.

BUG=b:181739551
TEST=make clean && make runtests
BRANCH=none

Signed-off-by: Joel Kitching <kitching@google.com>
Change-Id: Icf76ab6e92cca40810071def66aed13cdb3a7ec7
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/2872251
Commit-Queue: Joel Kitching <kitching@chromium.org>
Tested-by: Joel Kitching <kitching@chromium.org>
Reviewed-by: Julius Werner <jwerner@chromium.org>

8 files changed, 77 insertions, 36 deletions

diff --git a/firmware/include/vboot_api.h b/firmware/include/vboot_api.h
index 98cbd9bc..2b1d838b 100644
--- a/firmware/include/vboot_api.h
+++ b/firmware/include/vboot_api.h
@@ -81,13 +81,22 @@ vb2_error_t VbSelectAndLoadKernel(struct vb2_context *ctx,
 /* Disk access (previously in boot_device.h) */
 
 /* Flags for VbDisk APIs */
+
+/*
+ * Disk selection in the lower 16 bits (where the disk lives), and disk
+ * attributes in the higher 16 bits (extra information about the disk
+ * needed to access it correctly).
+ */
+#define VB_DISK_FLAG_SELECT_MASK 0xffff
+#define VB_DISK_FLAG_ATTRIBUTE_MASK (0xffff << 16)
+
 /* Disk is removable.  Example removable disks: SD cards, USB keys.  */
-#define VB_DISK_FLAG_REMOVABLE 0x00000001
+#define VB_DISK_FLAG_REMOVABLE (1 << 0)
 /*
  * Disk is fixed.  If this flag is present, disk is internal to the system and
  * not removable.  Example fixed disks: internal SATA SSD, eMMC.
  */
-#define VB_DISK_FLAG_FIXED     0x00000002
+#define VB_DISK_FLAG_FIXED (1 << 1)
 /*
  * Note that VB_DISK_FLAG_REMOVABLE and VB_DISK_FLAG_FIXED are
  * mutually-exclusive for a single disk.  VbExDiskGetInfo() may specify both
@@ -123,7 +132,7 @@ vb2_error_t VbSelectAndLoadKernel(struct vb2_context *ctx,
  * streaming aspects of the disk. If a disk is random-access (i.e.
  * not raw NAND) then these fields are equal.
  */
-#define VB_DISK_FLAG_EXTERNAL_GPT	0x00000004
+#define VB_DISK_FLAG_EXTERNAL_GPT (1 << 16)
 
 /* Information on a single disk */
 typedef struct VbDiskInfo {
diff --git a/firmware/lib/include/vboot_kernel.h b/firmware/lib/include/vboot_kernel.h
index b94502f6..a9dc8242 100644
--- a/firmware/lib/include/vboot_kernel.h
+++ b/firmware/lib/include/vboot_kernel.h
@@ -23,9 +23,9 @@ struct vb2_context;
  * VB2_SUCCESS.
  *
  * @param ctx			Vboot context
- * @param get_info_flags	Flags to pass to VbExDiskGetInfo()
+ * @param disk_flags		Flags to pass to VbExDiskGetInfo()
  * @return VB2_SUCCESS or the most specific VB2_ERROR_LK error.
  */
-vb2_error_t VbTryLoadKernel(struct vb2_context *ctx, uint32_t get_info_flags);
+vb2_error_t VbTryLoadKernel(struct vb2_context *ctx, uint32_t disk_flags);
 
 #endif  /* VBOOT_REFERENCE_VBOOT_KERNEL_H_ */
diff --git a/firmware/lib/vboot_api_kernel.c b/firmware/lib/vboot_api_kernel.c
index 541cd028..b28cc442 100644
--- a/firmware/lib/vboot_api_kernel.c
+++ b/firmware/lib/vboot_api_kernel.c
@@ -53,8 +53,18 @@ static vb2_error_t handle_battery_cutoff(struct vb2_context *ctx)
 	return VB2_SUCCESS;
 }
 
+static int is_valid_disk(VbDiskInfo *info, uint32_t disk_flags)
+{
+	return info->bytes_per_lba >= 512 &&
+		(info->bytes_per_lba & (info->bytes_per_lba - 1)) == 0 &&
+		info->lba_count >= 16 &&
+		(info->flags & disk_flags & VB_DISK_FLAG_SELECT_MASK) &&
+		((info->flags & VB_DISK_FLAG_SELECT_MASK) &
+		 ((info->flags & VB_DISK_FLAG_SELECT_MASK) - 1)) == 0;
+}
+
 test_mockable
-vb2_error_t VbTryLoadKernel(struct vb2_context *ctx, uint32_t get_info_flags)
+vb2_error_t VbTryLoadKernel(struct vb2_context *ctx, uint32_t disk_flags)
 {
 	vb2_error_t rv = VB2_ERROR_LK_NO_DISK_FOUND;
 	VbDiskInfo* disk_info = NULL;
@@ -64,26 +74,14 @@ vb2_error_t VbTryLoadKernel(struct vb2_context *ctx, uint32_t get_info_flags)
 	lkp.disk_handle = NULL;
 
 	/* Find disks */
-	if (VB2_SUCCESS != VbExDiskGetInfo(&disk_info, &disk_count,
-					   get_info_flags))
+	if (VB2_SUCCESS != VbExDiskGetInfo(&disk_info, &disk_count, disk_flags))
 		disk_count = 0;
 
 	/* Loop over disks */
 	for (i = 0; i < disk_count; i++) {
 		VB2_DEBUG("trying disk %d\n", (int)i);
-		/*
-		 * Validity-check what we can. FWIW, VbTryLoadKernel() is always
-		 * called with only a single bit set in get_info_flags.
-		 *
-		 * Ensure that we got a partition with only the flags we asked
-		 * for.
-		 */
-		if (disk_info[i].bytes_per_lba < 512 ||
-			(disk_info[i].bytes_per_lba &
-				(disk_info[i].bytes_per_lba  - 1)) != 0 ||
-					16 > disk_info[i].lba_count ||
-					get_info_flags != (disk_info[i].flags &
-					~VB_DISK_FLAG_EXTERNAL_GPT)) {
+
+		if (!is_valid_disk(&disk_info[i], disk_flags)) {
 			VB2_DEBUG("  skipping: bytes_per_lba=%" PRIu64
 				  " lba_count=%" PRIu64 " flags=%#x\n",
 				  disk_info[i].bytes_per_lba,
@@ -108,7 +106,7 @@ 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. */
-	if (get_info_flags & VB_DISK_FLAG_FIXED) {
+	if (disk_flags & VB_DISK_FLAG_FIXED) {
 		switch (rv) {
 		case VB2_ERROR_LK_INVALID_KERNEL_FOUND:
 			vb2api_fail(ctx, VB2_RECOVERY_RW_INVALID_OS, rv);
diff --git a/tests/vb2_kernel_tests.c b/tests/vb2_kernel_tests.c
index ebddafcd..0e1cb284 100644
--- a/tests/vb2_kernel_tests.c
+++ b/tests/vb2_kernel_tests.c
@@ -139,7 +139,7 @@ vb2_error_t vb2ex_read_resource(struct vb2_context *c,
 	return VB2_SUCCESS;
 }
 
-vb2_error_t VbTryLoadKernel(struct vb2_context *c, uint32_t get_info_flags)
+vb2_error_t VbTryLoadKernel(struct vb2_context *c, uint32_t disk_flags)
 {
 	/*
 	 * TODO: Currently we don't have a good way of testing for an ordered
@@ -150,10 +150,10 @@ vb2_error_t VbTryLoadKernel(struct vb2_context *c, uint32_t get_info_flags)
 		return mock_vbtlk_retval;
 
 	TEST_EQ(!!mock_vbtlk_expect_fixed,
-		!!(get_info_flags & VB_DISK_FLAG_FIXED),
+		!!(disk_flags & VB_DISK_FLAG_FIXED),
 		"  VbTryLoadKernel unexpected fixed disk call");
 	TEST_EQ(!!mock_vbtlk_expect_removable,
-		!!(get_info_flags & VB_DISK_FLAG_REMOVABLE),
+		!!(disk_flags & VB_DISK_FLAG_REMOVABLE),
 		"  VbTryLoadKernel unexpected removable disk call");
 
 	return mock_vbtlk_retval;
diff --git a/tests/vb2_ui_action_tests.c b/tests/vb2_ui_action_tests.c
index 66a776eb..6db75bfd 100644
--- a/tests/vb2_ui_action_tests.c
+++ b/tests/vb2_ui_action_tests.c
@@ -235,10 +235,10 @@ static void add_mock_keypress(uint32_t press)
 }
 
 
-static void set_mock_vbtlk(vb2_error_t retval, uint32_t get_info_flags)
+static void set_mock_vbtlk(vb2_error_t retval, uint32_t disk_flags)
 {
 	mock_vbtlk_retval = retval;
-	mock_vbtlk_expected_flag = get_info_flags;
+	mock_vbtlk_expected_flag = disk_flags;
 }
 
 static void displayed_eq(const char *text,
@@ -481,10 +481,10 @@ uint32_t VbExKeyboardReadWithFlags(uint32_t *key_flags)
 	return 0;
 }
 
-vb2_error_t VbTryLoadKernel(struct vb2_context *c, uint32_t get_info_flags)
+vb2_error_t VbTryLoadKernel(struct vb2_context *c, uint32_t disk_flags)
 {
-	TEST_EQ(mock_vbtlk_expected_flag, get_info_flags,
-		"  unexpected get_info_flags");
+	TEST_EQ(mock_vbtlk_expected_flag, disk_flags,
+		"  unexpected disk_flags");
 	return mock_vbtlk_retval;
 }
 
diff --git a/tests/vb2_ui_tests.c b/tests/vb2_ui_tests.c
index 3eb0c9f9..9a84a939 100644
--- a/tests/vb2_ui_tests.c
+++ b/tests/vb2_ui_tests.c
@@ -120,7 +120,7 @@ static void add_mock_keypress(uint32_t press)
 	add_mock_key(press, 0);
 }
 
-static void add_mock_vbtlk(vb2_error_t retval, uint32_t get_info_flags)
+static void add_mock_vbtlk(vb2_error_t retval, uint32_t disk_flags)
 {
 	if (mock_vbtlk_total >= ARRAY_SIZE(mock_vbtlk_retval) ||
 	    mock_vbtlk_total >= ARRAY_SIZE(mock_vbtlk_expected_flag)) {
@@ -129,7 +129,7 @@ static void add_mock_vbtlk(vb2_error_t retval, uint32_t get_info_flags)
 	}
 
 	mock_vbtlk_retval[mock_vbtlk_total] = retval;
-	mock_vbtlk_expected_flag[mock_vbtlk_total] = get_info_flags;
+	mock_vbtlk_expected_flag[mock_vbtlk_total] = disk_flags;
 	mock_vbtlk_total++;
 }
 
@@ -517,7 +517,7 @@ uint32_t vb2ex_get_altfw_count(void)
 	return mock_altfw_count;
 }
 
-vb2_error_t VbTryLoadKernel(struct vb2_context *c, uint32_t get_info_flags)
+vb2_error_t VbTryLoadKernel(struct vb2_context *c, uint32_t disk_flags)
 {
 	int i = mock_iters;
 
@@ -525,8 +525,8 @@ vb2_error_t VbTryLoadKernel(struct vb2_context *c, uint32_t get_info_flags)
 	if (i >= mock_vbtlk_total)
 		i = mock_vbtlk_total - 1;
 
-	TEST_EQ(mock_vbtlk_expected_flag[i], get_info_flags,
-		"  unexpected get_info_flags");
+	TEST_EQ(mock_vbtlk_expected_flag[i], disk_flags,
+		"  unexpected disk_flags");
 
 	return mock_vbtlk_retval[i];
 }
diff --git a/tests/vboot_api_kernel4_tests.c b/tests/vboot_api_kernel4_tests.c
index a25132f7..32eaf025 100644
--- a/tests/vboot_api_kernel4_tests.c
+++ b/tests/vboot_api_kernel4_tests.c
@@ -120,7 +120,7 @@ void vb2_secdata_kernel_set(struct vb2_context *c,
 	kernel_version = value;
 }
 
-vb2_error_t VbTryLoadKernel(struct vb2_context *c, uint32_t get_info_flags)
+vb2_error_t VbTryLoadKernel(struct vb2_context *c, uint32_t disk_flags)
 {
 	sd->kernel_version = new_version;
 
diff --git a/tests/vboot_api_kernel_tests.c b/tests/vboot_api_kernel_tests.c
index 9904770b..83b6d6c1 100644
--- a/tests/vboot_api_kernel_tests.c
+++ b/tests/vboot_api_kernel_tests.c
@@ -52,6 +52,40 @@ static const char pickme[] = "correct choice";
 
 test_case_t test[] = {
 	{
+		.name = "first drive (removable)",
+		.want_flags = VB_DISK_FLAG_REMOVABLE | VB_DISK_FLAG_FIXED,
+		.disks_to_provide = {
+			{4096, 100, VB_DISK_FLAG_REMOVABLE, pickme},
+			{4096, 100, VB_DISK_FLAG_FIXED, "holygrail"},
+		},
+		.disk_count_to_return = DEFAULT_COUNT,
+		.diskgetinfo_return_val = VB2_SUCCESS,
+		.loadkernel_return_val = {0},
+		.external_expected = {0},
+
+		.expected_recovery_request_val = VB2_RECOVERY_NOT_REQUESTED,
+		.expected_to_find_disk = pickme,
+		.expected_to_load_disk = pickme,
+		.expected_return_val = VB2_SUCCESS
+	},
+	{
+		.name = "first drive (fixed)",
+		.want_flags = VB_DISK_FLAG_REMOVABLE | VB_DISK_FLAG_FIXED,
+		.disks_to_provide = {
+			{4096, 100, VB_DISK_FLAG_FIXED, pickme},
+			{4096, 100, VB_DISK_FLAG_REMOVABLE, "holygrail"},
+		},
+		.disk_count_to_return = DEFAULT_COUNT,
+		.diskgetinfo_return_val = VB2_SUCCESS,
+		.loadkernel_return_val = {0},
+		.external_expected = {0},
+
+		.expected_recovery_request_val = VB2_RECOVERY_NOT_REQUESTED,
+		.expected_to_find_disk = pickme,
+		.expected_to_load_disk = pickme,
+		.expected_return_val = VB2_SUCCESS
+	},
+	{
 		.name = "first removable drive",
 		.want_flags = VB_DISK_FLAG_REMOVABLE,
 		.disks_to_provide = {