On Thu, 25 Nov 2021 at 08:36, [off-list ref] wrote:

Hello,

I apologize for delayed reply.

No worries.

The system in question runs in a firmware that tries to achieve
complete W^X protection. Both loader code and loader data
are not executable, so the suggested approach does not work.
If you would like to test this, you can set
the PcdDxeNxMemoryProtectionPolicy in any firmware available to you.

The PCD in question has the following note:

# NOTE: User must NOT set NX protection for EfiLoaderCode /
EfiBootServicesCode / EfiRuntimeServicesCode. <BR>

Any idea whether this is easily reproducible with OVMF? Restricting
the loader from creating executable regions seems rather daft, so we
should at least report this, and preferably fix it in EDK2.

As a justification for the approach itself, I can use the fact that
UEFI specification says nothing about the ability to execute
self-allocated EfiLoaderCode or any other types besides the areas
allocated by the firmware for UEFI Images. In fact, Table 7-5
explicitly states that EfiLoaderCode is used for:

quoted

The code portions of a loaded UEFI application.

Fair enough. So EfiLoaderCode is not the right type.

While we do not think it should be interpreted as one cannot allocate
such areas at all, it is clear that there are no guarantees about the
other use cases and permissions of the allocations of this type besides
those stated by 2.3.4:

quoted

Paging mode is enabled and any memory space defined by the UEFI memory
map is identity mapped (virtual address equals physical address),
although the attributes of certain regions may not have all read,
write, and execute attributes or be unmarked for purposes of platform
protection.

Long story short, the kernel is not allowed to allocate such areas and
assume they are executable,

OK

it should do paging itself,

Now you're going too fast. One does not necessarily imply the other.

and the changes
here address that. For the reference, Windows adheres to this convention
and works fine on the target system.

Given that this issue is specific to EDK2 based firmwares, would it be
possible to fix this using DXE services instead? In particular, could
we use

gDS->SetMemorySpaceAttributes()

to ensure that the regions have executable permissions?

On 2021-11-10 14:11, Ard Biesheuvel wrote:

quoted

On Wed, 10 Nov 2021 at 11:56, Baskov Evgeniy [off-list ref] wrote:

quoted

Note, that this patch series is RFC, since it is yet untested
and possibly incompatible with AMD SEV and related extensions.

The UEFI specification states that certain memory regions may
not have every permission, i.e. may not be writable or executable.

Furthermore there exist some implementations (at least on i386/x86_64)
that restrict execution of memory regions expected by the kernel to
be executable. E.g. first megabyte of address space, where trampoline
for switching between 4/5 level paging is placed and memory regions,
allocated as loader data.

This patch series allows Linux kernel to boot on such UEFI
implementations on i386 and x86_64.

The simplest way to achieve that on i386 is to disable paging
before jumping to potentially relocated code.

x86_64, on the other hand, does not allow disabling paging so it
is required to build temporary page tables containing memory regions
required for Linux kernel to boot with appropriate access permissions.

Hello Baskov,

To be honest, I am truly not a fan of this approach.

Which systems is this issue occurring on? Did you try something like
the below to allocate executable memory explicitly?

diff --git a/drivers/firmware/efi/libstub/relocate.c

b/drivers/firmware/efi/libstub/relocate.c
index 8ee9eb2b9039..b73012a7bcdc 100644

--- a/drivers/firmware/efi/libstub/relocate.c
+++ b/drivers/firmware/efi/libstub/relocate.c

@@ -80,7 +80,7 @@ efi_status_t efi_low_alloc_above(unsigned long size,

unsigned long align,
                        continue;

                status = efi_bs_call(allocate_pages,
EFI_ALLOCATE_ADDRESS,
-                                    EFI_LOADER_DATA, nr_pages,
&start);
+                                    EFI_LOADER_CODE, nr_pages,
&start);
                if (status == EFI_SUCCESS) {
                        *addr = start;
                        break;

@@ -146,7 +146,7 @@ efi_status_t efi_relocate_kernel(unsigned long

*image_addr,
         */
        nr_pages = round_up(alloc_size, EFI_ALLOC_ALIGN) /
EFI_PAGE_SIZE;
        status = efi_bs_call(allocate_pages, EFI_ALLOCATE_ADDRESS,
-                            EFI_LOADER_DATA, nr_pages, &efi_addr);
+                            EFI_LOADER_CODE, nr_pages, &efi_addr);
        new_addr = efi_addr;
        /*
         * If preferred address allocation failed allocate as low as