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

["Oliver Smith-Denny" <osde@linux.microsoft.com>]

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