From 9e8dde5438ece7ce1c2ecaeb754298d5bcb4bea8 Mon Sep 17 00:00:00 2001 From: Peter Jones Date: Thu, 11 Mar 2021 20:49:41 -0500 Subject: Fix a plausible NULL dereference. scan-build kindly pointed out: | shim.c:1568:10: warning: Array access (from variable 'start') results in a null pointer dereference [core.NullDereference] | while (start[loader_len++] != L'\0'); | ^~~~~~~~~~~~~~~~~~~ | 1 warning generated. It thinks that because of a bad assumption it's making because of the test immediately before it, which isn't currently necessary /at all/. In fact, neither is this loop; it appears to be vestigial and the goal was done in the loop above it. This patch just solves for how much space is left arithmetically instead. Signed-off-by: Peter Jones --- shim.c | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/shim.c b/shim.c index 9bc3d602..411833ee 100644 --- a/shim.c +++ b/shim.c @@ -1388,7 +1388,7 @@ EFI_STATUS set_second_stage (EFI_HANDLE image_handle) EFI_STATUS efi_status; EFI_LOADED_IMAGE *li = NULL; CHAR16 *start = NULL; - int remaining_size = 0; + UINTN remaining_size = 0; CHAR16 *loader_str = NULL; UINTN loader_len = 0; unsigned int i; @@ -1510,9 +1510,14 @@ EFI_STATUS set_second_stage (EFI_HANDLE image_handle) 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. + * In some cases we get strings == 1 because BDS is using L' ' as the + * delimeter: + * 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... + * + * If so replace it with NULs since the code already handles that + * case. */ if (strings == 1) { UINT16 *cur = li->LoadOptions; @@ -1561,18 +1566,12 @@ EFI_STATUS set_second_stage (EFI_HANDLE image_handle) break; } } - /* if we didn't find at least one NULL, something is wrong */ - if (start == li->LoadOptions) - return EFI_SUCCESS; - - while (start[loader_len++] != L'\0'); - loader_len *= 2; - remaining_size -= loader_len; + remaining_size -= i * 2 + 2; } 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: + * 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.| -- cgit v1.2.3