loader-protocol: add workaround for EDK2 2025.02 page fault on FreePages

EDK2 since version 2025.02 introduced the EFI memory attribute protocol:

tianocore/edk2@efaa102

This is used by shim to unset the writable bit and set the executable
bit on images when they are loaded.

EDK2 also (at least in the Debian/Ubuntu production builds) overwrites
pages with a fixed 0xaf pattern when BS->FreePages() is called:

https://github.com/tianocore/edk2/blob/399a40e5cba2ed70197ac61c8da9cf9805fb918e/MdePkg/Library/BaseDebugLibSerialPort/DebugLib.c#L256
https://github.com/tianocore/edk2/blob/399a40e5cba2ed70197ac61c8da9cf9805fb918e/MdePkg/Library/UefiDebugLibConOut/DebugLib.c#L256
https://github.com/tianocore/edk2/blob/399a40e5cba2ed70197ac61c8da9cf9805fb918e/MdePkg/Library/UefiDebugLibStdErr/DebugLib.c#L256

These two properties mix together as well as one may image:

systemd-stub@0x72c64000 v999-bluca
FSOpen: Open '\loader\addons' Success
FSOpen: Open '\loader\addons\1.addon.efi' Success
InstallProtocolInterface: 6E6BAEB8-7108-4179-949D-A3493415EC97 7D368D18
InstallProtocolInterface: 5B1B31A1-9562-11D2-8E3F-00A0C969723B 7D368D18
InstallProtocolInterface: BC62157E-3E33-4FEC-9920-2D3B36D750DF 7D376D18
!!!! X64 Exception Type - 0E(#PF - Page-Fault)  CPU Apic ID - 00000000 !!!!
ExceptionData - 0000000000000003  I:0 R:0 U:0 W:1 P:1 PK:0 SS:0 SGX:0
RIP  - 000000007EF0931A, CS  - 0000000000000038, RFLAGS - 0000000000010206
RAX  - 00000000000000AF, RCX - 0000000000005000, RDX - 000000007D352000
RBX  - 000000007D352000, RSP - 000000007EEEAAF0, RBP - 000000007EEEAB80
RSI  - 000000007EF10F20, RDI - 000000007D353000
R8   - 00000000000000AF, R9  - 000000007D357FFF, R10 - 0000000072C7C2F7
R11  - 0000000001CBD9D0, R12 - 000000007D357FFF, R13 - 000000007D358000
R14  - 000000007D35B300, R15 - 000000007D357FFF
DS   - 0000000000000030, ES  - 0000000000000030, FS  - 0000000000000030
GS   - 0000000000000030, SS  - 0000000000000030
CR0  - 0000000080010033, CR2 - 000000007D353000, CR3 - 000000007EC01000
CR4  - 0000000000000668, CR8 - 0000000000000000
DR0  - 0000000000000000, DR1 - 0000000000000000, DR2 - 0000000000000000
DR3  - 0000000000000000, DR6 - 00000000FFFF0FF0, DR7 - 0000000000000400
GDTR - 000000007E9E1000 0000000000000047, LDTR - 0000000000000000
IDTR - 000000007E407018 0000000000000FFF,   TR - 0000000000000000
FXSAVE_STATE - 000000007EEEA750
!!!! Find image based on IP(0x7EF0931A) /home/bluca/git/edk2/Build/OvmfX64/DEBUG_GCC5/X64/MdeModulePkg/Core/Dxe/DxeMain/DEBUG/DxeCore.dll (ImageBase=000000007EEEC000, EntryPoint=000000007EF026FD) !!!!

This issue can be reproduced by simply calling BS->UnloadImage() on any image
that was loaded by shim's BS->LoadImage(). The systemd stub used in UKIs does
so while loading and parsing addons.

While this is arguably a bug in EDK2 (it should check that pages are writable
before attempting to write them) and in the Debian/Ubuntu build (these debug
modules should be swapped out for the BaseDebugLibNull module where this is a no-op:
https://github.com/tianocore/edk2/blob/399a40e5cba2ed70197ac61c8da9cf9805fb918e/MdePkg/Library/BaseDebugLibNull/DebugLib.c#L137 ),
this is how things work currently in at least two distribution stable releases,
so add a workaround and set W+ X- before calling BS->FreePages().

Signed-off-by: Luca Boccassi <[email protected]>

Author: bluca

include/pe.h

@@ -41,7 +41,7 @@ handle_image (void *data, unsigned int datasize,
 	      EFI_LOADED_IMAGE *li,
 	      EFI_IMAGE_ENTRY_POINT *entry_point,
 	      EFI_PHYSICAL_ADDRESS *alloc_address,
-	      UINTN *alloc_pages);
+	      UINTN *alloc_pages, unsigned int *alloc_alignment);
 
 EFI_STATUS
 generate_hash (char *data, unsigned int datasize,

loader-proto.c

@@ -163,6 +163,28 @@ try_load_from_lf2(EFI_DEVICE_PATH *dp, buffer_properties_t *bprop)
 	return status;
 }
 
+static void
+free_pages_alloc_image(SHIM_LOADED_IMAGE *image)
+{
+	char *buffer;
+
+	if (!image || !image->alloc_address || !image->alloc_pages)
+		return;
+
+	/* EDKII overwrites memory pages with a fixed pattern in at least
+	 * production Debian/Ubuntu builds as of 2025.02. If the W- X+ bits
+	 * are set on a loaded image, this will cause a page fault when it
+	 * is freed. Ensure W+ X- are set instead before freeing. */
+
+	buffer = (void *)ALIGN_VALUE((unsigned long)image->alloc_address, image->alloc_alignment);
+	update_mem_attrs((uintptr_t)buffer, image->alloc_pages * PAGE_SIZE, MEM_ATTR_R|MEM_ATTR_W,
+			 MEM_ATTR_X);
+
+	BS->FreePages(image->alloc_address, image->alloc_pages);
+	image->alloc_address = 0;
+	image->alloc_pages = 0;
+}
+
 static EFI_STATUS EFIAPI
 shim_load_image(BOOLEAN BootPolicy, EFI_HANDLE ParentImageHandle,
                 EFI_DEVICE_PATH *DevicePath, VOID *SourceBuffer,
@@ -240,7 +262,7 @@ shim_load_image(BOOLEAN BootPolicy, EFI_HANDLE ParentImageHandle,
 	in_protocol = 1;
 	efi_status = handle_image(SourceBuffer, SourceSize, &image->li,
 	                          &image->entry_point, &image->alloc_address,
-	                          &image->alloc_pages);
+	                          &image->alloc_pages, &image->alloc_alignment);
 	in_protocol = 0;
 	if (EFI_ERROR(efi_status))
 		goto free_image;
@@ -261,7 +283,7 @@ shim_load_image(BOOLEAN BootPolicy, EFI_HANDLE ParentImageHandle,
 	return EFI_SUCCESS;
 
 free_alloc:
-	BS->FreePages(image->alloc_address, image->alloc_pages);
+	free_pages_alloc_image(image);
 free_image:
 	if (image->loaded_image_device_path)
 		FreePool(image->loaded_image_device_path);
@@ -317,7 +339,7 @@ shim_start_image(IN EFI_HANDLE ImageHandle, OUT UINTN *ExitDataSize,
 					image->loaded_image_device_path,
 					NULL);
 
-	BS->FreePages(image->alloc_address, image->alloc_pages);
+	free_pages_alloc_image(image);
 	if (image->li.FilePath)
 		BS->FreePool(image->li.FilePath);
 	if (image->loaded_image_device_path)
@@ -339,7 +361,7 @@ shim_unload_image(EFI_HANDLE ImageHandle)
 	if (efi_status == EFI_UNSUPPORTED)
 		return system_unload_image(ImageHandle);
 
-	BS->FreePages(image->alloc_address, image->alloc_pages);
+	free_pages_alloc_image(image);
 	FreePool(image);
 
 	return EFI_SUCCESS;

pe.c

@@ -465,7 +465,7 @@ handle_image (void *data, unsigned int datasize,
 	      EFI_LOADED_IMAGE *li,
 	      EFI_IMAGE_ENTRY_POINT *entry_point,
 	      EFI_PHYSICAL_ADDRESS *alloc_address,
-	      UINTN *alloc_pages)
+	      UINTN *alloc_pages, unsigned int *alloc_alignment)
 {
 	EFI_STATUS efi_status;
 	char *buffer;
@@ -474,7 +474,7 @@ handle_image (void *data, unsigned int datasize,
 	char *base, *end;
 	UINT32 size;
 	PE_COFF_LOADER_IMAGE_CONTEXT context;
-	unsigned int alignment, alloc_size;
+	unsigned int alloc_size;
 	int found_entry_point = 0;
 	UINT8 sha1hash[SHA1_DIGEST_SIZE];
 	UINT8 sha256hash[SHA256_DIGEST_SIZE];
@@ -547,9 +547,9 @@ handle_image (void *data, unsigned int datasize,
 	 *
 	 * We only support one page size, so if it's zero, nerf it to 4096.
 	 */
-	alignment = context.SectionAlignment;
-	if (!alignment)
-		alignment = 4096;
+	*alloc_alignment = context.SectionAlignment;
+	if (!*alloc_alignment)
+		*alloc_alignment = 4096;
 
 	alloc_size = ALIGN_VALUE(context.ImageSize + context.SectionAlignment,
 				 PAGE_SIZE);
@@ -562,7 +562,7 @@ handle_image (void *data, unsigned int datasize,
 		return EFI_OUT_OF_RESOURCES;
 	}
 
-	buffer = (void *)ALIGN_VALUE((unsigned long)*alloc_address, alignment);
+	buffer = (void *)ALIGN_VALUE((unsigned long)*alloc_address, *alloc_alignment);
 	dprint(L"Loading 0x%llx bytes at 0x%llx\n",
 	       (unsigned long long)context.ImageSize,
 	       (unsigned long long)(uintptr_t)buffer);

shim.c

@@ -1148,6 +1148,7 @@ EFI_STATUS start_image(EFI_HANDLE image_handle, CHAR16 *ImagePath)
 	CHAR16 *PathName = NULL;
 	void *data = NULL;
 	int datasize = 0;
+	unsigned int alloc_alignment;
 
 	efi_status = read_image(image_handle, ImagePath, &PathName, &data,
 				&datasize, 0);
@@ -1174,7 +1175,7 @@ EFI_STATUS start_image(EFI_HANDLE image_handle, CHAR16 *ImagePath)
 	 * Verify and, if appropriate, relocate and execute the executable
 	 */
 	efi_status = handle_image(data, datasize, shim_li, &entry_point,
-				  &alloc_address, &alloc_pages);
+				  &alloc_address, &alloc_pages, &alloc_alignment);
 	if (EFI_ERROR(efi_status)) {
 		perror(L"Failed to load image: %r\n", efi_status);
 		PrintErrors();

shim.h

@@ -330,6 +330,7 @@ typedef struct {
 	EFI_IMAGE_ENTRY_POINT	entry_point;
 	EFI_PHYSICAL_ADDRESS	alloc_address;
 	UINTN			alloc_pages;
+	unsigned int		alloc_alignment;
 	EFI_STATUS		exit_status;
 	CONST CHAR16		*exit_data;
 	UINTN			exit_data_size;