[edk2-devel] [PATCH v2] ArmVirt: Allow memory attributes protocol to be disabled on first boot

["Ard Biesheuvel" <ardb@kernel.org>]

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 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 <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 |  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 <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>

@@ -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 (#112179): https://edk2.groups.io/g/devel/message/112179
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]
-=-=-=-=-=-=-=-=-=-=-=-