regression: out of bounds read in CopyMem() in ad8692e

The CopyMem() introduced in "ad8692e avoid EFIv2 runtime services on
Apple x86 machines" copies 100 CHAR16s no matter what. NX enabled
firmware catches this and the boot breaks on those systems when the
value is smaller than that and it's up against a page boundary with a
page that's not mapped as readable.

https://uefi.org/specs/UEFI/2.10/04_EFI_System_Table.html says
that FirmwareVendor is a pointer to a NUL terminated string that
identifies the vendor that produces the system firmware for the platform.

Signed-off-by: Jan Setje-Eilers <Jan.SetjeEilers@oracle.com>

1 files changed, 29 insertions, 4 deletions

diff --git a/mok.c b/mok.c
index 2f721a6b..5ae91244 100644
--- a/mok.c
+++ b/mok.c
@@ -245,15 +245,40 @@ typedef UINTN SIZE_T;
 
 static BOOLEAN is_apple_firmware_vendor(void)
 {
-	CHAR16 vendorbuf[100] = L"";
+	CHAR16 vendorbuf[6] = L"";
 	CHAR16 *vendor = ST->FirmwareVendor;
 	if (!vendor)
 		return FALSE;
 
-	CopyMem(vendorbuf, vendor, sizeof(vendorbuf) - sizeof(vendorbuf[0]));
-	dprint(L"FirmwareVendor: \"%s\"\n", vendorbuf);
+	ZeroMem(vendorbuf, sizeof(vendorbuf));
 
-	if (StrnCmp(vendorbuf, L"Apple", 5) == 0)
+	/*
+	 * We've had a problem where ST->FirmwareVendor is only as big as
+	 * it needs to be (or at least less than the 200 bytes we formerly
+	 * defined vendorbuf as) and it's up against a page that's not
+	 * mapped readable, so we take a fault and reset when copying from
+	 * it.
+	 *
+	 * We modeled this after kernel, which has the 200 byte CHAR16
+	 * array and copies 198 bytes into it, so that there's a NUL
+	 * terminator.  They solve this issue by mapping the whole 200
+	 * bytes unconditionally and then unmapping it after the copy, but
+	 * we can't take that approach because we don't necessarily have
+	 * page permission primitives at all.
+	 *
+	 * The 200 bytes (CHAR16 [100]) is an arbitrary number anyway, but
+	 * it's likely larger than any sane vendor name, and we still want
+	 * to do the copy into an array larger than our copied data because
+	 * that's how we guard against failure to terminate with a NUL.
+	 *
+	 * So right now we're only copying ten bytes, because Apple is the
+	 * only vendor we're testing against.
+	 */
+	CopyMem(vendorbuf, vendor, 10);
+
+	dprint(L"FirmwareVendor: \"%s\"\n", vendor);
+
+	if (StrnCmp(vendor, L"Apple", 5) == 0)
 		return TRUE;
 
 	return FALSE;