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.web11.766.1672961833153355694 for ; Thu, 05 Jan 2023 15:37:13 -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 CB3E96080614; Fri, 6 Jan 2023 00:37:10 +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: Fri, 6 Jan 2023 00:37:00 +0100 Message-Id: <9312EFE2-1458-4888-A37E-E1E57DD699B8@csgraf.de> References: Cc: devel@edk2.groups.io, kraxel@redhat.com, dann frazier , Leif Lindholm In-Reply-To: To: Ard Biesheuvel X-Mailer: iPhone Mail (20C65) Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable > Am 05.01.2023 um 12:44 schrieb Ard Biesheuvel : >=20 > =EF=BB=BFOn Thu, 5 Jan 2023 at 12:19, Gerd Hoffmann wr= ote: >>=20 >> Hi, >>=20 >>>> What number would you expect? I'd hope that we get to <100 realisticall= y. >>>>=20 >>>> I'm happy to hear about alternatives to this approach. I'm very confide= nt 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 eventu= ally get bug reports from users that their shiny update regressed some legit= use case. >>>>=20 >>>> 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 fr= om the NX logic. Keep NX for any other non-code memory type as well as Loade= rData pool allocations. >>=20 >>> Another thing we might consider is trapping exec permission violations >>> and switching the pages in question from rw- to r-x. >>=20 >> 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. >>=20 >=20 > That, and a sleep(5) I like the direction this is moving :) >=20 >> 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? >>=20 >>> Does GRUB generally load/map executable modules at page granularity? >>=20 >> 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 ... >>=20 >=20 > OK, so the GRUB PE/COFF loader strikes again :-( >=20 > 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. >=20 > 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), t= hat 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 ma= ke LoaderData behave like LoaderCode plus warning plus sleep. Alex >=20 > Could someone check/confirm whether shim builds need to be take into > account here? Thanks.