Re: [PATCH] x86/mm+efi: Avoid creating W+X mappings

From: Guenter Roeck <linux@roeck-us.net>
To: Ard Biesheuvel <ardb@kernel.org>, Peter Zijlstra <peterz@infradead.org>
Cc: Dave Hansen <dave.hansen@linux.intel.com>,
	linux-kernel@vger.kernel.org, Darren Hart <dvhart@infradead.org>,
	Andy Shevchenko <andy@infradead.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	x86@kernel.org, linux-efi@vger.kernel.org,
	"H. Peter Anvin" <hpa@zytor.com>,
	Kees Cook <keescook@chromium.org>
Subject: Re: [PATCH] x86/mm+efi: Avoid creating W+X mappings
Date: Fri, 23 Sep 2022 06:58:13 -0700	[thread overview]
Message-ID: <5f443915-b38a-c78d-cccd-876501434cef@roeck-us.net> (raw)
In-Reply-To: <CAMj1kXEvt-TQzO5jO6srkC8jW5fbou95VKu=os3gt_y87ZPJWg@mail.gmail.com>

On 9/23/22 02:49, Ard Biesheuvel wrote:
> (cc Kees)
> 
> On Fri, 23 Sept 2022 at 09:00, Peter Zijlstra <peterz@infradead.org> wrote:
>>
>> On Fri, Sep 23, 2022 at 12:08:57AM +0200, Ard Biesheuvel wrote:
>>> On Thu, 22 Sept 2022 at 21:32, Dave Hansen <dave.hansen@linux.intel.com> wrote:
>>>>
>>>> From: Peter Zijlstra <peterz@infradead.org>
>>>>
>>>> I'm planning on sticking this in x86/mm so that it goes upstream
>>>> along with the W+X detection code.
>>>>
>>>> --
>>>>
>>>> A recent x86/mm change warns and refuses to create W+X mappings.
>>>>
>>>> The 32-bit EFI code tries to create such a mapping and trips over
>>>> the new W+X refusal.
>>>>
>>>> Make the EFI_RUNTIME_SERVICES_CODE mapping read-only to fix it.
>>>>
>>>
>>> This is not safe. EFI_RUNTIME_SERVICES_CODE covers both .text and
>>> .data sections of the EFI runtime PE/COFF executables in memory, so
>>> you are essentially making .data and .bss read-only. (Whether those
>>> executables actually modify their .data and .bss at runtime is a
>>> different matter, but the point is that it used to be possible)
>>>
>>> More recent firmwares may provide a 'memory attributes table'
>>> separately which describes the individual sections, but older 32-bit
>>> firmwares are not even built with 4k section alignment, so code and
>>> data may share a single page. Note that we haven't wired up this
>>> memory attributes table on i386 at the moment, and I seriously doubt
>>> that 32-bit firmware in the field exposes it.
>>>
>>> Can we just turn off this feature for 32-bit?
>>
>> Goodie; some seriously security minded people who did that EFI turd :/
> 
> To be fair, most people tended to care more about memory footprint
> than about security at the time. And I don't recall a lot of
> enthusiasm in the Linux community either for rounding up kernel
> sections so they could be mapped with W^X permissions. And without
> PAE, all memory is executable anyway.
> 
>> Let's just heap it on the pile of 32bit sucks and should not be
>> considered a security target anymore and indeed kill this feature.
>>
> 
> I take it this issue is triggered by the fact that i386 maps the EFI
> runtime regions into the kernel page tables, and are therefore always
> mapped, right? If anyone cares enough about this to go and fix it, we
> could switch to the approach we use everywhere else, i.e., treat EFI
> memory as user space mappings, and activate them only while a runtime
> service is in progress.
> 
> But frankly, why would anyone still be running this? With the EFI
> mixed mode support, only systems with CPUs that don't actually
> implement long mode still need this, and I am skeptical that such
> deployments would use recent kernels.

It is supported, thus I run qemu tests for it. That is the whole point
of testing, after all. If PAE (assuming that is what you are talking
about) is no longer supported or supportable, its support should be
removed. If so, I'll be very happy to stop testing it.

Thanks,
Guenter