On Wed, Dec 6, 2023 at 7:37 PM Oliver Smith-Denny <o...@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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
