Non-Exec stack patches

Non-Exec stack patches

[Kurt Garloff]

[-- Attachment #1.1: Type: text/plain, Size: 867 bytes --]

Hi,

find attached a patch to parse the elf binaries for a PT_GNU_STACK
section to set the stack non-executable if possible.
Most parts have been shamelessly stolen from Ingo Molnar's more
ambitious stackshield
http://people.redhat.com/mingo/exec-shield/exec-shield-2.6.4-C9

The toolchain has meainwhile support for marking the binaries with a
PT_GNU_STACK section with ot without x bit as needed.

If no such section is found, we leave the stack to whatever the 
arch defaults to. If there is one, we explicitly disabled the VM_EXEC 
bit if no x bit is found, otherwise explicitly enable.

I believe this part should be merged into official mainstream kernels.
Ingo, what do you think?

Regards,
-- 
Kurt Garloff  <garloff@suse.de>                            Cologne, DE 
SUSE LINUX AG, Nuernberg, DE                          SUSE Labs (Head)

[-- Attachment #1.2: noexec_stack4 --]
[-- Type: text/plain, Size: 13247 bytes --]

diff -uNrp linux-2.6.4.EXSTACK/arch/ia64/ia32/binfmt_elf32.c linux-2.6.4.noexec/arch/ia64/ia32/binfmt_elf32.c
--- linux-2.6.4.EXSTACK/arch/ia64/ia32/binfmt_elf32.c	2004-03-23 21:18:35.000000000 +0100
+++ linux-2.6.4.noexec/arch/ia64/ia32/binfmt_elf32.c	2004-03-23 22:48:31.000000000 +0100
@@ -35,7 +35,7 @@ extern void ia64_elf32_init (struct pt_r
 
 static void elf32_set_personality (void);
 
-#define setup_arg_pages(bprm)		ia32_setup_arg_pages(bprm)
+#define setup_arg_pages(bprm,exec)		ia32_setup_arg_pages(bprm,exec)
 #define elf_map				elf32_map
 
 #undef SET_PERSONALITY
@@ -152,7 +152,7 @@ ia64_elf32_init (struct pt_regs *regs)
 }
 
 int
-ia32_setup_arg_pages (struct linux_binprm *bprm)
+ia32_setup_arg_pages (struct linux_binprm *bprm, int executable_stack)
 {
 	unsigned long stack_base;
 	struct vm_area_struct *mpnt;
@@ -181,8 +181,14 @@ ia32_setup_arg_pages (struct linux_binpr
 		mpnt->vm_mm = current->mm;
 		mpnt->vm_start = PAGE_MASK & (unsigned long) bprm->p;
 		mpnt->vm_end = IA32_STACK_TOP;
-		mpnt->vm_page_prot = PAGE_COPY;
-		mpnt->vm_flags = VM_STACK_FLAGS;
+		if (executable_stack > 0)
+			mpnt->vm_flags = VM_STACK_FLAGS |  VM_EXEC;
+		else if (executable_stack < 0)
+			mpnt->vm_flags = VM_STACK_FLAGS & ~VM_EXEC;
+		else
+			mpnt->vm_flags = VM_STACK_FLAGS;
+		mpnt->vm_page_prot = (mpnt->vm_flags & VM_EXEC)?
+					PAGE_COPY_EXEC: PAGE_COPY;
 		mpnt->vm_ops = NULL;
 		mpnt->vm_pgoff = mpnt->vm_start >> PAGE_SHIFT;
 		mpnt->vm_file = NULL;
@@ -197,7 +203,7 @@ ia32_setup_arg_pages (struct linux_binpr
 		struct page *page = bprm->page[i];
 		if (page) {
 			bprm->page[i] = NULL;
-			put_dirty_page(current, page, stack_base, PAGE_COPY, mpnt);
+			put_dirty_page(current, page, stack_base, mpnt->vm_page_prot, mpnt);
 		}
 		stack_base += PAGE_SIZE;
 	}
diff -uNrp linux-2.6.4.EXSTACK/arch/ia64/ia32/ia32priv.h linux-2.6.4.noexec/arch/ia64/ia32/ia32priv.h
--- linux-2.6.4.EXSTACK/arch/ia64/ia32/ia32priv.h	2004-03-11 03:55:51.000000000 +0100
+++ linux-2.6.4.noexec/arch/ia64/ia32/ia32priv.h	2004-03-23 22:56:20.000000000 +0100
@@ -494,7 +494,7 @@ struct ia32_user_desc {
 struct linux_binprm;
 
 extern void ia32_init_addr_space (struct pt_regs *regs);
-extern int ia32_setup_arg_pages (struct linux_binprm *bprm);
+extern int ia32_setup_arg_pages (struct linux_binprm *bprm, int exec_stack);
 extern unsigned long ia32_do_mmap (struct file *, unsigned long, unsigned long, int, int, loff_t);
 extern void ia32_load_segment_descriptors (struct task_struct *task);
 
diff -uNrp linux-2.6.4.EXSTACK/arch/mips/kernel/irixelf.c linux-2.6.4.noexec/arch/mips/kernel/irixelf.c
--- linux-2.6.4.EXSTACK/arch/mips/kernel/irixelf.c	2004-03-11 03:56:03.000000000 +0100
+++ linux-2.6.4.noexec/arch/mips/kernel/irixelf.c	2004-03-23 22:56:42.000000000 +0100
@@ -688,7 +688,7 @@ static int load_irix_binary(struct linux
 	 * change some of these later.
 	 */
 	current->mm->rss = 0;
-	setup_arg_pages(bprm);
+	setup_arg_pages(bprm, 0);
 	current->mm->start_stack = bprm->p;
 
 	/* At this point, we assume that the image should be loaded at
diff -uNrp linux-2.6.4.EXSTACK/arch/s390/kernel/binfmt_elf32.c linux-2.6.4.noexec/arch/s390/kernel/binfmt_elf32.c
--- linux-2.6.4.EXSTACK/arch/s390/kernel/binfmt_elf32.c	2004-03-11 03:55:44.000000000 +0100
+++ linux-2.6.4.noexec/arch/s390/kernel/binfmt_elf32.c	2004-03-23 22:51:25.000000000 +0100
@@ -114,7 +114,7 @@ typedef s390_regs32 elf_gregset_t;
 #include <linux/binfmts.h>
 #include <linux/compat.h>
 
-int setup_arg_pages32(struct linux_binprm *bprm);
+int setup_arg_pages32(struct linux_binprm *bprm, int executable_stack);
 
 #define elf_prstatus elf_prstatus32
 struct elf_prstatus32
@@ -165,7 +165,7 @@ struct elf_prpsinfo32
 
 #undef start_thread
 #define start_thread                    start_thread31 
-#define setup_arg_pages(bprm)           setup_arg_pages32(bprm)
+#define setup_arg_pages(bprm, exec)     setup_arg_pages32(bprm, exec)
 #define elf_map				elf_map32
 
 MODULE_DESCRIPTION("Binary format loader for compatibility with 32bit Linux for S390 binaries,"
diff -uNrp linux-2.6.4.EXSTACK/arch/s390/kernel/compat_exec.c linux-2.6.4.noexec/arch/s390/kernel/compat_exec.c
--- linux-2.6.4.EXSTACK/arch/s390/kernel/compat_exec.c	2004-03-23 21:18:35.000000000 +0100
+++ linux-2.6.4.noexec/arch/s390/kernel/compat_exec.c	2004-03-23 22:58:15.000000000 +0100
@@ -37,7 +37,7 @@
 #undef STACK_TOP
 #define STACK_TOP TASK31_SIZE
 
-int setup_arg_pages32(struct linux_binprm *bprm)
+int setup_arg_pages32(struct linux_binprm *bprm, int executable_stack)
 {
 	unsigned long stack_base;
 	struct vm_area_struct *mpnt;
@@ -66,6 +66,7 @@ int setup_arg_pages32(struct linux_binpr
 		mpnt->vm_mm = mm;
 		mpnt->vm_start = PAGE_MASK & (unsigned long) bprm->p;
 		mpnt->vm_end = STACK_TOP;
+		/* executable stack setting would be applied here */
 		mpnt->vm_page_prot = PAGE_COPY;
 		mpnt->vm_flags = VM_STACK_FLAGS;
 		mpnt->vm_ops = NULL;
diff -uNrp linux-2.6.4.EXSTACK/arch/sparc64/kernel/binfmt_aout32.c linux-2.6.4.noexec/arch/sparc64/kernel/binfmt_aout32.c
--- linux-2.6.4.EXSTACK/arch/sparc64/kernel/binfmt_aout32.c	2004-03-11 03:55:27.000000000 +0100
+++ linux-2.6.4.noexec/arch/sparc64/kernel/binfmt_aout32.c	2004-03-23 22:49:50.000000000 +0100
@@ -310,7 +310,7 @@ beyond_if:
 	orig_thr_flags = current_thread_info()->flags;
 	current_thread_info()->flags |= _TIF_32BIT;
 
-	retval = setup_arg_pages(bprm);
+	retval = setup_arg_pages(bprm, 0);
 	if (retval < 0) { 
 		current_thread_info()->flags = orig_thr_flags;
 
diff -uNrp linux-2.6.4.EXSTACK/arch/x86_64/ia32/ia32_aout.c linux-2.6.4.noexec/arch/x86_64/ia32/ia32_aout.c
--- linux-2.6.4.EXSTACK/arch/x86_64/ia32/ia32_aout.c	2004-03-11 03:55:54.000000000 +0100
+++ linux-2.6.4.noexec/arch/x86_64/ia32/ia32_aout.c	2004-03-23 22:59:47.000000000 +0100
@@ -35,7 +35,7 @@
 #undef WARN_OLD
 #undef CORE_DUMP /* probably broken */
 
-extern int ia32_setup_arg_pages(struct linux_binprm *bprm);
+extern int ia32_setup_arg_pages(struct linux_binprm *bprm, int exec_stack);
 
 static int load_aout_binary(struct linux_binprm *, struct pt_regs * regs);
 static int load_aout_library(struct file*);
@@ -395,7 +395,7 @@ beyond_if:
 
 	set_brk(current->mm->start_brk, current->mm->brk);
 
-	retval = ia32_setup_arg_pages(bprm); 
+	retval = ia32_setup_arg_pages(bprm, 0);
 	if (retval < 0) { 
 		/* Someone check-me: is this error path enough? */ 
 		send_sig(SIGKILL, current, 0); 
diff -uNrp linux-2.6.4.EXSTACK/arch/x86_64/ia32/ia32_binfmt.c linux-2.6.4.noexec/arch/x86_64/ia32/ia32_binfmt.c
--- linux-2.6.4.EXSTACK/arch/x86_64/ia32/ia32_binfmt.c	2004-03-23 22:42:31.000000000 +0100
+++ linux-2.6.4.noexec/arch/x86_64/ia32/ia32_binfmt.c	2004-03-23 21:18:35.000000000 +0100
@@ -272,8 +272,8 @@ do {							\
 #define load_elf_binary load_elf32_binary
 
 #define ELF_PLAT_INIT(r, load_addr)	elf32_init(r)
-#define setup_arg_pages(bprm)		ia32_setup_arg_pages(bprm)
-int ia32_setup_arg_pages(struct linux_binprm *bprm);
+#define setup_arg_pages(bprm, exec_stack)	ia32_setup_arg_pages(bprm, exec_stack)
+int ia32_setup_arg_pages(struct linux_binprm *bprm, int executable_stack);
 
 #undef start_thread
 #define start_thread(regs,new_rip,new_rsp) do { \
@@ -325,7 +325,7 @@ static void elf32_init(struct pt_regs *r
 	me->thread.es = __USER_DS;
 }
 
-int setup_arg_pages(struct linux_binprm *bprm)
+int setup_arg_pages(struct linux_binprm *bprm, int executable_stack)
 {
 	unsigned long stack_base;
 	struct vm_area_struct *mpnt;
@@ -354,7 +354,12 @@ int setup_arg_pages(struct linux_binprm 
 		mpnt->vm_mm = mm;
 		mpnt->vm_start = PAGE_MASK & (unsigned long) bprm->p;
 		mpnt->vm_end = IA32_STACK_TOP;
-		mpnt->vm_flags = vm_stack_flags32; 
+		if (executable_stack > 0)
+			mpnt->vm_flags = vm_stack_flags32 |  VM_EXEC;
+		else if (executable_stack < 0)
+			mpnt->vm_flags = vm_stack_flags32 & ~VM_EXEC;
+		else
+			mpnt->vm_flags = vm_stack_flags32;
  		mpnt->vm_page_prot = (mpnt->vm_flags & VM_EXEC) ? 
  			PAGE_COPY_EXEC : PAGE_COPY;
 		mpnt->vm_ops = NULL;
@@ -373,7 +378,7 @@ int setup_arg_pages(struct linux_binprm 
 		struct page *page = bprm->page[i];
 		if (page) {
 			bprm->page[i] = NULL;
-			put_dirty_page(current,page,stack_base,PAGE_COPY_EXEC, mpnt);
+			put_dirty_page(current,page,stack_base,mpnt->vm_page_prot,mpnt);
 		}
 		stack_base += PAGE_SIZE;
 	}
diff -uNrp linux-2.6.4.EXSTACK/fs/binfmt_aout.c linux-2.6.4.noexec/fs/binfmt_aout.c
--- linux-2.6.4.EXSTACK/fs/binfmt_aout.c	2004-03-23 22:42:31.000000000 +0100
+++ linux-2.6.4.noexec/fs/binfmt_aout.c	2004-03-23 21:18:35.000000000 +0100
@@ -413,7 +413,7 @@ beyond_if:
 
 	set_brk(current->mm->start_brk, current->mm->brk);
 
-	retval = setup_arg_pages(bprm); 
+	retval = setup_arg_pages(bprm, 0); 
 	if (retval < 0) { 
 		/* Someone check-me: is this error path enough? */ 
 		send_sig(SIGKILL, current, 0); 
diff -uNrp linux-2.6.4.EXSTACK/fs/binfmt_elf.c linux-2.6.4.noexec/fs/binfmt_elf.c
--- linux-2.6.4.EXSTACK/fs/binfmt_elf.c	2004-03-23 22:42:31.000000000 +0100
+++ linux-2.6.4.noexec/fs/binfmt_elf.c	2004-03-23 21:18:35.000000000 +0100
@@ -476,6 +476,7 @@ static int load_elf_binary(struct linux_
   	struct exec interp_ex;
 	char passed_fileno[6];
 	struct files_struct *files;
+	int executable_stack = 0; 	/* Don't touch */
 	
 	/* Get the exec-header */
 	elf_ex = *((struct elfhdr *) bprm->buf);
@@ -599,6 +600,15 @@ static int load_elf_binary(struct linux_
 		elf_ppnt++;
 	}
 
+	elf_ppnt = elf_phdata;
+	for (i = 0; i < elf_ex.e_phnum; i++, elf_ppnt++)
+		if (elf_ppnt->p_type == PT_GNU_STACK) {
+			if (elf_ppnt->p_flags & PF_X)
+				executable_stack =  1;
+			else
+				executable_stack = -1;
+		}
+
 	/* Some simple consistency checks for the interpreter */
 	if (elf_interpreter) {
 		interpreter_type = INTERPRETER_ELF | INTERPRETER_AOUT;
@@ -674,7 +684,7 @@ static int load_elf_binary(struct linux_
 	   change some of these later */
 	current->mm->rss = 0;
 	current->mm->free_area_cache = TASK_UNMAPPED_BASE;
-	retval = setup_arg_pages(bprm);
+	retval = setup_arg_pages(bprm, executable_stack);
 	if (retval < 0) {
 		send_sig(SIGKILL, current, 0);
 		goto out_free_dentry;
diff -uNrp linux-2.6.4.EXSTACK/fs/binfmt_som.c linux-2.6.4.noexec/fs/binfmt_som.c
--- linux-2.6.4.EXSTACK/fs/binfmt_som.c	2004-03-23 22:42:31.000000000 +0100
+++ linux-2.6.4.noexec/fs/binfmt_som.c	2004-03-23 21:18:35.000000000 +0100
@@ -254,7 +254,7 @@ load_som_binary(struct linux_binprm * bp
 
 	set_binfmt(&som_format);
 	compute_creds(bprm);
-	setup_arg_pages(bprm);
+	setup_arg_pages(bprm, 0);
 
 	create_som_tables(bprm);
 
diff -uNrp linux-2.6.4.EXSTACK/fs/exec.c linux-2.6.4.noexec/fs/exec.c
--- linux-2.6.4.EXSTACK/fs/exec.c	2004-03-23 22:42:31.000000000 +0100
+++ linux-2.6.4.noexec/fs/exec.c	2004-03-23 21:18:35.000000000 +0100
@@ -342,7 +342,7 @@ out_sig:
 	return;
 }
 
-int setup_arg_pages(struct linux_binprm *bprm)
+int setup_arg_pages(struct linux_binprm *bprm, int executable_stack)
 {
 	unsigned long stack_base;
 	struct vm_area_struct *mpnt;
@@ -425,8 +425,13 @@ int setup_arg_pages(struct linux_binprm 
 		mpnt->vm_start = PAGE_MASK & (unsigned long) bprm->p;
 		mpnt->vm_end = STACK_TOP;
 #endif
-		mpnt->vm_page_prot = protection_map[VM_STACK_FLAGS & 0x7];
-		mpnt->vm_flags = VM_STACK_FLAGS;
+		if (executable_stack > 0)
+			mpnt->vm_flags = VM_STACK_FLAGS |  VM_EXEC;
+		else if (executable_stack < 0)
+			mpnt->vm_flags = VM_STACK_FLAGS & ~VM_EXEC;
+		else
+			mpnt->vm_flags = VM_STACK_FLAGS;
+		mpnt->vm_page_prot = protection_map[mpnt->vm_flags & 0x7];
 		mpnt->vm_ops = NULL;
 		mpnt->vm_pgoff = mpnt->vm_start >> PAGE_SHIFT;
 		mpnt->vm_file = NULL;
diff -uNrp linux-2.6.4.EXSTACK/include/linux/binfmts.h linux-2.6.4.noexec/include/linux/binfmts.h
--- linux-2.6.4.EXSTACK/include/linux/binfmts.h	2004-03-23 22:42:31.000000000 +0100
+++ linux-2.6.4.noexec/include/linux/binfmts.h	2004-03-23 21:18:35.000000000 +0100
@@ -58,7 +58,7 @@ extern int prepare_binprm(struct linux_b
 extern void remove_arg_zero(struct linux_binprm *);
 extern int search_binary_handler(struct linux_binprm *,struct pt_regs *);
 extern int flush_old_exec(struct linux_binprm * bprm);
-extern int setup_arg_pages(struct linux_binprm * bprm);
+extern int setup_arg_pages(struct linux_binprm * bprm, int executable_stack);
 extern int copy_strings(int argc,char __user * __user * argv,struct linux_binprm *bprm); 
 extern int copy_strings_kernel(int argc,char ** argv,struct linux_binprm *bprm);
 extern void compute_creds(struct linux_binprm *binprm);
diff -uNrp linux-2.6.4.EXSTACK/include/linux/elf.h linux-2.6.4.noexec/include/linux/elf.h
--- linux-2.6.4.EXSTACK/include/linux/elf.h	2004-03-23 22:42:31.000000000 +0100
+++ linux-2.6.4.noexec/include/linux/elf.h	2004-03-23 21:18:35.000000000 +0100
@@ -35,6 +35,8 @@ typedef __s64	Elf64_Sxword;
 #define PT_HIPROC  0x7fffffff
 #define PT_GNU_EH_FRAME		0x6474e550
 
+#define PT_GNU_STACK	(PT_LOOS + 0x474e551)
+
 /* These constants define the different elf file types */
 #define ET_NONE   0
 #define ET_REL    1

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: Non-Exec stack patches

[Ingo Molnar]

On Wed, 24 Mar 2004, Kurt Garloff wrote:

> find attached a patch to parse the elf binaries for a PT_GNU_STACK
> section to set the stack non-executable if possible.
> Most parts have been shamelessly stolen from Ingo Molnar's more
> ambitious stackshield
> http://people.redhat.com/mingo/exec-shield/exec-shield-2.6.4-C9
> 
> The toolchain has meainwhile support for marking the binaries with a
> PT_GNU_STACK section with ot without x bit as needed.
> 
> If no such section is found, we leave the stack to whatever the arch
> defaults to. If there is one, we explicitly disabled the VM_EXEC bit if
> no x bit is found, otherwise explicitly enable.
> 
> I believe this part should be merged into official mainstream kernels.
> Ingo, what do you think?

agreed, and the patch looks good to me.

	Ingo

Re: Non-Exec stack patches

[Andrew Morton]

Kurt Garloff <garloff@suse.de> wrote:
>
> find attached a patch to parse the elf binaries for a PT_GNU_STACK
> section to set the stack non-executable if possible.

This patch propagates the PT_GNU_STACK setting into the vma flags, allowing
the architecture to set the stack permissions non-executable if the
architecture chooses to do that, yes?

Which architectures are currently making their pre-page execute permissions
depend upon VM_EXEC?  Would additional arch patches be needed for this?

This may not get past Linus of course.  It doesn't even get past me with
that magical undocumented -1/0/+1 value of the executable_stack argument. 
Please replace that with a proper, commented, #defined-or-enumerated value,
thanks.

Re: Non-Exec stack patches

[Kurt Garloff]

[-- Attachment #1.1: Type: text/plain, Size: 1337 bytes --]

On Tue, Mar 23, 2004 at 03:49:37PM -0800, Andrew Morton wrote:
> This patch propagates the PT_GNU_STACK setting into the vma flags, allowing
> the architecture to set the stack permissions non-executable if the
> architecture chooses to do that, yes?

Right.

> Which architectures are currently making their pre-page execute permissions
> depend upon VM_EXEC?  Would additional arch patches be needed for this?

It works on AMD64 (not ia32e), both for 64bit and 32bit binaries.
I have not yet tested other archs.

If the values in the protection_map are different depending on bit 2,
the patch will be effecitve. (OK, the CPU/MMU needs to honour the
setting of course.) Most likely, the values for 
protection_map[7] is PAGE_COPY_EXEC and of protection_map[3] is
PAGE_COPY.

> This may not get past Linus of course.  It doesn't even get past me with
> that magical undocumented -1/0/+1 value of the executable_stack argument. 
> Please replace that with a proper, commented, #defined-or-enumerated value,

As you wish, master.
Slightly edited and untested patch attached.

Regards,
-- 
Kurt Garloff                   <kurt@garloff.de>             [Koeln, DE]
Physics:Plasma modeling <garloff@plasimo.phys.tue.nl> [TU Eindhoven, NL]
Linux: SUSE Labs (Head)        <garloff@suse.de>    [SUSE Nuernberg, DE]

[-- Attachment #1.2: noexec_stack5 --]
[-- Type: text/plain, Size: 13740 bytes --]

diff -uNrp linux-2.6.4.EXSTACK/arch/ia64/ia32/binfmt_elf32.c linux-2.6.4.noexec2/arch/ia64/ia32/binfmt_elf32.c
--- linux-2.6.4.EXSTACK/arch/ia64/ia32/binfmt_elf32.c	2004-03-23 21:18:35.000000000 +0100
+++ linux-2.6.4.noexec2/arch/ia64/ia32/binfmt_elf32.c	2004-03-24 01:07:35.000000000 +0100
@@ -35,7 +35,7 @@ extern void ia64_elf32_init (struct pt_r
 
 static void elf32_set_personality (void);
 
-#define setup_arg_pages(bprm)		ia32_setup_arg_pages(bprm)
+#define setup_arg_pages(bprm,exec)		ia32_setup_arg_pages(bprm,exec)
 #define elf_map				elf32_map
 
 #undef SET_PERSONALITY
@@ -152,7 +152,7 @@ ia64_elf32_init (struct pt_regs *regs)
 }
 
 int
-ia32_setup_arg_pages (struct linux_binprm *bprm)
+ia32_setup_arg_pages (struct linux_binprm *bprm, int executable_stack)
 {
 	unsigned long stack_base;
 	struct vm_area_struct *mpnt;
@@ -181,8 +181,14 @@ ia32_setup_arg_pages (struct linux_binpr
 		mpnt->vm_mm = current->mm;
 		mpnt->vm_start = PAGE_MASK & (unsigned long) bprm->p;
 		mpnt->vm_end = IA32_STACK_TOP;
-		mpnt->vm_page_prot = PAGE_COPY;
-		mpnt->vm_flags = VM_STACK_FLAGS;
+		if (executable_stack == EXSTACK_ENABLE_X)
+			mpnt->vm_flags = VM_STACK_FLAGS |  VM_EXEC;
+		else if (executable_stack == EXSTACK_DISABLE_X)
+			mpnt->vm_flags = VM_STACK_FLAGS & ~VM_EXEC;
+		else
+			mpnt->vm_flags = VM_STACK_FLAGS;
+		mpnt->vm_page_prot = (mpnt->vm_flags & VM_EXEC)?
+					PAGE_COPY_EXEC: PAGE_COPY;
 		mpnt->vm_ops = NULL;
 		mpnt->vm_pgoff = mpnt->vm_start >> PAGE_SHIFT;
 		mpnt->vm_file = NULL;
@@ -197,7 +203,7 @@ ia32_setup_arg_pages (struct linux_binpr
 		struct page *page = bprm->page[i];
 		if (page) {
 			bprm->page[i] = NULL;
-			put_dirty_page(current, page, stack_base, PAGE_COPY, mpnt);
+			put_dirty_page(current, page, stack_base, mpnt->vm_page_prot, mpnt);
 		}
 		stack_base += PAGE_SIZE;
 	}
diff -uNrp linux-2.6.4.EXSTACK/arch/ia64/ia32/ia32priv.h linux-2.6.4.noexec2/arch/ia64/ia32/ia32priv.h
--- linux-2.6.4.EXSTACK/arch/ia64/ia32/ia32priv.h	2004-03-11 03:55:51.000000000 +0100
+++ linux-2.6.4.noexec2/arch/ia64/ia32/ia32priv.h	2004-03-23 22:56:20.000000000 +0100
@@ -494,7 +494,7 @@ struct ia32_user_desc {
 struct linux_binprm;
 
 extern void ia32_init_addr_space (struct pt_regs *regs);
-extern int ia32_setup_arg_pages (struct linux_binprm *bprm);
+extern int ia32_setup_arg_pages (struct linux_binprm *bprm, int exec_stack);
 extern unsigned long ia32_do_mmap (struct file *, unsigned long, unsigned long, int, int, loff_t);
 extern void ia32_load_segment_descriptors (struct task_struct *task);
 
diff -uNrp linux-2.6.4.EXSTACK/arch/mips/kernel/irixelf.c linux-2.6.4.noexec2/arch/mips/kernel/irixelf.c
--- linux-2.6.4.EXSTACK/arch/mips/kernel/irixelf.c	2004-03-11 03:56:03.000000000 +0100
+++ linux-2.6.4.noexec2/arch/mips/kernel/irixelf.c	2004-03-24 01:08:11.000000000 +0100
@@ -688,7 +688,7 @@ static int load_irix_binary(struct linux
 	 * change some of these later.
 	 */
 	current->mm->rss = 0;
-	setup_arg_pages(bprm);
+	setup_arg_pages(bprm, EXSTACK_DEFAULT);
 	current->mm->start_stack = bprm->p;
 
 	/* At this point, we assume that the image should be loaded at
diff -uNrp linux-2.6.4.EXSTACK/arch/s390/kernel/binfmt_elf32.c linux-2.6.4.noexec2/arch/s390/kernel/binfmt_elf32.c
--- linux-2.6.4.EXSTACK/arch/s390/kernel/binfmt_elf32.c	2004-03-11 03:55:44.000000000 +0100
+++ linux-2.6.4.noexec2/arch/s390/kernel/binfmt_elf32.c	2004-03-23 22:51:25.000000000 +0100
@@ -114,7 +114,7 @@ typedef s390_regs32 elf_gregset_t;
 #include <linux/binfmts.h>
 #include <linux/compat.h>
 
-int setup_arg_pages32(struct linux_binprm *bprm);
+int setup_arg_pages32(struct linux_binprm *bprm, int executable_stack);
 
 #define elf_prstatus elf_prstatus32
 struct elf_prstatus32
@@ -165,7 +165,7 @@ struct elf_prpsinfo32
 
 #undef start_thread
 #define start_thread                    start_thread31 
-#define setup_arg_pages(bprm)           setup_arg_pages32(bprm)
+#define setup_arg_pages(bprm, exec)     setup_arg_pages32(bprm, exec)
 #define elf_map				elf_map32
 
 MODULE_DESCRIPTION("Binary format loader for compatibility with 32bit Linux for S390 binaries,"
diff -uNrp linux-2.6.4.EXSTACK/arch/s390/kernel/compat_exec.c linux-2.6.4.noexec2/arch/s390/kernel/compat_exec.c
--- linux-2.6.4.EXSTACK/arch/s390/kernel/compat_exec.c	2004-03-23 21:18:35.000000000 +0100
+++ linux-2.6.4.noexec2/arch/s390/kernel/compat_exec.c	2004-03-23 22:58:15.000000000 +0100
@@ -37,7 +37,7 @@
 #undef STACK_TOP
 #define STACK_TOP TASK31_SIZE
 
-int setup_arg_pages32(struct linux_binprm *bprm)
+int setup_arg_pages32(struct linux_binprm *bprm, int executable_stack)
 {
 	unsigned long stack_base;
 	struct vm_area_struct *mpnt;
@@ -66,6 +66,7 @@ int setup_arg_pages32(struct linux_binpr
 		mpnt->vm_mm = mm;
 		mpnt->vm_start = PAGE_MASK & (unsigned long) bprm->p;
 		mpnt->vm_end = STACK_TOP;
+		/* executable stack setting would be applied here */
 		mpnt->vm_page_prot = PAGE_COPY;
 		mpnt->vm_flags = VM_STACK_FLAGS;
 		mpnt->vm_ops = NULL;
diff -uNrp linux-2.6.4.EXSTACK/arch/sparc64/kernel/binfmt_aout32.c linux-2.6.4.noexec2/arch/sparc64/kernel/binfmt_aout32.c
--- linux-2.6.4.EXSTACK/arch/sparc64/kernel/binfmt_aout32.c	2004-03-11 03:55:27.000000000 +0100
+++ linux-2.6.4.noexec2/arch/sparc64/kernel/binfmt_aout32.c	2004-03-24 01:09:55.000000000 +0100
@@ -310,7 +310,7 @@ beyond_if:
 	orig_thr_flags = current_thread_info()->flags;
 	current_thread_info()->flags |= _TIF_32BIT;
 
-	retval = setup_arg_pages(bprm);
+	retval = setup_arg_pages(bprm, EXSTACK_DEFAULT);
 	if (retval < 0) { 
 		current_thread_info()->flags = orig_thr_flags;
 
diff -uNrp linux-2.6.4.EXSTACK/arch/x86_64/ia32/ia32_aout.c linux-2.6.4.noexec2/arch/x86_64/ia32/ia32_aout.c
--- linux-2.6.4.EXSTACK/arch/x86_64/ia32/ia32_aout.c	2004-03-11 03:55:54.000000000 +0100
+++ linux-2.6.4.noexec2/arch/x86_64/ia32/ia32_aout.c	2004-03-24 01:13:31.000000000 +0100
@@ -35,7 +35,7 @@
 #undef WARN_OLD
 #undef CORE_DUMP /* probably broken */
 
-extern int ia32_setup_arg_pages(struct linux_binprm *bprm);
+extern int ia32_setup_arg_pages(struct linux_binprm *bprm, int exec_stack);
 
 static int load_aout_binary(struct linux_binprm *, struct pt_regs * regs);
 static int load_aout_library(struct file*);
@@ -395,7 +395,7 @@ beyond_if:
 
 	set_brk(current->mm->start_brk, current->mm->brk);
 
-	retval = ia32_setup_arg_pages(bprm); 
+	retval = ia32_setup_arg_pages(bprm, EXSTACK_DEFAULT);
 	if (retval < 0) { 
 		/* Someone check-me: is this error path enough? */ 
 		send_sig(SIGKILL, current, 0); 
diff -uNrp linux-2.6.4.EXSTACK/arch/x86_64/ia32/ia32_binfmt.c linux-2.6.4.noexec2/arch/x86_64/ia32/ia32_binfmt.c
--- linux-2.6.4.EXSTACK/arch/x86_64/ia32/ia32_binfmt.c	2004-03-23 22:42:31.000000000 +0100
+++ linux-2.6.4.noexec2/arch/x86_64/ia32/ia32_binfmt.c	2004-03-24 01:11:21.000000000 +0100
@@ -272,8 +272,8 @@ do {							\
 #define load_elf_binary load_elf32_binary
 
 #define ELF_PLAT_INIT(r, load_addr)	elf32_init(r)
-#define setup_arg_pages(bprm)		ia32_setup_arg_pages(bprm)
-int ia32_setup_arg_pages(struct linux_binprm *bprm);
+#define setup_arg_pages(bprm, exec_stack)	ia32_setup_arg_pages(bprm, exec_stack)
+int ia32_setup_arg_pages(struct linux_binprm *bprm, int executable_stack);
 
 #undef start_thread
 #define start_thread(regs,new_rip,new_rsp) do { \
@@ -325,7 +325,7 @@ static void elf32_init(struct pt_regs *r
 	me->thread.es = __USER_DS;
 }
 
-int setup_arg_pages(struct linux_binprm *bprm)
+int setup_arg_pages(struct linux_binprm *bprm, int executable_stack)
 {
 	unsigned long stack_base;
 	struct vm_area_struct *mpnt;
@@ -354,7 +354,12 @@ int setup_arg_pages(struct linux_binprm 
 		mpnt->vm_mm = mm;
 		mpnt->vm_start = PAGE_MASK & (unsigned long) bprm->p;
 		mpnt->vm_end = IA32_STACK_TOP;
-		mpnt->vm_flags = vm_stack_flags32; 
+		if (executable_stack == EXSTACK_ENABLE_X)
+			mpnt->vm_flags = vm_stack_flags32 |  VM_EXEC;
+		else if (executable_stack == EXSTACK_DISABLE_X)
+			mpnt->vm_flags = vm_stack_flags32 & ~VM_EXEC;
+		else
+			mpnt->vm_flags = vm_stack_flags32;
  		mpnt->vm_page_prot = (mpnt->vm_flags & VM_EXEC) ? 
  			PAGE_COPY_EXEC : PAGE_COPY;
 		mpnt->vm_ops = NULL;
@@ -373,7 +378,7 @@ int setup_arg_pages(struct linux_binprm 
 		struct page *page = bprm->page[i];
 		if (page) {
 			bprm->page[i] = NULL;
-			put_dirty_page(current,page,stack_base,PAGE_COPY_EXEC, mpnt);
+			put_dirty_page(current,page,stack_base,mpnt->vm_page_prot,mpnt);
 		}
 		stack_base += PAGE_SIZE;
 	}
diff -uNrp linux-2.6.4.EXSTACK/fs/binfmt_aout.c linux-2.6.4.noexec2/fs/binfmt_aout.c
--- linux-2.6.4.EXSTACK/fs/binfmt_aout.c	2004-03-23 22:42:31.000000000 +0100
+++ linux-2.6.4.noexec2/fs/binfmt_aout.c	2004-03-24 01:05:35.000000000 +0100
@@ -413,7 +413,7 @@ beyond_if:
 
 	set_brk(current->mm->start_brk, current->mm->brk);
 
-	retval = setup_arg_pages(bprm); 
+	retval = setup_arg_pages(bprm, EXSTACK_DEFAULT); 
 	if (retval < 0) { 
 		/* Someone check-me: is this error path enough? */ 
 		send_sig(SIGKILL, current, 0); 
diff -uNrp linux-2.6.4.EXSTACK/fs/binfmt_elf.c linux-2.6.4.noexec2/fs/binfmt_elf.c
--- linux-2.6.4.EXSTACK/fs/binfmt_elf.c	2004-03-23 22:42:31.000000000 +0100
+++ linux-2.6.4.noexec2/fs/binfmt_elf.c	2004-03-24 01:03:32.000000000 +0100
@@ -476,6 +476,7 @@ static int load_elf_binary(struct linux_
   	struct exec interp_ex;
 	char passed_fileno[6];
 	struct files_struct *files;
+	int executable_stack = EXSTACK_DEFAULT;
 	
 	/* Get the exec-header */
 	elf_ex = *((struct elfhdr *) bprm->buf);
@@ -599,6 +600,15 @@ static int load_elf_binary(struct linux_
 		elf_ppnt++;
 	}
 
+	elf_ppnt = elf_phdata;
+	for (i = 0; i < elf_ex.e_phnum; i++, elf_ppnt++)
+		if (elf_ppnt->p_type == PT_GNU_STACK) {
+			if (elf_ppnt->p_flags & PF_X)
+				executable_stack = EXSTACK_ENABLE_X;
+			else
+				executable_stack = EXSTACK_DISABLE_X;
+		}
+
 	/* Some simple consistency checks for the interpreter */
 	if (elf_interpreter) {
 		interpreter_type = INTERPRETER_ELF | INTERPRETER_AOUT;
@@ -674,7 +684,7 @@ static int load_elf_binary(struct linux_
 	   change some of these later */
 	current->mm->rss = 0;
 	current->mm->free_area_cache = TASK_UNMAPPED_BASE;
-	retval = setup_arg_pages(bprm);
+	retval = setup_arg_pages(bprm, executable_stack);
 	if (retval < 0) {
 		send_sig(SIGKILL, current, 0);
 		goto out_free_dentry;
diff -uNrp linux-2.6.4.EXSTACK/fs/binfmt_som.c linux-2.6.4.noexec2/fs/binfmt_som.c
--- linux-2.6.4.EXSTACK/fs/binfmt_som.c	2004-03-23 22:42:31.000000000 +0100
+++ linux-2.6.4.noexec2/fs/binfmt_som.c	2004-03-24 01:05:55.000000000 +0100
@@ -254,7 +254,7 @@ load_som_binary(struct linux_binprm * bp
 
 	set_binfmt(&som_format);
 	compute_creds(bprm);
-	setup_arg_pages(bprm);
+	setup_arg_pages(bprm, EXSTACK_DEFAULT);
 
 	create_som_tables(bprm);
 
diff -uNrp linux-2.6.4.EXSTACK/fs/exec.c linux-2.6.4.noexec2/fs/exec.c
--- linux-2.6.4.EXSTACK/fs/exec.c	2004-03-23 22:42:31.000000000 +0100
+++ linux-2.6.4.noexec2/fs/exec.c	2004-03-24 01:18:14.000000000 +0100
@@ -342,7 +342,7 @@ out_sig:
 	return;
 }
 
-int setup_arg_pages(struct linux_binprm *bprm)
+int setup_arg_pages(struct linux_binprm *bprm, int executable_stack)
 {
 	unsigned long stack_base;
 	struct vm_area_struct *mpnt;
@@ -425,8 +425,16 @@ int setup_arg_pages(struct linux_binprm 
 		mpnt->vm_start = PAGE_MASK & (unsigned long) bprm->p;
 		mpnt->vm_end = STACK_TOP;
 #endif
-		mpnt->vm_page_prot = protection_map[VM_STACK_FLAGS & 0x7];
-		mpnt->vm_flags = VM_STACK_FLAGS;
+		/* Adjust stack execute permissions; explicitly enable 
+		 * for EXSTACK_ENABLE_X, disable for EXSTACK_DISABLE_X
+		 * and leave alone (arch default) otherwise. */
+		if (unlikely(executable_stack == EXSTACK_ENABLE_X))
+			mpnt->vm_flags = VM_STACK_FLAGS |  VM_EXEC;
+		else if (executable_stack == EXSTACK_DISABLE_X)
+			mpnt->vm_flags = VM_STACK_FLAGS & ~VM_EXEC;
+		else
+			mpnt->vm_flags = VM_STACK_FLAGS;
+		mpnt->vm_page_prot = protection_map[mpnt->vm_flags & 0x7];
 		mpnt->vm_ops = NULL;
 		mpnt->vm_pgoff = mpnt->vm_start >> PAGE_SHIFT;
 		mpnt->vm_file = NULL;
diff -uNrp linux-2.6.4.EXSTACK/include/linux/binfmts.h linux-2.6.4.noexec2/include/linux/binfmts.h
--- linux-2.6.4.EXSTACK/include/linux/binfmts.h	2004-03-23 22:42:31.000000000 +0100
+++ linux-2.6.4.noexec2/include/linux/binfmts.h	2004-03-24 01:15:09.000000000 +0100
@@ -58,7 +58,10 @@ extern int prepare_binprm(struct linux_b
 extern void remove_arg_zero(struct linux_binprm *);
 extern int search_binary_handler(struct linux_binprm *,struct pt_regs *);
 extern int flush_old_exec(struct linux_binprm * bprm);
-extern int setup_arg_pages(struct linux_binprm * bprm);
+#define EXSTACK_DEFAULT   0
+#define EXSTACK_DISABLE_X 1
+#define EXSTACK_ENABLE_X  2
+extern int setup_arg_pages(struct linux_binprm * bprm, int executable_stack);
 extern int copy_strings(int argc,char __user * __user * argv,struct linux_binprm *bprm); 
 extern int copy_strings_kernel(int argc,char ** argv,struct linux_binprm *bprm);
 extern void compute_creds(struct linux_binprm *binprm);
diff -uNrp linux-2.6.4.EXSTACK/include/linux/elf.h linux-2.6.4.noexec2/include/linux/elf.h
--- linux-2.6.4.EXSTACK/include/linux/elf.h	2004-03-23 22:42:31.000000000 +0100
+++ linux-2.6.4.noexec2/include/linux/elf.h	2004-03-23 21:18:35.000000000 +0100
@@ -35,6 +35,8 @@ typedef __s64	Elf64_Sxword;
 #define PT_HIPROC  0x7fffffff
 #define PT_GNU_EH_FRAME		0x6474e550
 
+#define PT_GNU_STACK	(PT_LOOS + 0x474e551)
+
 /* These constants define the different elf file types */
 #define ET_NONE   0
 #define ET_REL    1

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: Non-Exec stack patches

[David Mosberger]

>>>>> On Wed, 24 Mar 2004 01:21:49 +0100, Kurt Garloff <garloff@suse.de> said:

  >> Which architectures are currently making their pre-page execute
  >> permissions depend upon VM_EXEC?  Would additional arch patches
  >> be needed for this?

  Kurt> It works on AMD64 (not ia32e), both for 64bit and 32bit
  Kurt> binaries.  I have not yet tested other archs.

IA64 Linux had non-executable stack/data before PT_GNU_STACK was invented.
It's keyed off a ELF header flags bit:

/* Least-significant four bits of ELF header's e_flags are
   OS-specific.  The bits are interpreted as follows by Linux: */
#define EF_IA_64_LINUX_EXECUTABLE_STACK 0x1

I guess I never quiet understood why an entire program header is
needed for this, but that's just me.

	--david

Re: Non-Exec stack patches

[Andrew Morton]

Kurt Garloff <garloff@suse.de> wrote:
>
> > Which architectures are currently making their pre-page execute permissions
> > depend upon VM_EXEC?  Would additional arch patches be needed for this?
> 
> It works on AMD64 (not ia32e), both for 64bit and 32bit binaries.
> I have not yet tested other archs.
> 
> If the values in the protection_map are different depending on bit 2,
> the patch will be effecitve. (OK, the CPU/MMU needs to honour the
> setting of course.) Most likely, the values for 
> protection_map[7] is PAGE_COPY_EXEC and of protection_map[3] is
> PAGE_COPY.

OK.

> > This may not get past Linus of course.  It doesn't even get past me with
> > that magical undocumented -1/0/+1 value of the executable_stack argument. 
> > Please replace that with a proper, commented, #defined-or-enumerated value,
> 
> As you wish, master.
> Slightly edited and untested patch attached.

It gets rejects in arch/x86_64/ia32/ia32_binfmt.c and
arch/ia64/ia32/binfmt_elf32.c - someone has been dinking with your
put_dirty_page() prototype.  I dropped those bits.

And I added the missing bit:

--- 25/include/linux/binfmts.h~noexec-stack-comments	Tue Mar 23 16:35:50 2004
+++ 25-akpm/include/linux/binfmts.h	Tue Mar 23 16:37:11 2004
@@ -62,9 +62,12 @@ extern int prepare_binprm(struct linux_b
 extern void remove_arg_zero(struct linux_binprm *);
 extern int search_binary_handler(struct linux_binprm *,struct pt_regs *);
 extern int flush_old_exec(struct linux_binprm * bprm);
-#define EXSTACK_DEFAULT   0
-#define EXSTACK_DISABLE_X 1
-#define EXSTACK_ENABLE_X  2
+
+/* Stack area protections */
+#define EXSTACK_DEFAULT   0	/* Whatever the arch defaults to */
+#define EXSTACK_DISABLE_X 1	/* Disable executable stacks */
+#define EXSTACK_ENABLE_X  2	/* Enable executable stacks */
+
 extern int setup_arg_pages(struct linux_binprm * bprm, int executable_stack);
 extern int copy_strings(int argc,char __user * __user * argv,struct linux_binprm *bprm); 
 extern int copy_strings_kernel(int argc,char ** argv,struct linux_binprm *bprm);



Now, what should the kernel do if the executable requests EXSTACK_DISABLE_X
but the kernel cannot do that?  Is it not a bit misleading/dangerous to
permit the executable to run anyway?

Re: Non-Exec stack patches

[Ingo Molnar]

On Tue, 23 Mar 2004, Andrew Morton wrote:

> Now, what should the kernel do if the executable requests
> EXSTACK_DISABLE_X but the kernel cannot do that?  Is it not a bit
> misleading/dangerous to permit the executable to run anyway?

it's not really a problem. We already ignore the !X bit on x86.

	Ingo

Re: Non-Exec stack patches

[Ulrich Drepper]

David Mosberger wrote:

> I guess I never quiet understood why an entire program header is
> needed for this, but that's just me.

This just means you haven't looked at the problem.

First, the ELF bits are limited and very crowded on some archs.  There
is no central assignment so conflicts will happen.

And one single bit does not cut it.  If you'd take a look, the
PT_GNU_STACK entry's permissions field specifies what permissions the
stack must have, not the presence of the field.  So at least two bits
are needed which only adds to the problems of finding appropriate bits.

-- 
➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖

Re: Non-Exec stack patches

[David Mosberger]

>>>>> On Tue, 23 Mar 2004 17:20:12 -0800, Ulrich Drepper <drepper@redhat.com> said:

  Uli> David Mosberger wrote:
  >> I guess I never quiet understood why an entire program header is
  >> needed for this, but that's just me.

  Uli> First, the ELF bits are limited and very crowded on some archs.  There
  Uli> is no central assignment so conflicts will happen.

Fair enough, but I don't see why this should imply that platforms that
already do have support for no-exec data/stack should be forced into
using PT_GNU_STACK.  Just for uniformity's sake?  Or is there a real
benefit?

  Uli> And one single bit does not cut it.  If you'd take a look, the
  Uli> PT_GNU_STACK entry's permissions field specifies what permissions the
  Uli> stack must have, not the presence of the field.  So at least two bits
  Uli> are needed which only adds to the problems of finding appropriate bits.

What stack protections other than RW- and RWX are useful?

	--david

Re: Non-Exec stack patches

[Ulrich Drepper]

David Mosberger wrote:

> What stack protections other than RW- and RWX are useful?

It's not about "what protections".  The three currently recognized
states are PT_GNU_STACK not present, rwx, rw-.  Ingo's code documents
this.  For those who need more, I'll have a paper coming up for a
conference in Toronto in April.

-- 
➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖

Re: Non-Exec stack patches

[Andi Kleen]

Andrew Morton <akpm@osdl.org> writes:

> Which architectures are currently making their pre-page execute permissions
> depend upon VM_EXEC?  
> Would additional arch patches be needed for this?

Yes, they would need some straight forward minor patches e.g. in the
32bit emulation. IA64 would be a candidate I guess.

i386 could do it on NX capable CPUs with PAE kernels (but it would require 
backporting some fixes from x86-64). However currently it doesn't make
much sense because all x86 CPUs that support NX (AMD K8 currently only) 
support 64bit kernels and people can as well run 64bit kernels.
 
Doing it on i386 would only make sense if non 64bit capable CPUs ever get
NX. I heard VIA may be planning that, but so far there is nothing in their
shipping CPUs, so I guess we can skip that for now.

-Andi

Re: Non-Exec stack patches

[Jakub Jelinek]

On Tue, Mar 23, 2004 at 05:41:49PM -0800, David Mosberger wrote:
>   Uli> First, the ELF bits are limited and very crowded on some archs.  There
>   Uli> is no central assignment so conflicts will happen.
> 
> Fair enough, but I don't see why this should imply that platforms that
> already do have support for no-exec data/stack should be forced into
> using PT_GNU_STACK.  Just for uniformity's sake?  Or is there a real
> benefit?

Note that PT_GNU_STACK program header is not generated on EM_IA_64 and
EM_PPC64 ATM, because initially I thought it shouldn't be needed
(these 2 arches are the only ones which don't need executable stack
for GCC trampolines).
But I think we should change the toolchain and generate it on IA64 and PPC64
as well (only GCC would need changing, emitting
.section .note.GNU-stack,"",@progbits
at the end of every compile unit, ld takes care of the rest) exactly for
uniformity's sake and because you get ld.so handling free.
GLIBC dynamic linker will take care of making the stack executable if
say a binary which doesn't need executable stack depends on a shared library
which needs executable stack or even dlopens a library which needs
executable stack.

>   Uli> And one single bit does not cut it.  If you'd take a look, the
>   Uli> PT_GNU_STACK entry's permissions field specifies what permissions the
>   Uli> stack must have, not the presence of the field.  So at least two bits
>   Uli> are needed which only adds to the problems of finding appropriate bits.
> 
> What stack protections other than RW- and RWX are useful?

At least RW- (read-write but not executable stack), RWX (rw and executable
stack) and no PT_GNU_STACK segment (unknown, not marked binary), but it is
certainly extendable.

	Jakub

Re: Non-Exec stack patches

[David Mosberger]

>>>>> On Tue, 23 Mar 2004 18:01:14 -0800, Ulrich Drepper <drepper@redhat.com> said:

  Uli> David Mosberger wrote:
  >> What stack protections other than RW- and RWX are useful?

  Uli> It's not about "what protections".  The three currently
  Uli> recognized states are PT_GNU_STACK not present, rwx, rw-.
  Uli> Ingo's code documents this.  For those who need more, I'll have
  Uli> a paper coming up for a conference in Toronto in April.

I'd find the PT_GNU_STACK approach much more appealing if it actually
was used to get rid of the stack-special case in the kernel
altogether.  Certainly it seems like it could be used to get rid of
STACK_TOP (just use the segment's p_vaddr).  That would still leave
the TASK_UNMAPPED_BASE special-case, but at least it would be a step
in the right direction.  Also, in the case of ia64, p_memsz could be
used to give user-level control over whether they want the
register/memory stacks to grow towards each other or apart from each
other.

	--david

Re: Non-Exec stack patches

[David Mosberger]

>>>>> On Wed, 24 Mar 2004 02:00:20 -0500, Jakub Jelinek <jakub@redhat.com> said:

  Jakub> But I think we should change the toolchain and generate it on
  Jakub> IA64 and PPC64 as well (only GCC would need changing,
  Jakub> emitting .section .note.GNU-stack,"",@progbits at the end of
  Jakub> every compile unit, ld takes care of the rest) exactly for
  Jakub> uniformity's sake and because you get ld.so handling free.

I'm not following you on the "get ld.so handling free" part.  How is
that handling free?

  Jakub> GLIBC dynamic linker will take care of making the stack
  Jakub> executable if say a binary which doesn't need executable
  Jakub> stack depends on a shared library which needs executable
  Jakub> stack or even dlopens a library which needs executable stack.

Actually, that's something that worries me.  Somebody just needs to
succeed in loading any shared object with the right PT_GNU_STACK
header and then the entire program will be exposed to the risk of a
writable stack.  On ia64, I just don't see any need to ever implicitly
turn on execute-permission on the stack, so why allow this extra
backdoor?

	--david

Re: Non-Exec stack patches

[Jakub Jelinek]

On Tue, Mar 23, 2004 at 11:16:36PM -0800, David Mosberger wrote:
> I'm not following you on the "get ld.so handling free" part.  How is
> that handling free?

What I meant is that it is already written and tested.

> Actually, that's something that worries me.  Somebody just needs to
> succeed in loading any shared object with the right PT_GNU_STACK
> header and then the entire program will be exposed to the risk of a
> writable stack.  On ia64, I just don't see any need to ever implicitly
> turn on execute-permission on the stack, so why allow this extra
> backdoor?

What kind of backdoor is it?  If you dlopen untrusted shared libraries
into your program you have far bigger problem than executable
stack (you can execute any code it wants in its constructors).

If there is a shared library which needs executable stack for its use
(on !IA64 !PPC64 this is e.g. any library which takes address of
a nested function and passes it to some other function and/or stores
it into some variable which cannot be optimized out, on IA64 or PPC64
this is of course much rarer, but it is still possible some language
interpreter or something builds code on the fly on the stack).

	Jakub

Re: Non-Exec stack patches

[David Mosberger]

>>>>> On Wed, 24 Mar 2004 02:28:40 -0500, Jakub Jelinek <jakub@redhat.com> said:

  Jakub> On Tue, Mar 23, 2004 at 11:16:36PM -0800, David Mosberger
  Jakub> wrote:

  >> I'm not following you on the "get ld.so handling free" part.  How
  >> is that handling free?

  Jakub> What I meant is that it is already written and tested.

OK.  The ELF flags bit is even more free, then... ;-)

  >> Actually, that's something that worries me.  Somebody just needs
  >> to succeed in loading any shared object with the right
  >> PT_GNU_STACK header and then the entire program will be exposed
  >> to the risk of a writable stack.  On ia64, I just don't see any
  >> need to ever implicitly turn on execute-permission on the stack,
  >> so why allow this extra backdoor?

  Jakub> What kind of backdoor is it?  If you dlopen untrusted shared
  Jakub> libraries into your program you have far bigger problem than
  Jakub> executable stack (you can execute any code it wants in its
  Jakub> constructors).

Sure.  Theoretically, none of this matters at all (thanks to
mprotect()), so we're justs talking about raising the barrier.  With
PT_GNU_STACK, it's sufficient to tweak one bit in any dependent
library, whereas with the ELF flag bit, you need to tweak one bit in
the main executable.  Not a huge difference, I'll submit, but from an
admin perspective, I find it a lot easier to check the main program to
see if it has the "executable stack/data" bit set rather than all
dependent libraries.  Additionally, we could easily choose to drop
support for the ELF flag bit altogether.  The only program that I know
of that ever needed executable data was XFree86 and that was only due
to an oversight---and that has long been fixed.

  Jakub> If there is a shared library which needs executable stack for
  Jakub> its use (on !IA64 !PPC64 this is e.g. any library which takes
  Jakub> address of a nested function and passes it to some other
  Jakub> function and/or stores it into some variable which cannot be
  Jakub> optimized out, on IA64 or PPC64 this is of course much rarer,

For sufficiently small values of "much rarer", I agree. ;-)

  Jakub> but it is still possible some language interpreter or
  Jakub> something builds code on the fly on the stack).

That's why there is mprotect().

	--david

Re: Non-Exec stack patches

[Stefan Smietanowski]

Hi Andi.

>>Which architectures are currently making their pre-page execute permissions
>>depend upon VM_EXEC?  
>>Would additional arch patches be needed for this?
> 
> 
> Yes, they would need some straight forward minor patches e.g. in the
> 32bit emulation. IA64 would be a candidate I guess.
> 
> i386 could do it on NX capable CPUs with PAE kernels (but it would require 
> backporting some fixes from x86-64). However currently it doesn't make
> much sense because all x86 CPUs that support NX (AMD K8 currently only) 
> support 64bit kernels and people can as well run 64bit kernels.
>  
> Doing it on i386 would only make sense if non 64bit capable CPUs ever get
> NX. I heard VIA may be planning that, but so far there is nothing in their
> shipping CPUs, so I guess we can skip that for now.

Well, there's also the case that (unknown if rumour or confirmed) there
will be AthlonXPs based on the K8 core that do NOT run 64bit code.

I would THINK they would include the NX bit but that's just a guess of
course.

// Stefan

Re: Non-Exec stack patches

[Kurt Garloff]

[-- Attachment #1.1: Type: text/plain, Size: 1080 bytes --]

Hi Andrew, Ingo,

On Tue, Mar 23, 2004 at 04:41:04PM -0800, Andrew Morton wrote:
> It gets rejects in arch/x86_64/ia32/ia32_binfmt.c and
> arch/ia64/ia32/binfmt_elf32.c - someone has been dinking with your
> put_dirty_page() prototype. 

That's the anon_vma stuff from Andrea, which we have merged locally.
Sorry, I forgot that it touched these places.

>  I dropped those bits.

Not a good idea, as stack protection won't work in ia32 emulation of
x86_64 and ia64 then.

I readded and did a clean diff against 2.6.5-rc2 and also integrated 
your comments patch.

> And I added the missing bit:

Thx!

> Now, what should the kernel do if the executable requests EXSTACK_DISABLE_X
> but the kernel cannot do that?  Is it not a bit misleading/dangerous to
> permit the executable to run anyway?

Yes. It has to ... 
Otherwise, most binaries would not work in i386 any more :-/

Updated patch attached.
-- 
Kurt Garloff  <garloff@suse.de>                            Cologne, DE 
SUSE LINUX AG, Nuernberg, DE                          SUSE Labs (Head)

[-- Attachment #1.2: noexec-stack.patch --]
[-- Type: text/plain, Size: 15351 bytes --]


From: Kurt Garloff <garloff@suse.de>

A patch to parse the elf binaries for a PT_GNU_STACK section to set the
stack non-executable if possible.  Most parts have been shamelessly stolen
from Ingo Molnar's more ambitious stackshield
http://people.redhat.com/mingo/exec-shield/exec-shield-2.6.4-C9

The toolchain has meanwhile support for marking the binaries with a
PT_GNU_STACK section wwithout x bit as needed.

If no such section is found, we leave the stack to whatever the arch
defaults to.  If there is one, we explicitly disabled the VM_EXEC bit if no
x bit is found, otherwise explicitly enable.

---

 arch/ia64/ia32/binfmt_elf32.c       |   16 +++++++++++-----
 arch/ia64/ia32/ia32priv.h           |    2 +-
 arch/mips/kernel/irixelf.c          |    2 +-
 arch/s390/kernel/binfmt_elf32.c     |    4 ++--
 arch/s390/kernel/compat_exec.c      |    3 ++-
 arch/sparc64/kernel/binfmt_aout32.c |    2 +-
 arch/x86_64/ia32/ia32_aout.c        |    4 ++--
 arch/x86_64/ia32/ia32_binfmt.c      |   15 ++++++++++-----
 fs/binfmt_aout.c                    |    2 +-
 fs/binfmt_elf.c                     |   12 +++++++++++-
 fs/binfmt_som.c                     |    2 +-
 fs/exec.c                           |   14 +++++++++++---
 include/linux/binfmts.h             |    8 +++++++-
 include/linux/elf.h                 |    2 ++
 14 files changed, 63 insertions(+), 25 deletions(-)

diff -uNrp linux-2.6.5-rc2/arch/ia64/ia32/binfmt_elf32.c linux-2.6.5-rc2.nonexecstack/arch/ia64/ia32/binfmt_elf32.c
--- linux-2.6.5-rc2/arch/ia64/ia32/binfmt_elf32.c	2004-03-11 03:55:44.000000000 +0100
+++ linux-2.6.5-rc2.nonexecstack/arch/ia64/ia32/binfmt_elf32.c	2004-03-24 11:43:50.000000000 +0100
@@ -35,7 +35,7 @@ extern void ia64_elf32_init (struct pt_r
 
 static void elf32_set_personality (void);
 
-#define setup_arg_pages(bprm)		ia32_setup_arg_pages(bprm)
+#define setup_arg_pages(bprm,exec)		ia32_setup_arg_pages(bprm,exec)
 #define elf_map				elf32_map
 
 #undef SET_PERSONALITY
@@ -149,7 +149,7 @@ ia64_elf32_init (struct pt_regs *regs)
 }
 
 int
-ia32_setup_arg_pages (struct linux_binprm *bprm)
+ia32_setup_arg_pages (struct linux_binprm *bprm, int executable_stack)
 {
 	unsigned long stack_base;
 	struct vm_area_struct *mpnt;
@@ -178,8 +178,14 @@ ia32_setup_arg_pages (struct linux_binpr
 		mpnt->vm_mm = current->mm;
 		mpnt->vm_start = PAGE_MASK & (unsigned long) bprm->p;
 		mpnt->vm_end = IA32_STACK_TOP;
-		mpnt->vm_page_prot = PAGE_COPY;
-		mpnt->vm_flags = VM_STACK_FLAGS;
+		if (executable_stack == EXSTACK_ENABLE_X)
+			mpnt->vm_flags = VM_STACK_FLAGS |  VM_EXEC;
+		else if (executable_stack == EXSTACK_DISABLE_X)
+			mpnt->vm_flags = VM_STACK_FLAGS & ~VM_EXEC;
+		else
+			mpnt->vm_flags = VM_STACK_FLAGS;
+		mpnt->vm_page_prot = (mpnt->vm_flags & VM_EXEC)?
+					PAGE_COPY_EXEC: PAGE_COPY;
 		mpnt->vm_ops = NULL;
 		mpnt->vm_pgoff = 0;
 		mpnt->vm_file = NULL;
@@ -192,7 +198,7 @@ ia32_setup_arg_pages (struct linux_binpr
 		struct page *page = bprm->page[i];
 		if (page) {
 			bprm->page[i] = NULL;
-			put_dirty_page(current, page, stack_base, PAGE_COPY);
+			put_dirty_page(current, page, stack_base, mpnt->vm_page_prot);
 		}
 		stack_base += PAGE_SIZE;
 	}
diff -uNrp linux-2.6.5-rc2/arch/ia64/ia32/ia32priv.h linux-2.6.5-rc2.nonexecstack/arch/ia64/ia32/ia32priv.h
--- linux-2.6.5-rc2/arch/ia64/ia32/ia32priv.h	2004-03-11 03:55:51.000000000 +0100
+++ linux-2.6.5-rc2.nonexecstack/arch/ia64/ia32/ia32priv.h	2004-03-24 11:43:04.000000000 +0100
@@ -494,7 +494,7 @@ struct ia32_user_desc {
 struct linux_binprm;
 
 extern void ia32_init_addr_space (struct pt_regs *regs);
-extern int ia32_setup_arg_pages (struct linux_binprm *bprm);
+extern int ia32_setup_arg_pages (struct linux_binprm *bprm, int exec_stack);
 extern unsigned long ia32_do_mmap (struct file *, unsigned long, unsigned long, int, int, loff_t);
 extern void ia32_load_segment_descriptors (struct task_struct *task);
 
diff -uNrp linux-2.6.5-rc2/arch/mips/kernel/irixelf.c linux-2.6.5-rc2.nonexecstack/arch/mips/kernel/irixelf.c
--- linux-2.6.5-rc2/arch/mips/kernel/irixelf.c	2004-03-11 03:56:03.000000000 +0100
+++ linux-2.6.5-rc2.nonexecstack/arch/mips/kernel/irixelf.c	2004-03-24 11:43:04.000000000 +0100
@@ -688,7 +688,7 @@ static int load_irix_binary(struct linux
 	 * change some of these later.
 	 */
 	current->mm->rss = 0;
-	setup_arg_pages(bprm);
+	setup_arg_pages(bprm, EXSTACK_DEFAULT);
 	current->mm->start_stack = bprm->p;
 
 	/* At this point, we assume that the image should be loaded at
diff -uNrp linux-2.6.5-rc2/arch/s390/kernel/binfmt_elf32.c linux-2.6.5-rc2.nonexecstack/arch/s390/kernel/binfmt_elf32.c
--- linux-2.6.5-rc2/arch/s390/kernel/binfmt_elf32.c	2004-03-11 03:55:44.000000000 +0100
+++ linux-2.6.5-rc2.nonexecstack/arch/s390/kernel/binfmt_elf32.c	2004-03-24 11:43:04.000000000 +0100
@@ -114,7 +114,7 @@ typedef s390_regs32 elf_gregset_t;
 #include <linux/binfmts.h>
 #include <linux/compat.h>
 
-int setup_arg_pages32(struct linux_binprm *bprm);
+int setup_arg_pages32(struct linux_binprm *bprm, int executable_stack);
 
 #define elf_prstatus elf_prstatus32
 struct elf_prstatus32
@@ -165,7 +165,7 @@ struct elf_prpsinfo32
 
 #undef start_thread
 #define start_thread                    start_thread31 
-#define setup_arg_pages(bprm)           setup_arg_pages32(bprm)
+#define setup_arg_pages(bprm, exec)     setup_arg_pages32(bprm, exec)
 #define elf_map				elf_map32
 
 MODULE_DESCRIPTION("Binary format loader for compatibility with 32bit Linux for S390 binaries,"
diff -uNrp linux-2.6.5-rc2/arch/s390/kernel/compat_exec.c linux-2.6.5-rc2.nonexecstack/arch/s390/kernel/compat_exec.c
--- linux-2.6.5-rc2/arch/s390/kernel/compat_exec.c	2004-03-11 03:55:44.000000000 +0100
+++ linux-2.6.5-rc2.nonexecstack/arch/s390/kernel/compat_exec.c	2004-03-24 11:43:04.000000000 +0100
@@ -37,7 +37,7 @@
 #undef STACK_TOP
 #define STACK_TOP TASK31_SIZE
 
-int setup_arg_pages32(struct linux_binprm *bprm)
+int setup_arg_pages32(struct linux_binprm *bprm, int executable_stack)
 {
 	unsigned long stack_base;
 	struct vm_area_struct *mpnt;
@@ -66,6 +66,7 @@ int setup_arg_pages32(struct linux_binpr
 		mpnt->vm_mm = mm;
 		mpnt->vm_start = PAGE_MASK & (unsigned long) bprm->p;
 		mpnt->vm_end = STACK_TOP;
+		/* executable stack setting would be applied here */
 		mpnt->vm_page_prot = PAGE_COPY;
 		mpnt->vm_flags = VM_STACK_FLAGS;
 		mpnt->vm_ops = NULL;
diff -uNrp linux-2.6.5-rc2/arch/sparc64/kernel/binfmt_aout32.c linux-2.6.5-rc2.nonexecstack/arch/sparc64/kernel/binfmt_aout32.c
--- linux-2.6.5-rc2/arch/sparc64/kernel/binfmt_aout32.c	2004-03-11 03:55:27.000000000 +0100
+++ linux-2.6.5-rc2.nonexecstack/arch/sparc64/kernel/binfmt_aout32.c	2004-03-24 11:43:04.000000000 +0100
@@ -310,7 +310,7 @@ beyond_if:
 	orig_thr_flags = current_thread_info()->flags;
 	current_thread_info()->flags |= _TIF_32BIT;
 
-	retval = setup_arg_pages(bprm);
+	retval = setup_arg_pages(bprm, EXSTACK_DEFAULT);
 	if (retval < 0) { 
 		current_thread_info()->flags = orig_thr_flags;
 
diff -uNrp linux-2.6.5-rc2/arch/x86_64/ia32/ia32_aout.c linux-2.6.5-rc2.nonexecstack/arch/x86_64/ia32/ia32_aout.c
--- linux-2.6.5-rc2/arch/x86_64/ia32/ia32_aout.c	2004-03-11 03:55:54.000000000 +0100
+++ linux-2.6.5-rc2.nonexecstack/arch/x86_64/ia32/ia32_aout.c	2004-03-24 11:43:04.000000000 +0100
@@ -35,7 +35,7 @@
 #undef WARN_OLD
 #undef CORE_DUMP /* probably broken */
 
-extern int ia32_setup_arg_pages(struct linux_binprm *bprm);
+extern int ia32_setup_arg_pages(struct linux_binprm *bprm, int exec_stack);
 
 static int load_aout_binary(struct linux_binprm *, struct pt_regs * regs);
 static int load_aout_library(struct file*);
@@ -395,7 +395,7 @@ beyond_if:
 
 	set_brk(current->mm->start_brk, current->mm->brk);
 
-	retval = ia32_setup_arg_pages(bprm); 
+	retval = ia32_setup_arg_pages(bprm, EXSTACK_DEFAULT);
 	if (retval < 0) { 
 		/* Someone check-me: is this error path enough? */ 
 		send_sig(SIGKILL, current, 0); 
diff -uNrp linux-2.6.5-rc2/arch/x86_64/ia32/ia32_binfmt.c linux-2.6.5-rc2.nonexecstack/arch/x86_64/ia32/ia32_binfmt.c
--- linux-2.6.5-rc2/arch/x86_64/ia32/ia32_binfmt.c	2004-03-24 11:42:03.000000000 +0100
+++ linux-2.6.5-rc2.nonexecstack/arch/x86_64/ia32/ia32_binfmt.c	2004-03-24 11:44:30.000000000 +0100
@@ -272,8 +272,8 @@ do {							\
 #define load_elf_binary load_elf32_binary
 
 #define ELF_PLAT_INIT(r, load_addr)	elf32_init(r)
-#define setup_arg_pages(bprm)		ia32_setup_arg_pages(bprm)
-int ia32_setup_arg_pages(struct linux_binprm *bprm);
+#define setup_arg_pages(bprm, exec_stack)	ia32_setup_arg_pages(bprm, exec_stack)
+int ia32_setup_arg_pages(struct linux_binprm *bprm, int executable_stack);
 
 #undef start_thread
 #define start_thread(regs,new_rip,new_rsp) do { \
@@ -325,7 +325,7 @@ static void elf32_init(struct pt_regs *r
 	me->thread.es = __USER_DS;
 }
 
-int setup_arg_pages(struct linux_binprm *bprm)
+int setup_arg_pages(struct linux_binprm *bprm, int executable_stack)
 {
 	unsigned long stack_base;
 	struct vm_area_struct *mpnt;
@@ -354,7 +354,12 @@ int setup_arg_pages(struct linux_binprm 
 		mpnt->vm_mm = mm;
 		mpnt->vm_start = PAGE_MASK & (unsigned long) bprm->p;
 		mpnt->vm_end = IA32_STACK_TOP;
-		mpnt->vm_flags = vm_stack_flags32; 
+		if (executable_stack == EXSTACK_ENABLE_X)
+			mpnt->vm_flags = vm_stack_flags32 |  VM_EXEC;
+		else if (executable_stack == EXSTACK_DISABLE_X)
+			mpnt->vm_flags = vm_stack_flags32 & ~VM_EXEC;
+		else
+			mpnt->vm_flags = vm_stack_flags32;
  		mpnt->vm_page_prot = (mpnt->vm_flags & VM_EXEC) ? 
  			PAGE_COPY_EXEC : PAGE_COPY;
 		mpnt->vm_ops = NULL;
@@ -370,7 +375,7 @@ int setup_arg_pages(struct linux_binprm 
 		struct page *page = bprm->page[i];
 		if (page) {
 			bprm->page[i] = NULL;
-			put_dirty_page(current,page,stack_base,PAGE_COPY_EXEC);
+			put_dirty_page(current,page,stack_base,mpnt->vm_page_prot);
 		}
 		stack_base += PAGE_SIZE;
 	}
diff -uNrp linux-2.6.5-rc2/fs/binfmt_aout.c linux-2.6.5-rc2.nonexecstack/fs/binfmt_aout.c
--- linux-2.6.5-rc2/fs/binfmt_aout.c	2004-03-11 03:55:23.000000000 +0100
+++ linux-2.6.5-rc2.nonexecstack/fs/binfmt_aout.c	2004-03-24 11:43:04.000000000 +0100
@@ -413,7 +413,7 @@ beyond_if:
 
 	set_brk(current->mm->start_brk, current->mm->brk);
 
-	retval = setup_arg_pages(bprm); 
+	retval = setup_arg_pages(bprm, EXSTACK_DEFAULT);
 	if (retval < 0) { 
 		/* Someone check-me: is this error path enough? */ 
 		send_sig(SIGKILL, current, 0); 
diff -uNrp linux-2.6.5-rc2/fs/binfmt_elf.c linux-2.6.5-rc2.nonexecstack/fs/binfmt_elf.c
--- linux-2.6.5-rc2/fs/binfmt_elf.c	2004-03-24 11:42:03.000000000 +0100
+++ linux-2.6.5-rc2.nonexecstack/fs/binfmt_elf.c	2004-03-24 11:43:04.000000000 +0100
@@ -476,6 +476,7 @@ static int load_elf_binary(struct linux_
   	struct exec interp_ex;
 	char passed_fileno[6];
 	struct files_struct *files;
+	int executable_stack = EXSTACK_DEFAULT;
 	
 	/* Get the exec-header */
 	elf_ex = *((struct elfhdr *) bprm->buf);
@@ -599,6 +600,15 @@ static int load_elf_binary(struct linux_
 		elf_ppnt++;
 	}
 
+	elf_ppnt = elf_phdata;
+	for (i = 0; i < elf_ex.e_phnum; i++, elf_ppnt++)
+		if (elf_ppnt->p_type == PT_GNU_STACK) {
+			if (elf_ppnt->p_flags & PF_X)
+				executable_stack = EXSTACK_ENABLE_X;
+			else
+				executable_stack = EXSTACK_DISABLE_X;
+		}
+
 	/* Some simple consistency checks for the interpreter */
 	if (elf_interpreter) {
 		interpreter_type = INTERPRETER_ELF | INTERPRETER_AOUT;
@@ -674,7 +684,7 @@ static int load_elf_binary(struct linux_
 	   change some of these later */
 	current->mm->rss = 0;
 	current->mm->free_area_cache = TASK_UNMAPPED_BASE;
-	retval = setup_arg_pages(bprm);
+	retval = setup_arg_pages(bprm, executable_stack);
 	if (retval < 0) {
 		send_sig(SIGKILL, current, 0);
 		goto out_free_dentry;
diff -uNrp linux-2.6.5-rc2/fs/binfmt_som.c linux-2.6.5-rc2.nonexecstack/fs/binfmt_som.c
--- linux-2.6.5-rc2/fs/binfmt_som.c	2004-03-11 03:55:25.000000000 +0100
+++ linux-2.6.5-rc2.nonexecstack/fs/binfmt_som.c	2004-03-24 11:43:04.000000000 +0100
@@ -254,7 +254,7 @@ load_som_binary(struct linux_binprm * bp
 
 	set_binfmt(&som_format);
 	compute_creds(bprm);
-	setup_arg_pages(bprm);
+	setup_arg_pages(bprm, EXSTACK_DEFAULT);
 
 	create_som_tables(bprm);
 
diff -uNrp linux-2.6.5-rc2/fs/exec.c linux-2.6.5-rc2.nonexecstack/fs/exec.c
--- linux-2.6.5-rc2/fs/exec.c	2004-03-11 03:55:25.000000000 +0100
+++ linux-2.6.5-rc2.nonexecstack/fs/exec.c	2004-03-24 11:43:04.000000000 +0100
@@ -342,7 +342,7 @@ out_sig:
 	return;
 }
 
-int setup_arg_pages(struct linux_binprm *bprm)
+int setup_arg_pages(struct linux_binprm *bprm, int executable_stack)
 {
 	unsigned long stack_base;
 	struct vm_area_struct *mpnt;
@@ -425,8 +425,16 @@ int setup_arg_pages(struct linux_binprm 
 		mpnt->vm_start = PAGE_MASK & (unsigned long) bprm->p;
 		mpnt->vm_end = STACK_TOP;
 #endif
-		mpnt->vm_page_prot = protection_map[VM_STACK_FLAGS & 0x7];
-		mpnt->vm_flags = VM_STACK_FLAGS;
+		/* Adjust stack execute permissions; explicitly enable
+		 * for EXSTACK_ENABLE_X, disable for EXSTACK_DISABLE_X
+		 * and leave alone (arch default) otherwise. */
+		if (unlikely(executable_stack == EXSTACK_ENABLE_X))
+			mpnt->vm_flags = VM_STACK_FLAGS |  VM_EXEC;
+		else if (executable_stack == EXSTACK_DISABLE_X)
+			mpnt->vm_flags = VM_STACK_FLAGS & ~VM_EXEC;
+		else
+			mpnt->vm_flags = VM_STACK_FLAGS;
+		mpnt->vm_page_prot = protection_map[mpnt->vm_flags & 0x7];
 		mpnt->vm_ops = NULL;
 		mpnt->vm_pgoff = 0;
 		mpnt->vm_file = NULL;
diff -uNrp linux-2.6.5-rc2/include/linux/binfmts.h linux-2.6.5-rc2.nonexecstack/include/linux/binfmts.h
--- linux-2.6.5-rc2/include/linux/binfmts.h	2004-03-11 03:55:27.000000000 +0100
+++ linux-2.6.5-rc2.nonexecstack/include/linux/binfmts.h	2004-03-24 11:43:10.000000000 +0100
@@ -58,7 +58,13 @@ extern int prepare_binprm(struct linux_b
 extern void remove_arg_zero(struct linux_binprm *);
 extern int search_binary_handler(struct linux_binprm *,struct pt_regs *);
 extern int flush_old_exec(struct linux_binprm * bprm);
-extern int setup_arg_pages(struct linux_binprm * bprm);
+
+/* Stack area protections */
+#define EXSTACK_DEFAULT   0	/* Whatever the arch defaults to */
+#define EXSTACK_DISABLE_X 1	/* Disable executable stacks */
+#define EXSTACK_ENABLE_X  2	/* Enable executable stacks */
+
+extern int setup_arg_pages(struct linux_binprm * bprm, int executable_stack);
 extern int copy_strings(int argc,char __user * __user * argv,struct linux_binprm *bprm); 
 extern int copy_strings_kernel(int argc,char ** argv,struct linux_binprm *bprm);
 extern void compute_creds(struct linux_binprm *binprm);
diff -uNrp linux-2.6.5-rc2/include/linux/elf.h linux-2.6.5-rc2.nonexecstack/include/linux/elf.h
--- linux-2.6.5-rc2/include/linux/elf.h	2004-03-11 03:55:23.000000000 +0100
+++ linux-2.6.5-rc2.nonexecstack/include/linux/elf.h	2004-03-24 11:43:04.000000000 +0100
@@ -35,6 +35,8 @@ typedef __s64	Elf64_Sxword;
 #define PT_HIPROC  0x7fffffff
 #define PT_GNU_EH_FRAME		0x6474e550
 
+#define PT_GNU_STACK	(PT_LOOS + 0x474e551)
+
 /* These constants define the different elf file types */
 #define ET_NONE   0
 #define ET_REL    1

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: Non-Exec stack patches

[Andi Kleen]

> Well, there's also the case that (unknown if rumour or confirmed) there
> will be AthlonXPs based on the K8 core that do NOT run 64bit code.

Yes, you're right. Some HP laptops ship with such crippled chips. 

> I would THINK they would include the NX bit but that's just a guess of
> course.

Most likely yes. 

But who buys such crippled CPUs will have to live with that. Or do a patch.
NX support is mostly hype anyways, it doesn't give you much advantage.

-Andi

Re: Non-Exec stack patches

[John Reiser]

Jakub> but it is still possible some language interpreter or
Jakub> something builds code on the fly on the stack).

David> That's why there is mprotect().

But mprotect() costs enough (hundreds of cycles) to be a significant burden
in some cases.  Generating code to a stack region that is inherently
executable is inexpensive (even allowing for restrictive alignment and
avoiding I/D cache conflicts), is thread safe, is async-signal safe, and
takes less work than other alternatives.  Yes, the "black hats" do this;
so do the "white hats."  Please do not increase the minimum cost for
applications that want generate-and-execute on the stack at upredictable
high frequency.

-- 
John Reiser, jreiser@BitWagon.com

Re: Non-Exec stack patches

[David Mosberger]

>>>>> On Wed, 24 Mar 2004 08:29:24 -0800, John Reiser <jreiser@BitWagon.com> said:

  Jakub> but it is still possible some language interpreter or
  Jakub> something builds code on the fly on the stack).

  David> That's why there is mprotect().

  John> But mprotect() costs enough (hundreds of cycles) to be a
  John> significant burden in some cases.  Generating code to a stack
  John> region that is inherently executable is inexpensive (even
  John> allowing for restrictive alignment and avoiding I/D cache
  John> conflicts), is thread safe, is async-signal safe, and takes
  John> less work than other alternatives.  Yes, the "black hats" do
  John> this; so do the "white hats."  Please do not increase the
  John> minimum cost for applications that want generate-and-execute
  John> on the stack at upredictable high frequency.

Huh?  Only one mprotect() call is needed to make the entire stack
executable.

	--david

Re: Non-Exec stack patches

[Jakub Jelinek]

On Wed, Mar 24, 2004 at 09:12:30AM -0800, David Mosberger wrote:
>   David> That's why there is mprotect().
> 
>   John> But mprotect() costs enough (hundreds of cycles) to be a
>   John> significant burden in some cases.  Generating code to a stack
>   John> region that is inherently executable is inexpensive (even
>   John> allowing for restrictive alignment and avoiding I/D cache
>   John> conflicts), is thread safe, is async-signal safe, and takes
>   John> less work than other alternatives.  Yes, the "black hats" do
>   John> this; so do the "white hats."  Please do not increase the
>   John> minimum cost for applications that want generate-and-execute
>   John> on the stack at upredictable high frequency.
> 
> Huh?  Only one mprotect() call is needed to make the entire stack
> executable.

Nope.  Think about multithreaded apps.  Furthermore, getting the exact
extents of the particular stack is difficult to find for applications,
but e.g. the threading library has to know such things.

	Jakub

Re: Non-Exec stack patches

[Andi Kleen]

Jakub Jelinek <jakub@redhat.com> writes:
> 
> Nope.  Think about multithreaded apps.  Furthermore, getting the exact
> extents of the particular stack is difficult to find for applications,
> but e.g. the threading library has to know such things.

It's actually not that difficult. You just have to read /proc/self/maps
and check for the mapping of your current stack pointer.
For the main stack GROWSDOWN will inherit the x or nx on growing
down.

And cache a flag about this in a TLS variable.

-Andi

Re: Non-Exec stack patches

[Jakub Jelinek]

On Wed, Mar 24, 2004 at 06:49:21PM +0100, Andi Kleen wrote:
> It's actually not that difficult. You just have to read /proc/self/maps
> and check for the mapping of your current stack pointer.
> For the main stack GROWSDOWN will inherit the x or nx on growing
> down.

That assumes there are always gaps around thread stacks, which doesn't
have to be true.  You could make executable some other mapping
as well easily.

	Jakub

Re: Non-Exec stack patches

[David Mosberger]

>>>>> On Wed, 24 Mar 2004 12:24:54 -0500, Jakub Jelinek <jakub@redhat.com> said:

  >> Huh?  Only one mprotect() call is needed to make the entire stack
  >> executable.

  Jakub> Nope.  Think about multithreaded apps.

I said one mprotect() to make the entire stack executable.  Obviously,
if you have multiple stacks, you need one call per stack.  Big deal.

  Jakub> Furthermore, getting the exact extents of the particular
  Jakub> stack is difficult to find for applications, but e.g. the
  Jakub> threading library has to know such things.

And how is this a kernel problem?  There are other reasons why
applications need to know the stack extents (think garbage collector)
and it's entirely glibc's failing if it's difficult to determine the
stack extent.

	--david

Re: Non-Exec stack patches

[John Reiser]

> Only one mprotect() call is needed to make the entire stack
> executable.

mprotect() only works on the portion that is currently allocated.
If the stack grows, then another call is needed.  Tracking the high-
water mark is an expense.  Forcing the allocation of the maximum
extent for the stack of the current thread, even to the same copy-
on-write all-zero page, can cause memory overcommit problems.

-- 
John Reiser, jreiser@BitWagon.com

Re: Non-Exec stack patches

[David Mosberger]

>>>>> On Wed, 24 Mar 2004 11:02:45 -0800, John Reiser <jreiser@BitWagon.com> said:

  >> Only one mprotect() call is needed to make the entire stack
  >> executable.

  John> mprotect() only works on the portion that is currently allocated.
  John> If the stack grows, then another call is needed.

No, mprotect() on the entire stack will mark the vm_area with the
desired protection and VM_GROWSDOWN/VM_GROWSUP will expand
automatically with the new protection.  And if you want to expand the
stack in user-level, e.g., by intercepting SIGSEGV, you'll either do
an mmap() or mprotect() at any rate so there is zero extra cost there.

	--david

Re: Non-Exec stack patches

[Kurt Garloff]

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

Hi Andi,

On Wed, Mar 24, 2004 at 12:27:39PM +0100, Andi Kleen wrote:
Stefan Smietanowski <stesmi@stesmi.com> wrote:
> > I would THINK they would include the NX bit but that's just a guess of
> > course.
> 
> Most likely yes. 
> 
> But who buys such crippled CPUs will have to live with that. Or do a patch.

It should be straightforward. Put a different value in the
protection_map if such a CPU is detected. That should be all.

> NX support is mostly hype anyways, it doesn't give you much advantage.

It puts the hurdle somewhat higher and for some exploits does prevent
them. You can still overwrite the return address, but you need to have
put code into the data section prior to this to exploit. This may or may
not possible depending on how the daemon works. Marking the data section
non-executable would help further ...

It's always a tradeoff. Given the simplicity of the patch, I'm in favour
of having it.

Regards,
-- 
Kurt Garloff                   <kurt@garloff.de>             [Koeln, DE]
Physics:Plasma modeling <garloff@plasimo.phys.tue.nl> [TU Eindhoven, NL]
Linux: SUSE Labs (Head)        <garloff@suse.de>    [SUSE Nuernberg, DE]

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

RE: Non-Exec stack patches

[Siddha, Suresh B]

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

> -----Original Message-----
> From: linux-kernel-owner@vger.kernel.org
> [mailto:linux-kernel-owner@vger.kernel.org]On Behalf Of Andrew Morton
> Sent: Tuesday, March 23, 2004 4:41 PM
> To: Kurt Garloff
> Cc: linux-kernel@vger.kernel.org; mingo@redhat.com
> Subject: Re: Non-Exec stack patches
> 
> 
> Kurt Garloff <garloff@suse.de> wrote:
> >
> > > Which architectures are currently making their pre-page 
> execute permissions
> > > depend upon VM_EXEC?  Would additional arch patches be 
> needed for this?
> > 
> > It works on AMD64 (not ia32e), both for 64bit and 32bit binaries.
> > I have not yet tested other archs.
> > 
> > If the values in the protection_map are different depending 
> on bit 2,
> > the patch will be effecitve. (OK, the CPU/MMU needs to honour the
> > setting of course.) Most likely, the values for 
> > protection_map[7] is PAGE_COPY_EXEC and of protection_map[3] is
> > PAGE_COPY.
> 
> OK.
> 

Recent ia64 mm trees are broken because of this issue. Attached patch fixes protection_map[7] in IA64.

thanks,
suresh

[-- Attachment #2: noexec-ia64.fix --]
[-- Type: application/octet-stream, Size: 458 bytes --]

--- linux-265mm5/include/asm-ia64/pgtable.h~	2004-04-14 00:09:04.000000000 -0700
+++ linux-265mm5/include/asm-ia64/pgtable.h	2004-04-13 23:45:29.000000000 -0700
@@ -148,7 +148,7 @@
 #define __P100	__pgprot(__ACCESS_BITS | _PAGE_PL_3 | _PAGE_AR_X_RX)
 #define __P101	__pgprot(__ACCESS_BITS | _PAGE_PL_3 | _PAGE_AR_RX)
 #define __P110	PAGE_COPY
-#define __P111	PAGE_COPY
+#define __P111	PAGE_COPY_EXEC
 
 #define __S000	PAGE_NONE
 #define __S001	PAGE_READONLY

Re: Non-Exec stack patches

[Jamie Lokier]

Siddha, Suresh B wrote:
> Recent ia64 mm trees are broken because of this issue. Attached
> patch fixes protection_map[7] in IA64.

> --- linux-265mm5/include/asm-ia64/pgtable.h~    2004-04-14 00:09:04.000000000 -0700
> +++ linux-265mm5/include/asm-ia64/pgtable.h     2004-04-13 23:45:29.000000000 -0700
> @@ -148,7 +148,7 @@
>  #define __P100 __pgprot(__ACCESS_BITS | _PAGE_PL_3 | _PAGE_AR_X_RX)
>  #define __P101 __pgprot(__ACCESS_BITS | _PAGE_PL_3 | _PAGE_AR_RX)
>  #define __P110 PAGE_COPY
> -#define __P111 PAGE_COPY
> +#define __P111 PAGE_COPY_EXEC
>   
>  #define __S000 PAGE_NONE
>  #define __S001 PAGE_READONLY

I'm looking at 2.6.5, and it doesn't define PAGE_COPY_EXEC.  In 2.6.5,
asm-ia64/pgtable.h, PAGE_COPY is executable, and PAGE_READONLY is
non-executable.

Yes I know the naming is different from all the other asm-* files, but
that's what it says.  All of those definitions have confused names on
ia64, although they seem to have the rights bits set in the end.

Has the meaning of PAGE_COPY in asm-ia64/pgtable.h changed in 2.6.5-mm5?

If so, you want to change __P110 as well as __P111.

-- Jamie

PowerPC exec page protection

[Jamie Lokier]

<asm-ppc/pgtable.h> and <asm-ppc64/pgtable.h> both define the
following map of protection bits:

    #define __P000  PAGE_NONE
    #define __P001  PAGE_READONLY_X
    #define __P010  PAGE_COPY
    #define __P011  PAGE_COPY_X
    #define __P100  PAGE_READONLY
    #define __P101  PAGE_READONLY_X
    #define __P110  PAGE_COPY
    #define __P111  PAGE_COPY_X

    #define __S000  PAGE_NONE
    #define __S001  PAGE_READONLY_X
    #define __S010  PAGE_SHARED
    #define __S011  PAGE_SHARED_X
    #define __S100  PAGE_READONLY
    #define __S101  PAGE_READONLY_X
    #define __S110  PAGE_SHARED
    #define __S111  PAGE_SHARED_X

The _X flags seem wrongly placed, as bit 2 is the PROT_EXEC bit, not
bit 0.  Is the above intentional?

-- Jamie

Re: PowerPC exec page protection

[Anton Blanchard]

Hi,

> <asm-ppc/pgtable.h> and <asm-ppc64/pgtable.h> both define the
> following map of protection bits:
> 
>     #define __P000  PAGE_NONE
>     #define __P001  PAGE_READONLY_X
>     #define __P010  PAGE_COPY
>     #define __P011  PAGE_COPY_X
>     #define __P100  PAGE_READONLY
>     #define __P101  PAGE_READONLY_X
>     #define __P110  PAGE_COPY
>     #define __P111  PAGE_COPY_X
> 
>     #define __S000  PAGE_NONE
>     #define __S001  PAGE_READONLY_X
>     #define __S010  PAGE_SHARED
>     #define __S011  PAGE_SHARED_X
>     #define __S100  PAGE_READONLY
>     #define __S101  PAGE_READONLY_X
>     #define __S110  PAGE_SHARED
>     #define __S111  PAGE_SHARED_X
> 
> The _X flags seem wrongly placed, as bit 2 is the PROT_EXEC bit, not
> bit 0.  Is the above intentional?

Its backwards and we know it :) Ive got a patch to implement per page
execute on ppc64 and that did pop up.

Thanks for pointing it out, are you looking at ppc* page protection or
just chanced upon it?

Anton

RE: Non-Exec stack patches

[Siddha, Suresh B]

Jamie Lokier wrote:
> Has the meaning of PAGE_COPY in asm-ia64/pgtable.h changed in 
> 2.6.5-mm5?

Yes. Kurt's recent patch for parsing PT_GNU_STACK section introduced
this change.

Here is the relevant hunk.
-#define PAGE_COPY      __pgprot(__ACCESS_BITS | _PAGE_PL_3 |
_PAGE_AR_RX)
+#define PAGE_COPY      __pgprot(__ACCESS_BITS | _PAGE_PL_3 |
_PAGE_AR_R)
+#define PAGE_COPY_EXEC __pgprot(__ACCESS_BITS | _PAGE_PL_3 |
_PAGE_AR_RX)

> 
> If so, you want to change __P110 as well as __P111.

No. Only EXEC bit is the difference.

thanks,
suresh

Re: PowerPC exec page protection

[Jamie Lokier]

Anton Blanchard wrote:
> Thanks for pointing it out, are you looking at ppc* page protection or
> just chanced upon it?

Just chanced.

-- Jamie

Re: Non-Exec stack patches

[Jamie Lokier]

Siddha, Suresh B wrote:
> > If so, you want to change __P110 as well as __P111.
> 
> No. Only EXEC bit is the difference.

Yes.  __P110 means WRITE+EXEC.  __P111 means READ+WRITE+EXEC.

-- Jamie

Re: Non-Exec stack patches

[Jamie Lokier]

People looking at PROT_EXEC page table flags might want to be aware
that <asm-um/pgtable.h> mimics the behaviour of i386: read implies and
is implied by exec, write implies read.

That might mean user-mode linux doesn't provide no-exec-stack
protection even when the underlying kernel does offer it.  I'm not sure.

-- Jamie

[PATCH] (IA64) Fix ugly __[PS]* macros in <asm-ia64/pgtable.h>

[Jamie Lokier]

Patch: ia64-pgtable-2.6.5.patch

That mixture of PAGE_* and __pgprot() definitions for the __[PS]*
macros in asm-ia64/pgtable.h is really ugly and just makes the code
unnecessarily confusing:

	#define __P000  PAGE_NONE
	#define __P001  PAGE_READONLY
	#define __P010  PAGE_READONLY
	#define __P011  PAGE_READONLY
	#define __P100  __pgprot(__ACCESS_BITS | _PAGE_PL_3 | _PAGE_AR_X_RX)
	#define __P101  __pgprot(__ACCESS_BITS | _PAGE_PL_3 | _PAGE_AR_RX)
	#define __P110  PAGE_COPY
	#define __P111  PAGE_COPY

The PAGE_* macros which are used in __[PS]* aren't used anywhere else:
their entire reason for existing is to make the __[PS]* macros
clearer.  It looks as though the people who implemented the IA-64 port
didn't realise that.

Here is a page (untested) which cleans up those definitions.  It was
made from 2.6.5.

Enjoy,
-- Jamie

diff -ur orig-2.6.5/include/asm-ia64/pgtable.h ia64-2.6.5/include/asm-ia64/pgtable.h
--- orig-2.6.5/include/asm-ia64/pgtable.h	2004-04-12 21:45:39.000000000 +0100
+++ ia64-2.6.5/include/asm-ia64/pgtable.h	2004-04-14 12:29:01.000000000 +0100
@@ -116,13 +116,17 @@
  * they are used, the page is accessed. They are cleared only by the
  * page-out routines.
  */
-#define PAGE_NONE	__pgprot(_PAGE_PROTNONE | _PAGE_A)
-#define PAGE_SHARED	__pgprot(__ACCESS_BITS | _PAGE_PL_3 | _PAGE_AR_RW)
-#define PAGE_READONLY	__pgprot(__ACCESS_BITS | _PAGE_PL_3 | _PAGE_AR_R)
-#define PAGE_COPY	__pgprot(__ACCESS_BITS | _PAGE_PL_3 | _PAGE_AR_RX)
-#define PAGE_GATE	__pgprot(__ACCESS_BITS | _PAGE_PL_0 | _PAGE_AR_X_RX)
-#define PAGE_KERNEL	__pgprot(__DIRTY_BITS  | _PAGE_PL_0 | _PAGE_AR_RWX)
-#define PAGE_KERNELRX	__pgprot(__ACCESS_BITS | _PAGE_PL_0 | _PAGE_AR_RX)
+#define PAGE_NONE	   __pgprot(_PAGE_PROTNONE | _PAGE_A)
+#define PAGE_SHARED	   __pgprot(__ACCESS_BITS | _PAGE_PL_3 | _PAGE_AR_RW)
+#define PAGE_SHARED_EXEC   __pgprot(__ACCESS_BITS | _PAGE_PL_3 | _PAGE_AR_RWX)
+#define PAGE_READONLY	   __pgprot(__ACCESS_BITS | _PAGE_PL_3 | _PAGE_AR_R)
+#define PAGE_READONLY_EXEC __pgprot(__ACCESS_BITS | _PAGE_PL_3 | _PAGE_AR_RX)
+#define PAGE_COPY	   __pgprot(__ACCESS_BITS | _PAGE_PL_3 | _PAGE_AR_R)
+#define PAGE_COPY_EXEC	   __pgprot(__ACCESS_BITS | _PAGE_PL_3 | _PAGE_AR_RX)
+#define PAGE_EXEC	   __pgprot(__ACCESS_BITS | _PAGE_PL_3 | _PAGE_AR_X_RX)
+#define PAGE_GATE	   __pgprot(__ACCESS_BITS | _PAGE_PL_0 | _PAGE_AR_X_RX)
+#define PAGE_KERNEL	   __pgprot(__DIRTY_BITS  | _PAGE_PL_0 | _PAGE_AR_RWX)
+#define PAGE_KERNELRX	   __pgprot(__ACCESS_BITS | _PAGE_PL_0 | _PAGE_AR_RX)
 
 # ifndef __ASSEMBLY__
 
@@ -142,21 +146,21 @@
 	/* xwr */
 #define __P000	PAGE_NONE
 #define __P001	PAGE_READONLY
-#define __P010	PAGE_READONLY	/* write to priv pg -> copy & make writable */
-#define __P011	PAGE_READONLY	/* ditto */
-#define __P100	__pgprot(__ACCESS_BITS | _PAGE_PL_3 | _PAGE_AR_X_RX)
-#define __P101	__pgprot(__ACCESS_BITS | _PAGE_PL_3 | _PAGE_AR_RX)
-#define __P110	PAGE_COPY
-#define __P111	PAGE_COPY
+#define __P010	PAGE_COPY
+#define __P011	PAGE_COPY
+#define __P100	PAGE_EXEC
+#define __P101	PAGE_READONLY_EXEC
+#define __P110	PAGE_COPY_EXEC
+#define __P111	PAGE_COPY_EXEC
 
 #define __S000	PAGE_NONE
 #define __S001	PAGE_READONLY
 #define __S010	PAGE_SHARED	/* we don't have (and don't need) write-only */
 #define __S011	PAGE_SHARED
-#define __S100	__pgprot(__ACCESS_BITS | _PAGE_PL_3 | _PAGE_AR_X_RX)
-#define __S101	__pgprot(__ACCESS_BITS | _PAGE_PL_3 | _PAGE_AR_RX)
-#define __S110	__pgprot(__ACCESS_BITS | _PAGE_PL_3 | _PAGE_AR_RWX)
-#define __S111	__pgprot(__ACCESS_BITS | _PAGE_PL_3 | _PAGE_AR_RWX)
+#define __S100	PAGE_EXEC
+#define __S101	PAGE_READONLY_EXEC
+#define __S110	PAGE_SHARED_EXEC
+#define __S111	PAGE_SHARED_EXEC
 
 #define pgd_ERROR(e)	printk("%s:%d: bad pgd %016lx.\n", __FILE__, __LINE__, pgd_val(e))
 #define pmd_ERROR(e)	printk("%s:%d: bad pmd %016lx.\n", __FILE__, __LINE__, pmd_val(e))

Re: [PATCH] (IA64) Fix ugly __[PS]* macros in <asm-ia64/pgtable.h>

[David Mosberger]

 Jamie> Patch: ia64-pgtable-2.6.5.patch
 Jamie> That mixture of PAGE_* and __pgprot() definitions for the __[PS]*
 Jamie> macros in asm-ia64/pgtable.h is really ugly and just makes the code
 Jamie> unnecessarily confusing:

 Jamie> #define __P000  PAGE_NONE
 Jamie> #define __P001  PAGE_READONLY
 Jamie> #define __P010  PAGE_READONLY
 Jamie> #define __P011  PAGE_READONLY
 Jamie> #define __P100  __pgprot(__ACCESS_BITS | _PAGE_PL_3 | _PAGE_AR_X_RX)
 Jamie> #define __P101  __pgprot(__ACCESS_BITS | _PAGE_PL_3 | _PAGE_AR_RX)
 Jamie> #define __P110  PAGE_COPY
 Jamie> #define __P111  PAGE_COPY

 Jamie> The PAGE_* macros which are used in __[PS]* aren't used
 Jamie> anywhere else: their entire reason for existing is to make the
 Jamie> __[PS]* macros clearer.  It looks as though the people who
 Jamie> implemented the IA-64 port didn't realise that.

Huh?  You haven't actually checked, have you?
I don't pollute namespace for no good reason.

 Jamie> Here is a page (untested) which cleans up those definitions.
 Jamie> It was made from 2.6.5.

If the same names are adopted by the other platforms, I'm fine with it.
Otherwise, see my comment above.

	--david

Re: Non-Exec stack patches

[Kurt Garloff]

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

On Wed, Apr 14, 2004 at 10:47:02AM +0100, Jamie Lokier wrote:
> People looking at PROT_EXEC page table flags might want to be aware
> that <asm-um/pgtable.h> mimics the behaviour of i386: read implies and
> is implied by exec, write implies read.
> 
> That might mean user-mode linux doesn't provide no-exec-stack
> protection even when the underlying kernel does offer it.  I'm not sure.

I thought UML only runs on i386.
And on i386, you have no NX feature.
You can run i386 UML on AMD64 (with 64bit kernel) though.

Regards,
-- 
Kurt Garloff  <garloff@suse.de>                            Cologne, DE 
SUSE LINUX AG, Nuernberg, DE                          SUSE Labs (Head)

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: RE: Non-Exec stack patches

[Kurt Garloff]

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

On Wed, Apr 14, 2004 at 12:28:24AM -0700, Suresh B Siddha wrote:
> Recent ia64 mm trees are broken because of this issue. Attached patch 
> fixes protection_map[7] in IA64.

Ah, sorry. ia64 seems to be strangely different here.
My understanding is that it support NX page protection. And that you
don't have executable stacks in the ia64 ABI at all. But for i386
emulation you should be able to enable and disable executable stacks.
For some reason the needed defines are missing ...

Regards,
-- 
Kurt Garloff  <garloff@suse.de>                            Cologne, DE 
SUSE LINUX AG, Nuernberg, DE                          SUSE Labs (Head)

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] (IA64) Fix ugly __[PS]* macros in <asm-ia64/pgtable.h>

[Jamie Lokier]

David Mosberger wrote:
> Huh?  You haven't actually checked, have you?

Yes I have.  Quite thoroughly.

> I don't pollute namespace for no good reason.

In this instance, there is a certain amount of arch variation.  parisc
defines PAGE_EXECREAD and PAGE_RWX.  ppc defines PAGE_COPY_X,
PAGE_SHARED_X, PAGE_READONLY_X (X is for exec).  m68k defines
PAGE_COPY_C, PAGE_READONLY_C, PAGE_SHARED_C.  x86_64 defines
PAGE_COPY_EXEC, PAGE_SHARED_EXEC and PAGE_READONLY_EXEC.

Those are all arch-private names, some of them used in code in arch/*,
some just to define the __[PS]* constants.

>  Jamie> Here is a page (untested) which cleans up those definitions.
>  Jamie> It was made from 2.6.5.
> 
> If the same names are adopted by the other platforms, I'm fine with it.
> Otherwise, see my comment above.

I copied <asm-x86_64/pgtable.h>, which closely matches ia64, except
for PAGE_EXEC which I named because nothing else has it.  That name
isn't used anywhere.  On reflection, a better name for it is
PAGE_EXECONLY (like PAGE_READONLY).

In theory the Alpha can do exec-only pages, but it's __[PS]* map
always gives read permission when there's execute permission.  I'm not
sure if there's a reason for that, or if it just historically copied
the i386 behaviour (Alpha was the first port).

The constants PAGE_KERNEL and PAGE_READONLY are used in
arch-independent code with a common meaning.  Otherwise, the constants
are used only to defined __[PS]* and a few in arch-dependent
initialisation code.

I agree it is best to avoid namespace pollution.  However this is one
area where ia64 sticks out because it's approach is different from
other ports.  All of them except Alpha used PAGE_* names to clarify
the __[PS]* map, defining additional names as needed.

The Alpha is quite clean in a different way, or looks like it until
you realise the _PAGE_P() macro is equivalent to identity so just
obfuscates the code.  (The _PAGE_P() macro which is absurd because
it's a complicated expression that's equivalent to identity).

The thing I don't like about the ia64 file is the mixing of two
different styles of definition in the same table.  When I had to read
all the arch pgtable.h files to discover what is Linux's mmap()
behaviour on each one, ia64 stood out awkwardly.

-- Jamie

Re: [PATCH] (IA64) Fix ugly __[PS]* macros in <asm-ia64/pgtable.h>

[David Mosberger]

>>>>> On Wed, 14 Apr 2004 19:46:03 +0100, Jamie Lokier <jamie@shareable.org> said:

  Jamie> David Mosberger wrote:
  >> Huh?  You haven't actually checked, have you?

  Jamie> Yes I have.  Quite thoroughly.

Then you should have noticed that drivers/char/mem.c is using PAGE_COPY.
Various architecture-dependent code is also using PAGE_foo macros.

  Jamie> In theory the Alpha can do exec-only pages, but it's __[PS]*
  Jamie> map always gives read permission when there's execute
  Jamie> permission.  I'm not sure if there's a reason for that, or if
  Jamie> it just historically copied the i386 behaviour (Alpha was the
  Jamie> first port).

I know why: back in those days, GCC emitted code for nested C
functions that assumed an executable stack.  Also, Linus wasn't
terribly eager to turn off execute-permission on data/stacks.  Even on
ia64 we started out that way, until I saw the error in my ways.

  Jamie> I agree it is best to avoid namespace pollution.  However
  Jamie> this is one area where ia64 sticks out because it's approach
  Jamie> is different from other ports.  All of them except Alpha used
  Jamie> PAGE_* names to clarify the __[PS]* map, defining additional
  Jamie> names as needed.

The reality is that whenever you introduce a globally visible name
that is not used on x86, there is a very definite risk that someone
will use that same name and cause a conflict.  We have had that happen
several times and that's the reason I'm normally religious about
prefixing all ia64-specific names with a "ia64_" or "IA64_" (yes, this
makes code a bit uglier, but you can't have it both ways).  Your
argument that the Alpha and other ports are doing something different
doesn't buy me anything.  If the ia64 break builds, the Alpha
maintainer won't fix it up for me, after all.

	--david

Re: [PATCH] (IA64) Fix ugly __[PS]* macros in <asm-ia64/pgtable.h>

[Jamie Lokier]

David Mosberger wrote:
> If the ia64 break builds, the Alpha maintainer won't fix it up for
> me, after all.

Ok.  In this case PAGE_COPY_EXEC, PAGE_SHARED_EXEC and
PAGE_READONLY_EXEC are in x86_64, so those names are fairly safe.

You could write the other one as IA64_PAGE_EXECONLY to be very safe.

-- Jamie

RE: Non-Exec stack patches

[Siddha, Suresh B]

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

Jamie Lokier wrote:
> Siddha, Suresh B wrote:
> > > If so, you want to change __P110 as well as __P111.
> > 
> > No. Only EXEC bit is the difference.
> 
> Yes.  __P110 means WRITE+EXEC.  __P111 means READ+WRITE+EXEC.
> 

Thanks Jamie. Attached the updated patch. Andrew please apply.

thanks,
suresh


[-- Attachment #2: noexec-ia64.fix --]
[-- Type: application/octet-stream, Size: 531 bytes --]

--- linux-265mm5/include/asm-ia64/pgtable.h~	2004-04-14 00:09:04.000000000 -0700
+++ linux-265mm5/include/asm-ia64/pgtable.h	2004-04-14 11:21:09.000000000 -0700
@@ -147,8 +147,8 @@
 #define __P011	PAGE_READONLY	/* ditto */
 #define __P100	__pgprot(__ACCESS_BITS | _PAGE_PL_3 | _PAGE_AR_X_RX)
 #define __P101	__pgprot(__ACCESS_BITS | _PAGE_PL_3 | _PAGE_AR_RX)
-#define __P110	PAGE_COPY
-#define __P111	PAGE_COPY
+#define __P110	PAGE_COPY_EXEC
+#define __P111	PAGE_COPY_EXEC
 
 #define __S000	PAGE_NONE
 #define __S001	PAGE_READONLY

Re: [PATCH] (IA64) Fix ugly __[PS]* macros in <asm-ia64/pgtable.h>

[Jamie Lokier]

David Mosberger wrote:
>   Jamie> Yes I have.  Quite thoroughly.
> 
> Then you should have noticed that drivers/char/mem.c is using PAGE_COPY.

That's an interesting bug in drivers/char/mem.c.  PAGE_COPY is wrong,
for archs with separate execute permission or write-only mappings.

The correct argument in drivers/char/mem.c should be vma->vm_page_prot.

>   Jamie> In theory the Alpha can do exec-only pages, but it's __[PS]*
>   Jamie> map always gives read permission when there's execute
>   Jamie> permission.  I'm not sure if there's a reason for that, or if
>   Jamie> it just historically copied the i386 behaviour (Alpha was the
>   Jamie> first port).
> 
> I know why: back in those days, GCC emitted code for nested C
> functions that assumed an executable stack.  Also, Linus wasn't
> terribly eager to turn off execute-permission on data/stacks.  Even on
> ia64 we started out that way, until I saw the error in my ways.

We're both wrong.  I misread the Alpha code: it does have exec-only
pages.  What it doesn't have is write-only.  And exec-only pages
aren't relevant to GCC's requirements, which used to be
read-implies-exec (exec-only breaks exec-implies-read).

-- Jamie

Re: [PATCH] (IA64) Fix ugly __[PS]* macros in <asm-ia64/pgtable.h>

[David Mosberger]

>>>>> On Wed, 14 Apr 2004 20:28:44 +0100, Jamie Lokier <jamie@shareable.org> said:

  >>  I know why: back in those days, GCC emitted code for nested C
  >> functions that assumed an executable stack.  Also, Linus wasn't
  >> terribly eager to turn off execute-permission on data/stacks.
  >> Even on ia64 we started out that way, until I saw the error in my
  >> ways.

  Jamie> We're both wrong.

No, Alpha Linux didn't map data without execute permission.

  Jamie> What it doesn't have is write-only.

Yes, that one is because earlier Alphas couldn't do subword stores, so
WRITE-permission necessitated READ-permission.

	--david

Re: Non-Exec stack patches

[Jeff Dike]

garloff@suse.de said:
> I thought UML only runs on i386. And on i386, you have no NX feature.
> You can run i386 UML on AMD64 (with 64bit kernel) though. 

It also runs natively on and64 (i.e. a 64 bit UML).  The UML page table stuff
is going to need to be fixed for NX, but there's no reason that it won't work.

				Jeff

Re: [PATCH] (IA64) Fix ugly __[PS]* macros in <asm-ia64/pgtable.h>

[Jamie Lokier]

David Mosberger wrote:
> No, Alpha Linux didn't map data without execute permission.

That was true from Linux 1.1.67 (when Alpha was introduced) to 1.1.84
(when __[PS]* was introduced).  I'm not sure the Alpha target even
worked during those versions.  Since Linux 1.1.84, it has mapped pages
on the Alpha without execute permission: the _PAGE_FOE (fault on exec)
bit is set for mappings which don't have PROT_EXEC.

Btw, they used PAGE_EXECONLY in those days :)

-- Jamie

Re: [PATCH] (IA64) Fix ugly __[PS]* macros in <asm-ia64/pgtable.h>

[David Mosberger]

>>>>> On Wed, 14 Apr 2004 22:05:38 +0100, Jamie Lokier <jamie@shareable.org> said:

  Jamie> David Mosberger wrote:

  >> No, Alpha Linux didn't map data without execute permission.

  Jamie> That was true from Linux 1.1.67 (when Alpha was introduced)
  Jamie> to 1.1.84 (when __[PS]* was introduced).  I'm not sure the
  Jamie> Alpha target even worked during those versions.  Since Linux
  Jamie> 1.1.84, it has mapped pages on the Alpha without execute
  Jamie> permission: the _PAGE_FOE (fault on exec) bit is set for
  Jamie> mappings which don't have PROT_EXEC.

$ uname -a
Linux hostname 2.2.20 #2 Wed Mar 20 19:57:28 EST 2002 alpha GNU/Linux
$ cat /proc/self/maps | grep cat
0000000120000000-0000000120006000 r-xp 0000000000000000 08:01 309740     /bin/cat
0000000120014000-0000000120016000 rwxp 0000000000004000 08:01 309740     /bin/cat

As you can see, the data-segment is mapped with EXEC bit turned on.
Ditto for the stack segment, which I think is this one:

000000011fffe000-0000000120000000 rwxp 0000000000000000 00:00 0

	--david

Re: [PATCH] (IA64) Fix ugly __[PS]* macros in <asm-ia64/pgtable.h>

[Jamie Lokier]

David Mosberger wrote:
>   >> No, Alpha Linux didn't map data without execute permission.
> 
>   Jamie> That was true from Linux 1.1.67 (when Alpha was introduced)
>   Jamie> to 1.1.84 (when __[PS]* was introduced).  I'm not sure the
>   Jamie> Alpha target even worked during those versions.  Since Linux
>   Jamie> 1.1.84, it has mapped pages on the Alpha without execute
>   Jamie> permission: the _PAGE_FOE (fault on exec) bit is set for
>   Jamie> mappings which don't have PROT_EXEC.
> 
> $ uname -a
> Linux hostname 2.2.20 #2 Wed Mar 20 19:57:28 EST 2002 alpha GNU/Linux
> $ cat /proc/self/maps | grep cat
> 0000000120000000-0000000120006000 r-xp 0000000000000000 08:01 309740     /bin/cat
> 0000000120014000-0000000120016000 rwxp 0000000000004000 08:01 309740     /bin/cat
> 
> As you can see, the data-segment is mapped with EXEC bit turned on.
> Ditto for the stack segment, which I think is this one:
> 
> 000000011fffe000-0000000120000000 rwxp 0000000000000000 00:00 0

Lest we get more wires crossed, the "x" in /proc/self/maps indicates
that userspace _requested_ executable mapping with PROT_EXEC.

It is independent of whether the kernel and hardware can grant a
non-executable mapping.  On my Athlon:

08048000-0804c000 r-xp 00000000 03:02 95153      /bin/cat
0804c000-0804d000 rw-p 00003000 03:02 95153      /bin/cat
bfffe000-c0000000 rwxp fffff000 00:00 0

The stack is mapped executable, and the data segment is mapped
non-executable, according to /proc/self/maps which reflects the
PROT_EXEC request.  But in fact because of hardware limitations,
despite the "rw-p" my data segment is actually executable.

If you map a segment without PROT_EXEC on Alpha, using a kernel newer
than 1.1.84, you'll get a non-executable mapping.  That's what I mean
when I say the Alpha maps data without executable permission.  The ELF
loader appears to ask for exec permission anyway, which is another
thing entirely.

-- Jamie

Re: [PATCH] (IA64) Fix ugly __[PS]* macros in <asm-ia64/pgtable.h>

[David Mosberger]

>>>>> On Thu, 15 Apr 2004 16:26:00 +0100, Jamie Lokier <jamie@shareable.org> said:

  >> As you can see, the data-segment is mapped with EXEC bit turned on.
  >> Ditto for the stack segment, which I think is this one:

  >> 000000011fffe000-0000000120000000 rwxp 0000000000000000 00:00 0

  Jamie> The stack is mapped executable, and the data segment is mapped
  Jamie> non-executable, according to /proc/self/maps which reflects the
  Jamie> PROT_EXEC request.  But in fact because of hardware limitations,
  Jamie> despite the "rw-p" my data segment is actually executable.

Yes, but Alpha doesn't have this limitation.

  Jamie> If you map a segment without PROT_EXEC on Alpha, using a kernel newer
  Jamie> than 1.1.84, you'll get a non-executable mapping.

Sure.

  Jamie> That's what I mean when I say the Alpha maps data without
  Jamie> executable permission.  The ELF loader appears to ask for
  Jamie> exec permission anyway, which is another thing entirely.

It's not the ELF loader, it's the kernel.  See VM_DATA_DEFAULT_FLAGS
in asm-alpha/page.h.

	--david