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

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

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