On 04.01.23 14:13, Alexander Graf wrote:
On 04.01.23 10:35, Ard Biesheuvel wrote:
On Tue, 3 Jan 2023 at 23:47, dann frazier
<dann.fraz...@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.fraz...@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 :).
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.
Alex
diff --git a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
index 3ad1ecd9d2..365eb1c157 100644
--- a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
+++ b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
@@ -902,6 +902,25 @@ PlatformBootManagerBeforeConsole (
FilterAndProcess (&gEfiPciIoProtocolGuid, IsVirtioPciRng, Connect);
}
+static EFI_ALLOCATE_PAGES RealAllocatePages;
+
+static EFI_STATUS EFIAPI AllocatePagesForceLoaderCode(
+ IN EFI_ALLOCATE_TYPE Type,
+ IN EFI_MEMORY_TYPE MemoryType,
+ IN UINTN Pages,
+ IN OUT EFI_PHYSICAL_ADDRESS *Memory
+)
+{
+ /*
+ * Broken grub versions do LoaderData allocations for code. Let's patch
+ * them into LoaderCode allocations instead.
+ */
+ if (MemoryType == EfiLoaderData)
+ MemoryType = EfiLoaderCode;
+
+ return RealAllocatePages(Type, MemoryType, Pages, Memory);
+}
+
/**
Do the platform specific action after the console is ready
Possible things that can be done in PlatformBootManagerAfterConsole:
@@ -964,6 +983,14 @@ PlatformBootManagerAfterConsole (
SetBootOrderFromQemu ();
PlatformBmPrintScRegisterHandler ();
+
+ /* 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;
+ }
}
/**
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97954): https://edk2.groups.io/g/devel/message/97954
Mute This Topic: https://groups.io/mt/93922691/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
