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

["Alexander Graf via groups.io" <graf=amazon.com@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]
-=-=-=-=-=-=-=-=-=-=-=-