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

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

[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 executable permissions from
the memory allocation, 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.

  -fw_cfg opt/org.tianocore/DisableMemAttrProtocol,string=y

Cc: L�szl� �rsek <lersek@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Oliver Steffen <osteffen@redhat.com>
Cc: Alexander Graf <graf@amazon.com>
Link: https://gitlab.com/qemu-project/qemu/-/issues/1990
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf |  2 +
 ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c               | 56 ++++++++++++++++++++
 2 files changed, 58 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..e17899100e4a 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,24 @@ PlatformBootManagerAfterConsole (
   )

 {

   RETURN_STATUS  Status;

+  BOOLEAN        FwCfgBool;

 

   //

   // 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/DisableMemAttrProtocol",

+             &FwCfgBool);

+  if (!RETURN_ERROR (Status) && FwCfgBool) {

+    UninstallEfiMemoryAttributesProtocol ();

+  }

+

   //

   // Process QEMU's -kernel command line option. The kernel booted this way

   // will receive ACPI tables: in PlatformBootManagerBeforeConsole(), we

--
2.43.0.rc2.451.g8631bc7472-goog



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

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

[Ard Biesheuvel]

On Mon, 4 Dec 2023 at 10:52, Ard Biesheuvel <ardb@kernel.org> 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 executable permissions from
> the memory allocation, 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.
>
>   -fw_cfg opt/org.tianocore/DisableMemAttrProtocol,string=y
>
> Cc: L�szl� �rsek <lersek@redhat.com>

Apologies for the alphabet soup. The Google internal mailer appears to
have changed the content transfer encoding from 8bit to qp because it
spotted a long line (??)


> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Oliver Steffen <osteffen@redhat.com>
> Cc: Alexander Graf <graf@amazon.com>
> Link: https://gitlab.com/qemu-project/qemu/-/issues/1990
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf |  2 +
>  ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c               | 56 ++++++++++++++++++++
>  2 files changed, 58 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..e17899100e4a 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,24 @@ PlatformBootManagerAfterConsole (
>    )
>
>  {
>
>    RETURN_STATUS  Status;
>
> +  BOOLEAN        FwCfgBool;
>
>
>
>    //
>
>    // 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/DisableMemAttrProtocol",
>
> +             &FwCfgBool);
>
> +  if (!RETURN_ERROR (Status) && FwCfgBool) {
>
> +    UninstallEfiMemoryAttributesProtocol ();
>
> +  }
>
> +
>
>    //
>
>    // Process QEMU's -kernel command line option. The kernel booted this way
>
>    // will receive ACPI tables: in PlatformBootManagerBeforeConsole(), we
>
> --
> 2.43.0.rc2.451.g8631bc7472-goog
>
>
>
> 
>
>


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

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

[Alexander Graf via groups.io]

On 04.12.23 10:52, 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 executable permissions from
> the memory allocation, 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.
>
>    -fw_cfg opt/org.tianocore/DisableMemAttrProtocol,string=y
>
> Cc: L�szl� �rsek <lersek@redhat.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Oliver Steffen <osteffen@redhat.com>
> Cc: Alexander Graf <graf@amazon.com>
> Link: https://gitlab.com/qemu-project/qemu/-/issues/1990
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>


Could you please add a PCD value that allows us to set the default to 
disabled? I believe we want to have at least an interim phase where we 
allow the "old" behavior by default, without modification of QEMU 
command line issuing components.

I'm happy to leave the default to the new behavior upstream, but with 
the PCD value, distributions like homebrew can easily unblock themselves 
from updating to the latest edk2 without touching every single 
downstream user of QEMU.

> ---
>   ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf |  2 +
>   ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c               | 56 ++++++++++++++++++++
>   2 files changed, 58 insertions(+)
>

[...]

> diff --git a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
> index 85c01351b09d..e17899100e4a 100644
> --- a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
> +++ b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c


[...]


>
>
>
> +  //
>
> +  // 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/DisableMemAttrProtocol",
>
> +             &FwCfgBool);
>
> +  if (!RETURN_ERROR (Status) && FwCfgBool) {
>
> +    UninstallEfiMemoryAttributesProtocol ();
>
> +  }


In a nutshell, I think adding the PCD definition from 
https://edk2.groups.io/g/devel/topic/99631663 with modified lifetime 
scope and writing it as something like

   if (RETURN_ERROR (Status))
     FwCfgBool = FALSE;
   if (FwCfgBool || !PcdGetBool(PcdEnableEfiMemoryAttributeProtocol))
     UninstallEfiMemoryAttributesProtocol ();


would solve most cases. Distros can then set the PCD value downstream 
while tools can decide whether they want to bubble up the flag. It 
doesn't solve "old shim" automatically unfortunately, but with this 
hopefully we'll buy distros some time to clean up their shim story.

(hint: You really don't want or need shim on ARM. The only reason for 
shim is that on most x86 desktop systems, users will have the MS keys 
preinstalled. The MS Secure Boot concept however is terribly broken: Any 
compromise of any of the MS signed binaries jeopardizes your boot chain. 
You're a lot better off installing *only* your distribution's key 
material. That way you at least you know who you trust. Just remove 
shim. Have a look at how Amazon Linux 2023 did it [2] :))


Alex

[1] 
https://docs.aws.amazon.com/linux/al2023/ug/uefi-secure-boot.html#shim-use




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

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

[Gerd Hoffmann]

> So let's introduce a QEMU command line option to indicate that the
> protocol should not be exposed at all.
> 
>   -fw_cfg opt/org.tianocore/DisableMemAttrProtocol,string=y

Can we name this 'MemAttrProtocol={y,n}' so it works both ways (enabling
and disabling) without double negative?

The fedora distro builds have the protocol disabled, and I'll keep it
that way until we finally have fixed shim.efi builds.  Having the option
to enable this would be nice though.

take care,
  Gerd



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

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

[Ard Biesheuvel]

On Mon, Dec 4, 2023 at 11:45 AM Alexander Graf <graf@amazon.com> wrote:
>
>
> On 04.12.23 10:52, 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 executable permissions from
> > the memory allocation, 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.
> >
> >    -fw_cfg opt/org.tianocore/DisableMemAttrProtocol,string=y
> >
> > Cc: L�szl� �rsek <lersek@redhat.com>
> > Cc: Gerd Hoffmann <kraxel@redhat.com>
> > Cc: Oliver Steffen <osteffen@redhat.com>
> > Cc: Alexander Graf <graf@amazon.com>
> > Link: https://gitlab.com/qemu-project/qemu/-/issues/1990
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>
>
> Could you please add a PCD value that allows us to set the default to
> disabled? I believe we want to have at least an interim phase where we
> allow the "old" behavior by default, without modification of QEMU
> command line issuing components.
>

The old behavior is a working combination of firmware and QEMU. The
problem manifests itself now that QEMU is updating its bundled
firmware image.

So if the override needs to be on by default, QEMU can take care of that itself.

> I'm happy to leave the default to the new behavior upstream, but with
> the PCD value, distributions like homebrew can easily unblock themselves
> from updating to the latest edk2 without touching every single
> downstream user of QEMU.
>

edk2 is not a homebrew package. QEMU is, and it bundles the firmware.
So the right place to handle this is QEMU, and this patch gives them
an opportunity to do so without the need to fork the edk2 source code.

Adding a PCD is not going to help - we tried that 7+ years ago with
the default memory permissions on LoaderCode vs LoaderData, and the
distros simply ignored the upstream GRUB changes and kept carrying
their own hacks.

I think having an override like the one I am proposing here is as far
as I am willing to go in terms of disabling security features to
accommodate crap software like shim.



> (hint: You really don't want or need shim on ARM. The only reason for
> shim is that on most x86 desktop systems, users will have the MS keys
> preinstalled. The MS Secure Boot concept however is terribly broken: Any
> compromise of any of the MS signed binaries jeopardizes your boot chain.
> You're a lot better off installing *only* your distribution's key
> material. That way you at least you know who you trust. Just remove
> shim. Have a look at how Amazon Linux 2023 did it [2] :))
>

I have been saying the same thing since 2013, which is when I
implemented secure boot support in Tianocore for AArch64.

The distros want shim, and claim they know what they are doing - who
am I to challenge that.


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

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

[Ard Biesheuvel]

On Mon, Dec 4, 2023 at 11:53 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> > So let's introduce a QEMU command line option to indicate that the
> > protocol should not be exposed at all.
> >
> >   -fw_cfg opt/org.tianocore/DisableMemAttrProtocol,string=y
>
> Can we name this 'MemAttrProtocol={y,n}' so it works both ways (enabling
> and disabling) without double negative?
>

Sure, but with the same behavior, right?

=y means it will get installed
=n means it will get installed and uninstalled again

> The fedora distro builds have the protocol disabled, and I'll keep it
> that way until we finally have fixed shim.efi builds.  Having the option
> to enable this would be nice though.
>

So how did you disable the protocol? That part is not upstream afaik.

We can disable the protocol via this method but how would you set it
to =n by default?


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

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

[Gerd Hoffmann]

On Mon, Dec 04, 2023 at 11:57:43AM +0100, Ard Biesheuvel wrote:
> On Mon, Dec 4, 2023 at 11:53 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
> >
> > > So let's introduce a QEMU command line option to indicate that the
> > > protocol should not be exposed at all.
> > >
> > >   -fw_cfg opt/org.tianocore/DisableMemAttrProtocol,string=y
> >
> > Can we name this 'MemAttrProtocol={y,n}' so it works both ways (enabling
> > and disabling) without double negative?
> >
> 
> Sure, but with the same behavior, right?
> 
> =y means it will get installed
> =n means it will get installed and uninstalled again
> 
> > The fedora distro builds have the protocol disabled, and I'll keep it
> > that way until we finally have fixed shim.efi builds.  Having the option
> > to enable this would be nice though.
> >
> 
> So how did you disable the protocol? That part is not upstream afaik.

Yes, right now it's a fedora-specific patch.  Which I'd drop in favor of
this patch, or a slightly modified version of it.

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

take care,
  Gerd



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

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

[Gerd Hoffmann]

  Hi,

> (hint: You really don't want or need shim on ARM. The only reason for shim
> is that on most x86 desktop systems, users will have the MS keys
> preinstalled. The MS Secure Boot concept however is terribly broken: Any
> compromise of any of the MS signed binaries jeopardizes your boot chain.
> You're a lot better off installing *only* your distribution's key material.
> That way you at least you know who you trust. Just remove shim. Have a look
> at how Amazon Linux 2023 did it [2] :))

You are in the luxurious position to run your own distro on your own
platform, which makes this totally easy.

The RH bootloader team considers shim.efi being an essential part of the
boot chain (to the point that the distro grub.efi throws errors with
secure boot being enabled and shim.efi missing), and on x86 bare metal
it actually is essential because hardware usually ships with only the
microsoft certificate enrolled.

At least they promised to sign shim with both distro and microsoft keys
on the next update, so I have the option to enroll the distro instead of
the micosoft keys in 'db' on platforms where this is possible.

take care,
  Gerd



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

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

[Alexander Graf via groups.io]

On 04.12.23 13:20, Gerd Hoffmann wrote:
>    Hi,
>
>> (hint: You really don't want or need shim on ARM. The only reason for shim
>> is that on most x86 desktop systems, users will have the MS keys
>> preinstalled. The MS Secure Boot concept however is terribly broken: Any
>> compromise of any of the MS signed binaries jeopardizes your boot chain.
>> You're a lot better off installing *only* your distribution's key material.
>> That way you at least you know who you trust. Just remove shim. Have a look
>> at how Amazon Linux 2023 did it [2] :))
> You are in the luxurious position to run your own distro on your own
> platform, which makes this totally easy.


Sure, we're cheating a bit on x86. But for ARM, the same story holds 
true for RH as well. There are a solid number of ARM systems that 
implement UEFI Secure Boot today - and none of them (that I'm aware of) 
provision a Microsoft 3rd party key. I think we're all better off as 
community if we don't repeat the mistakes we did on x86 on ARM.

In fact, for virtual machines you're in the exact same position as EC2: 
If virt-install only provisions Red Hat Secure Boot keys by default when 
you install Fedora or RHEL guests, you've already increased your guests' 
security posture significantly.

The same applies to RHEL-on-EC2, where you can bring your own db.


> The RH bootloader team considers shim.efi being an essential part of the
> boot chain (to the point that the distro grub.efi throws errors with
> secure boot being enabled and shim.efi missing), and on x86 bare metal
> it actually is essential because hardware usually ships with only the
> microsoft certificate enrolled.


See above, the key (in your case) is to not treat ARM and x86 identical. 
And yes, the (downstream) shim patches for grub break normal grub secure 
boot support. But that's a bug - not a feature :).

Once you sorted that bit out, we can start talking about paths to remove 
shim on select x86 environments such as VMs.


> At least they promised to sign shim with both distro and microsoft keys
> on the next update, so I have the option to enroll the distro instead of
> the micosoft keys in 'db' on platforms where this is possible.


Shim really has no place in a world where you have a distro key 
enrolled. Fight the battle please, we'll all be off with an easier boot 
stack as a result :). This bug here alone already shows you why shim is 
a bad idea conceptually. Necessary in an MS dominated world, but still bad.

If there are concerns around tooling differences (like mokutil), let's 
look at how we can unify/simplify the user experience instead.


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

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

[Ard Biesheuvel]

On Mon, 4 Dec 2023 at 13:38, Alexander Graf <graf@amazon.com> wrote:
>
>
> On 04.12.23 13:20, Gerd Hoffmann wrote:
> >    Hi,
> >
> >> (hint: You really don't want or need shim on ARM. The only reason for shim
> >> is that on most x86 desktop systems, users will have the MS keys
> >> preinstalled. The MS Secure Boot concept however is terribly broken: Any
> >> compromise of any of the MS signed binaries jeopardizes your boot chain.
> >> You're a lot better off installing *only* your distribution's key material.
> >> That way you at least you know who you trust. Just remove shim. Have a look
> >> at how Amazon Linux 2023 did it [2] :))
> > You are in the luxurious position to run your own distro on your own
> > platform, which makes this totally easy.
>
>
> Sure, we're cheating a bit on x86. But for ARM, the same story holds
> true for RH as well. There are a solid number of ARM systems that
> implement UEFI Secure Boot today - and none of them (that I'm aware of)
> provision a Microsoft 3rd party key. I think we're all better off as
> community if we don't repeat the mistakes we did on x86 on ARM.
>
> In fact, for virtual machines you're in the exact same position as EC2:
> If virt-install only provisions Red Hat Secure Boot keys by default when
> you install Fedora or RHEL guests, you've already increased your guests'
> security posture significantly.
>
> The same applies to RHEL-on-EC2, where you can bring your own db.
>
>
> > The RH bootloader team considers shim.efi being an essential part of the
> > boot chain (to the point that the distro grub.efi throws errors with
> > secure boot being enabled and shim.efi missing), and on x86 bare metal
> > it actually is essential because hardware usually ships with only the
> > microsoft certificate enrolled.
>
>
> See above, the key (in your case) is to not treat ARM and x86 identical.
> And yes, the (downstream) shim patches for grub break normal grub secure
> boot support. But that's a bug - not a feature :).
>
> Once you sorted that bit out, we can start talking about paths to remove
> shim on select x86 environments such as VMs.
>
>
> > At least they promised to sign shim with both distro and microsoft keys
> > on the next update, so I have the option to enroll the distro instead of
> > the micosoft keys in 'db' on platforms where this is possible.
>
>
> Shim really has no place in a world where you have a distro key
> enrolled. Fight the battle please, we'll all be off with an easier boot
> stack as a result :). This bug here alone already shows you why shim is
> a bad idea conceptually. Necessary in an MS dominated world, but still bad.
>
> If there are concerns around tooling differences (like mokutil), let's
> look at how we can unify/simplify the user experience instead.
>

I don't think it helps to go off on a tangent about why shim exists
and why it is so terrible, as I don't think there is actually any
disagreement about that. But now that we are, let me add my 2c as well
:-)

For the patch under discussion here, I think that Gerd's suggestion is
to have both a PCD and a QEMU variable, and use the PCD unless the
variable has a value. I'm on the fence here: I would like to
accommodate users, but adding another control that the distros are
just going to set and forget is just going to make the mess bigger.

What is even worse: arm64 system firmware will have to deal with this
as well, and disable the protocol in order to run distro installers.
And once the tightened MS requirements for NX compat come into effect,
they will have to add another workaround for this as well, and so
we'll probably end up with generations of arm64 hardware with a
'enable memory attributes protocol' option in their BIOS menus. And
guess what the default setting is likely to be?

I am quite disappointed with the complete lack of involvement from the
folks who develop and deploy shim, and instead, third parties (and
users) are the ones chasing me and people like Gerd (who work on QEMU
or EDK2 rather than shim) to clean up the mess.

If the shim developers (or anyone else) can suggest some kind of
heuristic for deciding whether a broken shim is calling into the
protocol, I'd be more than happy to add more code to avoid the need
for a QEMU command line option in the common case. But just turning it
off via a PCD or otherwise is not something I am willing to entertain.


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

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

[Gerd Hoffmann]

  Hi,

> > > That way you at least you know who you trust. Just remove shim. Have a look
> > > at how Amazon Linux 2023 did it [2] :))

> > You are in the luxurious position to run your own distro on your own
> > platform, which makes this totally easy.

> Sure, we're cheating a bit on x86. But for ARM, the same story holds true
> for RH as well. There are a solid number of ARM systems that implement UEFI
> Secure Boot today - and none of them (that I'm aware of) provision a
> Microsoft 3rd party key.

Didn't got my hands on any such arm system.

How do you enroll the keys on those systems?

Do they offer the option to read the 'db' keys from files on distro boot
media?  Or do they expect the distro boot loader (or installer) to enroll
keys and enable secure boot (which is supported by systemd-boot I think)?

> In fact, for virtual machines you're in the exact same position as EC2: If
> virt-install only provisions Red Hat Secure Boot keys by default when you
> install Fedora or RHEL guests, you've already increased your guests'
> security posture significantly.

Well, no.  One problem is install media, where you really have only
one entry point (EFI/BOOT/BOOT${ARCH}.EFI) which must work under all
circumstances.  Which must be shim with microsoft signature (and ideally
distro signature too) on x64.

When booting cloud images and the platform allowing for
'bring-your-own-varstore', then yes, it is simpler and doable.

> > The RH bootloader team considers shim.efi being an essential part of the
> > boot chain (to the point that the distro grub.efi throws errors with
> > secure boot being enabled and shim.efi missing), and on x86 bare metal
> > it actually is essential because hardware usually ships with only the
> > microsoft certificate enrolled.

> See above, the key (in your case) is to not treat ARM and x86 identical. And
> yes, the (downstream) shim patches for grub break normal grub secure boot
> support. But that's a bug - not a feature :).

I'm with you on that.  Unfortunately the boot loader team is not.

https://bugzilla.redhat.com/show_bug.cgi?id=2108083

tl;dr: You can't boot Fedora in secure boot mode without microsoft keys
today.  grub.efi refuses to work without shim.efi, and shim.efi exists
only in a microsoft-signed version (which differed from rhel were a
second, redhat-signed shim binary exists).

Oh, and the above applies to x86 only.  On arm fedora shim.efi is not
signed and grub.efi is signed with the (public) redhat test keys.

> > At least they promised to sign shim with both distro and microsoft keys
> > on the next update, so I have the option to enroll the distro instead of
> > the micosoft keys in 'db' on platforms where this is possible.
> 
> Shim really has no place in a world where you have a distro key enrolled.

Except for https://github.com/rhboot/shim/blob/main/SBAT.md

> Fight the battle please, we'll all be off with an easier boot stack as a
> result :).

Trust me, I had my fair share of battles already, and I still have
multiple open bug reports.

> If there are concerns around tooling differences (like mokutil), let's look
> at how we can unify/simplify the user experience instead.

IMHO it essentially it comes down to standardize a few things.  Most
importantly placing the distro secure boot certificate on some
well-known location on the install media, so tooling like virt-install
can pre-load it into 'db' of the guest it is going to install.
Something similar for cloud images, so OpenStack / EC2 / whatever can do
the same.

take care,
  Gerd



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

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

[Ard Biesheuvel]

On Mon, 4 Dec 2023 at 15:52, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>   Hi,
>
> > > > That way you at least you know who you trust. Just remove shim. Have a look
> > > > at how Amazon Linux 2023 did it [2] :))
>
> > > You are in the luxurious position to run your own distro on your own
> > > platform, which makes this totally easy.
>
> > Sure, we're cheating a bit on x86. But for ARM, the same story holds true
> > for RH as well. There are a solid number of ARM systems that implement UEFI
> > Secure Boot today - and none of them (that I'm aware of) provision a
> > Microsoft 3rd party key.
>
> Didn't got my hands on any such arm system.
>
> How do you enroll the keys on those systems?
>
> Do they offer the option to read the 'db' keys from files on distro boot
> media?  Or do they expect the distro boot loader (or installer) to enroll
> keys and enable secure boot (which is supported by systemd-boot I think)?
>
> > In fact, for virtual machines you're in the exact same position as EC2: If
> > virt-install only provisions Red Hat Secure Boot keys by default when you
> > install Fedora or RHEL guests, you've already increased your guests'
> > security posture significantly.
>
> Well, no.  One problem is install media, where you really have only
> one entry point (EFI/BOOT/BOOT${ARCH}.EFI) which must work under all
> circumstances.  Which must be shim with microsoft signature (and ideally
> distro signature too) on x64.
>
> When booting cloud images and the platform allowing for
> 'bring-your-own-varstore', then yes, it is simpler and doable.
>
> > > The RH bootloader team considers shim.efi being an essential part of the
> > > boot chain (to the point that the distro grub.efi throws errors with
> > > secure boot being enabled and shim.efi missing), and on x86 bare metal
> > > it actually is essential because hardware usually ships with only the
> > > microsoft certificate enrolled.
>
> > See above, the key (in your case) is to not treat ARM and x86 identical. And
> > yes, the (downstream) shim patches for grub break normal grub secure boot
> > support. But that's a bug - not a feature :).
>
> I'm with you on that.  Unfortunately the boot loader team is not.
>
> https://bugzilla.redhat.com/show_bug.cgi?id=2108083
>
> tl;dr: You can't boot Fedora in secure boot mode without microsoft keys
> today.  grub.efi refuses to work without shim.efi, and shim.efi exists
> only in a microsoft-signed version (which differed from rhel were a
> second, redhat-signed shim binary exists).
>
> Oh, and the above applies to x86 only.  On arm fedora shim.efi is not
> signed and grub.efi is signed with the (public) redhat test keys.
>

So what is holding Fedora back from providing a fixed shim binary if
it doesn't need to be signed by Microsoft? And also, the only
problematic binary in the boot chain appears to be fbaa64.efi - that
one could be fixed as well, by using 4k aligned executable sections.

To be honest (and I know I am preaching to the choir here), the more i
learn about this, the less I am inclined to make *any* accommodations
whatsoever, given that the boot loader team obviously does not give a
shit about their crappy boot chain.


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

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

[Gerd Hoffmann]

  Hi,

> > I'm with you on that.  Unfortunately the boot loader team is not.
> >
> > https://bugzilla.redhat.com/show_bug.cgi?id=2108083
> >
> > tl;dr: You can't boot Fedora in secure boot mode without microsoft keys
> > today.  grub.efi refuses to work without shim.efi, and shim.efi exists
> > only in a microsoft-signed version (which differed from rhel were a
> > second, redhat-signed shim binary exists).
> >
> > Oh, and the above applies to x86 only.  On arm fedora shim.efi is not
> > signed and grub.efi is signed with the (public) redhat test keys.
> 
> So what is holding Fedora back from providing a fixed shim binary if
> it doesn't need to be signed by Microsoft?

I'd love to have an serious answer for that question, but I havn't.
Usually I get either no answer, or something along the lines of
"-ENOTIME because of $otherwork".

Right now waiting for v6.7-final before sending a new shim.efi to
microsoft for signing and the desire to keep all archs in sync also
plays a role (I guess).

Technically there is no good reason, fedora even has a separate
shim-unsigned-$arch.rpm which could be up-to-date all the time on all
architectures, independent from the microsoft signing process.  But that
is right now at v15.6, which is not even the latest (v15.7) release.
And 15.7 is more than one year old already ...

> To be honest (and I know I am preaching to the choir here), the more i
> learn about this, the less I am inclined to make *any* accommodations
> whatsoever, given that the boot loader team obviously does not give a
> shit about their crappy boot chain.

Can understand that sentiment.  Problem is this hits the wrong people,
and the fallout goes beyond rhel + fedora because the rh team also
maintains upstream shim.

take care,
  Gerd



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

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

[Marcin Juszkiewicz]

W dniu 4.12.2023 o 13:58, Ard Biesheuvel pisze:
> On Mon, 4 Dec 2023 at 13:38, Alexander Graf <graf@amazon.com> wrote:
>> On 04.12.23 13:20, Gerd Hoffmann wrote:

> I don't think it helps to go off on a tangent about why shim exists
> and why it is so terrible, as I don't think there is actually any
> disagreement about that. But now that we are, let me add my 2c as well
> :-)
> 
> For the patch under discussion here, I think that Gerd's suggestion is
> to have both a PCD and a QEMU variable, and use the PCD unless the
> variable has a value. I'm on the fence here: I would like to
> accommodate users, but adding another control that the distros are
> just going to set and forget is just going to make the mess bigger.
> 
> What is even worse: arm64 system firmware will have to deal with this
> as well, and disable the protocol in order to run distro installers.
> And once the tightened MS requirements for NX compat come into effect,
> they will have to add another workaround for this as well, and so
> we'll probably end up with generations of arm64 hardware with a
> 'enable memory attributes protocol' option in their BIOS menus. And
> guess what the default setting is likely to be?
> 
> I am quite disappointed with the complete lack of involvement from the
> folks who develop and deploy shim, and instead, third parties (and
> users) are the ones chasing me and people like Gerd (who work on QEMU
> or EDK2 rather than shim) to clean up the mess.

I use 'sbsa-ref' with QEMU and upstream EDK2. And cannot use either RHEL 
9.3 nor CentOS Stream 9 installers because they hang.

And this is not the only platform where upstream EDK2 is used.

Sure, I can hack something, use Grub from Debian or Fedora and get 
things working but that's not solution.

Adding flags in 'Broken OS support' section of EDK2 settings feels like 
bad idea. Especially when EFI app generating issues is developed by 
company known for FOSS work.



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

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

[Alexander Graf via groups.io]

On 04.12.23 15:52, Gerd Hoffmann wrote:
>    Hi,
>
>>>> That way you at least you know who you trust. Just remove shim. Have a look
>>>> at how Amazon Linux 2023 did it [2] :))
>>> You are in the luxurious position to run your own distro on your own
>>> platform, which makes this totally easy.
>> Sure, we're cheating a bit on x86. But for ARM, the same story holds true
>> for RH as well. There are a solid number of ARM systems that implement UEFI
>> Secure Boot today - and none of them (that I'm aware of) provision a
>> Microsoft 3rd party key.
> Didn't got my hands on any such arm system.
>
> How do you enroll the keys on those systems?
>
> Do they offer the option to read the 'db' keys from files on distro boot
> media?  Or do they expect the distro boot loader (or installer) to enroll
> keys and enable secure boot (which is supported by systemd-boot I think)?


I know of 3 categories:

1) U-Boot

https://docs.u-boot.org/en/latest/develop/uefi/uefi.html#configuring-uefi-secure-boot

The target audience for UEFI Secure Boot in U-Boot is more embedded: You 
would typically ship a system with a prepopulated db.

2) EC2

https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/create-ami-with-uefi-secure-boot.html

Similar to the above: Whoever builds an AMI is also responsible for 
shipping a viable 'db' that matches its payload.

3) Windows ARM notebooks

As far as I remember, these do use UEFI Secure Boot, but only contain 
the 1st party Microsoft keys, no 3rd party keys. So they're like the 
cases above, but with Windows as OS target.


I'm not aware of any environment today that makes installation of db 
keys easy. But that doesn't mean that nobody uses UEFI Secure Boot :).


>
>> In fact, for virtual machines you're in the exact same position as EC2: If
>> virt-install only provisions Red Hat Secure Boot keys by default when you
>> install Fedora or RHEL guests, you've already increased your guests'
>> security posture significantly.
> Well, no.  One problem is install media, where you really have only
> one entry point (EFI/BOOT/BOOT${ARCH}.EFI) which must work under all
> circumstances.  Which must be shim with microsoft signature (and ideally
> distro signature too) on x64.


What if that shim immediately on start tries to see if it can load and 
execute another binary at the same path instead? Then you could have it 
directly jump into your real payload if that works successfully. If it 
does not, it (most likely) means the signature is wrong and you need to 
jump through the actual shim code path.


That way you get shim out of the way for the non-secure-boot and the 
distro-key-installed cases.

> When booting cloud images and the platform allowing for
> 'bring-your-own-varstore', then yes, it is simpler and doable.
>
>>> The RH bootloader team considers shim.efi being an essential part of the
>>> boot chain (to the point that the distro grub.efi throws errors with
>>> secure boot being enabled and shim.efi missing), and on x86 bare metal
>>> it actually is essential because hardware usually ships with only the
>>> microsoft certificate enrolled.
>> See above, the key (in your case) is to not treat ARM and x86 identical. And
>> yes, the (downstream) shim patches for grub break normal grub secure boot
>> support. But that's a bug - not a feature :).
> I'm with you on that.  Unfortunately the boot loader team is not.
>
> https://bugzilla.redhat.com/show_bug.cgi?id=2108083
>
> tl;dr: You can't boot Fedora in secure boot mode without microsoft keys
> today.  grub.efi refuses to work without shim.efi, and shim.efi exists
> only in a microsoft-signed version (which differed from rhel were a
> second, redhat-signed shim binary exists).
>
> Oh, and the above applies to x86 only.  On arm fedora shim.efi is not
> signed and grub.efi is signed with the (public) redhat test keys.
>
>>> At least they promised to sign shim with both distro and microsoft keys
>>> on the next update, so I have the option to enroll the distro instead of
>>> the micosoft keys in 'db' on platforms where this is possible.
>> Shim really has no place in a world where you have a distro key enrolled.
> Except for https://github.com/rhboot/shim/blob/main/SBAT.md


I'm not quite sure what you're getting at :).

First, the fundamental problem with huge dbx files is *precisely* 
because the MS key is used in too many places. Smaller set of key scope 
means smaller dbx.

Second, I think the basic concept of introducing a new abstracted 
"version" that dbx can match against is great. It would make dbx updates 
a lot more efficient. So why don't we include that concept in the UEFI spec?


>
>> Fight the battle please, we'll all be off with an easier boot stack as a
>> result :).
> Trust me, I had my fair share of battles already, and I still have
> multiple open bug reports.


Thank you!


>
>> If there are concerns around tooling differences (like mokutil), let's look
>> at how we can unify/simplify the user experience instead.
> IMHO it essentially it comes down to standardize a few things.  Most
> importantly placing the distro secure boot certificate on some
> well-known location on the install media, so tooling like virt-install
> can pre-load it into 'db' of the guest it is going to install.
> Something similar for cloud images, so OpenStack / EC2 / whatever can do
> the same.


I don't know if putting it on a standardized location is really 
necessary. If you boot with SB disabled and just as part of the 
installation process install the distro keys, everything should fall 
into place nicely, no?

For OpenStack/EC2/whatever, you don't run the installer, so you don't 
need a standardized location for it. For EC2 the target image is 
completely opaque. We don't even have (or want to have) a storage or 
file system driver to access it. That's why it's in metadata (UefiData), 
separate from the disk image.


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

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

[Gerd Hoffmann]

  Hi,

> I know of 3 categories:
> 
> 1) U-Boot
> 2) EC2
> 3) Windows ARM notebooks

Thanks.

> > Well, no.  One problem is install media, where you really have only
> > one entry point (EFI/BOOT/BOOT${ARCH}.EFI) which must work under all
> > circumstances.  Which must be shim with microsoft signature (and ideally
> > distro signature too) on x64.
> 
> What if that shim immediately on start tries to see if it can load and
> execute another binary at the same path instead?

One more possible code path.  I have my doubts that (a) the shim
maintainers like the idea, and (b) that it actually improves the overall
situation.

Also note that it isn't a fixed binary.  While grub.efi is the default
you can pass something else on the command line, for example fwupd.efi
or an UKI (unified kernel image).  Also depending on circumstances shim
decides to load fallback,efi or mokmanager.efi instead of grub.efi.  So
it's more complex that "try load grub.efi using standard efi protocols
and if that works I'm done".

> > Except for https://github.com/rhboot/shim/blob/main/SBAT.md
> 
> I'm not quite sure what you're getting at :).
> 
> First, the fundamental problem with huge dbx files is *precisely* because
> the MS key is used in too many places. Smaller set of key scope means
> smaller dbx.
> 
> Second, I think the basic concept of introducing a new abstracted "version"
> that dbx can match against is great. It would make dbx updates a lot more
> efficient. So why don't we include that concept in the UEFI spec?

Not sure what the status of this is, whenever it is just some kind of
agreement between shim maintainers and microsoft, trying to keep dbx
grow under control, or whenever there are any plans to add that to the
uefi specs.  Peter?

Adding that to the uefi specs (and subsequently implement it in edk2)
has the advantage that it wouldn't depend on shim.efi, and have the
drawback that it'll take ages to have an effect due to the firmware
update support for a big chunks of physical hardware being shitty (not
that shim updates look that much better right now ...)

My impression is that microsoft tries to improve the firmware security
situation without depending on firmware updates if possible.  Being more
strict on the PE binaries they are willing to sign for secure boot is
one step which goes into that category.

> > IMHO it essentially it comes down to standardize a few things.  Most
> > importantly placing the distro secure boot certificate on some
> > well-known location on the install media, so tooling like virt-install
> > can pre-load it into 'db' of the guest it is going to install.
> > Something similar for cloud images, so OpenStack / EC2 / whatever can do
> > the same.
> 
> I don't know if putting it on a standardized location is really necessary.
> If you boot with SB disabled and just as part of the installation process
> install the distro keys, everything should fall into place nicely, no?

Yes.  Defining the key enrollment being job of the installer would work,
at least for the cases where an installation actually happens.

> For OpenStack/EC2/whatever, you don't run the installer, so you don't need a
> standardized location for it. For EC2 the target image is completely opaque.
> We don't even have (or want to have) a storage or file system driver to
> access it. That's why it's in metadata (UefiData), separate from the disk
> image.

There is no standard for metadata though, along the lines of "when using
this disk image with secure boot you should use that uefi varstore".
What we have today is ec2-specific, you have to take care when creating
the AMI.

take care,
  Gerd



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

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

[Gerd Hoffmann]

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

take care,
  Gerd

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

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

[Ard Biesheuvel]

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

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

[Gerd Hoffmann]

  Hi,

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

Can the fault handler deal with this?  Set a flag somewhere, print a
big'n'fat error message, wait 5 secs, reset machine?  After reset the
firmware will see the flag and come up in 'compat' instead of 'strict'
mode.

Not sure what a good place for such a flag would be.  Do we have other
options than a non-volatile efi variable?  When using a efi variable we
probably should add an 'expires' timestamp, so the machine doesn't stay
in 'compat' mode forever.

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

Memory policies would be another candidate which could possibly use a
less strict profile in 'compat' mode.  I'd love to see memory policies
land for the February stable tag.

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

Sure, I want that too.  Having an runtime switch is already an
improvement over having a compile time switch.  Having this working
fully automatic would be even better of course.

> And the deafening silence from the shim developers is not an
> encouragement either.

Indeed.

take care,
  Gerd



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

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

[Oliver Smith-Denny]

My first caveat here is I am writing this email from the depths of a
parental leave, so my brain is only half here and I haven't been up to
date for the past two months :). I'll return in Jan and hopefully be
a bit more sensical.

On 12/6/2023 5:23 AM, Ard Biesheuvel wrote:
> 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.
> 

I completely agree here. My thoughts on this specific issue tend to be:
- edk2 is reference FW and the mainline branch in particular. It should
"do the right thing" not the hacky thing.

- The bug here, as we all agree, is in shim. This is not a case where
FW made a change for the better that broke something and so needs to fix
the change it made, this is a case where FW offered a new option, which
shim took advantage of and completely borked. edk2 can't guarantee that
every OS will do the right thing and we should have guardrails that
give us the best chance of booting, which is after all our top goal.
However, we can't prevent an OS (or bootloader) from blowing its toes
off. There's a part of me that thinks well, if your OS/bootloader sucks,
get a better one...I understand this is complicated by the lack of
release of a new shim (as this bug has been fixed upstream in shim for
almost a year [1]).

These two points being said, I tend to prefer Ard's approach, if I am
reading it correctly in a quick skim, where this change is confined to
QEMU. The platform FW (ArmVirtPkg in this instance) is the right place
for the hacky stuff. The platform in the end is what has the requirement
to boot certain versions of an OS/bootloader. edk2's requirement is to
have sane and usable interfaces.

So, I personally think that if we think edk2 has a sane memory attribute
protocol (or at least as sane as we think it can be), then we should not
change it. Now, it is totally valid to say edk2 does not have a sane
memory attribute protocol and it should have better guardrails. However,
I agree with Ard that if we add any generic edk2 level ability to
disable the memory attribute protocol, it will never be used.

I think the problem with the pattern Ard is describing (don't set XP
based on some heuristic) is that in case it won't work. Because the XP
comes from shim first and then they ask us to unset XP for an
unaligned region. Aligning this call to a larger region seems fraught
with peril. To Gerd's point, we could attempt to catch this in the
fault handler, set some flag, not set XP in the next boot (different
memory policy) and then everything would work (hmm, although in this
case, the memory policy doesn't come into play right, because shim
explicitly asks us to set XP, so either you'd have to disable the
protocol or in the protocol check the memory policy, which feels
wrong). If something is worked out here it feels...better than
a boot time flag and I suppose the more reasonable way to go here. But,
I think the platform is still the place to implement this. This feels
much more like a specific workaround than a generic fault flow we are
going to catch. If this version of shim had been tested on all the
platforms it was going to support, this would have been caught
immediately. So I go back to, the platform can work around this by
enabling a custom compat memory policy (which I think is the benefit
of that framework), but that edk2 shouldn't back bend here and
compromise safety for a bad shim.

Requiring a platform to fix this is more of a burden, but this is a
burden the shim community created by not releasing a new shim and
not testing the old one. We can't mitigate that. I think memory
protections is an area where having the distros do this the right
way is important.

Sorry for the rambling and any context I'm missing, I'm typing this
with a baby in arm :)

Thanks,
Oliver



1: 
https://github.com/rhboot/shim/commit/c7b305152802c8db688605654f75e1195def9fd6


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

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

[Taylor Beebe]

[-- Attachment #1: Type: text/plain, Size: 3476 bytes --]

>> 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.
> Can the fault handler deal with this?  Set a flag somewhere, print a
> big'n'fat error message, wait 5 secs, reset machine?  After reset the
> firmware will see the flag and come up in 'compat' instead of 'strict'
> mode.
>
> Not sure what a good place for such a flag would be.  Do we have other
> options than a non-volatile efi variable?  When using a efi variable we
> probably should add an 'expires' timestamp, so the machine doesn't stay
> in 'compat' mode forever.

This is what we do in Project Mu currently and is what we would like to 
push into

EDK2. For x86 platforms, we use CMOS to communicate to the next boot 
that the

system needs to enter compatibility mode. Of course this doesn't work on ARM

platforms, so we'll have to come up with a more permanent mechanism to 
support

this functionality.

>> (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?
> Memory policies would be another candidate which could possibly use a
> less strict profile in 'compat' mode.  I'd love to see memory policies
> land for the February stable tag.

I don't think the policy can change how the SHIM sets attributes using the

protocol, but you can hook the installation of the Memory Attribute

Protocol into the policy system so it's not installed in compat mode.

I have not revisited the memory protection policy interface update

since Lazlo's feedback in October, but I'd be happy to return to it if 
there's

motivation to get it in over the finish line. Note that there are more 
changes

that will need to be made to add testing, compat mode

switching, support for the nx_compat flag, etc. The patch series that's

currently in flight is just meant to be a lateral move to a runtime 
configurable

interface.

>> 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.
> Sure, I want that too.  Having an runtime switch is already an
> improvement over having a compile time switch.  Having this working
> fully automatic would be even better of course.

If a fix for this issue is needed immediately, I'm fine with Ard's 
solution as a stop-gap.

Assuming we can make progress on committing the memory protection updates,

I can update the CpuDxe drivers to check the memory protection policy

before installing the Memory Attribute Protocol. When adding this policy 
config,

I would revert the change made here to uninstall the protocol.


Thanks :)

-Taylor


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



[-- Attachment #2: Type: text/html, Size: 5988 bytes --]

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

[Ard Biesheuvel]

On Wed, Dec 6, 2023 at 7:37 PM Oliver Smith-Denny
<osde@linux.microsoft.com> wrote:
>
> My first caveat here is I am writing this email from the depths of a
> parental leave, so my brain is only half here and I haven't been up to
> date for the past two months :). I'll return in Jan and hopefully be
> a bit more sensical.
>

No worries - and congrats!

> On 12/6/2023 5:23 AM, Ard Biesheuvel wrote:
> > 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.
> >
>
> I completely agree here. My thoughts on this specific issue tend to be:
> - edk2 is reference FW and the mainline branch in particular. It should
> "do the right thing" not the hacky thing.
>
> - The bug here, as we all agree, is in shim. This is not a case where
> FW made a change for the better that broke something and so needs to fix
> the change it made, this is a case where FW offered a new option, which
> shim took advantage of and completely borked. edk2 can't guarantee that
> every OS will do the right thing and we should have guardrails that
> give us the best chance of booting, which is after all our top goal.
> However, we can't prevent an OS (or bootloader) from blowing its toes
> off. There's a part of me that thinks well, if your OS/bootloader sucks,
> get a better one...I understand this is complicated by the lack of
> release of a new shim (as this bug has been fixed upstream in shim for
> almost a year [1]).
>
> These two points being said, I tend to prefer Ard's approach, if I am
> reading it correctly in a quick skim, where this change is confined to
> QEMU. The platform FW (ArmVirtPkg in this instance) is the right place
> for the hacky stuff. The platform in the end is what has the requirement
> to boot certain versions of an OS/bootloader. edk2's requirement is to
> have sane and usable interfaces.
>
> So, I personally think that if we think edk2 has a sane memory attribute
> protocol (or at least as sane as we think it can be), then we should not
> change it. Now, it is totally valid to say edk2 does not have a sane
> memory attribute protocol and it should have better guardrails. However,
> I agree with Ard that if we add any generic edk2 level ability to
> disable the memory attribute protocol, it will never be used.
>

OK

> I think the problem with the pattern Ard is describing (don't set XP
> based on some heuristic) is that in case it won't work. Because the XP
> comes from shim first and then they ask us to unset XP for an
> unaligned region. Aligning this call to a larger region seems fraught
> with peril. To Gerd's point, we could attempt to catch this in the
> fault handler, set some flag, not set XP in the next boot (different
> memory policy) and then everything would work (hmm, although in this
> case, the memory policy doesn't come into play right, because shim
> explicitly asks us to set XP, so either you'd have to disable the
> protocol or in the protocol check the memory policy, which feels
> wrong). If something is worked out here it feels...better than
> a boot time flag and I suppose the more reasonable way to go here. But,
> I think the platform is still the place to implement this. This feels
> much more like a specific workaround than a generic fault flow we are
> going to catch. If this version of shim had been tested on all the
> platforms it was going to support, this would have been caught
> immediately. So I go back to, the platform can work around this by
> enabling a custom compat memory policy (which I think is the benefit
> of that framework), but that edk2 shouldn't back bend here and
> compromise safety for a bad shim.
>

Yeah, and if we do decide to do this, it requires a more comprehensive
approach, along the lines of what Taylor has implemented.

> Requiring a platform to fix this is more of a burden, but this is a
> burden the shim community created by not releasing a new shim and
> not testing the old one. We can't mitigate that. I think memory
> protections is an area where having the distros do this the right
> way is important.
>
> Sorry for the rambling and any context I'm missing, I'm typing this
> with a baby in arm :)
>

Thanks for the insights - much appreciated.


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

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

[Ard Biesheuvel]

On Tue, 5 Dec 2023 at 10:56, Marcin Juszkiewicz
<marcin.juszkiewicz@linaro.org> wrote:
>
...
>
> I use 'sbsa-ref' with QEMU and upstream EDK2. And cannot use either RHEL
> 9.3 nor CentOS Stream 9 installers because they hang.
>
> And this is not the only platform where upstream EDK2 is used.
>
> Sure, I can hack something, use Grub from Debian or Fedora and get
> things working but that's not solution.
>
> Adding flags in 'Broken OS support' section of EDK2 settings feels like
> bad idea. Especially when EFI app generating issues is developed by
> company known for FOSS work.
>

Thanks Marcin - I agree that other EDK2 users are equally affected.

However, I strongly feel that it is up to the distros to clean up this
mess - please go and complain internally at RedHat about this.

In the mean time, there are end users of QEMU + edk2 on macOS (among
other places) whose stuff gets broken if they update their packages.
For these use cases in particular, I am willing to make an exception,
and implement this escape hatch.


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