[PATCH] x86_64: Disabling read-implies-exec when the stack is executable

[PATCH] x86_64: Disabling read-implies-exec when the stack is executable

[Hector Marco-Gisbert]

The READ_IMPLIES_EXEC personality was removed in 2005 for 64-bit processes,
(commit a3cc2546a54361b86b73557df5b85c4fc3fc27c3 form history.git).

But it's still possible to have all readable areas with EXEC permissions by
setting the stack as executable in 64-bit ELF executables (also in 32-bit).

This is because the macro elf_read_implies_exec() does not distinguish
between 32 and 64-bit executables: when the stack is executable then the
read-implies-exec personality is set (enabled) to the process.

We think that this is not a desirable behaviour, maybe read-implies-exec
could be used via personality but not by setting the stack as executable in
the ELF.

For x86_64 processes, is there any reason to disable read-implies-exec
personality and at the same time enable it when the stack is executable ?

With this patch it's no longer possible to enable the read-implies-exec on
the process by setting the stack as executable in the PT_GNU_STACK on
x86_64 executables.

Regarding 32-bits processes, is there any reason to enable
read-implies-exec by setting the stack as executable instead of using the
personality on X86_32 or when emulating IA32 on X86_64 ?

If not, I could re-send the patch which removes this possibility.


Signed-off-by: Hector Marco-Gisbert <hecmargi@upv.es>
Acked-by: Ismael Ripoll Ripoll <iripoll@upv.es>
---
 arch/x86/include/asm/elf.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/elf.h b/arch/x86/include/asm/elf.h
index 15340e3..87fd15e 100644
--- a/arch/x86/include/asm/elf.h
+++ b/arch/x86/include/asm/elf.h
@@ -271,8 +271,8 @@ extern int force_personality32;
  * An executable for which elf_read_implies_exec() returns TRUE will
  * have the READ_IMPLIES_EXEC personality flag set automatically.
  */
-#define elf_read_implies_exec(ex, executable_stack)	\
-	(executable_stack != EXSTACK_DISABLE_X)
+#define elf_read_implies_exec(ex, executable_stack) \
+	(mmap_is_ia32() ? (executable_stack != EXSTACK_DISABLE_X) : 0)
 
 struct task_struct;
 
-- 
1.9.1

Re: [PATCH] x86_64: Disabling read-implies-exec when the stack is executable

[Kees Cook]

On Wed, May 11, 2016 at 3:45 AM, Hector Marco-Gisbert <hecmargi@upv.es> wrote:
> The READ_IMPLIES_EXEC personality was removed in 2005 for 64-bit processes,
> (commit a3cc2546a54361b86b73557df5b85c4fc3fc27c3 form history.git).
>
> But it's still possible to have all readable areas with EXEC permissions by
> setting the stack as executable in 64-bit ELF executables (also in 32-bit).

My memory is fuzzy here, but IIRC, RIE is needed for loading binaries
that have no concept of no-exec permissions. In those cases, there's
no way to tell if the process expected to need execute permissions in
arbitrary memory regions.

> This is because the macro elf_read_implies_exec() does not distinguish
> between 32 and 64-bit executables: when the stack is executable then the
> read-implies-exec personality is set (enabled) to the process.

However, I would tend to agree: RIE should only be needed on 32-bit
since 64-bit started its life knowing about no-exec permissions.

set_personality_64bit()'s (which is confusingly just an initializer
and not called during the personality() syscall) comment about this
makes no sense to me:

        /* TBD: overwrites user setup. Should have two bits.
           But 64bit processes have always behaved this way,
           so it's not too bad. The main problem is just that
           32bit childs are affected again. */
        current->personality &= ~READ_IMPLIES_EXEC;

> We think that this is not a desirable behaviour, maybe read-implies-exec
> could be used via personality but not by setting the stack as executable in
> the ELF.

I'd like to point out there are two cases here:

- if there is no PT_GNU_STACK marking, RIE is set for both 32-bit and 64-bit.
- if PT_GNU_STACK == EXSTACK_ENABLE_X, RIE is set for both 32-bit and 64-bit.

For 32-bit, these are both correct since a missing stack mark means we
must assume it might need executable memory in unexpected places. For
32-bit if it's marked with an executable stack, it's pretty certain
it's going to need exec memory in unexpected places.

For 64-bit, the first is wrong: the default for 64-bit is a non-exec
stack, so RIE shouldn't get triggered. The question remains: is RIE
needed for explicitly marked PT_GNU_STACKs?

My understanding matches yours: 64-bit never wants RIE, but I'd like
to better understand why. :)

> For x86_64 processes, is there any reason to disable read-implies-exec
> personality and at the same time enable it when the stack is executable ?

I've expanded the CC to folks that might know better.

> With this patch it's no longer possible to enable the read-implies-exec on
> the process by setting the stack as executable in the PT_GNU_STACK on
> x86_64 executables.
>
> Regarding 32-bits processes, is there any reason to enable
> read-implies-exec by setting the stack as executable instead of using the
> personality on X86_32 or when emulating IA32 on X86_64 ?

Yeah, this is for backward compatibility with ancient binaries that
lacked PT_GNU_STACK. This needs to stay, IMO.

> If not, I could re-send the patch which removes this possibility.
>
>
> Signed-off-by: Hector Marco-Gisbert <hecmargi@upv.es>
> Acked-by: Ismael Ripoll Ripoll <iripoll@upv.es>
> ---
>  arch/x86/include/asm/elf.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/include/asm/elf.h b/arch/x86/include/asm/elf.h
> index 15340e3..87fd15e 100644
> --- a/arch/x86/include/asm/elf.h
> +++ b/arch/x86/include/asm/elf.h
> @@ -271,8 +271,8 @@ extern int force_personality32;
>   * An executable for which elf_read_implies_exec() returns TRUE will
>   * have the READ_IMPLIES_EXEC personality flag set automatically.
>   */
> -#define elf_read_implies_exec(ex, executable_stack)    \
> -       (executable_stack != EXSTACK_DISABLE_X)
> +#define elf_read_implies_exec(ex, executable_stack) \
> +       (mmap_is_ia32() ? (executable_stack != EXSTACK_DISABLE_X) : 0)
>
>  struct task_struct;
>
> --
> 1.9.1
>

Regardless, I think the comment above this needs to be expanded to
explain why what's happening is happening, since currently the comment
just says the same thing as the code. :)

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

Re: [PATCH] x86_64: Disabling read-implies-exec when the stack is executable

[Andi Kleen]

> However, I would tend to agree: RIE should only be needed on 32-bit
> since 64-bit started its life knowing about no-exec permissions.

NX was not in the original AMD K8 chips.  Was only added some time later.

> set_personality_64bit()'s (which is confusingly just an initializer
> and not called during the personality() syscall) comment about this
> makes no sense to me:
> 
>         /* TBD: overwrites user setup. Should have two bits.
>            But 64bit processes have always behaved this way,
>            so it's not too bad. The main problem is just that
>            32bit childs are affected again. */
>         current->personality &= ~READ_IMPLIES_EXEC;

What does not make sense?

-Andi

Re: [PATCH] x86_64: Disabling read-implies-exec when the stack is executable

[Kees Cook]

On Wed, May 11, 2016 at 3:40 PM, Andi Kleen <ak@linux.intel.com> wrote:
>> However, I would tend to agree: RIE should only be needed on 32-bit
>> since 64-bit started its life knowing about no-exec permissions.
>
> NX was not in the original AMD K8 chips.  Was only added some time later.

So we should retain this behavior for all of 64-bit?

>> set_personality_64bit()'s (which is confusingly just an initializer
>> and not called during the personality() syscall) comment about this
>> makes no sense to me:
>>
>>         /* TBD: overwrites user setup. Should have two bits.
>>            But 64bit processes have always behaved this way,
>>            so it's not too bad. The main problem is just that
>>            32bit childs are affected again. */
>>         current->personality &= ~READ_IMPLIES_EXEC;
>
> What does not make sense?

I just don't have enough context to make sense of it. What two bits?
Always behaved what way?Affected by what?

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

Re: [PATCH] x86_64: Disabling read-implies-exec when the stack is executable

[Ingo Molnar]

* Kees Cook <keescook@chromium.org> wrote:

> On Wed, May 11, 2016 at 3:45 AM, Hector Marco-Gisbert <hecmargi@upv.es> wrote:
> > The READ_IMPLIES_EXEC personality was removed in 2005 for 64-bit processes,
> > (commit a3cc2546a54361b86b73557df5b85c4fc3fc27c3 form history.git).
> >
> > But it's still possible to have all readable areas with EXEC permissions by
> > setting the stack as executable in 64-bit ELF executables (also in 32-bit).
> 
> My memory is fuzzy here, but IIRC, RIE is needed for loading binaries
> that have no concept of no-exec permissions. In those cases, there's
> no way to tell if the process expected to need execute permissions in
> arbitrary memory regions.
> 
> > This is because the macro elf_read_implies_exec() does not distinguish
> > between 32 and 64-bit executables: when the stack is executable then the
> > read-implies-exec personality is set (enabled) to the process.
> 
> However, I would tend to agree: RIE should only be needed on 32-bit
> since 64-bit started its life knowing about no-exec permissions.
> 
> set_personality_64bit()'s (which is confusingly just an initializer
> and not called during the personality() syscall) comment about this
> makes no sense to me:
> 
>         /* TBD: overwrites user setup. Should have two bits.
>            But 64bit processes have always behaved this way,
>            so it's not too bad. The main problem is just that
>            32bit childs are affected again. */
>         current->personality &= ~READ_IMPLIES_EXEC;

JFYI, that obfuscated comment was added over a decade ago to the x86_64 tree, see 
the commit from the historic git tree attached below.

Thanks,

	Ingo

=================>
>From a3cc2546a54361b86b73557df5b85c4fc3fc27c3 Mon Sep 17 00:00:00 2001
From: Andi Kleen <ak@suse.de>
Date: Wed, 9 Feb 2005 22:40:57 -0800
Subject: [PATCH] [PATCH] Force read implies exec for all 32bit processes in x86-64

This effectively enables executable stack and executable heap for all 32bit
programs on x86-64, except if noexec32=on is specified.

This does not support changing this with personality right now, this would
need more intrusive changes.  A 64bit process will always turn it off and a
32bit process turn it on.

Also readd the noexec32=on option to turn this off and fix a minor bug in
noexec=...  (would be reported as unknown option)

Signed-off-by: Andi Kleen <ak@suse.de>
Signed-off-by: Andrew Morton <akpm@osdl.org>
Signed-off-by: Linus Torvalds <torvalds@osdl.org>
---
 arch/x86_64/ia32/ia32_binfmt.c |  4 ++++
 arch/x86_64/kernel/process.c   |  6 ++++++
 arch/x86_64/kernel/setup64.c   | 25 +++++++++++++++++++++++--
 include/asm-x86_64/pgtable.h   |  2 +-
 4 files changed, 34 insertions(+), 3 deletions(-)

diff --git a/arch/x86_64/ia32/ia32_binfmt.c b/arch/x86_64/ia32/ia32_binfmt.c
index 445717b9c66b..93d568dfa762 100644
--- a/arch/x86_64/ia32/ia32_binfmt.c
+++ b/arch/x86_64/ia32/ia32_binfmt.c
@@ -249,6 +249,8 @@ elf_core_copy_task_xfpregs(struct task_struct *t, elf_fpxregset_t *xfpu)
 #define elf_check_arch(x) \
 	((x)->e_machine == EM_386)
 
+extern int force_personality32;
+
 #define ELF_EXEC_PAGESIZE PAGE_SIZE
 #define ELF_HWCAP (boot_cpu_data.x86_capability[0])
 #define ELF_PLATFORM  ("i686")
@@ -262,6 +264,8 @@ do {							\
 		set_thread_flag(TIF_ABI_PENDING);		\
 	else							\
 		clear_thread_flag(TIF_ABI_PENDING);		\
+	/* XXX This overwrites the user set personality */	\
+	current->personality |= force_personality32;		\
 } while (0)
 
 /* Override some function names */
diff --git a/arch/x86_64/kernel/process.c b/arch/x86_64/kernel/process.c
index bbe26dda5e79..3a3522b9c885 100644
--- a/arch/x86_64/kernel/process.c
+++ b/arch/x86_64/kernel/process.c
@@ -577,6 +577,12 @@ void set_personality_64bit(void)
 
 	/* Make sure to be in 64bit mode */
 	clear_thread_flag(TIF_IA32); 
+
+	/* TBD: overwrites user setup. Should have two bits.
+	   But 64bit processes have always behaved this way,
+	   so it's not too bad. The main problem is just that
+   	   32bit childs are affected again. */
+	current->personality &= ~READ_IMPLIES_EXEC;
 }
 
 asmlinkage long sys_fork(struct pt_regs *regs)
diff --git a/arch/x86_64/kernel/setup64.c b/arch/x86_64/kernel/setup64.c
index 248a86a80366..b5305b04bc40 100644
--- a/arch/x86_64/kernel/setup64.c
+++ b/arch/x86_64/kernel/setup64.c
@@ -50,7 +50,7 @@ Control non executable mappings for 64bit processes.
 on	Enable(default)
 off	Disable
 */ 
-void __init nonx_setup(const char *str)
+int __init nonx_setup(char *str)
 {
 	if (!strncmp(str, "on", 2)) {
                 __supported_pte_mask |= _PAGE_NX; 
@@ -58,8 +58,29 @@ void __init nonx_setup(const char *str)
 	} else if (!strncmp(str, "off", 3)) {
 		do_not_nx = 1;
 		__supported_pte_mask &= ~_PAGE_NX;
-        } 
+        }
+	return 0;
 } 
+__setup("noexec=", nonx_setup);	/* parsed early actually */
+
+int force_personality32 = READ_IMPLIES_EXEC;
+
+/* noexec32=on|off
+Control non executable heap for 32bit processes.
+To control the stack too use noexec=off
+
+on	PROT_READ does not imply PROT_EXEC for 32bit processes
+off	PROT_READ implies PROT_EXEC (default)
+*/
+static int __init nonx32_setup(char *str)
+{
+	if (!strcmp(str, "on"))
+		force_personality32 &= ~READ_IMPLIES_EXEC;
+	else if (!strcmp(str, "off"))
+		force_personality32 |= READ_IMPLIES_EXEC;
+	return 0;
+}
+__setup("noexec32=", nonx32_setup);
 
 /*
  * Great future plan:
diff --git a/include/asm-x86_64/pgtable.h b/include/asm-x86_64/pgtable.h
index c5773fd9b3d4..1e2cc99aebd8 100644
--- a/include/asm-x86_64/pgtable.h
+++ b/include/asm-x86_64/pgtable.h
@@ -20,7 +20,7 @@ extern unsigned long __supported_pte_mask;
 
 #define swapper_pg_dir init_level4_pgt
 
-extern void nonx_setup(const char *str);
+extern int nonx_setup(char *str);
 extern void paging_init(void);
 extern void clear_kernel_mapping(unsigned long addr, unsigned long size);
 

Re: [PATCH] x86_64: Disabling read-implies-exec when the stack is executable

[Kees Cook]

On Wed, May 11, 2016 at 5:45 AM Hector Marco-Gisbert <hecmargi@upv.es> wrote:
>
> The READ_IMPLIES_EXEC personality was removed in 2005 for 64-bit processes,
> (commit a3cc2546a54361b86b73557df5b85c4fc3fc27c3 form history.git).
>
> But it's still possible to have all readable areas with EXEC permissions by
> setting the stack as executable in 64-bit ELF executables (also in 32-bit).
>
> This is because the macro elf_read_implies_exec() does not distinguish
> between 32 and 64-bit executables: when the stack is executable then the
> read-implies-exec personality is set (enabled) to the process.
>
> We think that this is not a desirable behaviour, maybe read-implies-exec
> could be used via personality but not by setting the stack as executable in
> the ELF.
>
> For x86_64 processes, is there any reason to disable read-implies-exec
> personality and at the same time enable it when the stack is executable ?
>
> With this patch it's no longer possible to enable the read-implies-exec on
> the process by setting the stack as executable in the PT_GNU_STACK on
> x86_64 executables.
>
> Regarding 32-bits processes, is there any reason to enable
> read-implies-exec by setting the stack as executable instead of using the
> personality on X86_32 or when emulating IA32 on X86_64 ?
>
> If not, I could re-send the patch which removes this possibility.
>
>
> Signed-off-by: Hector Marco-Gisbert <hecmargi@upv.es>
> Acked-by: Ismael Ripoll Ripoll <iripoll@upv.es>
> ---
>  arch/x86/include/asm/elf.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/include/asm/elf.h b/arch/x86/include/asm/elf.h
> index 15340e3..87fd15e 100644
> --- a/arch/x86/include/asm/elf.h
> +++ b/arch/x86/include/asm/elf.h
> @@ -271,8 +271,8 @@ extern int force_personality32;
>   * An executable for which elf_read_implies_exec() returns TRUE will
>   * have the READ_IMPLIES_EXEC personality flag set automatically.
>   */
> -#define elf_read_implies_exec(ex, executable_stack)    \
> -       (executable_stack != EXSTACK_DISABLE_X)
> +#define elf_read_implies_exec(ex, executable_stack) \
> +       (mmap_is_ia32() ? (executable_stack != EXSTACK_DISABLE_X) : 0)
>
>  struct task_struct;
>
> --
> 1.9.1
>

*thread necromancy*

I'd still like to see this get landed. READ_IMPLIES_EXEC is way too
powerful (it impacts, for example, mmap() regions of device driver
memory, forcing drivers to not be able to disallow VM_EXEC[1]).

The only case it could break is on an AMD K8 (Athlon only, I assume?),
which seems unlikely to have a modern kernel run on it. If there is
still concern, then we could just test against the NX CPU feature:

diff --git a/arch/x86/include/asm/elf.h b/arch/x86/include/asm/elf.h
index 69c0f892e310..367cd36259a4 100644
--- a/arch/x86/include/asm/elf.h
+++ b/arch/x86/include/asm/elf.h
@@ -280,10 +280,12 @@ extern u32 elf_hwcap2;

 /*
  * An executable for which elf_read_implies_exec() returns TRUE will
- * have the READ_IMPLIES_EXEC personality flag set automatically.
+ * have the READ_IMPLIES_EXEC personality flag set automatically when
+ * a CPU did not support NX or is using a 32-bit memory layout.
  */
-#define elf_read_implies_exec(ex, executable_stack)    \
-       (executable_stack != EXSTACK_DISABLE_X)
+#define elf_read_implies_exec(ex, executable_stack)            \
+       (mmap_is_ia32() || !(__supported_pte_mask & _PAGE_NX) ? \
+               (executable_stack != EXSTACK_DISABLE_X) : 0)

 struct task_struct;


Additionally, I think architectures that always had NX (arm64) should
entirely remove their elf_read_implies_exec() macro (defaulting to the
removal of the stack-marking-based READ_IMPLIES_EXEC enabling):

diff --git a/arch/arm64/include/asm/elf.h b/arch/arm64/include/asm/elf.h
index 6adc1a90e7e6..87c2dd468eea 100644
--- a/arch/arm64/include/asm/elf.h
+++ b/arch/arm64/include/asm/elf.h
@@ -107,8 +107,6 @@
  */
 #define elf_check_arch(x)              ((x)->e_machine == EM_AARCH64)

-#define elf_read_implies_exec(ex,stk)  (stk != EXSTACK_DISABLE_X)
-
 #define CORE_DUMP_USE_REGSET
 #define ELF_EXEC_PAGESIZE      PAGE_SIZE


Thoughts?

-Kees

[1] https://lkml.kernel.org/r/20190418055759.GA3155@mellanox.com

--
Kees Cook

Re: [PATCH] x86_64: Disabling read-implies-exec when the stack is executable

[Thomas Gleixner]

On Thu, 18 Apr 2019, Kees Cook wrote:
> On Wed, May 11, 2016 at 5:45 AM Hector Marco-Gisbert <hecmargi@upv.es> wrote:
> *thread necromancy*
> 
> I'd still like to see this get landed. READ_IMPLIES_EXEC is way too
> powerful (it impacts, for example, mmap() regions of device driver
> memory, forcing drivers to not be able to disallow VM_EXEC[1]).
> 
> The only case it could break is on an AMD K8 (Athlon only, I assume?),
> which seems unlikely to have a modern kernel run on it. If there is
> still concern, then we could just test against the NX CPU feature:
> 
> diff --git a/arch/x86/include/asm/elf.h b/arch/x86/include/asm/elf.h
> index 69c0f892e310..367cd36259a4 100644
> --- a/arch/x86/include/asm/elf.h
> +++ b/arch/x86/include/asm/elf.h
> @@ -280,10 +280,12 @@ extern u32 elf_hwcap2;
> 
>  /*
>   * An executable for which elf_read_implies_exec() returns TRUE will
> - * have the READ_IMPLIES_EXEC personality flag set automatically.
> + * have the READ_IMPLIES_EXEC personality flag set automatically when
> + * a CPU did not support NX or is using a 32-bit memory layout.
>   */
> -#define elf_read_implies_exec(ex, executable_stack)    \
> -       (executable_stack != EXSTACK_DISABLE_X)
> +#define elf_read_implies_exec(ex, executable_stack)            \
> +       (mmap_is_ia32() || !(__supported_pte_mask & _PAGE_NX) ? \

What's special about ia32? All what matters is whether PAGE_NX is supported
or not. That has nothing to do with 32/64bit unless I'm missing something
(as usual).

Thanks,

	tglx

Re: [PATCH] x86_64: Disabling read-implies-exec when the stack is executable

[Kees Cook]

On Thu, Apr 18, 2019 at 3:17 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> On Thu, 18 Apr 2019, Kees Cook wrote:
> > On Wed, May 11, 2016 at 5:45 AM Hector Marco-Gisbert <hecmargi@upv.es> wrote:
> > *thread necromancy*
> >
> > I'd still like to see this get landed. READ_IMPLIES_EXEC is way too
> > powerful (it impacts, for example, mmap() regions of device driver
> > memory, forcing drivers to not be able to disallow VM_EXEC[1]).
> >
> > The only case it could break is on an AMD K8 (Athlon only, I assume?),
> > which seems unlikely to have a modern kernel run on it. If there is
> > still concern, then we could just test against the NX CPU feature:
> >
> > diff --git a/arch/x86/include/asm/elf.h b/arch/x86/include/asm/elf.h
> > index 69c0f892e310..367cd36259a4 100644
> > --- a/arch/x86/include/asm/elf.h
> > +++ b/arch/x86/include/asm/elf.h
> > @@ -280,10 +280,12 @@ extern u32 elf_hwcap2;
> >
> >  /*
> >   * An executable for which elf_read_implies_exec() returns TRUE will
> > - * have the READ_IMPLIES_EXEC personality flag set automatically.
> > + * have the READ_IMPLIES_EXEC personality flag set automatically when
> > + * a CPU did not support NX or is using a 32-bit memory layout.
> >   */
> > -#define elf_read_implies_exec(ex, executable_stack)    \
> > -       (executable_stack != EXSTACK_DISABLE_X)
> > +#define elf_read_implies_exec(ex, executable_stack)            \
> > +       (mmap_is_ia32() || !(__supported_pte_mask & _PAGE_NX) ? \
>
> What's special about ia32? All what matters is whether PAGE_NX is supported
> or not. That has nothing to do with 32/64bit unless I'm missing something
> (as usual).

I think the reasoning was that older toolchains from ia32 wouldn't
provide any permissions marking at all, and this was a signal that it
might need executable heaps as well as stack. So, since ia32 gained NX
along the way, lacking the gnu-stack marking was a reasonable
indicator that the ELF needed this crazy setting to effectively
disable NX for the ELF.

But yes, just looking at the NX feature should cover this.

One thing I'd also like to adjust is the logic about the gnu-stack
existing and requesting an executable stack should apply only to the
stack, and not trigger READ_IMPLIES_EXEC. i.e. if the ELF has
gnu-stack marking of any kind, it should never flag READ_IMPLIES_EXEC.
It would, of course, continue to control the stack perms.

And if this is wrong ... well, we'll find out how and we can more
correctly document this. So how about:

#define elf_read_implies_exec(ex, executable_stack)            \
       (!(__supported_pte_mask & _PAGE_NX) &&  \
        (executable_stack == EXSTACK_DEFAULT))


-- 
Kees Cook

Re: [PATCH] x86_64: Disabling read-implies-exec when the stack is executable

[Andy Lutomirski]

> On Apr 18, 2019, at 1:17 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> 
>> On Thu, 18 Apr 2019, Kees Cook wrote:
>> On Wed, May 11, 2016 at 5:45 AM Hector Marco-Gisbert <hecmargi@upv.es> wrote:
>> *thread necromancy*
>> 
>> I'd still like to see this get landed. READ_IMPLIES_EXEC is way too
>> powerful (it impacts, for example, mmap() regions of device driver
>> memory, forcing drivers to not be able to disallow VM_EXEC[1]).
>> 
>> The only case it could break is on an AMD K8 (Athlon only, I assume?),
>> which seems unlikely to have a modern kernel run on it. If there is
>> still concern, then we could just test against the NX CPU feature:
>> 
>> diff --git a/arch/x86/include/asm/elf.h b/arch/x86/include/asm/elf.h
>> index 69c0f892e310..367cd36259a4 100644
>> --- a/arch/x86/include/asm/elf.h
>> +++ b/arch/x86/include/asm/elf.h
>> @@ -280,10 +280,12 @@ extern u32 elf_hwcap2;
>> 
>> /*
>>  * An executable for which elf_read_implies_exec() returns TRUE will
>> - * have the READ_IMPLIES_EXEC personality flag set automatically.
>> + * have the READ_IMPLIES_EXEC personality flag set automatically when
>> + * a CPU did not support NX or is using a 32-bit memory layout.
>>  */
>> -#define elf_read_implies_exec(ex, executable_stack)    \
>> -       (executable_stack != EXSTACK_DISABLE_X)
>> +#define elf_read_implies_exec(ex, executable_stack)            \
>> +       (mmap_is_ia32() || !(__supported_pte_mask & _PAGE_NX) ? \
> 
> What's special about ia32? All what matters is whether PAGE_NX is supported
> or not. That has nothing to do with 32/64bit unless I'm missing something
> (as usual).
> 
> 

I have the opposite question: who cares if we have NX?  On a CPU without NX, read implies exec, full stop. Why should nasty personality stuff matter at all?  The personality stuff is about supporting old crufty binaries.

So: are there old 64-bit binaries that have their stacks marked RX that expect mmap to automatically return X memory?  If so, then the patch is a problem. If not, then maybe the patch is okay.

All that being said, the comment in the patch seems to be highly misleading.  If the patch is to be applied, the comment needs serious work.

Re: [PATCH] x86_64: Disabling read-implies-exec when the stack is executable

[Kees Cook]

On Thu, Apr 18, 2019 at 9:15 AM Andy Lutomirski <luto@amacapital.net> wrote:
> I have the opposite question: who cares if we have NX?  On a CPU without NX, read implies exec, full stop. Why should nasty personality stuff matter at all?  The personality stuff is about supporting old crufty binaries.
>
> So: are there old 64-bit binaries that have their stacks marked RX that expect mmap to automatically return X memory?  If so, then the patch is a problem. If not, then maybe the patch is okay.

That's what I'm wondering too. (Though remember that ia32 PAE has NX,
so it's also 32-bit binaries.) The matrix I have in my head is:

             CPU: |  lacks NX      |  has NX            |
ELF:              |                |                    |
--------------------------------------------------------|
missing GNU_STACK | doesn't matter | needs RIE          |
GNU_STACK == RWX  | doesn't matter | needs only stack X | *
GNU_STACK == RW   | doesn't matter | needs stack NX     |

(hopefully gmail doesn't mangle this whitespace)

The "*" line here is the question. The question is "when does
GNU_STACK == RWX also mean all mmaps must be X?" If it's only on ia32,
okay, fine we can adjust it, but why is it only an issue for ia32
toolchains? If it's a non-issue, then the above logic stands.

> All that being said, the comment in the patch seems to be highly misleading.  If the patch is to be applied, the comment needs serious work.

Yes, absolutely. (I'd include the chart above, for example...)

-- 
Kees Cook