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 <[email protected]>
Signed-off-by: Dave Hansen <[email protected]>
Cc: Ard Biesheuvel <[email protected]>
Cc: Darren Hart <[email protected]>
Cc: Andy Shevchenko <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: "H. Peter Anvin" <[email protected]>
Cc: Kees Cook <[email protected]>
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
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
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.
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 <[email protected]>
> Signed-off-by: Dave Hansen <[email protected]>
Reviewed-by: Kees Cook <[email protected]>
--
Kees Cook
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
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 <[email protected]>
> Signed-off-by: Dave Hansen <[email protected]>
> Cc: Ard Biesheuvel <[email protected]>
> Cc: Darren Hart <[email protected]>
> Cc: Andy Shevchenko <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Borislav Petkov <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: "H. Peter Anvin" <[email protected]>
> Cc: Kees Cook <[email protected]>
> Link: https://lore.kernel.org/all/CAMj1kXHcF_iK_g0OZSkSv56Wmr=eQGQwNstcNjLEfS=mm7a06w@mail.gmail.com/
Tested-by: Guenter Roeck <[email protected]>
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
>
On Sat, 24 Sept 2022 at 00:17, Dave Hansen <[email protected]> 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 <[email protected]>
> Signed-off-by: Dave Hansen <[email protected]>
> Cc: Ard Biesheuvel <[email protected]>
> Cc: Darren Hart <[email protected]>
> Cc: Andy Shevchenko <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Borislav Petkov <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: "H. Peter Anvin" <[email protected]>
> Cc: Kees Cook <[email protected]>
> Link: https://lore.kernel.org/all/CAMj1kXHcF_iK_g0OZSkSv56Wmr=eQGQwNstcNjLEfS=mm7a06w@mail.gmail.com/
Acked-by: Ard Biesheuvel <[email protected]>
> ---
> 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
>
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.