From 018b74d2d69ef35b43b79709f2ea60325f12dde2 Mon Sep 17 00:00:00 2001 From: Paul Moore Date: Mon, 24 Aug 2020 13:17:58 -0400 Subject: shim: attempt to improve the argument handling "So, load options are a giant pain in the ass." - Peter Jones This patch attempts to workaround two LoadOption problems seen on my test system: arguments separated with only whitespace (L" ") and strings that aren't properly NULL terminated. In addition to my test system, it appears at least one other person has run into a similar problem and I can reproduce the whitespace delimiter issue with QEMU+OVMF (OEMU v5.2.0, OVMF v202011). * https://github.com/rhboot/shim/issues/181 For reference, using QEMU+OVMF and a simple test application, "test.efi", which does a hexdump of LoadOptions I see the following when run via the UEFI shell: FS0:\> test.efi one two three LoadOptions: 0000: 74 00 65 00 73 00 74 00 2E 00 65 00 66 00 69 00 t.e.s.t...e.f.i. 0016: 20 00 6F 00 6E 00 65 00 20 00 74 00 77 00 6F 00 ..o.n.e...t.w.o. 0032: 20 00 74 00 68 00 72 00 65 00 65 00 00 00 ..t.h.r.e.e... Signed-off-by: Paul Moore --- shim.c | 96 ++++++++++++++++++++++++++++++++++-------------------------------- 1 file changed, 50 insertions(+), 46 deletions(-) diff --git a/shim.c b/shim.c index fd4c0322..5975feb8 100644 --- a/shim.c +++ b/shim.c @@ -1411,6 +1411,10 @@ EFI_STATUS set_second_stage (EFI_HANDLE image_handle) return efi_status; } + /* Sanity check since we make several assumptions about the length */ + if (li->LoadOptionsSize % 2 != 0) + return EFI_INVALID_PARAMETER; + /* So, load options are a giant pain in the ass. If we're invoked * from the EFI shell, we get something like this: @@ -1504,6 +1508,26 @@ EFI_STATUS set_second_stage (EFI_HANDLE image_handle) */ UINTN strings = count_ucs2_strings(li->LoadOptions, li->LoadOptionsSize); + + /* + * In the case where strings == 1 check to see if L' ' is being used as + * a delimiter, if so replace it with NULLs since the code already + * handles that case. + */ + if (strings == 1) { + UINT16 *cur = li->LoadOptions; + + /* replace L' ' with L'\0' if we find any */ + for (i = 0; i < li->LoadOptionsSize / 2; i++) { + if (cur[i] == L' ') + cur[i] = L'\0'; + } + + /* redo the string count */ + strings = count_ucs2_strings(li->LoadOptions, + li->LoadOptionsSize); + } + /* * If it's not string data, try it as an EFI_LOAD_OPTION. */ @@ -1516,77 +1540,57 @@ EFI_STATUS set_second_stage (EFI_HANDLE image_handle) li->LoadOptionsSize, (UINT8 **)&start, &loader_len); - if (EFI_ERROR(efi_status)) - return EFI_SUCCESS; + if (EFI_ERROR(efi_status)) { + /* maybe this is just a single string? */ + start = li->LoadOptions; + loader_len = li->LoadOptionsSize; + } remaining_size = 0; } else if (strings >= 2) { /* * UEFI shell copies the whole line of the command into - * LoadOptions. We ignore the string before the first L' ', + * LoadOptions. We ignore the string before the first L'\0', * i.e. the name of this program. - * Counting by two bytes is safe, because we know the size is - * compatible with a UCS2-LE string. */ - UINT8 *cur = li->LoadOptions; - for (i = 0; i < li->LoadOptionsSize - 2; i += 2) { - CHAR16 c = (cur[i+1] << 8) | cur[i]; - if (c == L' ') { - start = (CHAR16 *)&cur[i+2]; - remaining_size = li->LoadOptionsSize - i - 2; + UINT16 *cur = li->LoadOptions; + for (i = 1; i < li->LoadOptionsSize / 2; i++) { + if (cur[i - 1] == L'\0') { + start = &cur[i]; + remaining_size = li->LoadOptionsSize - (i * 2); break; } } - - if (!start || remaining_size <= 0 || start[0] == L'\0') + /* if we didn't find at least one NULL, something is wrong */ + if (start == li->LoadOptions) return EFI_SUCCESS; - for (i = 0; start[i] != '\0'; i++) { - if (start[i] == L' ') - start[i] = L'\0'; - if (start[i] == L'\0') { - loader_len = 2 * i + 2; - break; - } - } - if (loader_len) - remaining_size -= loader_len; - } else { - /* only find one string */ - start = li->LoadOptions; - loader_len = li->LoadOptionsSize; - } - - /* - * Just to be sure all that math is right... - */ - if (loader_len % 2 != 0) - return EFI_INVALID_PARAMETER; - - strings = count_ucs2_strings((UINT8 *)start, loader_len); - if (strings < 1) - return EFI_SUCCESS; + while (start[loader_len++] != L'\0'); + loader_len *= 2; - /* - * And then I found a version of BDS that gives us our own path in - * LoadOptions: + remaining_size -= loader_len; + } else if (strings == 1 && is_our_path(li, start)) { + /* + * And then I found a version of BDS that gives us our own path + * in LoadOptions: 77162C58 5c 00 45 00 46 00 49 00 |\.E.F.I.| 77162C60 5c 00 42 00 4f 00 4f 00 54 00 5c 00 42 00 4f 00 |\.B.O.O.T.\.B.O.| 77162C70 4f 00 54 00 58 00 36 00 34 00 2e 00 45 00 46 00 |O.T.X.6.4...E.F.| 77162C80 49 00 00 00 |I...| - * which is just cruel... So yeah, just don't use it. - */ - if (strings == 1 && is_our_path(li, start)) + * which is just cruel... So yeah, just don't use it. + */ return EFI_SUCCESS; + } /* * Set up the name of the alternative loader and the LoadOptions for * the loader */ if (loader_len > 0) { - loader_str = AllocatePool(loader_len); + /* we might not always have a NULL at the end */ + loader_str = AllocatePool(loader_len + 2); if (!loader_str) { perror(L"Failed to allocate loader string\n"); return EFI_OUT_OF_RESOURCES; @@ -1594,7 +1598,7 @@ EFI_STATUS set_second_stage (EFI_HANDLE image_handle) for (i = 0; i < loader_len / 2; i++) loader_str[i] = start[i]; - loader_str[loader_len/2-1] = L'\0'; + loader_str[loader_len/2] = L'\0'; second_stage = loader_str; load_options = remaining_size ? start + (loader_len/2) : NULL; -- cgit v1.2.3