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

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

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