On Mon, 4 Dec 2023 at 10:52, Ard Biesheuvel <a...@kernel.org> wrote: > > From: Ard Biesheuvel <a...@kernel.org> > > 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 executable permissions from > the memory allocation, 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. > > -fw_cfg opt/org.tianocore/DisableMemAttrProtocol,string=y > > Cc: L�szl� �rsek <ler...@redhat.com>
Apologies for the alphabet soup. The Google internal mailer appears to have changed the content transfer encoding from 8bit to qp because it spotted a long line (??) > Cc: Gerd Hoffmann <kra...@redhat.com> > Cc: Oliver Steffen <ostef...@redhat.com> > Cc: Alexander Graf <g...@amazon.com> > Link: https://gitlab.com/qemu-project/qemu/-/issues/1990 > Signed-off-by: Ard Biesheuvel <a...@kernel.org> > --- > ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf | 2 + > ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c | 56 > ++++++++++++++++++++ > 2 files changed, 58 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..e17899100e4a 100644 > --- a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c > +++ b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c > @@ -16,6 +16,7 @@ > #include <Library/PcdLib.h> > > #include <Library/PlatformBmPrintScLib.h> > > #include <Library/QemuBootOrderLib.h> > > +#include <Library/QemuFwCfgSimpleParserLib.h> > > #include <Library/TpmPlatformHierarchyLib.h> > > #include <Library/UefiBootManagerLib.h> > > #include <Protocol/DevicePath.h> > > @@ -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,24 @@ PlatformBootManagerAfterConsole ( > ) > > { > > RETURN_STATUS Status; > > + BOOLEAN FwCfgBool; > > > > // > > // 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/DisableMemAttrProtocol", > > + &FwCfgBool); > > + if (!RETURN_ERROR (Status) && FwCfgBool) { > > + UninstallEfiMemoryAttributesProtocol (); > > + } > > + > > // > > // Process QEMU's -kernel command line option. The kernel booted this way > > // will receive ACPI tables: in PlatformBootManagerBeforeConsole(), we > > -- > 2.43.0.rc2.451.g8631bc7472-goog > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112028): https://edk2.groups.io/g/devel/message/112028 Mute This Topic: https://groups.io/mt/102967690/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
