[RFC] arm64/linux/loader: Use EFI CODE allocations for the linux kernel

[RFC] arm64/linux/loader: Use EFI CODE allocations for the linux kernel

[Jeffrey Hugo]

Some UEFI implementations for ARM64 devices apply strict permissions on
the different allocation types.  In these implementations, DATA
allocations have XN (execute never) permissions, preventing code execution
from those pages.

On these implementations, the Linux kernel is loaded to DATA pages, which
causes a permission fault when GRUB attempts to kick off the kernel.  This
results in a device crash.

Fix this by allocating CODE pages for the Linux kernel.

Signed-off-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com>
---
 grub-core/kern/efi/mm.c        | 8 ++++++++
 grub-core/loader/arm64/linux.c | 2 +-
 include/grub/efi/efi.h         | 2 ++
 3 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
index b02fab1b1..725695e01 100644
--- a/grub-core/kern/efi/mm.c
+++ b/grub-core/kern/efi/mm.c
@@ -156,6 +156,14 @@ grub_efi_allocate_any_pages (grub_efi_uintn_t pages)
 				       GRUB_EFI_LOADER_DATA);
 }
 
+void *
+grub_efi_allocate_code_pages (grub_efi_uintn_t pages)
+{
+  return grub_efi_allocate_pages_real (GRUB_EFI_MAX_USABLE_ADDRESS,
+				       pages, GRUB_EFI_ALLOCATE_MAX_ADDRESS,
+				       GRUB_EFI_LOADER_CODE);
+}
+
 void *
 grub_efi_allocate_fixed (grub_efi_physical_address_t address,
 			 grub_efi_uintn_t pages)
diff --git a/grub-core/loader/arm64/linux.c b/grub-core/loader/arm64/linux.c
index ef3e9f944..877839072 100644
--- a/grub-core/loader/arm64/linux.c
+++ b/grub-core/loader/arm64/linux.c
@@ -313,7 +313,7 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
   grub_loader_unset();
 
   grub_dprintf ("linux", "kernel file size: %lld\n", (long long) kernel_size);
-  kernel_addr = grub_efi_allocate_any_pages (GRUB_EFI_BYTES_TO_PAGES (kernel_size));
+  kernel_addr = grub_efi_allocate_code_pages (GRUB_EFI_BYTES_TO_PAGES (kernel_size));
   grub_dprintf ("linux", "kernel numpages: %lld\n",
 		(long long) GRUB_EFI_BYTES_TO_PAGES (kernel_size));
   if (!kernel_addr)
diff --git a/include/grub/efi/efi.h b/include/grub/efi/efi.h
index e90e00dc4..697cabcd6 100644
--- a/include/grub/efi/efi.h
+++ b/include/grub/efi/efi.h
@@ -47,6 +47,8 @@ EXPORT_FUNC(grub_efi_allocate_fixed) (grub_efi_physical_address_t address,
 				      grub_efi_uintn_t pages);
 void *
 EXPORT_FUNC(grub_efi_allocate_any_pages) (grub_efi_uintn_t pages);
+void *
+EXPORT_FUNC(grub_efi_allocate_code_pages) (grub_efi_uintn_t pages);
 void EXPORT_FUNC(grub_efi_free_pages) (grub_efi_physical_address_t address,
 				       grub_efi_uintn_t pages);
 grub_efi_uintn_t EXPORT_FUNC(grub_efi_find_mmap_size) (void);
-- 
2.17.1

Re: [RFC] arm64/linux/loader: Use EFI CODE allocations for the linux kernel

[Daniel Kiper]

On Thu, Apr 04, 2019 at 07:54:55AM -0700, Jeffrey Hugo wrote:
> Some UEFI implementations for ARM64 devices apply strict permissions on
> the different allocation types.  In these implementations, DATA
> allocations have XN (execute never) permissions, preventing code execution
> from those pages.
>
> On these implementations, the Linux kernel is loaded to DATA pages, which
> causes a permission fault when GRUB attempts to kick off the kernel.  This
> results in a device crash.
>
> Fix this by allocating CODE pages for the Linux kernel.
>
> Signed-off-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com>

Make sense for me but I would like to hear Leif's opinion too. I treat
this a fix and if he is OK with it I am happy to take it into release.

Daniel

Re: [RFC] arm64/linux/loader: Use EFI CODE allocations for the linux kernel

[Leif Lindholm]

On Thu, Apr 04, 2019 at 06:57:29PM +0200, Daniel Kiper wrote:
> On Thu, Apr 04, 2019 at 07:54:55AM -0700, Jeffrey Hugo wrote:
> > Some UEFI implementations for ARM64 devices apply strict permissions on
> > the different allocation types.  In these implementations, DATA
> > allocations have XN (execute never) permissions, preventing code execution
> > from those pages.
> >
> > On these implementations, the Linux kernel is loaded to DATA pages, which
> > causes a permission fault when GRUB attempts to kick off the kernel.  This
> > results in a device crash.
> >
> > Fix this by allocating CODE pages for the Linux kernel.
> >
> > Signed-off-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com>
> 
> Make sense for me but I would like to hear Leif's opinion too. I treat
> this a fix and if he is OK with it I am happy to take it into release.

This complements f826330683675f0deb55b58fd229afd7d65fb053
("efi: change heap allocation type to GRUB_EFI_LOADER_CODE"), so I'm
all for it.

Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

This does bring to mind the clunkiness of the above. Marking
*everything* executable bypasses the improved security provided by the
firmware. Should I register a bug on Savannah to address this?
(blatantly not for the upcoming release)

/
     Leif

Re: [RFC] arm64/linux/loader: Use EFI CODE allocations for the linux kernel

[Daniel Kiper]

On Fri, Apr 05, 2019 at 04:06:57AM +0100, Leif Lindholm wrote:
> On Thu, Apr 04, 2019 at 06:57:29PM +0200, Daniel Kiper wrote:
> > On Thu, Apr 04, 2019 at 07:54:55AM -0700, Jeffrey Hugo wrote:
> > > Some UEFI implementations for ARM64 devices apply strict permissions on
> > > the different allocation types.  In these implementations, DATA
> > > allocations have XN (execute never) permissions, preventing code execution
> > > from those pages.
> > >
> > > On these implementations, the Linux kernel is loaded to DATA pages, which
> > > causes a permission fault when GRUB attempts to kick off the kernel.  This
> > > results in a device crash.
> > >
> > > Fix this by allocating CODE pages for the Linux kernel.
> > >
> > > Signed-off-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com>
> >
> > Make sense for me but I would like to hear Leif's opinion too. I treat
> > this a fix and if he is OK with it I am happy to take it into release.
>
> This complements f826330683675f0deb55b58fd229afd7d65fb053
> ("efi: change heap allocation type to GRUB_EFI_LOADER_CODE"), so I'm
> all for it.
>
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

Thanks!

> This does bring to mind the clunkiness of the above. Marking
> *everything* executable bypasses the improved security provided by the

Yeah, I agree. However, AIUI this issue is not ARM specific and also
applies to other UEFI platforms.

> firmware. Should I register a bug on Savannah to address this?
> (blatantly not for the upcoming release)

Please do.

Daniel

Re: [RFC] arm64/linux/loader: Use EFI CODE allocations for the linux kernel

[Alexander Graf]

On 05.04.19 06:06, Leif Lindholm wrote:
> On Thu, Apr 04, 2019 at 06:57:29PM +0200, Daniel Kiper wrote:
>> On Thu, Apr 04, 2019 at 07:54:55AM -0700, Jeffrey Hugo wrote:
>>> Some UEFI implementations for ARM64 devices apply strict permissions on
>>> the different allocation types.  In these implementations, DATA
>>> allocations have XN (execute never) permissions, preventing code execution
>>> from those pages.
>>>
>>> On these implementations, the Linux kernel is loaded to DATA pages, which
>>> causes a permission fault when GRUB attempts to kick off the kernel.  This
>>> results in a device crash.
>>>
>>> Fix this by allocating CODE pages for the Linux kernel.
>>>
>>> Signed-off-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com>
>> Make sense for me but I would like to hear Leif's opinion too. I treat
>> this a fix and if he is OK with it I am happy to take it into release.
> This complements f826330683675f0deb55b58fd229afd7d65fb053
> ("efi: change heap allocation type to GRUB_EFI_LOADER_CODE"), so I'm
> all for it.
>
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
>
> This does bring to mind the clunkiness of the above. Marking
> *everything* executable bypasses the improved security provided by the
> firmware. Should I register a bug on Savannah to address this?
> (blatantly not for the upcoming release)


I quite frankly don't understand why we need to mark the PE binary as
CODE in the first place. I thought the whole point of invoking the UEFI
loader protocol was to ensure that the placement of sections from that
binary into CODE/DATA happens properly?

Or are we not calling LoadImage?


Alex

Re: [RFC] arm64/linux/loader: Use EFI CODE allocations for the linux kernel

[Leif Lindholm]

On Mon, Apr 08, 2019 at 12:19:05PM +0300, Alexander Graf wrote:
> On 05.04.19 06:06, Leif Lindholm wrote:
> > This does bring to mind the clunkiness of the above. Marking
> > *everything* executable bypasses the improved security provided by the
> > firmware. Should I register a bug on Savannah to address this?
> > (blatantly not for the upcoming release)
> 
> I quite frankly don't understand why we need to mark the PE binary as
> CODE in the first place. I thought the whole point of invoking the UEFI
> loader protocol was to ensure that the placement of sections from that
> binary into CODE/DATA happens properly?

You have a point, but I don't think Jeffrey found this through code
review.

It is possible that my belt-and-braces approach of both adding a
memory mapped device path and setting SourceBuffer breaks assumptions
in the UEFI implementation.

Jeffrey - could you try changing
  status = b->load_image (0, grub_efi_image_handle,
                          (grub_efi_device_path_t *) mempath,
                          (void *) addr,
                          size, &image_handle);
to
  status = b->load_image (0, grub_efi_image_handle,
                          NULL,
                          (void *) addr,
                          size, &image_handle);
and see if that makes the problem go away without changing the
allocation type?

> Or are we not calling LoadImage?

We are.

/
    Leif

Re: [RFC] arm64/linux/loader: Use EFI CODE allocations for the linux kernel

[Jeffrey Hugo]

On Mon, Apr 8, 2019 at 3:56 AM Leif Lindholm <leif.lindholm@linaro.org> wrote:
>
> On Mon, Apr 08, 2019 at 12:19:05PM +0300, Alexander Graf wrote:
> > On 05.04.19 06:06, Leif Lindholm wrote:
> > > This does bring to mind the clunkiness of the above. Marking
> > > *everything* executable bypasses the improved security provided by the
> > > firmware. Should I register a bug on Savannah to address this?
> > > (blatantly not for the upcoming release)
> >
> > I quite frankly don't understand why we need to mark the PE binary as
> > CODE in the first place. I thought the whole point of invoking the UEFI
> > loader protocol was to ensure that the placement of sections from that
> > binary into CODE/DATA happens properly?
>
> You have a point, but I don't think Jeffrey found this through code
> review.
>
> It is possible that my belt-and-braces approach of both adding a
> memory mapped device path and setting SourceBuffer breaks assumptions
> in the UEFI implementation.
>
> Jeffrey - could you try changing
>   status = b->load_image (0, grub_efi_image_handle,
>                           (grub_efi_device_path_t *) mempath,
>                           (void *) addr,
>                           size, &image_handle);
> to
>   status = b->load_image (0, grub_efi_image_handle,
>                           NULL,
>                           (void *) addr,
>                           size, &image_handle);
> and see if that makes the problem go away without changing the
> allocation type?

I'll give that a shot and report back.

>
> > Or are we not calling LoadImage?
>
> We are.
>
> /
>     Leif

Re: [RFC] arm64/linux/loader: Use EFI CODE allocations for the linux kernel

[Jeffrey Hugo]

On Mon, Apr 8, 2019 at 7:47 AM Jeffrey Hugo <jeffrey.l.hugo@gmail.com> wrote:
>
> On Mon, Apr 8, 2019 at 3:56 AM Leif Lindholm <leif.lindholm@linaro.org> wrote:
> >
> > On Mon, Apr 08, 2019 at 12:19:05PM +0300, Alexander Graf wrote:
> > > On 05.04.19 06:06, Leif Lindholm wrote:
> > > > This does bring to mind the clunkiness of the above. Marking
> > > > *everything* executable bypasses the improved security provided by the
> > > > firmware. Should I register a bug on Savannah to address this?
> > > > (blatantly not for the upcoming release)
> > >
> > > I quite frankly don't understand why we need to mark the PE binary as
> > > CODE in the first place. I thought the whole point of invoking the UEFI
> > > loader protocol was to ensure that the placement of sections from that
> > > binary into CODE/DATA happens properly?
> >
> > You have a point, but I don't think Jeffrey found this through code
> > review.
> >
> > It is possible that my belt-and-braces approach of both adding a
> > memory mapped device path and setting SourceBuffer breaks assumptions
> > in the UEFI implementation.
> >
> > Jeffrey - could you try changing
> >   status = b->load_image (0, grub_efi_image_handle,
> >                           (grub_efi_device_path_t *) mempath,
> >                           (void *) addr,
> >                           size, &image_handle);
> > to
> >   status = b->load_image (0, grub_efi_image_handle,
> >                           NULL,
> >                           (void *) addr,
> >                           size, &image_handle);
> > and see if that makes the problem go away without changing the
> > allocation type?
>
> I'll give that a shot and report back.

Hmm.  I apparently wasn't very methodical about my previous testing.
Mainline GRUB doesn't appear to have the original issue, thus my fix
would appear to be not needed.
However, downstream distro GRUB appears to be affected (I was
originally attempting to use Ubuntu).

I suspect this change, which appears to remove the load_image call
while adding Secure Boot support -
https://git.launchpad.net/~ubuntu-core-dev/grub/+git/ubuntu/commit/grub-core/loader/arm64/linux.c?h=ubuntu&id=98bc4f52034abb18e7bd028d424bde15375c14af

Sorry for the noise, although I would appreciate some pointers on how
this should be addressed.  Should I go file bugs against the distros?

>
> >
> > > Or are we not calling LoadImage?
> >
> > We are.
> >
> > /
> >     Leif

Re: [RFC] arm64/linux/loader: Use EFI CODE allocations for the linux kernel

[Ard Biesheuvel]

On Mon, 8 Apr 2019 at 02:56, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>
> On Mon, Apr 08, 2019 at 12:19:05PM +0300, Alexander Graf wrote:
> > On 05.04.19 06:06, Leif Lindholm wrote:
> > > This does bring to mind the clunkiness of the above. Marking
> > > *everything* executable bypasses the improved security provided by the
> > > firmware. Should I register a bug on Savannah to address this?
> > > (blatantly not for the upcoming release)
> >
> > I quite frankly don't understand why we need to mark the PE binary as
> > CODE in the first place. I thought the whole point of invoking the UEFI
> > loader protocol was to ensure that the placement of sections from that
> > binary into CODE/DATA happens properly?
>
> You have a point, but I don't think Jeffrey found this through code
> review.
>

Whether a region is R-X, RW- or RWX and whether a region is carved out
of Efi*Code or Efi*Data memory are two different things. The sections
of a PE/COFF image need to be contiguous in memory, and you cannot
expect the heap allocator to return contiguous regions for separate
Efi*Code and Efi*Data allocations, so the memory holding a PE/COFF
image should be allocated in a single call.

So usually, we allocate Efi*Code regions for executable images (wbich
are guaranteed to be RWX when created) and rely on the
SetMemoryAttributes() DXE services to remove either the W or the X bit
depending on the nature of the section. This obviously leaves a small
window where RWX memory exists, but this is the best we can do without
violating the UEFI spec.

> It is possible that my belt-and-braces approach of both adding a
> memory mapped device path and setting SourceBuffer breaks assumptions
> in the UEFI implementation.
>
> Jeffrey - could you try changing
>   status = b->load_image (0, grub_efi_image_handle,
>                           (grub_efi_device_path_t *) mempath,
>                           (void *) addr,
>                           size, &image_handle);
> to
>   status = b->load_image (0, grub_efi_image_handle,
>                           NULL,
>                           (void *) addr,
>                           size, &image_handle);
> and see if that makes the problem go away without changing the
> allocation type?
>
> > Or are we not calling LoadImage?
>
> We are.
>
> /
>     Leif