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

[Ingo Molnar <mingo@kernel.org>]

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