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 7ED8F941534 for ; Fri, 8 Dec 2023 14:34:49 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=o7ph6GtdVVYDkqizIoaVSr0HV/jy5YTYknp8v9eZxjY=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1702046088; v=1; b=TXZtBTe/tniCJIo8wzvv/JrirZbM9LDH6LgVRuN/LELEcgX2OAZZUofRvRcTUi38jIIoc2kh 9Lrp7SCR671eDn+2neKz7wthCCnMn3nR9lLpFJ2ADgLK+fETPzJ0AKa8ISOkckzINfulFs61dxF Y2GYq0SpfNwKuRqhxZ9hB00g= X-Received: by 127.0.0.2 with SMTP id F0ZOYY7687511xVIen2VTAnY; Fri, 08 Dec 2023 06:34:48 -0800 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web11.36781.1702046087302940743 for ; Fri, 08 Dec 2023 06:34:47 -0800 X-Received: from mimecast-mx02.redhat.com (mx-ext.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-602-eHxKYpRKPXCqPUEyEkDm2w-1; Fri, 08 Dec 2023 09:34:41 -0500 X-MC-Unique: eHxKYpRKPXCqPUEyEkDm2w-1 X-Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.rdu2.redhat.com [10.11.54.7]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id ACF8E1C04B55; Fri, 8 Dec 2023 14:34:40 +0000 (UTC) X-Received: from [10.39.193.124] (unknown [10.39.193.124]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 27C751C060AF; Fri, 8 Dec 2023 14:34:38 +0000 (UTC) Message-ID: Date: Fri, 8 Dec 2023 15:34:37 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH v2] ArmVirt: Allow memory attributes protocol to be disabled on first boot To: devel@edk2.groups.io, ardb@kernel.org Cc: Gerd Hoffmann , Oliver Steffen , Alexander Graf , Oliver Smith-Denny , Taylor Beebe , Peter Jones , Leif Lindholm References: <20231207100603.2654084-1-ardb@google.com> From: "Laszlo Ersek" In-Reply-To: <20231207100603.2654084-1-ardb@google.com> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.7 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com 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,lersek@redhat.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: wghV6yJJTVSZRVmnQfGCCEyux7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b="TXZtBTe/"; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (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 On 12/7/23 11:06, Ard Biesheuvel wrote: > From: Ard Biesheuvel >=20 > 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. >=20 > 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. >=20 > 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) >=20 > -fw_cfg opt/org.tianocore/UninstallMemAttrProtocolOnFirstBoot,string=3D= y >=20 > Also introduce a fixed boolean PCD that sets the default. >=20 > 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 | 8= 5 ++++++++++++++++++++ > 3 files changed, 98 insertions(+) >=20 > 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. >=20 > # >=20 > gArmVirtTokenSpaceGuid.PcdCloudHvAcpiRsdpBaseAddress|0x0|UINT64|0x0000= 0005 >=20 > + >=20 > + ## >=20 > + # Whether the EFI memory attribus protocol should be uninstalled befor= e >=20 > + # invoking the OS loader on the first boot. This may be needed to work= around >=20 > + # problematic builds of shim that use the protocol incorrectly. >=20 > + gArmVirtTokenSpaceGuid.PcdUninstallMemAttrProtocolOnFirstBoot|FALSE|BO= OLEAN|0x00000006 >=20 (1) could be a feature PCD (although it couldn't be patchable-in-module then, and perhaps we don't consider this a "feature") (2) typo: "attribus" (3) for some reason, I see double line breaks. > diff --git a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManage= rLib.inf b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib= .inf > index 997eb1a4429f..5d119af6a3b3 100644 > --- a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.in= f > +++ b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.in= f > @@ -16,6 +16,7 @@ [Defines] > MODULE_TYPE =3D DXE_DRIVER >=20 > VERSION_STRING =3D 1.0 >=20 > LIBRARY_CLASS =3D PlatformBootManagerLib|DXE_DRIVER >=20 > + CONSTRUCTOR =3D PlatformBootManagerLibConstructor >=20 > =20 >=20 > # >=20 > # The following information is for reference only and not required by th= e build tools. >=20 > @@ -46,6 +47,7 @@ [LibraryClasses] > PcdLib >=20 > PlatformBmPrintScLib >=20 > QemuBootOrderLib >=20 > + QemuFwCfgSimpleParserLib >=20 > QemuLoadImageLib >=20 > ReportStatusCodeLib >=20 > TpmPlatformHierarchyLib >=20 > @@ -55,6 +57,7 @@ [LibraryClasses] > UefiRuntimeServicesTableLib >=20 > =20 >=20 > [FixedPcd] >=20 > + gArmVirtTokenSpaceGuid.PcdUninstallMemAttrProtocolOnFirstBoot >=20 > gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate >=20 > gEfiMdePkgTokenSpaceGuid.PcdUartDefaultDataBits >=20 > gEfiMdePkgTokenSpaceGuid.PcdUartDefaultParity >=20 > @@ -73,5 +76,9 @@ [Guids] > [Protocols] >=20 > gEfiFirmwareVolume2ProtocolGuid >=20 > gEfiGraphicsOutputProtocolGuid >=20 > + gEfiMemoryAttributeProtocolGuid >=20 > gEfiPciRootBridgeIoProtocolGuid >=20 > gVirtioDeviceProtocolGuid >=20 > + >=20 > +[Depex] >=20 > + gEfiVariableArchProtocolGuid >=20 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? I think constructors should be the last resort. (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.) > diff --git a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c b/Arm= VirtPkg/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 >=20 > #include >=20 > #include >=20 > +#include >=20 > #include >=20 > #include >=20 > #include >=20 > @@ -1274,3 +1275,87 @@ PlatformBootManagerUnableToBoot ( > EfiBootManagerBoot (&BootManagerMenu); >=20 > } >=20 > } >=20 > + >=20 > +/** >=20 > + Uninstall the EFI memory attribute protocol if it exists. >=20 > +**/ >=20 > +STATIC >=20 > +VOID >=20 > +UninstallEfiMemoryAttributesProtocol ( >=20 > + VOID >=20 > + ) >=20 > +{ >=20 > + EFI_STATUS Status; >=20 > + EFI_HANDLE Handle; >=20 > + UINTN Size; >=20 > + VOID *MemoryAttributeProtocol; >=20 > + >=20 > + Size =3D sizeof (Handle); >=20 > + Status =3D gBS->LocateHandle ( >=20 > + ByProtocol, >=20 > + &gEfiMemoryAttributeProtocolGuid, >=20 > + NULL, >=20 > + &Size, >=20 > + &Handle >=20 > + ); >=20 > + >=20 > + if (EFI_ERROR (Status)) { >=20 > + ASSERT (Status =3D=3D EFI_NOT_FOUND); >=20 > + return; >=20 > + } >=20 > + >=20 > + Status =3D gBS->HandleProtocol ( >=20 > + Handle, >=20 > + &gEfiMemoryAttributeProtocolGuid, >=20 > + &MemoryAttributeProtocol >=20 > + ); >=20 > + ASSERT_EFI_ERROR (Status); >=20 > + >=20 > + Status =3D gBS->UninstallProtocolInterface ( >=20 > + Handle, >=20 > + &gEfiMemoryAttributeProtocolGuid, >=20 > + MemoryAttributeProtocol >=20 > + ); >=20 > + ASSERT_EFI_ERROR (Status); >=20 > +} Looks OK to me. >=20 > + >=20 > +EFI_STATUS >=20 > +EFIAPI >=20 > +PlatformBootManagerLibConstructor ( >=20 > + IN EFI_HANDLE ImageHandle, >=20 > + IN EFI_SYSTEM_TABLE *SystemTable >=20 > + ) >=20 > +{ >=20 > + BOOLEAN Uninstall; >=20 > + UINTN VarSize; >=20 > + UINT32 Attr; >=20 > + >=20 > + // >=20 > + // Work around shim's terminally broken use of the EFI memory attribut= es >=20 > + // protocol, by uninstalling it if requested on the QEMU command line. >=20 > + // >=20 > + // E.g., >=20 > + // -fw_cfg opt/org.tianocore/UninstallMemAttrProtocolOnFirstBoot= ,string=3Dy >=20 > + // >=20 > + // This is only needed on the first boot, when fbaa64.efi is being inv= oked to >=20 > + // set the boot order variables. Subsequent boots involving GRUB are n= ot >=20 > + // affected. >=20 > + // >=20 > + VarSize =3D 0; >=20 > + if (gRT->GetVariable ( >=20 > + L"BootOrder", >=20 > + &gEfiGlobalVariableGuid, >=20 > + &Attr, (6) "Attr" is optional; we could / should pass NULL here. >=20 > + &VarSize, >=20 > + NULL >=20 > + ) =3D=3D EFI_NOT_FOUND) >=20 > + { >=20 > + Uninstall =3D FixedPcdGetBool (PcdUninstallMemAttrProtocolOnFirstBoo= t); >=20 > + QemuFwCfgParseBool ("opt/org.tianocore/UninstallMemAttrProtocolOnFir= stBoot", &Uninstall); >=20 > + if (Uninstall) { >=20 > + UninstallEfiMemoryAttributesProtocol (); >=20 > + } >=20 > + } >=20 > + >=20 > + return EFI_SUCCESS; >=20 > +} >=20 (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. 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). Thanks for considering (and sorry for butting in this late...) Laszlo -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112233): https://edk2.groups.io/g/devel/message/112233 Mute This Topic: https://groups.io/mt/103031504/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/19134562= 12/xyzzy [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-