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 <address@hidden> 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