From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from ams.source.kernel.org (ams.source.kernel.org [145.40.68.75]) by mx.groups.io with SMTP id smtpd.web10.9472.1672911735450631017 for ; Thu, 05 Jan 2023 01:42:15 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=MVR5aWAJ; spf=pass (domain: kernel.org, ip: 145.40.68.75, mailfrom: ardb@kernel.org) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id 91E48B8198D for ; Thu, 5 Jan 2023 09:42:12 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 564C5C43392 for ; Thu, 5 Jan 2023 09:42:11 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1672911731; bh=jk0AExprNG7YpJuJ/uJKspxcbSjhCxiTxqSM9jWIAgw=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=MVR5aWAJeddvzlApWYE+OZ+8WEvZvAWw0IOIqIPP6dReI9dyznUXByUcFeLv7po7R kdRe3xDsUr/1w8LZJs6s6d2d+XACTpfAJAzSHFoBo4pKcJ1BmruvAeL+zeDrkcaLVh 629gQNp9tkFR5rCqaCOfbE+8IeVAtaWoT1X4axDcWJ5TSDOYgP3GDvXWOFhsm/ZRHr JH1ix+VbRLCQjGaFq08EyO+BLBLzCMxRSOmGfDsX1VKqTeeUtFKU334B3XHazxXvhd nzzT+C9Sz6sAHg9+rb7P/IkOSuBPMXzQPwBiVB2aUo9UYYBweofLIL3IdwRguQXakQ s4sIJMmqoy2sQ== Received: by mail-lf1-f50.google.com with SMTP id y25so54330026lfa.9 for ; Thu, 05 Jan 2023 01:42:11 -0800 (PST) X-Gm-Message-State: AFqh2kq/P0GEXjrhizjxcukHKOkOcatyVecpb+lMBIQAJmKTzmgnAtpy JqTpNIgC1rbFXZ1J9CzAmAfTbFPjheMNbjD0JD8= X-Google-Smtp-Source: AMrXdXu7tAaXcisX3i+ME9WtraAGReOpSP2+4m7aJ2FaOQimuYgSwRMH/kS8JHAViBMYdOZyxPjzQiM6H0Pc1dIWNF4= X-Received: by 2002:ac2:5dfa:0:b0:4b7:3a0:45d2 with SMTP id z26-20020ac25dfa000000b004b703a045d2mr2654172lfq.228.1672911729338; Thu, 05 Jan 2023 01:42:09 -0800 (PST) MIME-Version: 1.0 References: <20230105081127.muckhpcw6agfkfrs@sirius.home.kraxel.org> <3F97900D-3C85-4E10-BBC2-360CECFD2D39@csgraf.de> In-Reply-To: <3F97900D-3C85-4E10-BBC2-360CECFD2D39@csgraf.de> From: "Ard Biesheuvel" Date: Thu, 5 Jan 2023 10:41:57 +0100 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel] [PATCH v3 03/16] ArmVirtPkg: make EFI_LOADER_DATA non-executable To: Alexander Graf Cc: Gerd Hoffmann , dann frazier , devel@edk2.groups.io, Leif Lindholm Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Thu, 5 Jan 2023 at 09:43, Alexander Graf wrote: > > > > > Am 05.01.2023 um 09:11 schrieb Gerd Hoffmann : > > > > =EF=BB=BF Hi, > > > >> To clarify, I mean something like the patch below, but with an additio= nal > >> callback notification similar to the Emu one in LoadImage(), so that w= e can > >> make sure we only enable the quirk when we load a known-bad grub binar= y. > >> That way we still force distros to ship fixed versions of their code, = but > >> enable old code to continue running. > > > >> + /* 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 =3D TRUE; > >> + if (is_broken_grub) { > >> + RealAllocatePages =3D gBS->AllocatePages; > >> + gBS->AllocatePages =3D AllocatePagesForceLoaderCode; > >> + } > > > > 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 t= heir old grub binaries working, they can send an upstream patch to add thei= r hash :). At least we'd have a path forward to make things work that is no= t "revert 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= 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 legi= t 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 fro= m the NX logic. Keep NX for any other non-code memory type as well as Loade= rData pool allocations. > Another thing we might consider is trapping exec permission violations and switching the pages in question from rw- to r-x. Does GRUB generally load/map executable modules at page granularity?