shim: Allow data after the end of device path node in load options

When looking for load option optional data, the parser asserts that the byte after the end of device path node is the same as what the file path length says it should be. While unusual, it is valid if the end of device path node comes before the end of the file path list. That supports some unusual Dell load options where there are two device paths in the list but the first is terminated by an End Entire Device Path. Maybe they intended to use an End Device Path Instance node there? Who knows. Either way, treating it as invalid ends up trying to read paths from the beginning of the option with obviously poor results. Fixes: rhboot#649 Signed-off-by: Dan Nicholson <[email protected]>
dbnicholson · Oct 2, 2024 · 2cabe53 · 2cabe53
1 parent e064e7d
commit 2cabe53
Show file tree

Hide file tree

Showing 2 changed files with 78 additions and 7 deletions.
diff --git a/load-options.c b/load-options.c
@@ -207,14 +207,14 @@ get_load_option_optional_data(VOID *data, UINT32 data_size,
 		 */
 		i += dp.len;
 	}
-	if (i != fplistlen)
+	if (i > fplistlen)
 		return EFI_INVALID_PARAMETER;
 
 	/*
-	 * if there's any space left, it's "optional data"
+	 * Anything left after the file path list is optional data.
 	 */
-	*od = cur + i;
-	*ods = limit - i;
+	*od = cur + fplistlen;
+	*ods = limit - fplistlen;
 	return EFI_SUCCESS;
 }
 

diff --git a/test-load-options.c b/test-load-options.c
@@ -68,9 +68,12 @@ test_parse_load_options(char *load_option_data,
 	assert_false_goto(EFI_ERROR(status), err, "\n");
 
 	assert_nonzero_goto(second_stage, err, "\n");
-	assert_not_equal_goto(second_stage, dummy_second_stage, err, "%p == %p\n");
-	assert_zero_goto(StrnCmp(second_stage, target_second_stage, 90),
-			 err_print_second_stage, "%d != 0\n");
+	if (target_second_stage) {
+		assert_not_equal_goto(second_stage, dummy_second_stage, err, "%p == %p\n");
+		assert_zero_goto(StrnCmp(second_stage, target_second_stage, 90),
+				 err_print_second_stage, "%d != 0\n");
+	} else
+		assert_equal_goto(second_stage, dummy_second_stage, err, "%p != %p\n");
 
 	assert_equal_goto(load_options_size, target_remaining_size, err_remaining, "%zu != %zu\n");
 	assert_equal_goto(load_options, target_remaining, err_remaining, "%p != %p\n");
@@ -255,6 +258,73 @@ test_bds_2(void)
 				       target_remaining_size);
 }
 
+int
+test_multi_end_dp(void)
+{
+	/*
+00000000  01 00 00 00 d0 00 4f 00  6e 00 62 00 6f 00 61 00  |......O.n.b.o.a.|
+00000010  72 00 64 00 20 00 4e 00  49 00 43 00 28 00 49 00  |r.d. .N.I.C.(.I.|
+00000020  50 00 56 00 34 00 29 00  00 00 02 01 0c 00 d0 41  |P.V.4.)........A|
+00000030  03 0a 00 00 00 00 01 01  06 00 06 1f 03 0b 25 00  |..............%.|
+00000040  2c ea 7f 0a 9f 69 00 00  00 00 00 00 00 00 00 00  |,....i..........|
+00000050  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
+00000060  00 03 0c 1b 00 00 00 00  00 00 00 00 00 00 00 00  |................|
+00000070  00 00 00 00 00 00 00 00  00 00 00 00 7f ff 04 00  |................|
+00000080  01 04 76 00 ef 47 64 2d  c9 3b a0 41 ac 19 4d 51  |..v..Gd-.;.A..MQ|
+00000090  d0 1b 4c e6 50 00 58 00  45 00 20 00 49 00 50 00  |..L.P.X.E. .I.P.|
+000000a0  34 00 20 00 49 00 6e 00  74 00 65 00 6c 00 28 00  |4. .I.n.t.e.l.(.|
+000000b0  52 00 29 00 20 00 45 00  74 00 68 00 65 00 72 00  |R.). .E.t.h.e.r.|
+000000c0  6e 00 65 00 74 00 20 00  43 00 6f 00 6e 00 6e 00  |n.e.t. .C.o.n.n.|
+000000d0  65 00 63 00 74 00 69 00  6f 00 6e 00 20 00 28 00  |e.c.t.i.o.n. .(.|
+000000e0  36 00 29 00 20 00 49 00  32 00 31 00 39 00 2d 00  |6.). .I.2.1.9.-.|
+000000f0  4c 00 4d 00 00 00 7f ff  04 00 00 00 42 4f        |L.M.........BO|
+	 */
+	char load_option_data [] = {
+		0x01, 0x00, 0x00, 0x00, 0xd0, 0x00, 0x4f, 0x00,
+		0x6e, 0x00, 0x62, 0x00, 0x6f, 0x00, 0x61, 0x00,
+		0x72, 0x00, 0x64, 0x00, 0x20, 0x00, 0x4e, 0x00,
+		0x49, 0x00, 0x43, 0x00, 0x28, 0x00, 0x49, 0x00,
+		0x50, 0x00, 0x56, 0x00, 0x34, 0x00, 0x29, 0x00,
+		0x00, 0x00, 0x02, 0x01, 0x0c, 0x00, 0xd0, 0x41,
+		0x03, 0x0a, 0x00, 0x00, 0x00, 0x00, 0x01, 0x01,
+		0x06, 0x00, 0x06, 0x1f, 0x03, 0x0b, 0x25, 0x00,
+		0x2c, 0xea, 0x7f, 0x0a, 0x9f, 0x69, 0x00, 0x00,
+		0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+		0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+		0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+		0x00, 0x03, 0x0c, 0x1b, 0x00, 0x00, 0x00, 0x00,
+		0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+		0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+		0x00, 0x00, 0x00, 0x00, 0x7f, 0xff, 0x04, 0x00,
+		0x01, 0x04, 0x76, 0x00, 0xef, 0x47, 0x64, 0x2d,
+		0xc9, 0x3b, 0xa0, 0x41, 0xac, 0x19, 0x4d, 0x51,
+		0xd0, 0x1b, 0x4c, 0xe6, 0x50, 0x00, 0x58, 0x00,
+		0x45, 0x00, 0x20, 0x00, 0x49, 0x00, 0x50, 0x00,
+		0x34, 0x00, 0x20, 0x00, 0x49, 0x00, 0x6e, 0x00,
+		0x74, 0x00, 0x65, 0x00, 0x6c, 0x00, 0x28, 0x00,
+		0x52, 0x00, 0x29, 0x00, 0x20, 0x00, 0x45, 0x00,
+		0x74, 0x00, 0x68, 0x00, 0x65, 0x00, 0x72, 0x00,
+		0x6e, 0x00, 0x65, 0x00, 0x74, 0x00, 0x20, 0x00,
+		0x43, 0x00, 0x6f, 0x00, 0x6e, 0x00, 0x6e, 0x00,
+		0x65, 0x00, 0x63, 0x00, 0x74, 0x00, 0x69, 0x00,
+		0x6f, 0x00, 0x6e, 0x00, 0x20, 0x00, 0x28, 0x00,
+		0x36, 0x00, 0x29, 0x00, 0x20, 0x00, 0x49, 0x00,
+		0x32, 0x00, 0x31, 0x00, 0x39, 0x00, 0x2d, 0x00,
+		0x4c, 0x00, 0x4d, 0x00, 0x00, 0x00, 0x7f, 0xff,
+		0x04, 0x00, 0x00, 0x00, 0x42, 0x4f
+	};
+	size_t load_option_data_size = sizeof(load_option_data);
+	char *target_remaining = &load_option_data[load_option_data_size - 2];
+	size_t target_remaining_size = 2;
+
+	return test_parse_load_options(load_option_data,
+				       load_option_data_size,
+				       "test.efi",
+				       NULL,
+				       target_remaining,
+				       target_remaining_size);
+}
+
 int
 main(void)
 {
@@ -263,6 +333,7 @@ main(void)
 	test(test_bds_0);
 	test(test_bds_1);
 	test(test_bds_2);
+	test(test_multi_end_dp);
 	return status;
 }