From 2cabe53cc16b32e4af6e70e7f31feaabece66b0a Mon Sep 17 00:00:00 2001 From: Dan Nicholson Date: Wed, 2 Oct 2024 16:51:18 -0600 Subject: [PATCH] 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: #649 Signed-off-by: Dan Nicholson --- load-options.c | 8 ++--- test-load-options.c | 77 +++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 78 insertions(+), 7 deletions(-) diff --git a/load-options.c b/load-options.c index 8b92e37f9..29d6bdac8 100644 --- 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 index daf02d93e..dfabe861f 100644 --- 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; }