diff mbox series

x86/mm+efi: Avoid creating W+X mappings

Message ID 20220922193157.1673623-1-dave.hansen@linux.intel.com
State New
Headers show
Series x86/mm+efi: Avoid creating W+X mappings | expand

Commit Message

Dave Hansen Sept. 22, 2022, 7:31 p.m. UTC
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.

Reported-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
Tested-by: Guenter Roeck <linux@roeck-us.net>
Cc: Ard Biesheuvel <ardb@kernel.org>
Cc: Darren Hart <dvhart@infradead.org>
Cc: Andy Shevchenko <andy@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: x86@kernel.org
Cc: linux-efi@vger.kernel.org
Cc: "H. Peter Anvin" <hpa@zytor.com>
Link: https://lore.kernel.org/all/d8cd7c7e-24c1-7f70-24a9-91c77aa634af@roeck-us.net/
---
 arch/x86/platform/efi/efi_32.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Ard Biesheuvel Sept. 23, 2022, 7:53 p.m. UTC | #1
On Fri, 23 Sept 2022 at 20:31, Kees Cook <keescook@chromium.org> wrote:
>
> On Fri, Sep 23, 2022 at 04:26:58PM +0200, Ard Biesheuvel wrote:
> > I was basically making the point that we still support i386 without
> > PAE (which is a prerequisite for supporting non-executable mappings),
> > and if we are going to be pedantic about security on this
> > architecture, we should probably make PAE mandatory as well.
>
> My expectation would be that if someone is running modern kernels on i386,
> they're not using PAE. If they care about PAE, I'd expect them to have
> long since moved to x86_64.
>

Not sure I follow. If they care about PAE, they turn it on. Or do you
mean 'if they care about being able to address lots of physical
memory'? Because the *other* reason you might care about PAE is
because it gives you NX support.

But currently, PAE is not even enabled in the i386_defconfig, and
defaults to off. This means people that are unaware of this won't
enable it, and will be running without NX support.

> > If we are ok with the current state, enabling this permission check on
> > i386 makes no sense.
>
> I'd agree. If it's a choice between "spend a lot of time making sure
> this works correctly on i386" and "don't do this at all on i386", I
> would pick the latter. If someone steps up to do the former, then by
> all means take the patches.
>

OK, so it seems we're all in violent agreement here. And if there is
ever a push for enabling security features on 32-bit, we can add this
to the laundry list of things that need to be looked at.
Dave Hansen Sept. 23, 2022, 10:15 p.m. UTC | #2
On 9/23/22 14:19, Kees Cook wrote:
>> But currently, PAE is not even enabled in the i386_defconfig, and
>> defaults to off. This means people that are unaware of this won't
>> enable it, and will be running without NX support.
> And they all make me cry. ;)

It's been like that for a long time, presumably because the defconfig
should *boot* in as many cases as possible.  It wouldn't be hard to
change.  It also wouldn't be hard to default to HIGHMEM4G (non-PAE) on
targeted builds for CPUs that don't support it.  Patch attached to do
that, if anyone else has an opinion.

We should probably just leave i386 alone, but it breaks my heart to see
Kees in tears.
Eric W. Biederman Sept. 23, 2022, 10:32 p.m. UTC | #3
Dave Hansen <dave.hansen@intel.com> writes:

> On 9/23/22 14:19, Kees Cook wrote:
>>> But currently, PAE is not even enabled in the i386_defconfig, and
>>> defaults to off. This means people that are unaware of this won't
>>> enable it, and will be running without NX support.
>> And they all make me cry. ;)
>
> It's been like that for a long time, presumably because the defconfig
> should *boot* in as many cases as possible.  It wouldn't be hard to
> change.  It also wouldn't be hard to default to HIGHMEM4G (non-PAE) on
> targeted builds for CPUs that don't support it.  Patch attached to do
> that, if anyone else has an opinion.
>
> We should probably just leave i386 alone, but it breaks my heart to see
> Kees in tears.

Is it at all possible to simply drop efi support for 32bit builds?

Last I looked (and it was quite a while ago) efi was only supported
same architecture.  So we are talking about 32bit efi for 32bit kernels.

I think there were only a handful of systems that ever shipped 32bit
efi, because when 32bit efi came out 64bit processors had been shipping
for several years already.

We still probably need to deal with whatever is needed for the BIOS.

If there are enough interesting systems to care to keep the few systems
that shipped with 32bit efi support going it probably does make sense to
change how it is implemented because using the kernel's page tables has
been nasty and given kexec all kinds of challenges to support because
not only does efi happen strange mapping attributes but efi also winds
up living at a fixed virtual address, that can't be changed.  So if you
care about anything like address space layout randomization efi provides
a well know fixed target that defeats all of your work there as well.

Can we do something to isolate 32bit efi so it is not a painpoint?

Given how long 8bit and 16bit systems have lasted I rather suspect 32bit
x86 will last in some embedded form for a very long time.  PAE came in
about the first pentium's I think so most embedded i386 processors
should support it.

Eric
diff mbox series

Patch

diff --git a/arch/x86/platform/efi/efi_32.c b/arch/x86/platform/efi/efi_32.c
index e06a199423c0..d81e379fcd43 100644
--- a/arch/x86/platform/efi/efi_32.c
+++ b/arch/x86/platform/efi/efi_32.c
@@ -136,6 +136,7 @@  void __init efi_runtime_update_mappings(void)
 			if (md->type != EFI_RUNTIME_SERVICES_CODE)
 				continue;
 
+			set_memory_ro(md->virt_addr, md->num_pages);
 			set_memory_x(md->virt_addr, md->num_pages);
 		}
 	}