From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from zulu616.server4you.de (zulu616.server4you.de [85.25.223.15]) by mx.groups.io with SMTP id smtpd.web10.8802.1672908210357505457 for ; Thu, 05 Jan 2023 00:43:31 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: csgraf.de, ip: 85.25.223.15, mailfrom: agraf@csgraf.de) Received: from smtpclient.apple (dynamic-077-009-017-072.77.9.pool.telefonica.de [77.9.17.72]) by csgraf.de (Postfix) with ESMTPSA id A4BD3608065B; Thu, 5 Jan 2023 09:43:27 +0100 (CET) From: "Alexander Graf" Mime-Version: 1.0 (1.0) Subject: Re: [edk2-devel] [PATCH v3 03/16] ArmVirtPkg: make EFI_LOADER_DATA non-executable Date: Thu, 5 Jan 2023 09:43:16 +0100 Message-Id: <3F97900D-3C85-4E10-BBC2-360CECFD2D39@csgraf.de> References: <20230105081127.muckhpcw6agfkfrs@sirius.home.kraxel.org> Cc: Ard Biesheuvel , dann frazier , devel@edk2.groups.io, Leif Lindholm In-Reply-To: <20230105081127.muckhpcw6agfkfrs@sirius.home.kraxel.org> To: Gerd Hoffmann X-Mailer: iPhone Mail (20C65) Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable > Am 05.01.2023 um 09:11 schrieb Gerd Hoffmann : >=20 > =EF=BB=BF Hi, >=20 >> 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 c= an >> 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. >=20 >> + /* TODO: Only run this as part of a notify callback in ImageLoad() whe= n >> we >> + load a grub binary with a known-broken hash */ >> + BOOLEAN is_broken_grub =3D TRUE; >> + if (is_broken_grub) { >> + RealAllocatePages =3D gBS->AllocatePages; >> + gBS->AllocatePages =3D AllocatePagesForceLoaderCode; >> + } >=20 > 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 thei= r old grub binaries working, they can send an upstream patch to add their ha= sh :). At least we'd have a path forward to make things work that is not "re= vert 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 th= at 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 g= et bug reports from users that their shiny update regressed some legit use c= ase. The only alternative I can think of would be logic similar to the patch I se= nt without any grub hash check: Exclude AllocatePages for LoaderData from th= e NX logic. Keep NX for any other non-code memory type as well as LoaderData= pool allocations. Alex