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

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

[Ard Biesheuvel]

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

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

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

[Ard Biesheuvel]

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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Alexander Graf via groups.io]

On 08.12.23 12:20, Ard Biesheuvel wrote:
> 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.


No concerns from me :)

Alex





Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879




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

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

[Laszlo Ersek]

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")

(2) typo: "attribus"

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

> 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?

I think constructors should be the last resort.

(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.)

> 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.

> 
> +             &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.

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).

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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112233): https://edk2.groups.io/g/devel/message/112233
Mute This Topic: https://groups.io/mt/103031504/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Laszlo Ersek]

On 12/8/23 15:34, Laszlo Ersek wrote:

> (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.
> 
> 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).

Or maybe avoid modifying PlatformBootManagerLib completely; instead,
move the logic into CpuDxe, into a ready-to-boot event handler?

At that time, variable services should be available to CpuDxe as well
(for the BootOrder UEFI var check).

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112234): https://edk2.groups.io/g/devel/message/112234
Mute This Topic: https://groups.io/mt/103031504/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Ard Biesheuvel]

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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Laszlo Ersek]

On 12/8/23 16:34, Ard Biesheuvel wrote:
> On Fri, 8 Dec 2023 at 15:34, Laszlo Ersek <lersek@redhat.com> wrote:

>>> 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?

Yes, just a general remark; I generally don't have a surefire handle on
when to use a feature PCD versus a fixed-at-build (and/or
patchable-in-module) BOOLEAN PCD.

>> (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.

Ah, so the idea is to inject code between the memory attributes
protocol's installation and BdsDxe launching.


>> (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,

Understood. I agree to keep this contained, location-wise.

(I notice Gerd reports downthread though that limiting the protocol's
masking time-wise as well (i.e. to first boot) is not helpful, because
even rhel-9.3 grubaa64 has problems when the protocol is exposed. So a
simpler but broader approach could be better.)

Thanks!
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112300): https://edk2.groups.io/g/devel/message/112300
Mute This Topic: https://groups.io/mt/103031504/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Gerd Hoffmann]

On Thu, Dec 07, 2023 at 11:06:03AM +0100, 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.

Did some more testing meanwhile with latest shim.  Noticed things can
explode in other ways as well in case the memory attribute protocol is
present.

Specifically rhel-9.3 grub on aa64 crashes with latest shim.  Which I
suspect is that grub version not being NX-clean, and shim setting page
permissions via memory attribute protocol triggers that bug.  Didn't
analyze it yet though.

So, while I'd love to see some automatic way here I suspect trying to be
too clever does more harm than good.

take care,
  Gerd



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

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

[Ard Biesheuvel]

On Mon, Dec 11, 2023 at 10:06 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> On Thu, Dec 07, 2023 at 11:06:03AM +0100, 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.
>
> Did some more testing meanwhile with latest shim.  Noticed things can
> explode in other ways as well in case the memory attribute protocol is
> present.
>
> Specifically rhel-9.3 grub on aa64 crashes with latest shim.  Which I
> suspect is that grub version not being NX-clean, and shim setting page
> permissions via memory attribute protocol triggers that bug.  Didn't
> analyze it yet though.
>
> So, while I'd love to see some automatic way here I suspect trying to be
> too clever does more harm than good.
>

OK, so not worth the trouble of trying to detect the first boot, I guess.

For my info, is rhel-9.3 an old GRUB?


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

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

[Gerd Hoffmann]

  Hi,

> OK, so not worth the trouble of trying to detect the first boot, I guess.
> 
> For my info, is rhel-9.3 an old GRUB?

2.06 with a huge stack of downstream patches.

take care,
  Gerd



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