From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id C63F3D8106C for ; Wed, 6 Dec 2023 13:24:14 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=08TdsEMpY84jYpK3zv2c20XV9fUT6faRcbuALqGI/Z8=; c=relaxed/simple; d=groups.io; h=MIME-Version:References:In-Reply-To:From:Date:Message-ID:Subject:To:Cc:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type; s=20140610; t=1701869053; v=1; b=aeLHubtW9D/NXQLqszVxYAmuJ9NTOappaf8u3k7umofrTuJCqL/OmXtV5NyjX2WHtaFdrTSg MDtghSxH88T+FtKjANywgyzXeyc1LD/wdZ1PL2LvqqiUcJ+y2X2dy5/486BcbjeBLCeoaIE4/ab nihdz/ATE08bCfX6SuB5N5Mk= X-Received: by 127.0.0.2 with SMTP id EfNIYY7687511xoUNKp3A3g3; Wed, 06 Dec 2023 05:24:13 -0800 X-Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by mx.groups.io with SMTP id smtpd.web10.31371.1701869052378017984 for ; Wed, 06 Dec 2023 05:24:12 -0800 X-Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by dfw.source.kernel.org (Postfix) with ESMTP id CA25D61B89 for ; Wed, 6 Dec 2023 13:24:11 +0000 (UTC) X-Received: by smtp.kernel.org (Postfix) with ESMTPSA id 7B221C433C9 for ; Wed, 6 Dec 2023 13:24:11 +0000 (UTC) X-Received: by mail-lf1-f41.google.com with SMTP id 2adb3069b0e04-50be9e6427dso4814396e87.1 for ; Wed, 06 Dec 2023 05:24:11 -0800 (PST) X-Gm-Message-State: q6zZ8jExgPAd6zCAfdI5e8Yyx7686176AA= X-Google-Smtp-Source: AGHT+IHPK9a1uZr3cM30BWeH1j8xsTvkf3WjtWBu2RO2WBgF1Lp1KkNAsYGtz8f2DUYXU8cqX4zbuzy9ieGnKQl5GiE= X-Received: by 2002:a05:6512:3ba5:b0:50b:eab5:d70d with SMTP id g37-20020a0565123ba500b0050beab5d70dmr667137lfv.11.1701869049641; Wed, 06 Dec 2023 05:24:09 -0800 (PST) MIME-Version: 1.0 References: <20231204095215.1053032-1-ardb@google.com> In-Reply-To: From: "Ard Biesheuvel" Date: Wed, 6 Dec 2023 14:23:58 +0100 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel] [PATCH] ArmVirtPkg: Allow EFI memory attributes protocol to be disabled To: Gerd Hoffmann , Taylor Beebe , Oliver Smith-Denny , Peter Jones Cc: devel@edk2.groups.io, Ard Biesheuvel , =?UTF-8?B?TO+/vXN6bO+/vSDvv71yc2Vr?= , Oliver Steffen , Alexander Graf , Leif Lindholm Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,ardb@kernel.org List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: Content-Type: text/plain; charset="UTF-8" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=aeLHubtW; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=kernel.org (policy=none) For context: https://openfw.io/edk2-devel/g2ulyam7plpgrqlganhb5u2wtswq26civqlt4gpnxmjgq65yt7@umm3dta22cdz/T/#t On Wed, 6 Dec 2023 at 13:51, Gerd Hoffmann wrote: > > > > We can disable the protocol via this method but how would you set it > > > to =n by default? > > > > if (Status != EFI_SUCCESS) > > // opt/org.tiabocode/MemAttrProtocol not present on the qemu cmdline > > MemAttrProtocol = ThisBuildsDefault > > } > > FYI: Below is what I'll add to the fedora builds. > > Rough plan: Keep this until we have a fixed shim.efi and release media > with that (hopefully Fedora 40 next spring). At that point flip default > to TRUE and keep it that way for a year or two. Then drop the patch. > Yeah. I am not /quite/ ready to admit defeat, especially because other systems (such as sbsa-ref) are suffering from the same problem, and so fixing this in a QEMU specific way is probably not sufficient. So what happens is: - shim intends to load fbaa64.efi - it allocates the region as EfiLoaderCode - it sets XP and clears RP/RO on the entire region - it copies and relocates the individual sections, and remaps them but only if the alignment is >= 4k - it calls the entrypoint which resides in a section that is still mapped XP and boom (this is based on the ubuntu cloud image). Note that loading grub works fine, so once we've gone through fbaa64.efi once, the issue goes away. Shim itself does not have the NX compat attribute, nor does the fbaa64.efi image that it is loading (afaict) The EfiLoaderCode region has RWX permissions in this case. Future, tightened firmware will not create EfiLoaderCode with RWX permissions, but require the use of the EFI memory attributes protocol to create executable regions. The difficulty here is that shim never bothers to call the protocol at all to remap the individual sections, as it notices that the alignment is insufficient. So overriding the behavior at this point is impossible. But what we might do is invent a way to avoid setting the XP attribute on the entire region based on some heuristic. Given that the main purpose of the EFI memory attribute protocol is to provide the ability to remove XP (and set RO instead), perhaps we can avoid the set entirely? Just brainstorming here. (cc'ing Taylor and Oliver given that this is related to the memory policy work as well) Perhaps we can use the fact that the active image is non-NX compat to make some tweaks? What I really want to avoid is derail our effort to tighten things down and comply with the NX compat related policies, by adding some build time control that the distros will enable now and never disable again, citing backward compat concerns. And the deafening silence from the shim developers is not an encouragement either. > ------------------------------- cut here ---------------------------- > From c174197c65d2346f519418ded2e645d57423be41 Mon Sep 17 00:00:00 2001 > From: Gerd Hoffmann > Date: Wed, 6 Dec 2023 13:00:53 +0100 > Subject: [PATCH 1/1] ArmVirtPkg: add runtime option to enable/disable > MemoryAttributesProtocol > > Based on a patch by Ard Biesheuvel > > Usage: > qemu-system-aarch64 $args \ > -fw_cfg name=opt/org.tianocore/MemAttrProtocol,string=y > > Default to 'n' (disabled) for now. > > Signed-off-by: Gerd Hoffmann > --- > .../PlatformBootManagerLib.inf | 2 + > .../PlatformBootManagerLib/PlatformBm.c | 69 +++++++++++++++++++ > 2 files changed, 71 insertions(+) > > diff --git a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf > index 997eb1a4429f..facd81a5d036 100644 > --- a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf > +++ b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf > @@ -46,6 +46,7 @@ [LibraryClasses] > PcdLib > PlatformBmPrintScLib > QemuBootOrderLib > + QemuFwCfgSimpleParserLib > QemuLoadImageLib > ReportStatusCodeLib > TpmPlatformHierarchyLib > @@ -73,5 +74,6 @@ [Guids] > [Protocols] > gEfiFirmwareVolume2ProtocolGuid > gEfiGraphicsOutputProtocolGuid > + gEfiMemoryAttributeProtocolGuid > gEfiPciRootBridgeIoProtocolGuid > gVirtioDeviceProtocolGuid > diff --git a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c > index 85c01351b09d..a50b9aec0f2c 100644 > --- a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c > +++ b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c > @@ -16,6 +16,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -1111,6 +1112,49 @@ PlatformBootManagerBeforeConsole ( > FilterAndProcess (&gEfiPciIoProtocolGuid, IsVirtioPciSerial, SetupVirtioSerial); > } > > +/** > + Uninstall the EFI memory attribute protocol if it exists. > +**/ > +STATIC > +VOID > +UninstallEfiMemoryAttributesProtocol ( > + VOID > + ) > +{ > + EFI_STATUS Status; > + EFI_HANDLE Handle; > + UINTN Size; > + VOID *MemoryAttributeProtocol; > + > + Size = sizeof (Handle); > + Status = gBS->LocateHandle ( > + ByProtocol, > + &gEfiMemoryAttributeProtocolGuid, > + NULL, > + &Size, > + &Handle > + ); > + > + if (EFI_ERROR (Status)) { > + ASSERT (Status == EFI_NOT_FOUND); > + return; > + } > + > + Status = gBS->HandleProtocol ( > + Handle, > + &gEfiMemoryAttributeProtocolGuid, > + &MemoryAttributeProtocol > + ); > + ASSERT_EFI_ERROR (Status); > + > + Status = gBS->UninstallProtocolInterface ( > + Handle, > + &gEfiMemoryAttributeProtocolGuid, > + MemoryAttributeProtocol > + ); > + ASSERT_EFI_ERROR (Status); > +} > + > /** > Do the platform specific action after the console is ready > Possible things that can be done in PlatformBootManagerAfterConsole: > @@ -1129,12 +1173,37 @@ PlatformBootManagerAfterConsole ( > ) > { > RETURN_STATUS Status; > + BOOLEAN MemAttrProtocol; > > // > // Show the splash screen. > // > BootLogoEnableLogo (); > > + // > + // Work around shim's terminally broken use of the EFI memory attributes > + // protocol, by just uninstalling it when requested on the QEMU command line. > + // > + Status = QemuFwCfgParseBool ( > + "opt/org.tianocore/MemAttrProtocol", > + &MemAttrProtocol > + ); > + if (RETURN_ERROR (Status)) { > + // default > + MemAttrProtocol = FALSE; > + } > + > + DEBUG (( > + DEBUG_ERROR, > + "%a: MemAttrProtocol = %a\n", > + __func__, > + MemAttrProtocol ? "yes" : "no" > + )); > + > + if (!MemAttrProtocol) { > + UninstallEfiMemoryAttributesProtocol (); > + } > + > // > // Process QEMU's -kernel command line option. The kernel booted this way > // will receive ACPI tables: in PlatformBootManagerBeforeConsole(), we > -- > 2.43.0 > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112124): https://edk2.groups.io/g/devel/message/112124 Mute This Topic: https://groups.io/mt/102967690/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=-