public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [edk2-devel] [PATCH v4] ArmVirt: Allow memory attributes protocol to be disabled
@ 2023-12-12  8:36 Ard Biesheuvel
  2023-12-12 10:07 ` Gerd Hoffmann
  0 siblings, 1 reply; 4+ messages in thread
From: Ard Biesheuvel @ 2023-12-12  8:36 UTC (permalink / raw)
  To: devel
  Cc: Ard Biesheuvel, Gerd Hoffmann, Oliver Steffen, Alexander Graf,
	Oliver Smith-Denny, Taylor Beebe, Peter Jones, Leif Lindholm,
	Laszlo Ersek

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: 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
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
v4: - add diagnostic print to DEBUG builds, drop v2 related comment
v3: - drop first boot logic
v2: - uninstall protocol on first boot only, add PCD to define default

 ArmVirtPkg/ArmVirtPkg.dec                                            |  6 ++
 ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf |  3 +
 ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c               | 64 ++++++++++++++++++++
 3 files changed, 73 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..8e93f3cfed1b 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,32 @@ 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
+  //
+  Uninstall = FixedPcdGetBool (PcdUninstallMemAttrProtocol);
+  QemuFwCfgParseBool ("opt/org.tianocore/UninstallMemAttrProtocol", &Uninstall);
+  DEBUG ((
+    DEBUG_WARN,
+    "%a: %auninstalling EFI memory protocol\n",
+    __func__,
+    Uninstall ? "" : "not "
+    ));
+  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 (#112361): https://edk2.groups.io/g/devel/message/112361
Mute This Topic: https://groups.io/mt/103126734/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [edk2-devel] [PATCH v4] ArmVirt: Allow memory attributes protocol to be disabled
  2023-12-12  8:36 [edk2-devel] [PATCH v4] ArmVirt: Allow memory attributes protocol to be disabled Ard Biesheuvel
@ 2023-12-12 10:07 ` Gerd Hoffmann
  2023-12-12 10:42   ` Ard Biesheuvel
  0 siblings, 1 reply; 4+ messages in thread
From: Gerd Hoffmann @ 2023-12-12 10:07 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: devel, Ard Biesheuvel, Oliver Steffen, Alexander Graf,
	Oliver Smith-Denny, Taylor Beebe, Peter Jones, Leif Lindholm,
	Laszlo Ersek

On Tue, Dec 12, 2023 at 09:36:00AM +0100, Ard Biesheuvel wrote:
> 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

Tested-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112384): https://edk2.groups.io/g/devel/message/112384
Mute This Topic: https://groups.io/mt/103126734/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [edk2-devel] [PATCH v4] ArmVirt: Allow memory attributes protocol to be disabled
  2023-12-12 10:07 ` Gerd Hoffmann
@ 2023-12-12 10:42   ` Ard Biesheuvel
  2023-12-12 15:22     ` Laszlo Ersek
  0 siblings, 1 reply; 4+ messages in thread
From: Ard Biesheuvel @ 2023-12-12 10:42 UTC (permalink / raw)
  To: Gerd Hoffmann
  Cc: Ard Biesheuvel, devel, Oliver Steffen, Alexander Graf,
	Oliver Smith-Denny, Taylor Beebe, Peter Jones, Leif Lindholm,
	Laszlo Ersek

On Tue, 12 Dec 2023 at 11:08, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> On Tue, Dec 12, 2023 at 09:36:00AM +0100, Ard Biesheuvel wrote:
> > 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
>
> Tested-by: Gerd Hoffmann <kraxel@redhat.com>
> Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>
>

Thanks all - I've queued this up now.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112388): https://edk2.groups.io/g/devel/message/112388
Mute This Topic: https://groups.io/mt/103126734/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [edk2-devel] [PATCH v4] ArmVirt: Allow memory attributes protocol to be disabled
  2023-12-12 10:42   ` Ard Biesheuvel
@ 2023-12-12 15:22     ` Laszlo Ersek
  0 siblings, 0 replies; 4+ messages in thread
From: Laszlo Ersek @ 2023-12-12 15:22 UTC (permalink / raw)
  To: Ard Biesheuvel, Gerd Hoffmann
  Cc: Ard Biesheuvel, devel, Oliver Steffen, Alexander Graf,
	Oliver Smith-Denny, Taylor Beebe, Peter Jones, Leif Lindholm

On 12/12/23 11:42, Ard Biesheuvel wrote:
> On Tue, 12 Dec 2023 at 11:08, Gerd Hoffmann <kraxel@redhat.com> wrote:
>>
>> On Tue, Dec 12, 2023 at 09:36:00AM +0100, Ard Biesheuvel wrote:
>>> 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
>>
>> Tested-by: Gerd Hoffmann <kraxel@redhat.com>
>> Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>
>>
> 
> Thanks all - I've queued this up now.
> 

If it hasn't been merged yet, add:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

thanks
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112447): https://edk2.groups.io/g/devel/message/112447
Mute This Topic: https://groups.io/mt/103126734/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2023-12-12 15:23 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-12-12  8:36 [edk2-devel] [PATCH v4] ArmVirt: Allow memory attributes protocol to be disabled Ard Biesheuvel
2023-12-12 10:07 ` Gerd Hoffmann
2023-12-12 10:42   ` Ard Biesheuvel
2023-12-12 15:22     ` Laszlo Ersek

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox