PE Loader: support and require NX

This adds support in our PE loader for NX support utilizing the
EFI_MEMORY_ATTRIBUTE protocol.  Specifically, it changes the loader such
that:

- binaries without the EFI_IMAGE_DLLCHARACTERISTICS_NX_COMPAT flag set
  in the Optional Header are rejected as EFI_UNSUPPORTED
- binaries with non-discardable sections that have both the
  EFI_SCN_MEM_WRITE and EFI_SCN_MEM_EXECUTE flags set are rejected as
  EFI_UNSUPPORTED
- if the EFI_MEMORY_ATTRIBUTE protocol is installed, then:
  - sections without the EFI_SCN_MEM_READ flag set will be marked with
    EFI_MEMORY_RP
  - sections without the EFI_SCN_MEM_WRITE flag set will be marked with
    EFI_MEMORY_RO
  - sections without the EFI_SCN_MEM_EXECUTE flag set will be marked
    with EFI_MEMORY_XP

Signed-off-by: Peter Jones <pjones@redhat.com>

4 files changed, 209 insertions, 2 deletions

diff --git a/include/guid.h b/include/guid.h
index 07a19a91..d9910ff1 100644
--- a/include/guid.h
+++ b/include/guid.h
@@ -33,8 +33,8 @@ extern EFI_GUID EFI_SECURE_BOOT_DB_GUID;
 extern EFI_GUID EFI_SIMPLE_FILE_SYSTEM_GUID;
 extern EFI_GUID SECURITY_PROTOCOL_GUID;
 extern EFI_GUID SECURITY2_PROTOCOL_GUID;
+extern EFI_GUID EFI_MEMORY_ATTRIBUTE_PROTOCOL_GUID;
 extern EFI_GUID SHIM_LOCK_GUID;
-
 extern EFI_GUID MOK_VARIABLE_STORE;
 
 #endif /* SHIM_GUID_H */
diff --git a/lib/guid.c b/lib/guid.c
index 143e0bbd..e100c92e 100644
--- a/lib/guid.c
+++ b/lib/guid.c
@@ -32,6 +32,6 @@ EFI_GUID EFI_SECURE_BOOT_DB_GUID =  { 0xd719b2cb, 0x3d3a, 0x4596, { 0xa3, 0xbc,
 EFI_GUID EFI_SIMPLE_FILE_SYSTEM_GUID = SIMPLE_FILE_SYSTEM_PROTOCOL;
 EFI_GUID SECURITY_PROTOCOL_GUID = { 0xA46423E3, 0x4617, 0x49f1, {0xB9, 0xFF, 0xD1, 0xBF, 0xA9, 0x11, 0x58, 0x39 } };
 EFI_GUID SECURITY2_PROTOCOL_GUID = { 0x94ab2f58, 0x1438, 0x4ef1, {0x91, 0x52, 0x18, 0x94, 0x1a, 0x3a, 0x0e, 0x68 } };
-
+EFI_GUID EFI_MEMORY_ATTRIBUTE_PROTOCOL_GUID = { 0xf4560cf6, 0x40ec, 0x4b4a, {0xa1, 0x92, 0xbf, 0x1d, 0x57, 0xd0, 0xb1, 0x89} };
 EFI_GUID SHIM_LOCK_GUID = {0x605dab50, 0xe046, 0x4300, {0xab, 0xb6, 0x3d, 0xd8, 0x10, 0xdd, 0x8b, 0x23 } };
 EFI_GUID MOK_VARIABLE_STORE = {0xc451ed2b, 0x9694, 0x45d3, {0xba, 0xba, 0xed, 0x9f, 0x89, 0x88, 0xa3, 0x89} };
diff --git a/pe.c b/pe.c
index 535d463a..9fa6fffd 100644
--- a/pe.c
+++ b/pe.c
@@ -696,6 +696,7 @@ read_header(void *data, unsigned int datasize,
 	EFI_IMAGE_OPTIONAL_HEADER_UNION *PEHdr = data;
 	unsigned long HeaderWithoutDataDir, SectionHeaderOffset, OptHeaderSize;
 	unsigned long FileAlignment = 0;
+	UINT16 DllFlags;
 
 	if (datasize < sizeof (PEHdr->Pe32)) {
 		perror(L"Invalid image\n");
@@ -790,13 +791,20 @@ read_header(void *data, unsigned int datasize,
 		context->EntryPoint = PEHdr->Pe32Plus.OptionalHeader.AddressOfEntryPoint;
 		context->RelocDir = &PEHdr->Pe32Plus.OptionalHeader.DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_BASERELOC];
 		context->SecDir = &PEHdr->Pe32Plus.OptionalHeader.DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_SECURITY];
+		DllFlags = PEHdr->Pe32Plus.OptionalHeader.DllCharacteristics;
 	} else {
 		context->ImageAddress = PEHdr->Pe32.OptionalHeader.ImageBase;
 		context->EntryPoint = PEHdr->Pe32.OptionalHeader.AddressOfEntryPoint;
 		context->RelocDir = &PEHdr->Pe32.OptionalHeader.DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_BASERELOC];
 		context->SecDir = &PEHdr->Pe32.OptionalHeader.DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_SECURITY];
+		DllFlags = PEHdr->Pe32.OptionalHeader.DllCharacteristics;
 	}
 
+	if (!(DllFlags & EFI_IMAGE_DLLCHARACTERISTICS_NX_COMPAT)) {
+		perror(L"Image does not support NX\n");
+		return EFI_UNSUPPORTED;
+        }
+
 	context->FirstSection = (EFI_IMAGE_SECTION_HEADER *)((char *)PEHdr + PEHdr->Pe32.FileHeader.SizeOfOptionalHeader + sizeof(UINT32) + sizeof(EFI_IMAGE_FILE_HEADER));
 
 	if (context->ImageSize < context->SizeOfHeaders) {
@@ -878,6 +886,139 @@ err:
 	return efi_status;
 }
 
+static inline uint64_t
+shim_mem_attrs_to_uefi_mem_attrs (uint64_t attrs)
+{
+	uint64_t ret = EFI_MEMORY_RP |
+		       EFI_MEMORY_RO |
+		       EFI_MEMORY_XP;
+
+	if (attrs & MEM_ATTR_R)
+		ret &= ~EFI_MEMORY_RP;
+
+	if (attrs & MEM_ATTR_W)
+		ret &= ~EFI_MEMORY_RO;
+
+	if (attrs & MEM_ATTR_X)
+		ret &= ~EFI_MEMORY_XP;
+
+	return ret;
+}
+
+static inline uint64_t
+uefi_mem_attrs_to_shim_mem_attrs (uint64_t attrs)
+{
+	uint64_t ret = MEM_ATTR_R |
+		       MEM_ATTR_W |
+		       MEM_ATTR_X;
+
+	if (attrs & EFI_MEMORY_RP)
+		ret &= ~MEM_ATTR_R;
+
+	if (attrs & EFI_MEMORY_RO)
+		ret &= ~MEM_ATTR_W;
+
+	if (attrs & EFI_MEMORY_XP)
+		ret &= ~MEM_ATTR_X;
+
+	return ret;
+}
+
+static EFI_STATUS
+get_mem_attrs (uintptr_t addr, size_t size, uint64_t *attrs)
+{
+	EFI_MEMORY_ATTRIBUTE_PROTOCOL *proto = NULL;
+	EFI_PHYSICAL_ADDRESS physaddr = addr;
+	EFI_STATUS efi_status;
+
+	efi_status = LibLocateProtocol(&EFI_MEMORY_ATTRIBUTE_PROTOCOL_GUID,
+				       (VOID **)&proto);
+	if (EFI_ERROR(efi_status) || !proto)
+		return efi_status;
+
+	if (physaddr & 0xfff || size & 0xfff || size == 0 || attrs == NULL) {
+		dprint(L"%a called on 0x%llx-0x%llx and attrs 0x%llx\n",
+		       __func__, (unsigned long long)physaddr,
+		       (unsigned long long)(physaddr+size-1),
+		       attrs);
+		return EFI_SUCCESS;
+	}
+
+	efi_status = proto->GetMemoryAttributes(proto, physaddr, size, attrs);
+	*attrs = uefi_mem_attrs_to_shim_mem_attrs (*attrs);
+
+	return efi_status;
+}
+
+static EFI_STATUS
+update_mem_attrs(uintptr_t addr, uint64_t size,
+		 uint64_t set_attrs, uint64_t clear_attrs)
+{
+	EFI_MEMORY_ATTRIBUTE_PROTOCOL *proto = NULL;
+	EFI_PHYSICAL_ADDRESS physaddr = addr;
+	EFI_STATUS efi_status, ret;
+	uint64_t before = 0, after = 0, uefi_set_attrs, uefi_clear_attrs;
+
+	efi_status = LibLocateProtocol(&EFI_MEMORY_ATTRIBUTE_PROTOCOL_GUID,
+				       (VOID **)&proto);
+	if (EFI_ERROR(efi_status) || !proto)
+		return efi_status;
+
+	efi_status = get_mem_attrs (addr, size, &before);
+	if (EFI_ERROR(efi_status))
+		dprint(L"get_mem_attrs(0x%llx, 0x%llx, 0x%llx) -> 0x%lx\n",
+		       (unsigned long long)addr, (unsigned long long)size,
+		       &before, efi_status);
+
+	if (physaddr & 0xfff || size & 0xfff || size == 0) {
+		dprint(L"%a called on 0x%llx-0x%llx (size 0x%llx) +%a%a%a -%a%a%a\n",
+		       __func__, (unsigned long long)physaddr,
+		       (unsigned long long)(physaddr + size - 1),
+		       (unsigned long long)size,
+		       (set_attrs & MEM_ATTR_R) ? "r" : "",
+		       (set_attrs & MEM_ATTR_W) ? "w" : "",
+		       (set_attrs & MEM_ATTR_X) ? "x" : "",
+		       (clear_attrs & MEM_ATTR_R) ? "r" : "",
+		       (clear_attrs & MEM_ATTR_W) ? "w" : "",
+		       (clear_attrs & MEM_ATTR_X) ? "x" : "");
+		return 0;
+	}
+
+	uefi_set_attrs = shim_mem_attrs_to_uefi_mem_attrs (set_attrs);
+	dprint("translating set_attrs from 0x%lx to 0x%lx\n", set_attrs, uefi_set_attrs);
+	uefi_clear_attrs = shim_mem_attrs_to_uefi_mem_attrs (clear_attrs);
+	dprint("translating clear_attrs from 0x%lx to 0x%lx\n", clear_attrs, uefi_clear_attrs);
+	efi_status = EFI_SUCCESS;
+	if (uefi_set_attrs)
+		efi_status = proto->SetMemoryAttributes(proto, physaddr, size, uefi_set_attrs);
+	if (!EFI_ERROR(efi_status) && uefi_clear_attrs)
+		efi_status = proto->ClearMemoryAttributes(proto, physaddr, size, uefi_clear_attrs);
+	ret = efi_status;
+
+	efi_status = get_mem_attrs (addr, size, &after);
+	if (EFI_ERROR(efi_status))
+		dprint(L"get_mem_attrs(0x%llx, %llu, 0x%llx) -> 0x%lx\n",
+		       (unsigned long long)addr, (unsigned long long)size,
+		       &after, efi_status);
+
+	dprint(L"set +%a%a%a -%a%a%a on 0x%llx-0x%llx before:%c%c%c after:%c%c%c\n",
+	       (set_attrs & MEM_ATTR_R) ? "r" : "",
+	       (set_attrs & MEM_ATTR_W) ? "w" : "",
+	       (set_attrs & MEM_ATTR_X) ? "x" : "",
+	       (clear_attrs & MEM_ATTR_R) ? "r" : "",
+	       (clear_attrs & MEM_ATTR_W) ? "w" : "",
+	       (clear_attrs & MEM_ATTR_X) ? "x" : "",
+	       (unsigned long long)addr, (unsigned long long)(addr + size - 1),
+	       (before & MEM_ATTR_R) ? 'r' : '-',
+	       (before & MEM_ATTR_W) ? 'w' : '-',
+	       (before & MEM_ATTR_X) ? 'x' : '-',
+	       (after & MEM_ATTR_R) ? 'r' : '-',
+	       (after & MEM_ATTR_W) ? 'w' : '-',
+	       (after & MEM_ATTR_X) ? 'x' : '-');
+
+	return ret;
+}
+
 EFI_STATUS verify_image(void *data, unsigned int datasize,
 			EFI_LOADED_IMAGE *li,
 			PE_COFF_LOADER_IMAGE_CONTEXT *context)
@@ -1013,6 +1154,11 @@ handle_image (void *data, unsigned int datasize,
 	}
 
 	buffer = (void *)ALIGN_VALUE((unsigned long)*alloc_address, alignment);
+	dprint(L"Loading 0x%llx bytes at 0x%llx\n",
+	       (unsigned long long)context.ImageSize,
+	       (unsigned long long)(uintptr_t)buffer);
+	update_mem_attrs((uintptr_t)buffer, alloc_size, MEM_ATTR_R|MEM_ATTR_W,
+			 MEM_ATTR_X);
 
 	CopyMem(buffer, data, context.SizeOfHeaders);
 
@@ -1049,6 +1195,19 @@ handle_image (void *data, unsigned int datasize,
 		    !Section->Misc.VirtualSize)
 			continue;
 
+		/*
+		 * Skip sections that aren't marked readable.
+		 */
+		if (!(Section->Characteristics & EFI_IMAGE_SCN_MEM_READ))
+			continue;
+
+		if (!(Section->Characteristics & EFI_IMAGE_SCN_MEM_DISCARDABLE) &&
+		    (Section->Characteristics & EFI_IMAGE_SCN_MEM_WRITE) &&
+		    (Section->Characteristics & EFI_IMAGE_SCN_MEM_EXECUTE)) {
+			perror(L"Section %d is writable and executable\n", i);
+			return EFI_UNSUPPORTED;
+		}
+
 		base = ImageAddress (buffer, context.ImageSize,
 				     Section->VirtualAddress);
 		end = ImageAddress (buffer, context.ImageSize,
@@ -1160,6 +1319,50 @@ handle_image (void *data, unsigned int datasize,
 	}
 
 	/*
+	 * Now set the page permissions appropriately.
+	 */
+	Section = context.FirstSection;
+	for (i = 0; i < context.NumberOfSections; i++, Section++) {
+		uint64_t set_attrs = MEM_ATTR_R;
+		uint64_t clear_attrs = MEM_ATTR_W|MEM_ATTR_X;
+		uintptr_t addr;
+		uint64_t length;
+
+		/*
+		 * Skip discardable sections with zero size
+		 */
+		if ((Section->Characteristics & EFI_IMAGE_SCN_MEM_DISCARDABLE) &&
+		    !Section->Misc.VirtualSize)
+			continue;
+
+		/*
+		 * Skip sections that aren't marked readable.
+		 */
+		if (!(Section->Characteristics & EFI_IMAGE_SCN_MEM_READ))
+			continue;
+
+		base = ImageAddress (buffer, context.ImageSize,
+				     Section->VirtualAddress);
+		end = ImageAddress (buffer, context.ImageSize,
+				    Section->VirtualAddress
+				     + Section->Misc.VirtualSize - 1);
+
+		addr = (uintptr_t)base;
+		length = (uintptr_t)end - (uintptr_t)base + 1;
+
+		if (Section->Characteristics & EFI_IMAGE_SCN_MEM_WRITE) {
+			set_attrs |= MEM_ATTR_W;
+			clear_attrs &= ~MEM_ATTR_W;
+		}
+		if (Section->Characteristics & EFI_IMAGE_SCN_MEM_EXECUTE) {
+			set_attrs |= MEM_ATTR_X;
+			clear_attrs &= ~MEM_ATTR_X;
+		}
+		update_mem_attrs(addr, length, set_attrs, clear_attrs);
+	}
+
+
+	/*
 	 * grub needs to know its location and size in memory, so fix up
 	 * the loaded image protocol values
 	 */
diff --git a/shim.h b/shim.h
index 703c1145..dc3cda73 100644
--- a/shim.h
+++ b/shim.h
@@ -195,6 +195,10 @@
 #include "Cryptlib/Include/OpenSslSupport.h"
 #endif
 
+#define MEM_ATTR_R	4
+#define MEM_ATTR_W	2
+#define MEM_ATTR_X	1
+
 INTERFACE_DECL(_SHIM_LOCK);
 
 typedef