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

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

> Am 05.01.2023 um 09:11 schrieb Gerd Hoffmann <kraxel@redhat.com>:
> 
>   Hi,
> 
>> To clarify, I mean something like the patch below, but with an additional
>> callback notification similar to the Emu one in LoadImage(), so that we can
>> make sure we only enable the quirk when we load a known-bad grub binary.
>> That way we still force distros to ship fixed versions of their code, but
>> enable old code to continue running.
> 
>> +  /* TODO: Only run this as part of a notify callback in ImageLoad() when
>> we
>> +           load a grub binary with a known-broken hash */
>> +  BOOLEAN is_broken_grub = TRUE;
>> +  if (is_broken_grub) {
>> +    RealAllocatePages = gBS->AllocatePages;
>> +    gBS->AllocatePages = AllocatePagesForceLoaderCode;
>> +  }
> 
> You left out the hard part, which is the list of hashes.

Yes, I'd crowd source that list. If someone has vested interest to keep their old grub binaries working, they can send an upstream patch to add their hash :). At least we'd have a path forward to make things work that is not "revert NX enablement".

>  And I suspect
> you underestimate the number of broken grub binaries in the wild ...

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.


Alex