Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 29 Mar 2022 14:47:12 -0400
From:   Peter Jones <pjones@redhat.com>
To:     baskov@ispras.ru
Cc:     ardb@kernel.org, Matthew Garrett <mjg59@srcf.ucam.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        X86 ML <x86@kernel.org>, linux-efi <linux-efi@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH RFC v2 0/2] Handle UEFI NX-restricted page tables
Message-ID: <20220329184712.vl7jzq23h5m5kvqh@redhat.com>
References: <20220224154330.26564-1-baskov@ispras.ru>
 <CAMj1kXGg=HAv3P_NKqUHCg6bRFsB0qhfa_z-TOdmi-G8EqPrZA@mail.gmail.com>
 <20220228183044.GA18400@srcf.ucam.org>
 <9787f1c1948cc640e70a50e4b929f44f@ispras.ru>
 <20220303204759.GA20294@srcf.ucam.org>
 <20220318163739.5doimyda5e3kdcef@redhat.com>
 <20e2f5b9ab008b12f14e763127f5ca1b@ispras.ru>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20e2f5b9ab008b12f14e763127f5ca1b@ispras.ru>
User-Agent: NeoMutt/20180716
Precedence: bulk

On Thu, Mar 24, 2022 at 07:39:47PM +0300, baskov@ispras.ru wrote:
> On 2022-03-18 19:37, Peter Jones wrote:
> > On Thu, Mar 03, 2022 at 08:47:59PM +0000, Matthew Garrett wrote:
> > > On Thu, Mar 03, 2022 at 04:42:07PM +0300, baskov@ispras.ru wrote:
> > > > On 2022-02-28 21:30, Matthew Garrett wrote:
> > > > > On Mon, Feb 28, 2022 at 05:45:53PM +0100, Ard Biesheuvel wrote:
> > > > >
> > > > > > Given that this is a workaround for a very specific issue arising on
> > > > > > PI based implementations of UEFI, I consider this a quirk, and so I
> > > > > > think this approach is reasonable. I'd still like to gate it on some
> > > > > > kind of identification, though - perhaps something related to DMI like
> > > > > > the x86 core kernel does as well.
> > > > >
> > > > > When the V1 patches were reviewed, you suggested allocating
> > > > > EFI_LOADER_CODE rather than EFI_LOADER_DATA. The example given for a
> > > > > failure case is when NxMemoryProtectionPolicy is set to 0x7fd4, in which
> > > > > case EFI_LOADER_CODE, EFI_BOOT_SERVICES_CODE and
> > > > > EFI_RUNTIEM_SERVICES_CODE should not have the nx policy applied. So it
> > > > > seems like your initial suggestion (s/LOADER_DATA/LOADER_CODE/) should
> > > > > have worked, even if there was disagreement about whether the spec
> > > > > required it to. Is this firmware applying a stricter policy?
> > > >
> > > > Yes, this firmware is being modified to enforce stricter policy.
> > > 
> > > Ok. I think this should really go through the UEFI spec process - I
> > > agree that from a strict interpretation of the spec, what this
> > > firmware
> > > is doing is legitimate, but I don't like having a situation where we
> > > have to depend on the DXE spec.
> > 
> > It's in the process of getting into the UEFI spec now as
> > https://bugzilla.tianocore.org/show_bug.cgi?id=3519 .
> > 
> > > How does Windows handle this? Just update the page tables itself for
> > > any
> > > regions it needs during boot?
> > 
> > Microsoft's bootloader sets up its own pagetables, though I believe
> > they're switching it to use the (soon to be) standardized API.
> 
> The third version of the patch is the most close in structure
> to the proposed protocol. And until the protocol is standardized and
> implemented on problematic firmware, I think, it remains the better
> solution in terms of simplicity and further porting to the new
> protocol.

The ECR was approved at last week's meeting, it'll be in the next UEFI
spec.  Details of what spec version that'll be and when it will
officially be released are still under discussion, but it's been
approved in its current form.  Microsoft has been kind enough to provide
us code for test firmware, though the build process is a little rough.

I've done some builds of it here: https://copr.fedorainfracloud.org/coprs/pjones/mu-qemuq35/builds/ .
The src rpm there is a bit absurdly large, because I've done the very
quick-and-dirty hack of just shoving a pile of git repos into it instead
of trying to make release tarballs of everything, and it needs the
network enabled to rebuild it for fairly dumb reasons.  But the result
is a firmware that works in QEMU.

> It is desirable to get the issue resolved, and make the kernel stricter
> comply to the spec, without waiting for the new API implementation.
> And later, switch the kernel to be using the protocol with
> subsequent patches as soon as it gets usable.

It works for the bootloaders in my development trees; I've booted a
kernel with your patches.  From the bootloader POV we do need one more
simple patch to enable the compatibility flag in the headers, I'll send
it as a follow-up to this mail.

-- 
        Peter