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

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

On Thu, 7 Dec 2023 at 11:06, Ard Biesheuvel <ardb@google.com> 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 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>

OK, if nobody has problems with this approach, I intend to merge it
and give a snapshot build to the lima folks to incorporate in their
scripts.


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