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 6FADE740049 for ; Fri, 8 Dec 2023 15:34:30 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=egO05I7nktHE4Icb5zXSp+Ka1OH9NoC378RlbGpUyY0=; 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=1702049669; v=1; b=VOVdblpd7f4yzN6uLbXZN7CmWiSgkvTObTcZFEtQRgWCrAoePc7zgdeuIwqf72aYFPLmuJs4 HvGezw4AJWDC1ivfZBOkEcarUnc6bymliw7/Esiv5bqts5KLEzD+ym8zmfiXojiVcs0nNWUfWH2 F2eIArDyveDkc+LCeGnML+tI= X-Received: by 127.0.0.2 with SMTP id mw02YY7687511xFt21iiTjQo; Fri, 08 Dec 2023 07:34:29 -0800 X-Received: from sin.source.kernel.org (sin.source.kernel.org [145.40.73.55]) by mx.groups.io with SMTP id smtpd.web11.38449.1702049668162298951 for ; Fri, 08 Dec 2023 07:34:28 -0800 X-Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by sin.source.kernel.org (Postfix) with ESMTP id 9176ACE2612 for ; Fri, 8 Dec 2023 15:34:24 +0000 (UTC) X-Received: by smtp.kernel.org (Postfix) with ESMTPSA id CD513C433C7 for ; Fri, 8 Dec 2023 15:34:23 +0000 (UTC) X-Received: by mail-lj1-f172.google.com with SMTP id 38308e7fff4ca-2c9f413d6b2so26765151fa.1 for ; Fri, 08 Dec 2023 07:34:23 -0800 (PST) X-Gm-Message-State: VlIL0NywmOsZuLdRfJyn8P1kx7686176AA= X-Google-Smtp-Source: AGHT+IE7ry2pK+GPPlsW97WZ9y8sGbWr+1RqjERJAzGUh3qXz9WJe2Qx4BclUDGZCSO6PglUSRx2ypMCzrew6MOYRPg= X-Received: by 2002:a2e:86da:0:b0:2c9:d773:894b with SMTP id n26-20020a2e86da000000b002c9d773894bmr32091ljj.30.1702049662037; Fri, 08 Dec 2023 07:34:22 -0800 (PST) MIME-Version: 1.0 References: <20231207100603.2654084-1-ardb@google.com> In-Reply-To: From: "Ard Biesheuvel" Date: Fri, 8 Dec 2023 16:34:11 +0100 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel] [PATCH v2] ArmVirt: Allow memory attributes protocol to be disabled on first boot To: Laszlo Ersek Cc: devel@edk2.groups.io, Gerd Hoffmann , Oliver Steffen , Alexander Graf , Oliver Smith-Denny , Taylor Beebe , Peter Jones , 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=VOVdblpd; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=kernel.org (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io Thanks for the review. On Fri, 8 Dec 2023 at 15:34, Laszlo Ersek wrote: > > On 12/7/23 11:06, Ard Biesheuvel wrote: > > From: Ard Biesheuvel > > > > Shim's PE loader uses the EFI memory attributes protocol in a way that > > results in an immediate crash when invoking the loaded image, unless the > > base and size of its executable segment are both aligned to 4k. > > > > If this is not the case, it will strip the memory allocation of its > > executable permissions, but fail to add them back for the executable > > region, resulting in non-executable code. Unfortunately, the PE loader > > does not even bother invoking the protocol in this case (as it notices > > the misalignment), making it very hard for system firmware to work > > around this by attempting to infer the intent of the caller. > > > > So let's introduce a QEMU command line option to indicate that the > > protocol should not be exposed at all on the first boot, which is when > > the issue is triggered. (fbaa64.efi is broken but grubaa64.efi boots > > fine) > > > > -fw_cfg opt/org.tianocore/UninstallMemAttrProtocolOnFirstBoot,string=y > > > > Also introduce a fixed boolean PCD that sets the default. > > > > Cc: Laszlo Ersek > > Cc: Gerd Hoffmann > > Cc: Oliver Steffen > > Cc: Alexander Graf > > Cc: Oliver Smith-Denny > > Cc: Taylor Beebe > > Cc: Peter Jones > > Cc: Leif Lindholm > > Link: https://gitlab.com/qemu-project/qemu/-/issues/1990 > > Signed-off-by: Ard Biesheuvel > > --- > > ArmVirtPkg/ArmVirtPkg.dec | 6 ++ > > ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf | 7 ++ > > ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c | 85 ++++++++++++++++++++ > > 3 files changed, 98 insertions(+) > > > > diff --git a/ArmVirtPkg/ArmVirtPkg.dec b/ArmVirtPkg/ArmVirtPkg.dec > > index 0f2d7873279f..c55978f75c19 100644 > > --- a/ArmVirtPkg/ArmVirtPkg.dec > > +++ b/ArmVirtPkg/ArmVirtPkg.dec > > @@ -68,3 +68,9 @@ [PcdsFixedAtBuild, PcdsPatchableInModule] > > # Cloud Hypervisor has no other way to pass Rsdp address to the guest except use a PCD. > > > > # > > > > gArmVirtTokenSpaceGuid.PcdCloudHvAcpiRsdpBaseAddress|0x0|UINT64|0x00000005 > > > > + > > > > + ## > > > > + # Whether the EFI memory attribus protocol should be uninstalled before > > > > + # invoking the OS loader on the first boot. This may be needed to work around > > > > + # problematic builds of shim that use the protocol incorrectly. > > > > + gArmVirtTokenSpaceGuid.PcdUninstallMemAttrProtocolOnFirstBoot|FALSE|BOOLEAN|0x00000006 > > > > (1) could be a feature PCD (although it couldn't be patchable-in-module > then, and perhaps we don't consider this a "feature") > Is this a general remark on the similarity between feature PCDs and boolean PCDs? > (2) typo: "attribus" > Ack > (3) for some reason, I see double line breaks. > Yeah :-( I am struggling with the internal Google 'sendgmr' mailer which switches to QP transfer encoding for some reason. (I lost the hosting venue for my ThunderX2 workstation so I am now relying on Google infrastructure to host my development system). I am working on this, > > diff --git a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf > > index 997eb1a4429f..5d119af6a3b3 100644 > > --- a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf > > +++ b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf > > @@ -16,6 +16,7 @@ [Defines] > > MODULE_TYPE = DXE_DRIVER > > > > VERSION_STRING = 1.0 > > > > LIBRARY_CLASS = PlatformBootManagerLib|DXE_DRIVER > > > > + CONSTRUCTOR = PlatformBootManagerLibConstructor > > > > > > > > # > > > > # The following information is for reference only and not required by the build tools. > > > > @@ -46,6 +47,7 @@ [LibraryClasses] > > PcdLib > > > > PlatformBmPrintScLib > > > > QemuBootOrderLib > > > > + QemuFwCfgSimpleParserLib > > > > QemuLoadImageLib > > > > ReportStatusCodeLib > > > > TpmPlatformHierarchyLib > > > > @@ -55,6 +57,7 @@ [LibraryClasses] > > UefiRuntimeServicesTableLib > > > > > > > > [FixedPcd] > > > > + gArmVirtTokenSpaceGuid.PcdUninstallMemAttrProtocolOnFirstBoot > > > > gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate > > > > gEfiMdePkgTokenSpaceGuid.PcdUartDefaultDataBits > > > > gEfiMdePkgTokenSpaceGuid.PcdUartDefaultParity > > > > @@ -73,5 +76,9 @@ [Guids] > > [Protocols] > > > > gEfiFirmwareVolume2ProtocolGuid > > > > gEfiGraphicsOutputProtocolGuid > > > > + gEfiMemoryAttributeProtocolGuid > > > > gEfiPciRootBridgeIoProtocolGuid > > > > gVirtioDeviceProtocolGuid > > > > + > > > > +[Depex] > > > > + gEfiVariableArchProtocolGuid > > > > I've made an effort to read through the v1 discussion (exhausting). Some > quetions remain: > > (4) Why the change from an explicit call from AfterConsole to a > constructor? Was AfterConsole too late somehow? > Yes. Checking for the existence of "BootOrder" needs to occur earlier, or it will have been created by the BDS. > I think constructors should be the last resort. > Not disagreeing with that. > (5) Is the depex really necessary? BDS is supposed to start when all > drivers have been dispatched, and so by that time, all of the UEFI > architectural protocols should be available. (BDS will launch UEFI > drivers, and all the UEFI drivers have an implicit depex on all the > architectural protocols.) > The BDS arch protocol will be invoked at that point. but the BdsDxe itself could be dispatched much earlier, at which point the constructor of this library will be invoked. And I'll need to include the CPU arch protocol as well here, as this is installed at the same time as the EFI memory attributes protocol by the CPU dxe driver. > > diff --git a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c > > index 85c01351b09d..5306d9ea0a05 100644 > > --- a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c > > +++ b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c > > @@ -16,6 +16,7 @@ > > #include > > > > #include > > > > #include > > > > +#include > > > > #include > > > > #include > > > > #include > > > > @@ -1274,3 +1275,87 @@ PlatformBootManagerUnableToBoot ( > > EfiBootManagerBoot (&BootManagerMenu); > > > > } > > > > } > > > > + > > > > +/** > > > > + 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); > > > > +} > > Looks OK to me. > > > > > + > > > > +EFI_STATUS > > > > +EFIAPI > > > > +PlatformBootManagerLibConstructor ( > > > > + IN EFI_HANDLE ImageHandle, > > > > + IN EFI_SYSTEM_TABLE *SystemTable > > > > + ) > > > > +{ > > > > + BOOLEAN Uninstall; > > > > + UINTN VarSize; > > > > + UINT32 Attr; > > > > + > > > > + // > > > > + // Work around shim's terminally broken use of the EFI memory attributes > > > > + // protocol, by uninstalling it if requested on the QEMU command line. > > > > + // > > > > + // E.g., > > > > + // -fw_cfg opt/org.tianocore/UninstallMemAttrProtocolOnFirstBoot,string=y > > > > + // > > > > + // This is only needed on the first boot, when fbaa64.efi is being invoked to > > > > + // set the boot order variables. Subsequent boots involving GRUB are not > > > > + // affected. > > > > + // > > > > + VarSize = 0; > > > > + if (gRT->GetVariable ( > > > > + L"BootOrder", > > > > + &gEfiGlobalVariableGuid, > > > > + &Attr, > > (6) "Attr" is optional; we could / should pass NULL here. > Good to know. > > > > + &VarSize, > > > > + NULL > > > > + ) == EFI_NOT_FOUND) > > > > + { > > > > + Uninstall = FixedPcdGetBool (PcdUninstallMemAttrProtocolOnFirstBoot); > > > > + QemuFwCfgParseBool ("opt/org.tianocore/UninstallMemAttrProtocolOnFirstBoot", &Uninstall); > > > > + if (Uninstall) { > > > > + UninstallEfiMemoryAttributesProtocol (); > > > > + } > > > > + } > > > > + > > > > + return EFI_SUCCESS; > > > > +} > > > > (7) Tying back to my point (4) -- I understand this is a hack anyway, > but I'm still uncomfortable with platform BDS uninstalling a protocol > that is owned by / provided by the CPU driver. Feels like a significant > layering violation. > It is. > Can we modify the CPU driver instead, to listen to a new event group, > upon which being signaled, the CPU driver would uninstall the protocol > (and close the listening event)? > > This PlatformBootManagerLib instance would act more or less the same > (I'd suggest signaling the event group from within AfterConsole, in case > the PCD default and/or the fw_cfg knob dictated that), but the protocol > uninstallation would occur in "ArmPkg/Drivers/CpuDxe". > > In more technical terms, the layering violation IMO is that we mess with > CpuDxe's "mCpuHandle" and "mMemoryAttribute" static variables from > within BDS. Adding the new event group requires more boiler-plate code > for sure, but there's a small code-size benefit as well: we'd not have > to look up either the handle (with LocateHandle) or the protocol > interface (with HandleProtocol), as CpuDxe inherently knows those > (mCpuHandle, mMemoryAttribute). > I agree with your analysis here. But I am reluctant to introduce elaborate infrastructure across drivers to implement a feature that should not exist in the first place. As I mentioned a couple of times, I am rather unhappy with the complete lack of involvement of the people who created this mess in the first place, and what I am after is really a minimal, local hack that unblocks the actual end users (people running LIMA on ARM based Macs) without creating building blocks that will be used by the distro forks to erode the original functionality even further, > Thanks for considering (and sorry for butting in this late...) No worries. Thanks for the review. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112235): https://edk2.groups.io/g/devel/message/112235 Mute This Topic: https://groups.io/mt/103031504/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=-