linux-efi.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] x86/mm+efi: Avoid creating W+X mappings
       [not found] <08906193-246b-c874-8bac-1d98d2313ac4@roeck-us.net>
@ 2022-09-22 19:31 ` Dave Hansen
  2022-09-22 22:08   ` Ard Biesheuvel
  0 siblings, 1 reply; 12+ messages in thread
From: Dave Hansen @ 2022-09-22 19:31 UTC (permalink / raw)
  To: linux-kernel
  Cc: Peter Zijlstra, Guenter Roeck, Ard Biesheuvel, Darren Hart,
	Andy Shevchenko, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	x86, linux-efi, H. Peter Anvin

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(+)

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);
 		}
 	}
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] x86/mm+efi: Avoid creating W+X mappings
  2022-09-22 19:31 ` [PATCH] x86/mm+efi: Avoid creating W+X mappings Dave Hansen
@ 2022-09-22 22:08   ` Ard Biesheuvel
  2022-09-23  6:59     ` Peter Zijlstra
  0 siblings, 1 reply; 12+ messages in thread
From: Ard Biesheuvel @ 2022-09-22 22:08 UTC (permalink / raw)
  To: Dave Hansen
  Cc: linux-kernel, Peter Zijlstra, Guenter Roeck, Darren Hart,
	Andy Shevchenko, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	x86, linux-efi, H. Peter Anvin

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?

> 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(+)
>
> 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);
>                 }
>         }

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] x86/mm+efi: Avoid creating W+X mappings
  2022-09-22 22:08   ` Ard Biesheuvel
@ 2022-09-23  6:59     ` Peter Zijlstra
  2022-09-23  9:49       ` Ard Biesheuvel
  0 siblings, 1 reply; 12+ messages in thread
From: Peter Zijlstra @ 2022-09-23  6:59 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Dave Hansen, linux-kernel, Guenter Roeck, Darren Hart,
	Andy Shevchenko, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	x86, linux-efi, H. Peter Anvin

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 :/
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.



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] x86/mm+efi: Avoid creating W+X mappings
  2022-09-23  6:59     ` Peter Zijlstra
@ 2022-09-23  9:49       ` Ard Biesheuvel
  2022-09-23 13:58         ` Guenter Roeck
  0 siblings, 1 reply; 12+ messages in thread
From: Ard Biesheuvel @ 2022-09-23  9:49 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Dave Hansen, linux-kernel, Guenter Roeck, Darren Hart,
	Andy Shevchenko, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	x86, linux-efi, H. Peter Anvin, Kees Cook

(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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] x86/mm+efi: Avoid creating W+X mappings
  2022-09-23  9:49       ` Ard Biesheuvel
@ 2022-09-23 13:58         ` Guenter Roeck
  2022-09-23 14:26           ` Ard Biesheuvel
  0 siblings, 1 reply; 12+ messages in thread
From: Guenter Roeck @ 2022-09-23 13:58 UTC (permalink / raw)
  To: Ard Biesheuvel, Peter Zijlstra
  Cc: Dave Hansen, linux-kernel, Darren Hart, Andy Shevchenko,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86, linux-efi,
	H. Peter Anvin, Kees Cook

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] x86/mm+efi: Avoid creating W+X mappings
  2022-09-23 13:58         ` Guenter Roeck
@ 2022-09-23 14:26           ` Ard Biesheuvel
  2022-09-23 18:31             ` Kees Cook
  0 siblings, 1 reply; 12+ messages in thread
From: Ard Biesheuvel @ 2022-09-23 14:26 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Peter Zijlstra, Dave Hansen, linux-kernel, Darren Hart,
	Andy Shevchenko, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	x86, linux-efi, H. Peter Anvin, Kees Cook

On Fri, 23 Sept 2022 at 15:58, Guenter Roeck <linux@roeck-us.net> wrote:
>
> 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.

I completely agree with that, and I think all the testing you do is
extremely valuable.

> If PAE (assuming that is what you are talking about)

Not at all - I was referring to i386 support in general.

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.

If we are ok with the current state, enabling this permission check on
i386 makes no sense.

> is no longer supported or supportable, its support should be
> removed. If so, I'll be very happy to stop testing it.
>

I'd say there are better ways to spend those cycles, but for the time
being, I think we should continue testing it, as otherwise it will
just bit rot.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] x86/mm+efi: Avoid creating W+X mappings
  2022-09-23 14:26           ` Ard Biesheuvel
@ 2022-09-23 18:31             ` Kees Cook
  2022-09-23 19:53               ` Ard Biesheuvel
  0 siblings, 1 reply; 12+ messages in thread
From: Kees Cook @ 2022-09-23 18:31 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Guenter Roeck, Peter Zijlstra, Dave Hansen, linux-kernel,
	Darren Hart, Andy Shevchenko, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov, x86, linux-efi, H. Peter Anvin

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.

> 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.

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] x86/mm+efi: Avoid creating W+X mappings
  2022-09-23 18:31             ` Kees Cook
@ 2022-09-23 19:53               ` Ard Biesheuvel
  2022-09-23 21:19                 ` Kees Cook
  0 siblings, 1 reply; 12+ messages in thread
From: Ard Biesheuvel @ 2022-09-23 19:53 UTC (permalink / raw)
  To: Kees Cook
  Cc: Guenter Roeck, Peter Zijlstra, Dave Hansen, linux-kernel,
	Darren Hart, Andy Shevchenko, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov, x86, linux-efi, H. Peter Anvin

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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] x86/mm+efi: Avoid creating W+X mappings
  2022-09-23 19:53               ` Ard Biesheuvel
@ 2022-09-23 21:19                 ` Kees Cook
  2022-09-23 22:15                   ` Dave Hansen
  0 siblings, 1 reply; 12+ messages in thread
From: Kees Cook @ 2022-09-23 21:19 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Guenter Roeck, Peter Zijlstra, Dave Hansen, linux-kernel,
	Darren Hart, Andy Shevchenko, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov, x86, linux-efi, H. Peter Anvin

On Fri, Sep 23, 2022 at 09:53:02PM +0200, Ard Biesheuvel wrote:
> 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.

Right, I meant if they care about NX (and the topic of this thread)
they want PAE, and if they want PAE, they likely moved to x86_64 long
long ago for new kernels.

> 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. ;)

> > > 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.

Yup.

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] x86/mm+efi: Avoid creating W+X mappings
  2022-09-23 21:19                 ` Kees Cook
@ 2022-09-23 22:15                   ` Dave Hansen
  2022-09-23 22:32                     ` Eric W. Biederman
  2022-09-24  0:04                     ` Kees Cook
  0 siblings, 2 replies; 12+ messages in thread
From: Dave Hansen @ 2022-09-23 22:15 UTC (permalink / raw)
  To: Kees Cook, Ard Biesheuvel
  Cc: Guenter Roeck, Peter Zijlstra, Dave Hansen, linux-kernel,
	Darren Hart, Andy Shevchenko, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov, x86, linux-efi, H. Peter Anvin

[-- Attachment #1: Type: text/plain, Size: 662 bytes --]

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.

[-- Attachment #2: pae.patch --]
[-- Type: text/x-patch, Size: 924 bytes --]

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index f9920f1341c8..fad978c7b7c5 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1363,9 +1363,14 @@ config X86_CPUID
 	  with major 203 and minors 0 to 31 for /dev/cpu/0/cpuid to
 	  /dev/cpu/31/cpuid.
 
+config CPU_HAS_PAE
+	def_bool y
+	depends on !M486SX && !M486 && !M586 && !M586TSC && !M586MMX && !MGEODE_LX && !MGEODEGX1 && !MCYRIXIII && !MELAN && !MWINCHIPC6 && !MWINCHIP3D && !MK6
+
 choice
 	prompt "High Memory Support"
 	default HIGHMEM4G
+	default HIGHMEM64G if CPU_HAS_PAE
 	depends on X86_32
 
 config NOHIGHMEM
@@ -1412,7 +1417,7 @@ config HIGHMEM4G
 
 config HIGHMEM64G
 	bool "64GB"
-	depends on !M486SX && !M486 && !M586 && !M586TSC && !M586MMX && !MGEODE_LX && !MGEODEGX1 && !MCYRIXIII && !MELAN && !MWINCHIPC6 && !MWINCHIP3D && !MK6
+	depends on CPU_HAS_PAE
 	select X86_PAE
 	help
 	  Select this if you have a 32-bit processor and more than 4

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] x86/mm+efi: Avoid creating W+X mappings
  2022-09-23 22:15                   ` Dave Hansen
@ 2022-09-23 22:32                     ` Eric W. Biederman
  2022-09-24  0:04                     ` Kees Cook
  1 sibling, 0 replies; 12+ messages in thread
From: Eric W. Biederman @ 2022-09-23 22:32 UTC (permalink / raw)
  To: Dave Hansen
  Cc: Kees Cook, Ard Biesheuvel, Guenter Roeck, Peter Zijlstra,
	Dave Hansen, linux-kernel, Darren Hart, Andy Shevchenko,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86, linux-efi,
	H. Peter Anvin

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] x86/mm+efi: Avoid creating W+X mappings
  2022-09-23 22:15                   ` Dave Hansen
  2022-09-23 22:32                     ` Eric W. Biederman
@ 2022-09-24  0:04                     ` Kees Cook
  1 sibling, 0 replies; 12+ messages in thread
From: Kees Cook @ 2022-09-24  0:04 UTC (permalink / raw)
  To: Dave Hansen
  Cc: Ard Biesheuvel, Guenter Roeck, Peter Zijlstra, Dave Hansen,
	linux-kernel, Darren Hart, Andy Shevchenko, Thomas Gleixner,
	Ingo Molnar, Borislav Petkov, x86, linux-efi, H. Peter Anvin

On Fri, Sep 23, 2022 at 03:15:15PM -0700, Dave Hansen wrote:
> 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.

*dabs his eyes with tissue*

> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index f9920f1341c8..fad978c7b7c5 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -1363,9 +1363,14 @@ config X86_CPUID
>  	  with major 203 and minors 0 to 31 for /dev/cpu/0/cpuid to
>  	  /dev/cpu/31/cpuid.
>  
> +config CPU_HAS_PAE
> +	def_bool y
> +	depends on !M486SX && !M486 && !M586 && !M586TSC && !M586MMX && !MGEODE_LX && !MGEODEGX1 && !MCYRIXIII && !MELAN && !MWINCHIPC6 && !MWINCHIP3D && !MK6
> +
>  choice
>  	prompt "High Memory Support"
>  	default HIGHMEM4G
> +	default HIGHMEM64G if CPU_HAS_PAE
>  	depends on X86_32
>  
>  config NOHIGHMEM
> @@ -1412,7 +1417,7 @@ config HIGHMEM4G
>  
>  config HIGHMEM64G
>  	bool "64GB"
> -	depends on !M486SX && !M486 && !M586 && !M586TSC && !M586MMX && !MGEODE_LX && !MGEODEGX1 && !MCYRIXIII && !MELAN && !MWINCHIPC6 && !MWINCHIP3D && !MK6
> +	depends on CPU_HAS_PAE
>  	select X86_PAE
>  	help
>  	  Select this if you have a 32-bit processor and more than 4

I feel happy now! :)

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2022-09-24  0:04 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <08906193-246b-c874-8bac-1d98d2313ac4@roeck-us.net>
2022-09-22 19:31 ` [PATCH] x86/mm+efi: Avoid creating W+X mappings Dave Hansen
2022-09-22 22:08   ` Ard Biesheuvel
2022-09-23  6:59     ` Peter Zijlstra
2022-09-23  9:49       ` Ard Biesheuvel
2022-09-23 13:58         ` Guenter Roeck
2022-09-23 14:26           ` Ard Biesheuvel
2022-09-23 18:31             ` Kees Cook
2022-09-23 19:53               ` Ard Biesheuvel
2022-09-23 21:19                 ` Kees Cook
2022-09-23 22:15                   ` Dave Hansen
2022-09-23 22:32                     ` Eric W. Biederman
2022-09-24  0:04                     ` Kees Cook

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).