arm64: Use proper memory type for kernel allocation #101

qzed · 2022-06-28T21:29:36Z

Currently, the kernel pages are allocated with type EFI_LOADER_DATA.
While the vast majority of systems will happily execute code from those
pages (i.e. don't care about memory protection), the Microsoft Surface
Pro X stalls, as this memory is not designated as "executable".

Therefore, allocate the kernel pages as EFI_LOADER_CODE to request
memory that is actually executable.

Signed-off-by: Maximilian Luz luzmaximilian@gmail.com

frozencemetery

Awesome, thank you for testing and tracking that down!

martinezjavier · 2022-06-28T21:42:31Z

grub-core/loader/arm64/linux.c

@@ -389,7 +391,10 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
  grub_loader_unset();

  kernel_alloc_pages = GRUB_EFI_BYTES_TO_PAGES (kernel_size + align - 1);
-  kernel_alloc_addr = grub_efi_allocate_any_pages (kernel_alloc_pages);
+  kernel_alloc_addr = grub_efi_allocate_pages_real (GRUB_EFI_MAX_USABLE_ADDRESS,


I wonder if isn't better to instead make grub_efi_allocate_any_pages() to use GRUB_EFI_LOADER_CODE rather than open coding it the params that use it to call grub_efi_allocate_pages_real().

Since AFAICT is only used to allocate memory for the kernel image anyways, the memory for the initrd (that's data and not code) is already allocated using a different helper allocate_initrd_mem() so that can continue to use GRUB_EFI_LOADER_DATA.

Also, by changing grub_efi_allocate_any_pages() you will fix this not only for arm64 but also for x86.

It seems that it's used in grub_efi_mm_init() to allocate memory maps. How about a helper (perhaps grub_efi_allocate_executable_pages() or grub_efi_allocate_kernel_pages())?

I see, probably grub_efi_allocate_kernel_pages() then to be consistent with how the memory for the initrd is allocated ?

i386 already has 8a2d168 (although that uses EFI_RUNTIME_SERVICES_CODE instead). I assume that's also used for x86_64?

To me, the EFI stub of the kernel seems more like a UEFI application than a runtime driver, but again, I don't know much about this stuff.

Yes, I agree with you here.

Fwiw: chainloader also seems to use GRUB_EFI_LOADER_CODE:

grub2/grub-core/loader/efi/chainloader.c

Lines 1052 to 1054 in b1ce9c4

status = efi_call_4 (b->allocate_pages, GRUB_EFI_ALLOCATE_ANY_PAGES,

GRUB_EFI_LOADER_CODE,

pages, &address);

It seems that it's used in grub_efi_mm_init() to allocate memory maps. How about a helper (perhaps grub_efi_allocate_executable_pages() or grub_efi_allocate_kernel_pages())?

Probably just move kernel_alloc() from the i386 loader to some common code and make the policy list be passed in?

That seems to depend on some interaction with the max_addresses field, where parts of it are read in from the i386/x64 kernel header. I'm not sure how to adapt this for AArch64, but if you have some patches I'd be happy to test.

Probably just move kernel_alloc() from the i386 loader to some common code and make the policy list be passed in?

So after having another look at this, the max_addresses thing shouldn't be an issue. As you've said, this could just be passed in. However, I think using kernel_alloc() here doesn't make that much sense to do this in this PR:

We could use it for the kernel image + command-line memory, passing in a policy array with a single entry. But given that it only uses a single entry and none of the re-try logic that's used on x86, this seems a bit too convoluted to me. Also, it's orthogonal to the change to GRUB_EFI_LOADER_CODE, because that is still passed in to kernel_alloc(). Further, adapting it for the initrd would need some changes (or workarounds) as the arm64 code relies on grub_efi_get_ram_base() whereas the x86 code doesn't.

ausil · 2023-03-20T18:41:11Z

Could we get this PR down to the one relevant patch for discussion, please?

qzed · 2023-05-08T11:59:44Z

Sorry for the long silence. I will try to revisit this in the next couple of days.

Currently, the kernel pages are allocated with type EFI_LOADER_DATA. While the vast majority of systems will happily execute code from those pages (i.e. don't care about memory protection), the Microsoft Surface Pro X stalls, as this memory is not designated as "executable". Therefore, allocate the kernel pages as EFI_LOADER_CODE to request memory that is actually executable. Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>

jlinton · 2023-07-31T17:53:34Z

The current version of this code also fixes the lenovo x13s, and presumably other machines based on the QC SoC/firmware. It also matches a bit of debugging/hacking I did last week before being pointed at this PR. My only reservations are that maybe it should only be done on machines that don't have the EFI_MEMORY_ATTRIBUTE_PROTOCOL. I have a slight worry there are some old/unsupported machines that have +Write turned off for EfiLoaderCode pages, but at the moment we have a known set of broken machines without this change.

Is there a more concrete problem with it?

nfrayer

lgtm

frozencemetery approved these changes Jun 28, 2022

View reviewed changes

martinezjavier reviewed Jun 28, 2022

View reviewed changes

qzed mentioned this pull request Jun 29, 2022

Some GRUB versions do not work linux-surface/surface-pro-x#12

Open

frozencemetery changed the base branch from fedora-37 to fedora-38 August 16, 2022 19:48

frozencemetery force-pushed the fedora-38 branch from b8e58aa to b90ccf8 Compare August 30, 2022 15:00

frozencemetery force-pushed the fedora-38 branch from b90ccf8 to 4ba911f Compare September 8, 2022 20:01

frozencemetery force-pushed the fedora-38 branch from cad2fc1 to ecf3d74 Compare November 1, 2022 19:55

frozencemetery force-pushed the fedora-38 branch from b77167c to 9e97e00 Compare December 6, 2022 15:58

frozencemetery force-pushed the fedora-38 branch 2 times, most recently from 178d2c4 to 8a96f14 Compare January 31, 2023 21:18

frozencemetery force-pushed the fedora-38 branch from 199a61d to 7f8daed Compare February 8, 2023 20:53

qzed force-pushed the arm64-efi-loader-code branch from 55b95a3 to 71402fa Compare May 15, 2023 14:34

qzed changed the base branch from fedora-38 to fedora-39 May 15, 2023 14:34

nfrayer self-requested a review August 29, 2023 10:37

nfrayer approved these changes Aug 29, 2023

View reviewed changes

nfrayer merged commit 4f9d3f4 into rhboot:fedora-39 Aug 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

arm64: Use proper memory type for kernel allocation #101

arm64: Use proper memory type for kernel allocation #101

qzed commented Jun 28, 2022

frozencemetery left a comment

martinezjavier Jun 28, 2022

martinezjavier Jun 28, 2022

qzed Jun 28, 2022

martinezjavier Jun 28, 2022

qzed Jun 28, 2022

martinezjavier Jun 28, 2022

qzed Jun 28, 2022 •

edited

vathpela Aug 2, 2022

qzed Aug 2, 2022

qzed May 17, 2023

ausil commented Mar 20, 2023

qzed commented May 8, 2023

jlinton commented Jul 31, 2023

nfrayer left a comment

	status = efi_call_4 (b->allocate_pages, GRUB_EFI_ALLOCATE_ANY_PAGES,
	GRUB_EFI_LOADER_CODE,
	pages, &address);

arm64: Use proper memory type for kernel allocation #101

arm64: Use proper memory type for kernel allocation #101

Conversation

qzed commented Jun 28, 2022

frozencemetery left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qzed Jun 28, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ausil commented Mar 20, 2023

qzed commented May 8, 2023

jlinton commented Jul 31, 2023

nfrayer left a comment

Choose a reason for hiding this comment

qzed Jun 28, 2022 •

edited