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

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

For context:

https://openfw.io/edk2-devel/g2ulyam7plpgrqlganhb5u2wtswq26civqlt4gpnxmjgq65yt7@umm3dta22cdz/T/#t

On Wed, 6 Dec 2023 at 13:51, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> > > We can disable the protocol via this method but how would you set it
> > > to =n by default?
> >
> > if (Status != EFI_SUCCESS)
> >     // opt/org.tiabocode/MemAttrProtocol not present on the qemu cmdline
> >     MemAttrProtocol = ThisBuildsDefault
> > }
>
> FYI: Below is what I'll add to the fedora builds.
>
> Rough plan:  Keep this until we have a fixed shim.efi and release media
> with that (hopefully Fedora 40 next spring).  At that point flip default
> to TRUE and keep it that way for a year or two.  Then drop the patch.
>

Yeah. I am not /quite/ ready to admit defeat, especially because other
systems (such as sbsa-ref) are suffering from the same problem, and so
fixing this in a QEMU specific way is probably not sufficient.

So what happens is:
- shim intends to load fbaa64.efi
- it allocates the region as EfiLoaderCode
- it sets XP and clears RP/RO on the entire region
- it copies and relocates the individual sections, and remaps them but
only if the alignment is >= 4k
- it calls the entrypoint which resides in a section that is still
mapped XP and boom

(this is based on the ubuntu cloud image).

Note that loading grub works fine, so once we've gone through
fbaa64.efi once, the issue goes away.

Shim itself does not have the NX compat attribute, nor does the
fbaa64.efi image that it is loading (afaict)

The EfiLoaderCode region has RWX permissions in this case. Future,
tightened firmware will not create EfiLoaderCode with RWX permissions,
but require the use of the EFI memory attributes protocol to create
executable regions.

The difficulty here is that shim never bothers to call the protocol at
all to remap the individual sections, as it notices that the alignment
is insufficient. So overriding the behavior at this point is
impossible.

But what we might do is invent a way to avoid setting the XP attribute
on the entire region based on some heuristic. Given that the main
purpose of the EFI memory attribute protocol is to provide the ability
to remove XP (and set RO instead), perhaps we can avoid the set
entirely? Just brainstorming here.

(cc'ing Taylor and Oliver given that this is related to the memory
policy work as well) Perhaps we can use the fact that the active image
is non-NX compat to make some tweaks?

What I really want to avoid is derail our effort to tighten things
down and comply with the NX compat related policies, by adding some
build time control that the distros will enable now and never disable
again, citing backward compat concerns.
And the deafening silence from the shim developers is not an
encouragement either.




> ------------------------------- cut here ----------------------------
> From c174197c65d2346f519418ded2e645d57423be41 Mon Sep 17 00:00:00 2001
> From: Gerd Hoffmann <kraxel@redhat.com>
> Date: Wed, 6 Dec 2023 13:00:53 +0100
> Subject: [PATCH 1/1] ArmVirtPkg: add runtime option to enable/disable
>  MemoryAttributesProtocol
>
> Based on a patch by Ard Biesheuvel <ardb@google.com>
>
> Usage:
>     qemu-system-aarch64 $args \
>     -fw_cfg name=opt/org.tianocore/MemAttrProtocol,string=y
>
> Default to 'n' (disabled) for now.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  .../PlatformBootManagerLib.inf                |  2 +
>  .../PlatformBootManagerLib/PlatformBm.c       | 69 +++++++++++++++++++
>  2 files changed, 71 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..a50b9aec0f2c 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,37 @@ PlatformBootManagerAfterConsole (
>    )
>  {
>    RETURN_STATUS  Status;
> +  BOOLEAN        MemAttrProtocol;
>
>    //
>    // 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/MemAttrProtocol",
> +             &MemAttrProtocol
> +             );
> +  if (RETURN_ERROR (Status)) {
> +    // default
> +    MemAttrProtocol = FALSE;
> +  }
> +
> +  DEBUG ((
> +    DEBUG_ERROR,
> +    "%a: MemAttrProtocol = %a\n",
> +    __func__,
> +    MemAttrProtocol ? "yes" : "no"
> +    ));
> +
> +  if (!MemAttrProtocol) {
> +    UninstallEfiMemoryAttributesProtocol ();
> +  }
> +
>    //
>    // Process QEMU's -kernel command line option. The kernel booted this way
>    // will receive ACPI tables: in PlatformBootManagerBeforeConsole(), we
> --
> 2.43.0
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112124): https://edk2.groups.io/g/devel/message/112124
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]
-=-=-=-=-=-=-=-=-=-=-=-