Re: [edk2-devel] [PATCH v3 03/16] ArmVirtPkg: make EFI_LOADER_DATA non-executable

["Ard Biesheuvel" <ardb@kernel.org>]

On Thu, 5 Jan 2023 at 12:19, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>   Hi,
>
> > > What number would you expect? I'd hope that we get to <100 realistically.
> > >
> > > I'm happy to hear about alternatives to this approach. I'm very confident that forcing NX on always is going to have the opposite effect of what we want: Everyone who ships AAVMF binaries will disable NX because they eventually get bug reports from users that their shiny update regressed some legit use case.
> > >
> > > The only alternative I can think of would be logic similar to the patch I sent without any grub hash check: Exclude AllocatePages for LoaderData from the NX logic. Keep NX for any other non-code memory type as well as LoaderData pool allocations.
>
> > Another thing we might consider is trapping exec permission violations
> > and switching the pages in question from rw- to r-x.
>
> That sounds neat, especially as we can print a big'n'fat warning in that
> case, so the problem gets attention without actually breaking users.
>

That, and a sleep(5)

> Looking at the efi calls it looks like edk2 doesn't track the owner of
> an allocation (say by image handle), so I suspect it is not possible to
> automatically figure who is to blame?
>
> > Does GRUB generally load/map executable modules at page granularity?
>
> I don't think so, at least the code handles modules not being page
> aligned.  But I think it's not grub modules, that fix was actually
> picked up meanwhile.  But there are downstream patches for image
> loader code which look suspicious to me ...
>

OK, so the GRUB PE/COFF loader strikes again :-(

So shim may be broken in the exact same way, and the things shim loads
may not adhere to page alignment for the sections. Loading the kernel
itself this way should be fine, though - we always had section
alignment in the EFI stub kernel.

The downside of this approach is that it can only be done on a
page-by-page basis, given that the EFI_LOADER_DATA region in question
will cover both the .text/.rodata and the .data/.bss of the loaded
image.

Could someone check/confirm whether shim builds need to be take into
account here? Thanks.