[edk2-devel] [PATCH v3 03/16] ArmVirtPkg: make EFI_LOADER_DATA non-executable
Alexander Graf
agraf at csgraf.de
Thu Jan 5 23:37:00 UTC 2023
> Am 05.01.2023 um 12:44 schrieb Ard Biesheuvel <ardb at kernel.org>:
>
> On Thu, 5 Jan 2023 at 12:19, Gerd Hoffmann <kraxel at 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.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98038): https://edk2.groups.io/g/devel/message/98038
Mute This Topic: https://groups.io/mt/93922691/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list