[PATCH RFC v2 0/2] Handle UEFI NX-restricted page tables

[PATCH RFC v2 0/2] Handle UEFI NX-restricted page tables

[Baskov Evgeniy]

This is another implementation of this patch. It uses DXE services
to change memory protection flags as you suggested earlier.

As I mentioned, you can reproduce this issue with any firmware,
including OVMF by setting the PcdDxeNxMemoryProtectionPolicy policy:
gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy|0x7FD4
Restricting the user from creating executable pages is not an goal,
but restricting one from creating both executable and writable pages
is a goal, which is enforced in the firmware we use. We cannot allow
allocating pages of any type that have RWX permissions.
gDS->SetMemorySpaceAttributes() is technically part of the UEFI PI
specification, so it is not too bad to rely on it. However:
- DXE services is not something designed to be used by an UEFI application.
- From what we know, no other operating system uses this interface,
which means that it can easily break in production firmware on the
boards we do not control before anyone could even notice.
We do not strictly mind experimenting with this route, but it would be
preferable for this interface to become more standard in this case:
move it to UEFI Boot Services or a separate protocol and include it in
UEFI conformance suite. It will also help if we enable this feature in
Linux by default.

Baskov Evgeniy (2):
       efi: declare DXE services table
       libstub: ensure allocated memory to be executable

 arch/x86/include/asm/efi.h              |  5 ++
 drivers/firmware/efi/libstub/efistub.h  | 53 ++++++++++++++++++++
 drivers/firmware/efi/libstub/x86-stub.c | 73 ++++++++++++++++++++++++++--
 include/linux/efi.h                     |  2 +
 4 files changed, 128 insertions(+), 5 deletions(-)

[PATCH RFC v2 1/2] libstub: declare DXE services table

[Baskov Evgeniy]

UEFI DXE services are not yet used in kernel code
but are required to manipulate page table memory
protection flags.

Add required declarations to use DXE services functions.

Signed-off-by: Baskov Evgeniy <baskov@ispras.ru>

diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index 03cb12775043..4614d54383ac 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -352,6 +352,11 @@ static inline u32 efi64_convert_status(efi_status_t status)
 						   runtime),		\
 				    func, __VA_ARGS__))
 
+#define efi_dxe_call(func, ...)						\
+	(efi_is_native()						\
+		? efi_dxe_table->func(__VA_ARGS__)			\
+		: __efi64_thunk_map(efi_dxe_table, func, __VA_ARGS__))
+
 #else /* CONFIG_EFI_MIXED */
 
 static inline bool efi_is_64bit(void)
diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
index edb77b0621ea..647f060c7318 100644
--- a/drivers/firmware/efi/libstub/efistub.h
+++ b/drivers/firmware/efi/libstub/efistub.h
@@ -36,6 +36,9 @@ extern bool efi_novamap;
 
 extern const efi_system_table_t *efi_system_table;
 
+typedef union efi_dxe_services_table efi_dxe_services_table_t;
+extern const efi_dxe_services_table_t *efi_dxe_table;
+
 efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
 				   efi_system_table_t *sys_table_arg);
 
@@ -44,6 +47,7 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
 #define efi_is_native()		(true)
 #define efi_bs_call(func, ...)	efi_system_table->boottime->func(__VA_ARGS__)
 #define efi_rt_call(func, ...)	efi_system_table->runtime->func(__VA_ARGS__)
+#define efi_dxe_call(func, ...)	efi_dxe_table->func(__VA_ARGS__)
 #define efi_table_attr(inst, attr)	(inst->attr)
 #define efi_call_proto(inst, func, ...) inst->func(inst, ##__VA_ARGS__)
 
@@ -329,6 +333,55 @@ union efi_boot_services {
 	} mixed_mode;
 };
 
+/*
+ * EFI DXE Services table
+ */
+union efi_dxe_services_table {
+	struct {
+		efi_table_hdr_t hdr;
+		void *add_memory_space;
+		void *allocate_memory_space;
+		void *free_memory_space;
+		void *remove_memory_space;
+		void *get_memory_space_descriptor;
+		efi_status_t (__efiapi *set_memory_space_attributes)(efi_physical_addr_t,
+								     u64, u64);
+		void *get_memory_space_map;
+		void *add_io_space;
+		void *allocate_io_space;
+		void *free_io_space;
+		void *remove_io_space;
+		void *get_io_space_descriptor;
+		void *get_io_space_map;
+		void *dispatch;
+		void *schedule;
+		void *trust;
+		void *process_firmware_volume;
+		void *set_memory_space_capabilities;
+	};
+	struct {
+		efi_table_hdr_t hdr;
+		u32 add_memory_space;
+		u32 allocate_memory_space;
+		u32 free_memory_space;
+		u32 remove_memory_space;
+		u32 get_memory_space_descriptor;
+		u32 set_memory_space_attributes;
+		u32 get_memory_space_map;
+		u32 add_io_space;
+		u32 allocate_io_space;
+		u32 free_io_space;
+		u32 remove_io_space;
+		u32 get_io_space_descriptor;
+		u32 get_io_space_map;
+		u32 dispatch;
+		u32 schedule;
+		u32 trust;
+		u32 process_firmware_volume;
+		u32 set_memory_space_capabilities;
+	} mixed_mode;
+};
+
 typedef union efi_uga_draw_protocol efi_uga_draw_protocol_t;
 
 union efi_uga_draw_protocol {
diff --git a/include/linux/efi.h b/include/linux/efi.h
index ccd4d3f91c98..c5ad6245c7ec 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -383,6 +383,7 @@ void efi_native_runtime_setup(void);
 #define EFI_LOAD_FILE_PROTOCOL_GUID		EFI_GUID(0x56ec3091, 0x954c, 0x11d2,  0x8e, 0x3f, 0x00, 0xa0, 0xc9, 0x69, 0x72, 0x3b)
 #define EFI_LOAD_FILE2_PROTOCOL_GUID		EFI_GUID(0x4006c0c1, 0xfcb3, 0x403e,  0x99, 0x6d, 0x4a, 0x6c, 0x87, 0x24, 0xe0, 0x6d)
 #define EFI_RT_PROPERTIES_TABLE_GUID		EFI_GUID(0xeb66918a, 0x7eef, 0x402a,  0x84, 0x2e, 0x93, 0x1d, 0x21, 0xc3, 0x8a, 0xe9)
+#define EFI_DXE_SERVICES_TABLE_GUID		EFI_GUID(0x05ad34ba, 0x6f02, 0x4214,  0x95, 0x2e, 0x4d, 0xa0, 0x39, 0x8e, 0x2b, 0xb9)
 
 #define EFI_IMAGE_SECURITY_DATABASE_GUID	EFI_GUID(0xd719b2cb, 0x3d3a, 0x4596,  0xa3, 0xbc, 0xda, 0xd0, 0x0e, 0x67, 0x65, 0x6f)
 #define EFI_SHIM_LOCK_GUID			EFI_GUID(0x605dab50, 0xe046, 0x4300,  0xab, 0xb6, 0x3d, 0xd8, 0x10, 0xdd, 0x8b, 0x23)
@@ -435,6 +436,7 @@ typedef struct {
 } efi_config_table_type_t;
 
 #define EFI_SYSTEM_TABLE_SIGNATURE ((u64)0x5453595320494249ULL)
+#define EFI_DXE_SERVICES_TABLE_SIGNATURE ((u64)0x565245535f455844ULL)
 
 #define EFI_2_30_SYSTEM_TABLE_REVISION  ((2 << 16) | (30))
 #define EFI_2_20_SYSTEM_TABLE_REVISION  ((2 << 16) | (20))
-- 
2.35.1

[PATCH RFC v2 2/2] libstub: ensure allocated memory to be executable

[Baskov Evgeniy]

There are UEFI versions that restrict execution of memory regions,
preventing the kernel from booting. Parts that needs to be executable
are:

* Area used for trampoline placement.
* All memory regions that the kernel may be relocated before
  and during extraction.

Use DXE services to ensure aforementioned address ranges
to be executable.

Signed-off-by: Baskov Evgeniy <baskov@ispras.ru>

diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
index 01ddd4502e28..ca7d5379b480 100644
--- a/drivers/firmware/efi/libstub/x86-stub.c
+++ b/drivers/firmware/efi/libstub/x86-stub.c
@@ -22,6 +22,7 @@
 #define MAXMEM_X86_64_4LEVEL (1ull << 46)
 
 const efi_system_table_t *efi_system_table;
+const efi_dxe_services_table_t *efi_dxe_table;
 extern u32 image_offset;
 static efi_loaded_image_t *image = NULL;
 
@@ -211,9 +212,41 @@ static void retrieve_apple_device_properties(struct boot_params *boot_params)
 	}
 }
 
+
+static void unprotect_memory_range(unsigned long base, unsigned long size)
+{
+	unsigned long rounded_base, rounded_size;
+	efi_status_t res;
+
+	if (efi_dxe_table == NULL)
+		return;
+
+	rounded_base = rounddown(base, EFI_PAGE_SIZE);
+	rounded_size = roundup(base + size, EFI_PAGE_SIZE) - rounded_base;
+
+	res = efi_dxe_call(set_memory_space_attributes,
+			   rounded_base, rounded_size, EFI_MEMORY_WB);
+	if (res != EFI_SUCCESS) {
+		efi_warn("Unable to unprotect memory range [%08lx,%08lx]: %d\n",
+			 base, size + base, (int)res);
+	}
+}
+
+/*
+ * Trampoline takes 2 pages and can be loaded in first megabyte of memory
+ * with its end placed between 128k and 640k where BIOS might start
+ */
+
+#define TRAMPOLINE_PLACEMENT_BASE ((128 - 8)*1024)
+#define TRAMPOLINE_PLACEMENT_SIZE (640*1024 - (128 - 8)*1024)
+
+void startup_32(struct boot_params *boot_params);
+
 static const efi_char16_t apple[] = L"Apple";
 
-static void setup_quirks(struct boot_params *boot_params)
+static void setup_quirks(struct boot_params *boot_params,
+			 unsigned long image_base,
+			 unsigned long image_size)
 {
 	efi_char16_t *fw_vendor = (efi_char16_t *)(unsigned long)
 		efi_table_attr(efi_system_table, fw_vendor);
@@ -222,6 +255,31 @@ static void setup_quirks(struct boot_params *boot_params)
 		if (IS_ENABLED(CONFIG_APPLE_PROPERTIES))
 			retrieve_apple_device_properties(boot_params);
 	}
+
+	/*
+	 * Allow execution of possible trampoline used
+	 * for switching between 4- and 5-level page tables
+	 * and relocated kernel image.
+	 */
+
+	unprotect_memory_range(TRAMPOLINE_PLACEMENT_BASE,
+	                       TRAMPOLINE_PLACEMENT_SIZE);
+
+#ifdef CONFIG_64BIT
+	if (image_base != (unsigned long)startup_32)
+		unprotect_memory_range(image_base, image_size);
+#else
+	/*
+	 * Clear protection flags on a whole range of possible
+	 * addresses used for KASLR. We don't need to do that
+	 * on x86_64, since KASLR/extraction is performed after
+	 * dedicated identity page tables are built and we only
+	 * need to remove possible protection on relocated image
+	 * itself disregarding further relocations.
+	 */
+	unprotect_memory_range(LOAD_PHYSICAL_ADDR,
+			       KERNEL_IMAGE_SIZE - LOAD_PHYSICAL_ADDR);
+#endif
 }
 
 /*
@@ -341,8 +399,6 @@ static void __noreturn efi_exit(efi_handle_t handle, efi_status_t status)
 		asm("hlt");
 }
 
-void startup_32(struct boot_params *boot_params);
-
 void __noreturn efi_stub_entry(efi_handle_t handle,
 			       efi_system_table_t *sys_table_arg,
 			       struct boot_params *boot_params);
@@ -677,11 +733,18 @@ unsigned long efi_main(efi_handle_t handle,
 	efi_status_t status;
 
 	efi_system_table = sys_table_arg;
-
 	/* Check if we were booted by the EFI firmware */
 	if (efi_system_table->hdr.signature != EFI_SYSTEM_TABLE_SIGNATURE)
 		efi_exit(handle, EFI_INVALID_PARAMETER);
 
+	efi_dxe_table = get_efi_config_table(EFI_DXE_SERVICES_TABLE_GUID);
+
+	if (efi_dxe_table == NULL ||
+	    efi_dxe_table->hdr.signature != EFI_DXE_SERVICES_TABLE_SIGNATURE) {
+		efi_warn("Unable to locate EFI DXE services table\n");
+		efi_dxe_table = NULL;
+	}
+
 	/*
 	 * If the kernel isn't already loaded at a suitable address,
 	 * relocate it.
@@ -791,7 +854,7 @@ unsigned long efi_main(efi_handle_t handle,
 
 	setup_efi_pci(boot_params);
 
-	setup_quirks(boot_params);
+	setup_quirks(boot_params, bzimage_addr, buffer_end - buffer_start);
 
 	status = exit_boot(boot_params, handle);
 	if (status != EFI_SUCCESS) {
-- 
2.35.1

Re: [PATCH RFC v2 0/2] Handle UEFI NX-restricted page tables

[Ard Biesheuvel]

(cc Matt and Peter)

On Thu, 24 Feb 2022 at 16:45, Baskov Evgeniy <baskov@ispras.ru> wrote:
>
> This is another implementation of this patch. It uses DXE services
> to change memory protection flags as you suggested earlier.
>
> As I mentioned, you can reproduce this issue with any firmware,
> including OVMF by setting the PcdDxeNxMemoryProtectionPolicy policy:
> gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy|0x7FD4
> Restricting the user from creating executable pages is not an goal,
> but restricting one from creating both executable and writable pages
> is a goal, which is enforced in the firmware we use. We cannot allow
> allocating pages of any type that have RWX permissions.
> gDS->SetMemorySpaceAttributes() is technically part of the UEFI PI
> specification, so it is not too bad to rely on it. However:
> - DXE services is not something designed to be used by an UEFI application.
> - From what we know, no other operating system uses this interface,
> which means that it can easily break in production firmware on the
> boards we do not control before anyone could even notice.
> We do not strictly mind experimenting with this route, but it would be
> preferable for this interface to become more standard in this case:
> move it to UEFI Boot Services or a separate protocol and include it in
> UEFI conformance suite. It will also help if we enable this feature in
> Linux by default.
>

Thanks for exploring my suggestion to use the DXE services for this.

Given that this is a workaround for a very specific issue arising on
PI based implementations of UEFI, I consider this a quirk, and so I
think this approach is reasonable. I'd still like to gate it on some
kind of identification, though - perhaps something related to DMI like
the x86 core kernel does as well.

I've cc'ed Peter and Matt, who have much more experience dealing with
these kinds of things on x86 - my experience is mostly based on ARM,
which tends to be less quirky when it comes to UEFI support, given
that vendors that implement EFI actually care about being compliant
(instead of only about getting a windows sticker)

Matt, Peter, any thoughts?


> Baskov Evgeniy (2):
>        efi: declare DXE services table
>        libstub: ensure allocated memory to be executable
>
>  arch/x86/include/asm/efi.h              |  5 ++
>  drivers/firmware/efi/libstub/efistub.h  | 53 ++++++++++++++++++++
>  drivers/firmware/efi/libstub/x86-stub.c | 73 ++++++++++++++++++++++++++--
>  include/linux/efi.h                     |  2 +
>  4 files changed, 128 insertions(+), 5 deletions(-)

Re: [PATCH RFC v2 0/2] Handle UEFI NX-restricted page tables

[Matthew Garrett]

On Mon, Feb 28, 2022 at 05:45:53PM +0100, Ard Biesheuvel wrote:

> Given that this is a workaround for a very specific issue arising on
> PI based implementations of UEFI, I consider this a quirk, and so I
> think this approach is reasonable. I'd still like to gate it on some
> kind of identification, though - perhaps something related to DMI like
> the x86 core kernel does as well.

When the V1 patches were reviewed, you suggested allocating 
EFI_LOADER_CODE rather than EFI_LOADER_DATA. The example given for a 
failure case is when NxMemoryProtectionPolicy is set to 0x7fd4, in which 
case EFI_LOADER_CODE, EFI_BOOT_SERVICES_CODE and 
EFI_RUNTIEM_SERVICES_CODE should not have the nx policy applied. So it 
seems like your initial suggestion (s/LOADER_DATA/LOADER_CODE/) should 
have worked, even if there was disagreement about whether the spec 
required it to. Is this firmware applying a stricter policy?

Re: [PATCH RFC v2 0/2] Handle UEFI NX-restricted page tables

[baskov]

On 2022-02-28 21:30, Matthew Garrett wrote:
> On Mon, Feb 28, 2022 at 05:45:53PM +0100, Ard Biesheuvel wrote:
> 
>> Given that this is a workaround for a very specific issue arising on
>> PI based implementations of UEFI, I consider this a quirk, and so I
>> think this approach is reasonable. I'd still like to gate it on some
>> kind of identification, though - perhaps something related to DMI like
>> the x86 core kernel does as well.
> 
> When the V1 patches were reviewed, you suggested allocating
> EFI_LOADER_CODE rather than EFI_LOADER_DATA. The example given for a
> failure case is when NxMemoryProtectionPolicy is set to 0x7fd4, in 
> which
> case EFI_LOADER_CODE, EFI_BOOT_SERVICES_CODE and
> EFI_RUNTIEM_SERVICES_CODE should not have the nx policy applied. So it
> seems like your initial suggestion (s/LOADER_DATA/LOADER_CODE/) should
> have worked, even if there was disagreement about whether the spec
> required it to. Is this firmware applying a stricter policy?

Yes, this firmware is being modified to enforce stricter policy.

Also, the kernel still uses memory, that is not allocated via
EFI boot services, for trampoline placement in the first 640K of
address space, for which NX bit is also set.
The exact address for trampoline code is chosen only after
the EfiExitBootServices() call. And, I think, changing the
code in such way that trampoline is allocated beforehand will
touch more code paths.

Thanks,
Baskov Evgeniy

Re: [PATCH RFC v2 0/2] Handle UEFI NX-restricted page tables

[baskov]

On 2022-02-28 19:45, Ard Biesheuvel wrote:
> (cc Matt and Peter)
> 
> 
> Thanks for exploring my suggestion to use the DXE services for this.
> 
> Given that this is a workaround for a very specific issue arising on
> PI based implementations of UEFI, I consider this a quirk, and so I
> think this approach is reasonable. I'd still like to gate it on some
> kind of identification, though - perhaps something related to DMI like
> the x86 core kernel does as well.
> 
> I've cc'ed Peter and Matt, who have much more experience dealing with
> these kinds of things on x86 - my experience is mostly based on ARM,
> which tends to be less quirky when it comes to UEFI support, given
> that vendors that implement EFI actually care about being compliant
> (instead of only about getting a windows sticker)
> 
> Matt, Peter, any thoughts?
> 
> 
>> Baskov Evgeniy (2):
>>        efi: declare DXE services table
>>        libstub: ensure allocated memory to be executable
>> 
>>  arch/x86/include/asm/efi.h              |  5 ++
>>  drivers/firmware/efi/libstub/efistub.h  | 53 ++++++++++++++++++++
>>  drivers/firmware/efi/libstub/x86-stub.c | 73 
>> ++++++++++++++++++++++++++--
>>  include/linux/efi.h                     |  2 +
>>  4 files changed, 128 insertions(+), 5 deletions(-)

We now have tested the patch on major platforms, and it works without 
any
issues. But in case of firmware bugs I have changed the code to only
modify attributes if either EFI_MEMORY_RO or EFI_MEMORY_WP is set and
the memory has type EfiGcdMemoryTypeSystemMemory.

I also added option CONFIG_EFI_DXE_MEM_ATTRIBUTES (enabled by default),
to allow this code to be disabled at compile time.

These changes will be sent in version 3 of the patch.

Thanks,
Baskov Evgeniy

Re: [PATCH RFC v2 0/2] Handle UEFI NX-restricted page tables

[Matthew Garrett]

On Thu, Mar 03, 2022 at 04:42:07PM +0300, baskov@ispras.ru wrote:
> On 2022-02-28 21:30, Matthew Garrett wrote:
> > On Mon, Feb 28, 2022 at 05:45:53PM +0100, Ard Biesheuvel wrote:
> > 
> > > Given that this is a workaround for a very specific issue arising on
> > > PI based implementations of UEFI, I consider this a quirk, and so I
> > > think this approach is reasonable. I'd still like to gate it on some
> > > kind of identification, though - perhaps something related to DMI like
> > > the x86 core kernel does as well.
> > 
> > When the V1 patches were reviewed, you suggested allocating
> > EFI_LOADER_CODE rather than EFI_LOADER_DATA. The example given for a
> > failure case is when NxMemoryProtectionPolicy is set to 0x7fd4, in which
> > case EFI_LOADER_CODE, EFI_BOOT_SERVICES_CODE and
> > EFI_RUNTIEM_SERVICES_CODE should not have the nx policy applied. So it
> > seems like your initial suggestion (s/LOADER_DATA/LOADER_CODE/) should
> > have worked, even if there was disagreement about whether the spec
> > required it to. Is this firmware applying a stricter policy?
> 
> Yes, this firmware is being modified to enforce stricter policy.

Ok. I think this should really go through the UEFI spec process - I 
agree that from a strict interpretation of the spec, what this firmware 
is doing is legitimate, but I don't like having a situation where we 
have to depend on the DXE spec.

How does Windows handle this? Just update the page tables itself for any 
regions it needs during boot?

Re: [PATCH RFC v2 0/2] Handle UEFI NX-restricted page tables

[baskov]

On 2022-03-03 23:47, Matthew Garrett wrote:
> 
> Ok. I think this should really go through the UEFI spec process - I
> agree that from a strict interpretation of the spec, what this firmware
> is doing is legitimate, but I don't like having a situation where we
> have to depend on the DXE spec.
> 
> How does Windows handle this? Just update the page tables itself for 
> any
> regions it needs during boot?

Sorry for delay.

Windows is closed source, so we cannot give guarantees on its
behavior, but this is our belief regarding its behavior.
Added Bret Barkelew (bret.barkelew@microsoft.com)
to the CC-list in case he can add something.

Regarding the spec changes, we agree it is reasonable,
but whether the spec changes or not it will take some time
to update the edk2.

Our first solution was safer in regards to the use of the services,
yet as Ard suggested, using DXE services is much cleaner
as long as it works.

We can post it to edk2-devel, but our opinion
is that these issues are independent.

Thanks,
Baskov Evgeniy

Re: [PATCH RFC v2 0/2] Handle UEFI NX-restricted page tables

[Peter Jones]

On Thu, Mar 03, 2022 at 08:47:59PM +0000, Matthew Garrett wrote:
> On Thu, Mar 03, 2022 at 04:42:07PM +0300, baskov@ispras.ru wrote:
> > On 2022-02-28 21:30, Matthew Garrett wrote:
> > > On Mon, Feb 28, 2022 at 05:45:53PM +0100, Ard Biesheuvel wrote:
> > > 
> > > > Given that this is a workaround for a very specific issue arising on
> > > > PI based implementations of UEFI, I consider this a quirk, and so I
> > > > think this approach is reasonable. I'd still like to gate it on some
> > > > kind of identification, though - perhaps something related to DMI like
> > > > the x86 core kernel does as well.
> > > 
> > > When the V1 patches were reviewed, you suggested allocating
> > > EFI_LOADER_CODE rather than EFI_LOADER_DATA. The example given for a
> > > failure case is when NxMemoryProtectionPolicy is set to 0x7fd4, in which
> > > case EFI_LOADER_CODE, EFI_BOOT_SERVICES_CODE and
> > > EFI_RUNTIEM_SERVICES_CODE should not have the nx policy applied. So it
> > > seems like your initial suggestion (s/LOADER_DATA/LOADER_CODE/) should
> > > have worked, even if there was disagreement about whether the spec
> > > required it to. Is this firmware applying a stricter policy?
> > 
> > Yes, this firmware is being modified to enforce stricter policy.
> 
> Ok. I think this should really go through the UEFI spec process - I 
> agree that from a strict interpretation of the spec, what this firmware 
> is doing is legitimate, but I don't like having a situation where we 
> have to depend on the DXE spec.

It's in the process of getting into the UEFI spec now as
https://bugzilla.tianocore.org/show_bug.cgi?id=3519 .

> How does Windows handle this? Just update the page tables itself for any 
> regions it needs during boot?

Microsoft's bootloader sets up its own pagetables, though I believe
they're switching it to use the (soon to be) standardized API.

-- 
        Peter

Re: [PATCH RFC v2 0/2] Handle UEFI NX-restricted page tables

[baskov]

On 2022-03-18 19:37, Peter Jones wrote:
> On Thu, Mar 03, 2022 at 08:47:59PM +0000, Matthew Garrett wrote:
>> On Thu, Mar 03, 2022 at 04:42:07PM +0300, baskov@ispras.ru wrote:
>> > On 2022-02-28 21:30, Matthew Garrett wrote:
>> > > On Mon, Feb 28, 2022 at 05:45:53PM +0100, Ard Biesheuvel wrote:
>> > >
>> > > > Given that this is a workaround for a very specific issue arising on
>> > > > PI based implementations of UEFI, I consider this a quirk, and so I
>> > > > think this approach is reasonable. I'd still like to gate it on some
>> > > > kind of identification, though - perhaps something related to DMI like
>> > > > the x86 core kernel does as well.
>> > >
>> > > When the V1 patches were reviewed, you suggested allocating
>> > > EFI_LOADER_CODE rather than EFI_LOADER_DATA. The example given for a
>> > > failure case is when NxMemoryProtectionPolicy is set to 0x7fd4, in which
>> > > case EFI_LOADER_CODE, EFI_BOOT_SERVICES_CODE and
>> > > EFI_RUNTIEM_SERVICES_CODE should not have the nx policy applied. So it
>> > > seems like your initial suggestion (s/LOADER_DATA/LOADER_CODE/) should
>> > > have worked, even if there was disagreement about whether the spec
>> > > required it to. Is this firmware applying a stricter policy?
>> >
>> > Yes, this firmware is being modified to enforce stricter policy.
>> 
>> Ok. I think this should really go through the UEFI spec process - I
>> agree that from a strict interpretation of the spec, what this 
>> firmware
>> is doing is legitimate, but I don't like having a situation where we
>> have to depend on the DXE spec.
> 
> It's in the process of getting into the UEFI spec now as
> https://bugzilla.tianocore.org/show_bug.cgi?id=3519 .
> 
>> How does Windows handle this? Just update the page tables itself for 
>> any
>> regions it needs during boot?
> 
> Microsoft's bootloader sets up its own pagetables, though I believe
> they're switching it to use the (soon to be) standardized API.

The third version of the patch is the most close in structure
to the proposed protocol. And until the protocol is standardized and
implemented on problematic firmware, I think, it remains the better
solution in terms of simplicity and further porting to the new
protocol.

It is desirable to get the issue resolved, and make the kernel stricter
comply to the spec, without waiting for the new API implementation.
And later, switch the kernel to be using the protocol with
subsequent patches as soon as it gets usable.

So, is there a chance for these patches to be accepted in current
form, or with some modifications?

Thanks,
Baskov Evgeniy

Re: [PATCH RFC v2 0/2] Handle UEFI NX-restricted page tables

[Ard Biesheuvel]

On Thu, 24 Mar 2022 at 17:39, <baskov@ispras.ru> wrote:
>
> On 2022-03-18 19:37, Peter Jones wrote:
> > On Thu, Mar 03, 2022 at 08:47:59PM +0000, Matthew Garrett wrote:
> >> On Thu, Mar 03, 2022 at 04:42:07PM +0300, baskov@ispras.ru wrote:
> >> > On 2022-02-28 21:30, Matthew Garrett wrote:
> >> > > On Mon, Feb 28, 2022 at 05:45:53PM +0100, Ard Biesheuvel wrote:
> >> > >
> >> > > > Given that this is a workaround for a very specific issue arising on
> >> > > > PI based implementations of UEFI, I consider this a quirk, and so I
> >> > > > think this approach is reasonable. I'd still like to gate it on some
> >> > > > kind of identification, though - perhaps something related to DMI like
> >> > > > the x86 core kernel does as well.
> >> > >
> >> > > When the V1 patches were reviewed, you suggested allocating
> >> > > EFI_LOADER_CODE rather than EFI_LOADER_DATA. The example given for a
> >> > > failure case is when NxMemoryProtectionPolicy is set to 0x7fd4, in which
> >> > > case EFI_LOADER_CODE, EFI_BOOT_SERVICES_CODE and
> >> > > EFI_RUNTIEM_SERVICES_CODE should not have the nx policy applied. So it
> >> > > seems like your initial suggestion (s/LOADER_DATA/LOADER_CODE/) should
> >> > > have worked, even if there was disagreement about whether the spec
> >> > > required it to. Is this firmware applying a stricter policy?
> >> >
> >> > Yes, this firmware is being modified to enforce stricter policy.
> >>
> >> Ok. I think this should really go through the UEFI spec process - I
> >> agree that from a strict interpretation of the spec, what this
> >> firmware
> >> is doing is legitimate, but I don't like having a situation where we
> >> have to depend on the DXE spec.
> >
> > It's in the process of getting into the UEFI spec now as
> > https://bugzilla.tianocore.org/show_bug.cgi?id=3519 .
> >
> >> How does Windows handle this? Just update the page tables itself for
> >> any
> >> regions it needs during boot?
> >
> > Microsoft's bootloader sets up its own pagetables, though I believe
> > they're switching it to use the (soon to be) standardized API.
>
> The third version of the patch is the most close in structure
> to the proposed protocol. And until the protocol is standardized and
> implemented on problematic firmware, I think, it remains the better
> solution in terms of simplicity and further porting to the new
> protocol.
>
> It is desirable to get the issue resolved, and make the kernel stricter
> comply to the spec, without waiting for the new API implementation.
> And later, switch the kernel to be using the protocol with
> subsequent patches as soon as it gets usable.
>
> So, is there a chance for these patches to be accepted in current
> form, or with some modifications?
>

I am fine with taking the v3, as it is the most likely to only affect
the systems that actually need this change in behavior.

So unless there are any objections, I will queue these up after the
merge window.

Re: [PATCH RFC v2 0/2] Handle UEFI NX-restricted page tables

[Peter Jones]

On Thu, Mar 24, 2022 at 07:39:47PM +0300, baskov@ispras.ru wrote:
> On 2022-03-18 19:37, Peter Jones wrote:
> > On Thu, Mar 03, 2022 at 08:47:59PM +0000, Matthew Garrett wrote:
> > > On Thu, Mar 03, 2022 at 04:42:07PM +0300, baskov@ispras.ru wrote:
> > > > On 2022-02-28 21:30, Matthew Garrett wrote:
> > > > > On Mon, Feb 28, 2022 at 05:45:53PM +0100, Ard Biesheuvel wrote:
> > > > >
> > > > > > Given that this is a workaround for a very specific issue arising on
> > > > > > PI based implementations of UEFI, I consider this a quirk, and so I
> > > > > > think this approach is reasonable. I'd still like to gate it on some
> > > > > > kind of identification, though - perhaps something related to DMI like
> > > > > > the x86 core kernel does as well.
> > > > >
> > > > > When the V1 patches were reviewed, you suggested allocating
> > > > > EFI_LOADER_CODE rather than EFI_LOADER_DATA. The example given for a
> > > > > failure case is when NxMemoryProtectionPolicy is set to 0x7fd4, in which
> > > > > case EFI_LOADER_CODE, EFI_BOOT_SERVICES_CODE and
> > > > > EFI_RUNTIEM_SERVICES_CODE should not have the nx policy applied. So it
> > > > > seems like your initial suggestion (s/LOADER_DATA/LOADER_CODE/) should
> > > > > have worked, even if there was disagreement about whether the spec
> > > > > required it to. Is this firmware applying a stricter policy?
> > > >
> > > > Yes, this firmware is being modified to enforce stricter policy.
> > > 
> > > Ok. I think this should really go through the UEFI spec process - I
> > > agree that from a strict interpretation of the spec, what this
> > > firmware
> > > is doing is legitimate, but I don't like having a situation where we
> > > have to depend on the DXE spec.
> > 
> > It's in the process of getting into the UEFI spec now as
> > https://bugzilla.tianocore.org/show_bug.cgi?id=3519 .
> > 
> > > How does Windows handle this? Just update the page tables itself for
> > > any
> > > regions it needs during boot?
> > 
> > Microsoft's bootloader sets up its own pagetables, though I believe
> > they're switching it to use the (soon to be) standardized API.
> 
> The third version of the patch is the most close in structure
> to the proposed protocol. And until the protocol is standardized and
> implemented on problematic firmware, I think, it remains the better
> solution in terms of simplicity and further porting to the new
> protocol.

The ECR was approved at last week's meeting, it'll be in the next UEFI
spec.  Details of what spec version that'll be and when it will
officially be released are still under discussion, but it's been
approved in its current form.  Microsoft has been kind enough to provide
us code for test firmware, though the build process is a little rough.

I've done some builds of it here: https://copr.fedorainfracloud.org/coprs/pjones/mu-qemuq35/builds/ .
The src rpm there is a bit absurdly large, because I've done the very
quick-and-dirty hack of just shoving a pile of git repos into it instead
of trying to make release tarballs of everything, and it needs the
network enabled to rebuild it for fairly dumb reasons.  But the result
is a firmware that works in QEMU.

> It is desirable to get the issue resolved, and make the kernel stricter
> comply to the spec, without waiting for the new API implementation.
> And later, switch the kernel to be using the protocol with
> subsequent patches as soon as it gets usable.

It works for the bootloaders in my development trees; I've booted a
kernel with your patches.  From the bootloader POV we do need one more
simple patch to enable the compatibility flag in the headers, I'll send
it as a follow-up to this mail.

-- 
        Peter

[PATCH] x86: Set the NX-compatibility flag in the PE header

[Peter Jones]

Following Baskov Evgeniy's "Handle UEFI NX-restricted page tables"
patches, it's safe to set this compatibility flag to let loaders know
they don't need to make special accommodations for kernel to load if
pre-boot NX is enabled.

Signed-off-by: Peter Jones <pjones@redhat.com>
---
 arch/x86/boot/header.S | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/x86/boot/header.S b/arch/x86/boot/header.S
index 6dbd7e9f74c..0352e4589ef 100644
--- a/arch/x86/boot/header.S
+++ b/arch/x86/boot/header.S
@@ -163,7 +163,11 @@ extra_header_fields:
 	.long	0x200				# SizeOfHeaders
 	.long	0				# CheckSum
 	.word	IMAGE_SUBSYSTEM_EFI_APPLICATION	# Subsystem (EFI application)
+#ifdef CONFIG_DXE_MEM_ATTRIBUTES
+	.word	IMAGE_DLL_CHARACTERISTICS_NX_COMPAT	# DllCharacteristics
+#else
 	.word	0				# DllCharacteristics
+#endif
 #ifdef CONFIG_X86_32
 	.long	0				# SizeOfStackReserve
 	.long	0				# SizeOfStackCommit
-- 
2.35.1

Re: [PATCH] x86: Set the NX-compatibility flag in the PE header

[Ard Biesheuvel]

On Tue, 29 Mar 2022 at 20:47, Peter Jones <pjones@redhat.com> wrote:
>
> Following Baskov Evgeniy's "Handle UEFI NX-restricted page tables"
> patches, it's safe to set this compatibility flag to let loaders know
> they don't need to make special accommodations for kernel to load if
> pre-boot NX is enabled.
>
> Signed-off-by: Peter Jones <pjones@redhat.com>

Thanks, I've queued this up.

> ---
>  arch/x86/boot/header.S | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/arch/x86/boot/header.S b/arch/x86/boot/header.S
> index 6dbd7e9f74c..0352e4589ef 100644
> --- a/arch/x86/boot/header.S
> +++ b/arch/x86/boot/header.S
> @@ -163,7 +163,11 @@ extra_header_fields:
>         .long   0x200                           # SizeOfHeaders
>         .long   0                               # CheckSum
>         .word   IMAGE_SUBSYSTEM_EFI_APPLICATION # Subsystem (EFI application)
> +#ifdef CONFIG_DXE_MEM_ATTRIBUTES
> +       .word   IMAGE_DLL_CHARACTERISTICS_NX_COMPAT     # DllCharacteristics
> +#else
>         .word   0                               # DllCharacteristics
> +#endif
>  #ifdef CONFIG_X86_32
>         .long   0                               # SizeOfStackReserve
>         .long   0                               # SizeOfStackCommit
> --
> 2.35.1
>

Re: [PATCH RFC v2 0/2] Handle UEFI NX-restricted page tables

[Ard Biesheuvel]

On Fri, 25 Mar 2022 at 09:06, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Thu, 24 Mar 2022 at 17:39, <baskov@ispras.ru> wrote:
> >
> > On 2022-03-18 19:37, Peter Jones wrote:
> > > On Thu, Mar 03, 2022 at 08:47:59PM +0000, Matthew Garrett wrote:
> > >> On Thu, Mar 03, 2022 at 04:42:07PM +0300, baskov@ispras.ru wrote:
> > >> > On 2022-02-28 21:30, Matthew Garrett wrote:
> > >> > > On Mon, Feb 28, 2022 at 05:45:53PM +0100, Ard Biesheuvel wrote:
> > >> > >
> > >> > > > Given that this is a workaround for a very specific issue arising on
> > >> > > > PI based implementations of UEFI, I consider this a quirk, and so I
> > >> > > > think this approach is reasonable. I'd still like to gate it on some
> > >> > > > kind of identification, though - perhaps something related to DMI like
> > >> > > > the x86 core kernel does as well.
> > >> > >
> > >> > > When the V1 patches were reviewed, you suggested allocating
> > >> > > EFI_LOADER_CODE rather than EFI_LOADER_DATA. The example given for a
> > >> > > failure case is when NxMemoryProtectionPolicy is set to 0x7fd4, in which
> > >> > > case EFI_LOADER_CODE, EFI_BOOT_SERVICES_CODE and
> > >> > > EFI_RUNTIEM_SERVICES_CODE should not have the nx policy applied. So it
> > >> > > seems like your initial suggestion (s/LOADER_DATA/LOADER_CODE/) should
> > >> > > have worked, even if there was disagreement about whether the spec
> > >> > > required it to. Is this firmware applying a stricter policy?
> > >> >
> > >> > Yes, this firmware is being modified to enforce stricter policy.
> > >>
> > >> Ok. I think this should really go through the UEFI spec process - I
> > >> agree that from a strict interpretation of the spec, what this
> > >> firmware
> > >> is doing is legitimate, but I don't like having a situation where we
> > >> have to depend on the DXE spec.
> > >
> > > It's in the process of getting into the UEFI spec now as
> > > https://bugzilla.tianocore.org/show_bug.cgi?id=3519 .
> > >
> > >> How does Windows handle this? Just update the page tables itself for
> > >> any
> > >> regions it needs during boot?
> > >
> > > Microsoft's bootloader sets up its own pagetables, though I believe
> > > they're switching it to use the (soon to be) standardized API.
> >
> > The third version of the patch is the most close in structure
> > to the proposed protocol. And until the protocol is standardized and
> > implemented on problematic firmware, I think, it remains the better
> > solution in terms of simplicity and further porting to the new
> > protocol.
> >
> > It is desirable to get the issue resolved, and make the kernel stricter
> > comply to the spec, without waiting for the new API implementation.
> > And later, switch the kernel to be using the protocol with
> > subsequent patches as soon as it gets usable.
> >
> > So, is there a chance for these patches to be accepted in current
> > form, or with some modifications?
> >
>
> I am fine with taking the v3, as it is the most likely to only affect
> the systems that actually need this change in behavior.
>
> So unless there are any objections, I will queue these up after the
> merge window.

I have queued these up now.