From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id B0ABFD801B1 for ; Wed, 6 Dec 2023 18:37:28 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=hNTd31lU8MkB8WSMRYTBTBTybMgSy1OCXFg9queHG70=; c=relaxed/simple; d=groups.io; h=DKIM-Filter:Message-ID:Date:MIME-Version:User-Agent:Subject:To:Cc:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1701887847; v=1; b=VlkpKOAAPOb/nSQAHP5aFGpaX2x0EqipmIiC8t7RnUcybM3uyHdxorhICHMdPAWEPLwKSXt4 p+sTgeVmLIjZAyDmeknuSZ8iDnSimIAQR/OALzAuVEwzRUC/qiL7ghYyILq6OcKRz0Y2DkNDYpt 1loe/S2ycU0D+1H7c8JKmqnQ= X-Received: by 127.0.0.2 with SMTP id MhK9YY7687511xYUfBTDOpNp; Wed, 06 Dec 2023 10:37:27 -0800 X-Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mx.groups.io with SMTP id smtpd.web11.40770.1701887846720784295 for ; Wed, 06 Dec 2023 10:37:26 -0800 X-Received: from [10.137.194.171] (unknown [131.107.1.171]) by linux.microsoft.com (Postfix) with ESMTPSA id 2173120B74C0; Wed, 6 Dec 2023 10:37:26 -0800 (PST) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 2173120B74C0 Message-ID: <51c8485a-9f61-4412-b332-0b5d5bd2a295@linux.microsoft.com> Date: Wed, 6 Dec 2023 10:37:25 -0800 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [edk2-devel] [PATCH] ArmVirtPkg: Allow EFI memory attributes protocol to be disabled To: devel@edk2.groups.io, ardb@kernel.org, Gerd Hoffmann , Taylor Beebe , Oliver Smith-Denny , Peter Jones Cc: Ard Biesheuvel , =?UTF-8?B?TO+/vXN6bO+/vSDvv71yc2Vr?= , Oliver Steffen , Alexander Graf , Leif Lindholm References: <20231204095215.1053032-1-ardb@google.com> From: "Oliver Smith-Denny" In-Reply-To: Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,osde@linux.microsoft.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: pKa6pt1QNoangEPHvuWEYcX7x7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=VlkpKOAA; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=linux.microsoft.com (policy=none) 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. >=20 > (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? >=20 > 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. >=20 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:=20 https://github.com/rhboot/shim/commit/c7b305152802c8db688605654f75e1195def9= fd6 -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- 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] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-