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

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

On 04.01.23 10:35, Ard Biesheuvel wrote:
> On Tue, 3 Jan 2023 at 23:47, dann frazier <dann.frazier@canonical.com> wrote:
>> On Tue, Jan 03, 2023 at 08:39:24PM +0100, Alexander Graf wrote:
>>> Hey Ard,
>>>
>>> On 03.01.23 10:59, Ard Biesheuvel wrote:
>>>> On Thu, 29 Dec 2022 at 19:00, dann frazier <dann.frazier@canonical.com> wrote:
>>>>> On Mon, Nov 28, 2022 at 04:46:10PM +0100, Gerd Hoffmann wrote:
>>>>>> On Mon, Sep 26, 2022 at 10:24:58AM +0200, Ard Biesheuvel wrote:
>>>>>>> When the memory protections were implemented and enabled on ArmVirtQemu
>>>>>>> 5+ years ago, we had to work around the fact that GRUB at the time
>>>>>>> expected EFI_LOADER_DATA to be executable, as that is the memory type it
>>>>>>> allocates when loading its modules.
>>>>>>>
>>>>>>> This has been fixed in GRUB in August 2017, so by now, we should be able
>>>>>>> to tighten this, and remove execute permissions from EFI_LOADER_DATA
>>>>>>> allocations.
>>>>>> Data point: https://bugzilla.redhat.com/show_bug.cgi?id=2149020
>>>>>> tl;dr: fedora 37 grub.efi is still broken.
>>>>> This is also the case with existing Ubuntu releases, as well as
>>>>> AlmaLinux 9.1 and RHEL 8.7[*]. While it does appear to be fixed for
>>>>> the upcoming Ubuntu 23.04 (presumably via [**]), I plan to revert this
>>>>> patch in Debian/Ubuntu until it is more ubiquitous. Do you want to do
>>>>> the same upstream? I'm not sure at what point it would make sense to
>>>>> reintroduce it, given we can't force users to upgrade their bootloaders.
>>>>>
>>>> Thanks for the report.
>>>>
>>>> You can override PCDs on the build command line, so I suggest you use
>>>> that for building these images as long as it is needed.
>>>>
>>>> E.g,, append this to the build.sh command line
>>>>
>>>> --pcd PcdDxeNxMemoryProtectionPolicy=0xC000000000007FD1
>>>>
>>>> to undo the effects of this patch.
>>>>
>>>> I do not intend to revert this patch - the trend under EFI is towards
>>>> much stricter memory permissions, also on the MS side, and this is
>>>> especially important under CC scenarios. And if 5+ years is not
>>>> sufficient for out-of-tree GRUB to catch up, what is the point of
>>>> waiting for it?
>>>
>>> I think we need to be smarter here. ArmVirtPkg is used by a lot of
>>> virtualization software - such as QEMU. Typically, users (and developers)
>>> expect that an update to a newer version will preserve compatibility.
>>>
>>> Let me contrive an example: In 1 year, QEMU updates to the latest AAVMF. It
>>> ships that as part of its pc-bios directory. Suddenly, we accidentally break
>>> old (immutable!) iso images that used to work. So someone that updates QEMU
>>> to a newer version will have a regression in running a Fedora 37
>>> installation. Or RHEL 8.7 installation. Not good :).
>>>
>>> Outside of OSVs providing new iso images, there is also not much you can do
>>> about this. The grub binary here runs way before any updater code that could
>>> pull a fixed version from the internet.
>>>
>>> What alternatives do we have? Assuming grub is the only offender we have to
>>> worry about, is there a way we can identify that the allocation came from an
>>> unpatched version? Or have a database of hashes (with all known grub
>>> binaries that have this bug in existing isos) that would force disable NX
>>> protection for it if we hit it? Or disable NX when Secure Boot is disabled?
>>>
>>> I really think we need to be a bit more creative than make NX mandatory in
>>> all cases when we know the are immutable images out there that won't work
>>> with it, otherwise we break very legit use cases.
>> While it has its own issues, I suppose another way to go about it is
>> for distributors to provide multiple AAVMF_CODE images, and perhaps
>> invent a "feature" flag for the json descriptor for management tools
>> to select an appropriate one.
>>
> I don't think having different versions of the image makes sense, tbh,
> but of course, this is up to the distros.
>
> Compatibility with ancient downstream GRUB builds is not a goal of the
> EDK2 upstream, so as long as distros can tweak the build to their
> needs, I don't see a reason to revert this change upstream.


First of all, I don't think we should revert this change. We should 
augment it to give users the out-of-the-box experience they expect.

On top of that, I don't think it's a problem of "distros". Every 
consumer of AAVMF will run into this problem as soon as their users will 
want to run any Red Hat OS (installer / image) all the way into 2022. 
That's  very likely >90% of the user base. Because of that, I'm pretty 
sure no Cloud vendor will be able to enable NX in its current shape for 
example.

I'm very happy to see NX proliferate through the stack, but let's please 
make sure we do it compatibly by default, otherwise the net result is 
that *everyone* who compiles AAVMF will disable NX by default again - 
and all you end up with is more frustration and more downstream code / 
forks.

IMHO the most obvious approach would be a fingerprint based override. 
There should be a finite number of known broken grub binaries. If we 
just maintain a database with them and then apply some magic when we 
detect such a binary gets loaded, we'll have tightened security by 
default, without breaking backwards compat.

For environments that know they're running in environments with CC 
requirements, we can automatically disable the fingerprint override :).


Alex