ArmVirtPkg/ArmVirtPkg.dec | 6 ++ ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf | 3 + ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c | 62 ++++++++++++++++++++ 3 files changed, 71 insertions(+)
From: Ard Biesheuvel <ardb@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 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, and a PCD to set the default for
this option when it is omitted.
-fw_cfg opt/org.tianocore/UninstallMemAttrProtocol,string=y
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Oliver Steffen <osteffen@redhat.com>
Cc: Alexander Graf <graf@amazon.com>
Cc: Oliver Smith-Denny <osde@linux.microsoft.com>
Cc: Taylor Beebe <taylor.d.beebe@gmail.com>
Cc: Peter Jones <pjones@redhat.com>
Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Link: https://gitlab.com/qemu-project/qemu/-/issues/1990
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
ArmVirtPkg/ArmVirtPkg.dec | 6 ++
ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf | 3 +
ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c | 62 ++++++++++++++++++++
3 files changed, 71 insertions(+)
diff --git a/ArmVirtPkg/ArmVirtPkg.dec b/ArmVirtPkg/ArmVirtPkg.dec
index 0f2d7873279f..313aebda902a 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 attributes protocol should be uninstalled before
+ # invoking the OS loader. This may be needed to work around problematic
+ # builds of shim that use the protocol incorrectly.
+ gArmVirtTokenSpaceGuid.PcdUninstallMemAttrProtocol|FALSE|BOOLEAN|0x00000006
diff --git a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
index 997eb1a4429f..70e4ebf94ad9 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
@@ -55,6 +56,7 @@ [LibraryClasses]
UefiRuntimeServicesTableLib
[FixedPcd]
+ gArmVirtTokenSpaceGuid.PcdUninstallMemAttrProtocol
gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate
gEfiMdePkgTokenSpaceGuid.PcdUartDefaultDataBits
gEfiMdePkgTokenSpaceGuid.PcdUartDefaultParity
@@ -73,5 +75,6 @@ [Guids]
[Protocols]
gEfiFirmwareVolume2ProtocolGuid
gEfiGraphicsOutputProtocolGuid
+ gEfiMemoryAttributeProtocolGuid
gEfiPciRootBridgeIoProtocolGuid
gVirtioDeviceProtocolGuid
diff --git a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
index 85c01351b09d..a1a2bca2af3d 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,30 @@ PlatformBootManagerAfterConsole (
)
{
RETURN_STATUS Status;
+ BOOLEAN Uninstall;
//
// Show the splash screen.
//
BootLogoEnableLogo ();
+ //
+ // 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/UninstallMemAttrProtocol,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.
+ //
+ Uninstall = FixedPcdGetBool (PcdUninstallMemAttrProtocol);
+ QemuFwCfgParseBool ("opt/org.tianocore/UninstallMemAttrProtocol", &Uninstall);
+ if (Uninstall) {
+ UninstallEfiMemoryAttributesProtocol ();
+ }
+
//
// Process QEMU's -kernel command line option. The kernel booted this way
// will receive ACPI tables: in PlatformBootManagerBeforeConsole(), we
--
2.43.0.472.g3155946c3a-goog
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112289): https://edk2.groups.io/g/devel/message/112289
Mute This Topic: https://groups.io/mt/103106391/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
> + // > + // 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/UninstallMemAttrProtocol,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. > + // > + Uninstall = FixedPcdGetBool (PcdUninstallMemAttrProtocol); > + QemuFwCfgParseBool ("opt/org.tianocore/UninstallMemAttrProtocol", &Uninstall); > + if (Uninstall) { > + UninstallEfiMemoryAttributesProtocol (); > + } Can we please have a log message here, for both uninstall and keep-installed cases? Otherwise the patch looks good to me. thanks, Gerd -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112290): https://edk2.groups.io/g/devel/message/112290 Mute This Topic: https://groups.io/mt/103106391/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 12/11/23 11:55, Gerd Hoffmann wrote: >> + // >> + // 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/UninstallMemAttrProtocol,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. >> + // (1) I think this last paragraph of the comment no longer applies; is that right? We don't restrict the proto masking to first boot any longer (i.e., in v3). >> + Uninstall = FixedPcdGetBool (PcdUninstallMemAttrProtocol); >> + QemuFwCfgParseBool ("opt/org.tianocore/UninstallMemAttrProtocol", &Uninstall); >> + if (Uninstall) { >> + UninstallEfiMemoryAttributesProtocol (); >> + } > > Can we please have a log message here, for both uninstall and > keep-installed cases? Good idea! > > Otherwise the patch looks good to me. With those two updates (assuming I'm right about (1)): Reviewed-by: Laszlo Ersek <lersek@redhat.com> Thanks! Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112301): https://edk2.groups.io/g/devel/message/112301 Mute This Topic: https://groups.io/mt/103106391/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On Mon, 11 Dec 2023 at 15:33, Laszlo Ersek <lersek@redhat.com> wrote: > > On 12/11/23 11:55, Gerd Hoffmann wrote: > >> + // > >> + // 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/UninstallMemAttrProtocol,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. > >> + // > > (1) I think this last paragraph of the comment no longer applies; is > that right? We don't restrict the proto masking to first boot any longer > (i.e., in v3). > Indeed. > >> + Uninstall = FixedPcdGetBool (PcdUninstallMemAttrProtocol); > >> + QemuFwCfgParseBool ("opt/org.tianocore/UninstallMemAttrProtocol", &Uninstall); > >> + if (Uninstall) { > >> + UninstallEfiMemoryAttributesProtocol (); > >> + } > > > > Can we please have a log message here, for both uninstall and > > keep-installed cases? > > Good idea! > > > > > Otherwise the patch looks good to me. > > With those two updates (assuming I'm right about (1)): > > Reviewed-by: Laszlo Ersek <lersek@redhat.com> > Thanks. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112302): https://edk2.groups.io/g/devel/message/112302 Mute This Topic: https://groups.io/mt/103106391/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.