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

["Alexander Graf" <agraf@csgraf.de>]

> Am 05.01.2023 um 12:44 schrieb 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)

I like the direction this is moving :)

> 
>> 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.

Does it have to be r-x instead of rwx? If we add the warning and sleep(5), that should hopefully give enough incentive already to OSVs to fix their grub binaries :). And then we don't even need to think about alignment.

If I map a region as LoaderCode, I get rwx as well, no? So we effectively make LoaderData behave like LoaderCode plus warning plus sleep.


Alex

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