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

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

Thanks for the review.

On Fri, 8 Dec 2023 at 15:34, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 12/7/23 11:06, 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 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>
> > ---
> >  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
> >
>
> (1) could be a feature PCD (although it couldn't be patchable-in-module
> then, and perhaps we don't consider this a "feature")
>

Is this a general remark on the similarity between feature PCDs and
boolean PCDs?

> (2) typo: "attribus"
>

Ack

> (3) for some reason, I see double line breaks.
>

Yeah :-(

I am struggling with the internal Google 'sendgmr' mailer which
switches to QP transfer encoding for some reason. (I lost the hosting
venue for my ThunderX2 workstation so I am now relying on Google
infrastructure to host my development system). I am working on this,

> > 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
> >
>
> I've made an effort to read through the v1 discussion (exhausting). Some
> quetions remain:
>
> (4) Why the change from an explicit call from AfterConsole to a
> constructor? Was AfterConsole too late somehow?
>

Yes. Checking for the existence of "BootOrder" needs to occur earlier,
or it will have been created by the BDS.

> I think constructors should be the last resort.
>

Not disagreeing with that.

> (5) Is the depex really necessary? BDS is supposed to start when all
> drivers have been dispatched, and so by that time, all of the UEFI
> architectural protocols should be available. (BDS will launch UEFI
> drivers, and all the UEFI drivers have an implicit depex on all the
> architectural protocols.)
>

The BDS arch protocol will be invoked at that point. but the BdsDxe
itself could be dispatched much earlier, at which point the
constructor of this library will be invoked.

And I'll need to include the CPU arch protocol as well here, as this
is installed at the same time as the EFI memory attributes protocol by
the CPU dxe driver.

> > 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);
> >
> > +}
>
> Looks OK to me.
>
> >
> > +
> >
> > +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,
>
> (6) "Attr" is optional; we could / should pass NULL here.
>

Good to know.

> >
> > +             &VarSize,
> >
> > +             NULL
> >
> > +             ) == EFI_NOT_FOUND)
> >
> > +  {
> >
> > +    Uninstall = FixedPcdGetBool (PcdUninstallMemAttrProtocolOnFirstBoot);
> >
> > +    QemuFwCfgParseBool ("opt/org.tianocore/UninstallMemAttrProtocolOnFirstBoot", &Uninstall);
> >
> > +    if (Uninstall) {
> >
> > +      UninstallEfiMemoryAttributesProtocol ();
> >
> > +    }
> >
> > +  }
> >
> > +
> >
> > +  return EFI_SUCCESS;
> >
> > +}
> >
>
> (7) Tying back to my point (4) -- I understand this is a hack anyway,
> but I'm still uncomfortable with platform BDS uninstalling a protocol
> that is owned by / provided by the CPU driver. Feels like a significant
> layering violation.
>

It is.

> Can we modify the CPU driver instead, to listen to a new event group,
> upon which being signaled, the CPU driver would uninstall the protocol
> (and close the listening event)?
>
> This PlatformBootManagerLib instance would act more or less the same
> (I'd suggest signaling the event group from within AfterConsole, in case
> the PCD default and/or the fw_cfg knob dictated that), but the protocol
> uninstallation would occur in "ArmPkg/Drivers/CpuDxe".
>
> In more technical terms, the layering violation IMO is that we mess with
> CpuDxe's "mCpuHandle" and "mMemoryAttribute" static variables from
> within BDS. Adding the new event group requires more boiler-plate code
> for sure, but there's a small code-size benefit as well: we'd not have
> to look up either the handle (with LocateHandle) or the protocol
> interface (with HandleProtocol), as CpuDxe inherently knows those
> (mCpuHandle, mMemoryAttribute).
>

I agree with your analysis here. But I am reluctant to introduce
elaborate infrastructure across drivers to implement a feature that
should not exist in the first place.

As I mentioned a couple of times, I am rather unhappy with the
complete lack of involvement of the people who created this mess in
the first place, and what I am after is really a minimal, local hack
that unblocks the actual end users (people running LIMA on ARM based
Macs) without creating building blocks that will be used by the distro
forks to erode the original functionality even further,


> Thanks for considering (and sorry for butting in this late...)

No worries. Thanks for the review.


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