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 <pmoore2@cisco.com>

1 files 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;