On Thu, 7 Dec 2023 at 11:06, Ard Biesheuvel <a...@google.com> wrote:
>
> From: Ard Biesheuvel <a...@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 <ler...@redhat.com>
> Cc: Gerd Hoffmann <kra...@redhat.com>
> Cc: Oliver Steffen <ostef...@redhat.com>
> Cc: Alexander Graf <g...@amazon.com>
> Cc: Oliver Smith-Denny <o...@linux.microsoft.com>
> Cc: Taylor Beebe <taylor.d.be...@gmail.com>
> Cc: Peter Jones <pjo...@redhat.com>
> Cc: Leif Lindholm <quic_llind...@quicinc.com>
> Link: https://gitlab.com/qemu-project/qemu/-/issues/1990
> Signed-off-by: Ard Biesheuvel <a...@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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
