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 80DCF9417E3 for ; Fri, 8 Dec 2023 11:20:45 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=sqBEXFHl9yUpIj82tIR5T8QeVoWrChgy2Q3RTYAh9D4=; 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=1702034444; v=1; b=EZHTrtRQXVuBdXx2AFzf/KUsotq25FqQi9DrI3k3HsA288IoIBaUFhTHA21Q8qSAV89v7W2p HFleb98ntHEVvV6gWy+64zu6B0EroOqRQZuQp/BlAbrW1H3zTx99CbY38v4xbNXGVgPI6KvMtn1 iT5OOxBNqmaTpq/ILswLaI1M= X-Received: by 127.0.0.2 with SMTP id uPTGYY7687511xVIBdZJ4wza; Fri, 08 Dec 2023 03:20:44 -0800 X-Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by mx.groups.io with SMTP id smtpd.web11.21408.1702034443354786074 for ; Fri, 08 Dec 2023 03:20:43 -0800 X-Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by dfw.source.kernel.org (Postfix) with ESMTP id A9F6562364 for ; Fri, 8 Dec 2023 11:20:42 +0000 (UTC) X-Received: by smtp.kernel.org (Postfix) with ESMTPSA id 5F109C433CB for ; Fri, 8 Dec 2023 11:20:42 +0000 (UTC) X-Received: by mail-lj1-f177.google.com with SMTP id 38308e7fff4ca-2ca0c36f5beso24540611fa.1 for ; Fri, 08 Dec 2023 03:20:42 -0800 (PST) X-Gm-Message-State: 9Vmvhn5PYDgrgdxnhgb1xgchx7686176AA= X-Google-Smtp-Source: AGHT+IGl+5IyzLDVQ/olAHEtd4lJyV2RO/Czt23IScW3OlPf4Pymh7T64SkQPtZ0Phyl8crFz1Hcz6S3FlFhOi5cJR8= X-Received: by 2002:a2e:808f:0:b0:2ca:693:f82c with SMTP id i15-20020a2e808f000000b002ca0693f82cmr2340552ljg.67.1702034440580; Fri, 08 Dec 2023 03:20:40 -0800 (PST) MIME-Version: 1.0 References: <20231207100603.2654084-1-ardb@google.com> In-Reply-To: <20231207100603.2654084-1-ardb@google.com> From: "Ard Biesheuvel" Date: Fri, 8 Dec 2023 12:20:29 +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: Ard Biesheuvel Cc: devel@edk2.groups.io, Laszlo Ersek , 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=EZHTrtRQ; 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 On Thu, 7 Dec 2023 at 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 OK, if nobody has problems with this approach, I intend to merge it and give a snapshot build to the lima folks to incorporate in their scripts. > --- > 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 > > 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 > > 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); > > +} > > + > > +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, > > + &VarSize, > > + NULL > > + ) == EFI_NOT_FOUND) > > + { > > + Uninstall = FixedPcdGetBool (PcdUninstallMemAttrProtocolOnFirstBoot); > > + QemuFwCfgParseBool ("opt/org.tianocore/UninstallMemAttrProtocolOnFirstBoot", &Uninstall); > > + if (Uninstall) { > > + UninstallEfiMemoryAttributesProtocol (); > > + } > > + } > > + > > + return EFI_SUCCESS; > > +} > > -- > 2.43.0.rc2.451.g8631bc7472-goog > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112225): https://edk2.groups.io/g/devel/message/112225 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] -=-=-=-=-=-=-=-=-=-=-=-