Re: [PATCH v2] x86/mm: warn on W+x mappings

From: Borislav Petkov <bp@alien8.de>
To: Matt Fleming <matt@codeblueprint.co.uk>
Cc: Andy Lutomirski <luto@amacapital.net>,
	Ingo Molnar <mingo@kernel.org>,
	Stephen Smalley <sds@tycho.nsa.gov>, X86 ML <x86@kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Kees Cook <keescook@chromium.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	"H. Peter Anvin" <hpa@zytor.com>,
	Peter Zijlstra <a.p.zijlstra@chello.nl>,
	Andy Lutomirski <luto@kernel.org>,
	Denys Vlasenko <dvlasenk@redhat.com>,
	Brian Gerst <brgerst@gmail.com>,
	"linux-efi@vger.kernel.org" <linux-efi@vger.kernel.org>,
	Ard Biesheuvel <ard.biesheuvel@linaro.org>,
	Ricardo Neri <ricardo.neri-calderon@linux.intel.com>,
	luv@ml01.01.org
Subject: Re: [PATCH v2] x86/mm: warn on W+x mappings
Date: Thu, 15 Oct 2015 12:33:32 +0200	[thread overview]
Message-ID: <20151015103332.GE4267@pd.tnic> (raw)
In-Reply-To: <20151015101016.GB2975@codeblueprint.co.uk>

On Thu, Oct 15, 2015 at 11:10:16AM +0100, Matt Fleming wrote:
> We do this for the Linux UEFI Validation project kernel [1]. There, we
> do not map EFI Boot Services regions by default, only if the firmware
> tries to access them.
> 
> This gives us the opporunity to print an error message if Boot
> Services regions are accessed after ExitBootServices() (which is the
> bug mjg59 describes in commit 916f676f8dc0 ("x86, efi: Retain boot
> service code until after switching to virtual mode")).

Yeah, that's actually a good idea. Why not upstream it for the wider
audience so that people can actually start reporting b0rked UEFIs? With
a big and nice FW_BUG splat in there...

> But for the issue being discussed in this thread, the thing unmapping
> the EFI regions buys you is that they're no longer accessible from the
> x86 sleep/wakeup code paths, since those also use trampoline_pgd which
> is where the EFI page tables are mapped.
> 
> And that's probably a good idea.
> 
> [1] - https://git.kernel.org/cgit/linux/kernel/git/mfleming/efi.git/commit/?h=stable&id=9b78793058bf93958aa9529400cb2617ec1bc958

In reading that commit message above, the fact that the braindead
decision of allowing SetVirtualAddressMap() to be called only once
reminds me that we can't really have a PF handler for runtime
services as *all* mappings need to be ready before calling
SetVirtualAddressMap().

Or, alternatively, we can prep them, call SetVirtualAddressMap() and
then unmap them all and map them again at the same addresses only in the
PF handler, each time a runtime call happens. When that call finishes,
we unmap them again...

Hmm, perhaps not worth the trouble...

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.