Re: [edk2-devel] [PATCH] ArmVirtPkg: Allow EFI memory attributes protocol to be disabled

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

On Mon, 4 Dec 2023 at 10:52, Ard Biesheuvel <ardb@kernel.org> 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 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 <lersek@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 <kraxel@redhat.com>
> Cc: Oliver Steffen <osteffen@redhat.com>
> Cc: Alexander Graf <graf@amazon.com>
> Link: https://gitlab.com/qemu-project/qemu/-/issues/1990
> Signed-off-by: Ard Biesheuvel <ardb@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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-