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 38E677803CE for ; Thu, 7 Dec 2023 07:59:38 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=+92Ztyt+ONx8QtPNI+30Y5Gn8R4mH1dwWhtkMX93Qc0=; c=relaxed/simple; d=groups.io; h=MIME-Version:References:In-Reply-To:From:Date:Message-ID:Subject:To:Cc:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type:Content-Transfer-Encoding; s=20140610; t=1701935976; v=1; b=Q+o8/zPmfub5x21P95H92WtzYjtturqRhMH6dfydX5urIsSHS1UfRMsbBpgs3SQ73/fKxlRN CUiGWhU/R3r1QNW8dArGEIicqUOtEwDQc5l0ifeXWXEdqzt7R+RRShi5OQBvbqxKju7If38JdsE THnkpyNKjYjynf4AQERqvsAA= X-Received: by 127.0.0.2 with SMTP id 2aWTYY7687511xzQXmmrx7rS; Wed, 06 Dec 2023 23:59:36 -0800 X-Received: from mail-qt1-f179.google.com (mail-qt1-f179.google.com [209.85.160.179]) by mx.groups.io with SMTP id smtpd.web10.79127.1701935976220630282 for ; Wed, 06 Dec 2023 23:59:36 -0800 X-Received: by mail-qt1-f179.google.com with SMTP id d75a77b69052e-425928c24easo24361cf.0 for ; Wed, 06 Dec 2023 23:59:36 -0800 (PST) X-Gm-Message-State: plLlfZXBnv3wu0fZtb3e1lM5x7686176AA= X-Google-Smtp-Source: AGHT+IEtI+ddbmp0dYM7bz2XlRA7PuMcWqe2gu+aVb917653Hybvw6LTVYPVgLLd2XMZODzsyl8eF77OZyZfu2uoRtU= X-Received: by 2002:ac8:5909:0:b0:423:7fc7:c74d with SMTP id 9-20020ac85909000000b004237fc7c74dmr521506qty.16.1701935973727; Wed, 06 Dec 2023 23:59:33 -0800 (PST) MIME-Version: 1.0 References: <20231204095215.1053032-1-ardb@google.com> <51c8485a-9f61-4412-b332-0b5d5bd2a295@linux.microsoft.com> In-Reply-To: <51c8485a-9f61-4412-b332-0b5d5bd2a295@linux.microsoft.com> From: "Ard Biesheuvel" Date: Thu, 7 Dec 2023 08:59:22 +0100 Message-ID: Subject: Re: [edk2-devel] [PATCH] ArmVirtPkg: Allow EFI memory attributes protocol to be disabled To: Oliver Smith-Denny Cc: devel@edk2.groups.io, ardb@kernel.org, Gerd Hoffmann , Taylor Beebe , Oliver Smith-Denny , Peter Jones , =?UTF-8?B?TO+/vXN6bO+/vSDvv71yc2Vr?= , Oliver Steffen , Alexander Graf , Leif Lindholm 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,ardb@kernel.org List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: Content-Type: text/plain; charset="UTF-8" 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="Q+o8/zPm"; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=kernel.org (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io On Wed, Dec 6, 2023 at 7:37=E2=80=AFPM Oliver Smith-Denny 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. -=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 (#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] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-