[PATCH] x86/mm: Disable W^X detection and enforcement on 32-bit

[PATCH] x86/mm: Disable W^X detection and enforcement on 32-bit

[Dave Hansen]

The 32-bit code is in a weird spot.  Some 32-bit builds (non-PAE) do not
even have NX support.  Even PAE builds that support NX have to contend
with things like EFI data and code mixed in the same pages where W+X
is unavoidable.

The folks still running X86_32=y kernels are unlikely to care much about
NX.  That combined with the fundamental inability fix _all_ of the W+X
things means this code had little value on X86_32=y.  Disable the checks.

Reported-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
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>
Cc: Kees Cook <keescook@chromium.org>
Link: https://lore.kernel.org/all/CAMj1kXHcF_iK_g0OZSkSv56Wmr=eQGQwNstcNjLEfS=mm7a06w@mail.gmail.com/
---
 arch/x86/mm/pat/set_memory.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c
index 20b1e24baa85..efe882c753ca 100644
--- a/arch/x86/mm/pat/set_memory.c
+++ b/arch/x86/mm/pat/set_memory.c
@@ -587,6 +587,14 @@ static inline pgprot_t verify_rwx(pgprot_t old, pgprot_t new, unsigned long star
 {
 	unsigned long end;
 
+	/*
+	 * 32-bit has some unfixable W+X issues, like EFI code
+	 * and writeable data being in the same page.  Disable
+	 * detection and enforcement there.
+	 */
+	if (IS_ENABLED(CONFIG_X86_32))
+		return new;
+
 	/* Only enforce when NX is supported: */
 	if (!(__supported_pte_mask & _PAGE_NX))
 		return new;
-- 
2.34.1

Re: [PATCH] x86/mm: Disable W^X detection and enforcement on 32-bit

[Kirill A. Shutemov]

On Fri, Sep 23, 2022 at 03:17:30PM -0700, Dave Hansen wrote:
> The 32-bit code is in a weird spot.  Some 32-bit builds (non-PAE) do not
> even have NX support.  Even PAE builds that support NX have to contend
> with things like EFI data and code mixed in the same pages where W+X
> is unavoidable.
> 
> The folks still running X86_32=y kernels are unlikely to care much about
> NX.  That combined with the fundamental inability fix _all_ of the W+X
> things means this code had little value on X86_32=y.  Disable the checks.

Maybe downgrade the check to a warning for X86_32=y?

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCH] x86/mm: Disable W^X detection and enforcement on 32-bit

[Dave Hansen]

On 9/23/22 17:09, Kirill A. Shutemov wrote:
> On Fri, Sep 23, 2022 at 03:17:30PM -0700, Dave Hansen wrote:
>> The 32-bit code is in a weird spot.  Some 32-bit builds (non-PAE) do not
>> even have NX support.  Even PAE builds that support NX have to contend
>> with things like EFI data and code mixed in the same pages where W+X
>> is unavoidable.
>>
>> The folks still running X86_32=y kernels are unlikely to care much about
>> NX.  That combined with the fundamental inability fix _all_ of the W+X
>> things means this code had little value on X86_32=y.  Disable the checks.
> Maybe downgrade the check to a warning for X86_32=y?

But for this EFI case, we really don't want the warning.  It's unfixable.

I'm also not sure we want to go to the trouble to properly silence the
warning in these unfixable cases.  There was an argument elsewhere in
the thread that we really shouldn't be warning on things that we don't
have full intentions to fix.  I buy that argument.

Re: [PATCH] x86/mm: Disable W^X detection and enforcement on 32-bit

[Kees Cook]

On Fri, Sep 23, 2022 at 03:17:30PM -0700, Dave Hansen wrote:
> The 32-bit code is in a weird spot.  Some 32-bit builds (non-PAE) do not
> even have NX support.  Even PAE builds that support NX have to contend
> with things like EFI data and code mixed in the same pages where W+X
> is unavoidable.
> 
> The folks still running X86_32=y kernels are unlikely to care much about
> NX.  That combined with the fundamental inability fix _all_ of the W+X
> things means this code had little value on X86_32=y.  Disable the checks.
> 
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [PATCH] x86/mm: Disable W^X detection and enforcement on 32-bit

[Guenter Roeck]

On 9/23/22 17:12, Dave Hansen wrote:
> On 9/23/22 17:09, Kirill A. Shutemov wrote:
>> On Fri, Sep 23, 2022 at 03:17:30PM -0700, Dave Hansen wrote:
>>> The 32-bit code is in a weird spot.  Some 32-bit builds (non-PAE) do not
>>> even have NX support.  Even PAE builds that support NX have to contend
>>> with things like EFI data and code mixed in the same pages where W+X
>>> is unavoidable.
>>>
>>> The folks still running X86_32=y kernels are unlikely to care much about
>>> NX.  That combined with the fundamental inability fix _all_ of the W+X
>>> things means this code had little value on X86_32=y.  Disable the checks.
>> Maybe downgrade the check to a warning for X86_32=y?
> 
> But for this EFI case, we really don't want the warning.  It's unfixable.
> 
> I'm also not sure we want to go to the trouble to properly silence the
> warning in these unfixable cases.  There was an argument elsewhere in
> the thread that we really shouldn't be warning on things that we don't
> have full intentions to fix.  I buy that argument.

Yes, there are already way too many such useless warnings around.
Please don't add more of them.

Guenter

Re: [PATCH] x86/mm: Disable W^X detection and enforcement on 32-bit

[Guenter Roeck]

On Fri, Sep 23, 2022 at 03:17:30PM -0700, Dave Hansen wrote:
> The 32-bit code is in a weird spot.  Some 32-bit builds (non-PAE) do not
> even have NX support.  Even PAE builds that support NX have to contend
> with things like EFI data and code mixed in the same pages where W+X
> is unavoidable.
> 
> The folks still running X86_32=y kernels are unlikely to care much about
> NX.  That combined with the fundamental inability fix _all_ of the W+X
> things means this code had little value on X86_32=y.  Disable the checks.
> 
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
> 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>
> Cc: Kees Cook <keescook@chromium.org>
> Link: https://lore.kernel.org/all/CAMj1kXHcF_iK_g0OZSkSv56Wmr=eQGQwNstcNjLEfS=mm7a06w@mail.gmail.com/

Tested-by: Guenter Roeck <linux@roeck-us.net>

Guenter

> ---
>  arch/x86/mm/pat/set_memory.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c
> index 20b1e24baa85..efe882c753ca 100644
> --- a/arch/x86/mm/pat/set_memory.c
> +++ b/arch/x86/mm/pat/set_memory.c
> @@ -587,6 +587,14 @@ static inline pgprot_t verify_rwx(pgprot_t old, pgprot_t new, unsigned long star
>  {
>  	unsigned long end;
>  
> +	/*
> +	 * 32-bit has some unfixable W+X issues, like EFI code
> +	 * and writeable data being in the same page.  Disable
> +	 * detection and enforcement there.
> +	 */
> +	if (IS_ENABLED(CONFIG_X86_32))
> +		return new;
> +
>  	/* Only enforce when NX is supported: */
>  	if (!(__supported_pte_mask & _PAGE_NX))
>  		return new;
> -- 
> 2.34.1
> 

Re: [PATCH] x86/mm: Disable W^X detection and enforcement on 32-bit

[Ard Biesheuvel]

On Sat, 24 Sept 2022 at 00:17, Dave Hansen <dave.hansen@linux.intel.com> wrote:
>
> The 32-bit code is in a weird spot.  Some 32-bit builds (non-PAE) do not
> even have NX support.  Even PAE builds that support NX have to contend
> with things like EFI data and code mixed in the same pages where W+X
> is unavoidable.
>
> The folks still running X86_32=y kernels are unlikely to care much about
> NX.  That combined with the fundamental inability fix _all_ of the W+X
> things means this code had little value on X86_32=y.  Disable the checks.
>
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
> 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>
> Cc: Kees Cook <keescook@chromium.org>
> Link: https://lore.kernel.org/all/CAMj1kXHcF_iK_g0OZSkSv56Wmr=eQGQwNstcNjLEfS=mm7a06w@mail.gmail.com/

Acked-by: Ard Biesheuvel <ardb@kernel.org>

> ---
>  arch/x86/mm/pat/set_memory.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c
> index 20b1e24baa85..efe882c753ca 100644
> --- a/arch/x86/mm/pat/set_memory.c
> +++ b/arch/x86/mm/pat/set_memory.c
> @@ -587,6 +587,14 @@ static inline pgprot_t verify_rwx(pgprot_t old, pgprot_t new, unsigned long star
>  {
>         unsigned long end;
>
> +       /*
> +        * 32-bit has some unfixable W+X issues, like EFI code
> +        * and writeable data being in the same page.  Disable
> +        * detection and enforcement there.
> +        */
> +       if (IS_ENABLED(CONFIG_X86_32))
> +               return new;
> +
>         /* Only enforce when NX is supported: */
>         if (!(__supported_pte_mask & _PAGE_NX))
>                 return new;
> --
> 2.34.1
>

Re: [PATCH] x86/mm: Disable W^X detection and enforcement on 32-bit

[Pavel Machek]

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

Hi!

> The 32-bit code is in a weird spot.  Some 32-bit builds (non-PAE) do not
> even have NX support.  Even PAE builds that support NX have to contend
> with things like EFI data and code mixed in the same pages where W+X
> is unavoidable.
> 
> The folks still running X86_32=y kernels are unlikely to care much about
> NX.  That combined with the fundamental inability fix _all_ of the W+X
> things means this code had little value on X86_32=y.  Disable the checks.

> ---
>  arch/x86/mm/pat/set_memory.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c
> index 20b1e24baa85..efe882c753ca 100644
> --- a/arch/x86/mm/pat/set_memory.c
> +++ b/arch/x86/mm/pat/set_memory.c
> @@ -587,6 +587,14 @@ static inline pgprot_t verify_rwx(pgprot_t old, pgprot_t new, unsigned long star
>  {
>  	unsigned long end;
>  
> +	/*
> +	 * 32-bit has some unfixable W+X issues, like EFI code
> +	 * and writeable data being in the same page.  Disable
> +	 * detection and enforcement there.
> +	 */
> +	if (IS_ENABLED(CONFIG_X86_32))
> +		return new;
> +

You are going from extreme to extreme. W^X is useful on x86-32 at
least in some configs, so it would make sense to detect and inform
about the issues (perhaps with something like KERN_INFO).

Best regards,
							Pavel
-- 
People of Russia, stop Putin before his war on Ukraine escalates.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]