public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [PROPOSAL/PATCH] Remove PT_GNU_STACK support before 2.6.11
@ 2005-02-06 11:36 Andi Kleen
  2005-02-06 11:47 ` Arjan van de Ven
                   ` (2 more replies)
  0 siblings, 3 replies; 36+ messages in thread
From: Andi Kleen @ 2005-02-06 11:36 UTC (permalink / raw)
  To: akpm, torvalds; +Cc: mingo, linux-kernel


Hallo,

I'm getting more and more reports that the PT_GNU_STACK change breaks
a lot of programs. In particular it breaks grub which implies that nobody
could have really tested it. Mono seems to break too, there are undoubtedly
others. There are some reports of Wine breaking too, I suspect
they are related to this too. The testing for this was obviously not very 
effective.

PT_GNU_STACK assumes that any program with a PT_GNU_STACK header
always pass correct PROT_EXEC flags to mmap/mprotect etc. before
allocating mappings. 

The problem is that people are recompiling these programs with
new compilers, the new compilers set PT_GNU_STACK and then they
break. Sometimes obviously (like grub), I bet in some other cases
they break only in obscure cases that are hard to catch.

Worse is that even when the program has trampolines and has PT_GNU_STACK
header with an E bit on the stack it still won't get an executable
heap by default  (this is what broke grub) 

This is a major source of source level and binary incompatibility. 

People reporting this to me all the time for x86-64 because few people
run PAE/NX enabled kernels on i386, and without PAE this breakage
is not visible. Also you need a NX capable CPU of course,
and basically all NX capable CPUs are able to run x86-64, so a 
lot of them run 64bit kernels. That is why the full impact of these
changes was not felt on i386.

I'm not really willing to handle all these problems on the x86-64 side, 
for what was ultimatively a half baked change comming from i386. 

My proposal is to turn this all off at least for 2.6.11. 

If it should be reenabled later probably needs a lot of discussion.
IMHO it's not a very good idea in 2.6 because it's an extremly invasive change
in the ABI and the security advantages of using NX are not that great anyways. 

I understand that some people want to have it as part of a marketing
campaign as "answer to Microsoft SP2", but IMHO source & binary compatibility
is too important to be sacrified for such things. If they really
want it they can continue to maintain it as private patches.

I also don't remember a thread on linux-kernel discussion these
trade offs and what significant impact it has on user space.
Before reenabling it I think we would need a lot of discussion
if it's really worth it, and a lot more education of user land
people on what they need to do to cope with this.

One way would be to have special command line flags or sysctls like
x86-64 used to have for this (but they were removed as "too confusing"),
and then only people who know what they are doing or can tolerate
some of their programs breaking can enable it.

But forcing it on all users doesn't seem to work very well.

Here's a patch to disable PT_GNU_STACK parsing for now. I would 
prefer it to be merged before 2.6.11.

Thanks,
-Andi

Remove PT_GNU_STACK support because user land is not ready for it yet.

Signed-off-by: Andi Kleen <ak@suse.de>

diff -u linux-2.6.11rc3/fs/binfmt_elf.c-o linux-2.6.11rc3/fs/binfmt_elf.c
--- linux-2.6.11rc3/fs/binfmt_elf.c-o	2005-02-04 09:13:18.000000000 +0100
+++ linux-2.6.11rc3/fs/binfmt_elf.c	2005-02-06 12:30:59.000000000 +0100
@@ -675,6 +675,13 @@
 		elf_ppnt++;
 	}
 
+#if 1
+	/* 
+	 * The user land is not ready for PT_GNU_STACK yet. Disable
+	 * it for now.
+	 */
+	have_pt_gnu_stack = 0;
+#else
 	elf_ppnt = elf_phdata;
 	for (i = 0; i < loc->elf_ex.e_phnum; i++, elf_ppnt++)
 		if (elf_ppnt->p_type == PT_GNU_STACK) {
@@ -685,6 +692,7 @@
 			break;
 		}
 	have_pt_gnu_stack = (i < loc->elf_ex.e_phnum);
+#endif
 
 	/* Some simple consistency checks for the interpreter */
 	if (elf_interpreter) {



^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PROPOSAL/PATCH] Remove PT_GNU_STACK support before 2.6.11
  2005-02-06 11:36 [PROPOSAL/PATCH] Remove PT_GNU_STACK support before 2.6.11 Andi Kleen
@ 2005-02-06 11:47 ` Arjan van de Ven
  2005-02-06 12:02   ` Ingo Molnar
  2005-02-06 12:33   ` Andi Kleen
  2005-02-06 12:11 ` Paweł Sikora
       [not found] ` <200502061303.12377.pluto@pld-linux.org>
  2 siblings, 2 replies; 36+ messages in thread
From: Arjan van de Ven @ 2005-02-06 11:47 UTC (permalink / raw)
  To: Andi Kleen; +Cc: akpm, torvalds, mingo, linux-kernel, drepper

On Sun, Feb 06, 2005 at 12:36:35PM +0100, Andi Kleen wrote:

> PT_GNU_STACK assumes that any program with a PT_GNU_STACK header
> always pass correct PROT_EXEC flags to mmap/mprotect etc. before
> allocating mappings. 

that is incorrect and was introduced later


> Worse is that even when the program has trampolines and has PT_GNU_STACK
> header with an E bit on the stack it still won't get an executable
> heap by default  (this is what broke grub) 

this I can fix easy, see the patch below

the problem is in the read_implies_exec() design, it passed in "does it have
a PT_GNU_STACK flag" not the value. Easy fix.


Your main objection is that *incorrect* programs that assume they can
execute malloc() code without PROT_EXEC protection. For legacy binaries
keeping this behavior makes sense, no objection from me.

For newly compiled programs this is just wrong and incorrect.

You mention grub (which has RWE and the patch below thus makes that work)
and mono. mono has patches for this on their mailinglist and bugzilla since
2003 according to google, I'm surprised the novell mono guys haven't fixed
this bug in their code.

FWIW all jvm's don't suffer from this. They are either legacy binaries or
mprotect properly (only i386 traditionally had this behavior, all others
already required PROT_EXEC anyway so any half portable app already did this,
as well as any app portable to BSD since they enforce this on x86 as well)

So I rather see the patch below merged instead; it fixes the worst problems
(RWE not marking the heap executable) while keeping this useful feature
enabled.


Signed-off-by: Arjan van de Ven <arjan@infradead.org>


diff -purN linux-2.6.11-rc2-bk4/fs/binfmt_elf.c linux-foo/fs/binfmt_elf.c
--- linux-2.6.11-rc2-bk4/fs/binfmt_elf.c	2005-01-26 18:24:50.000000000 +0100
+++ linux-foo/fs/binfmt_elf.c	2005-02-06 12:29:02.000000000 +0100
@@ -757,7 +757,7 @@ static int load_elf_binary(struct linux_
 	/* Do this immediately, since STACK_TOP as used in setup_arg_pages
 	   may depend on the personality.  */
 	SET_PERSONALITY(loc->elf_ex, ibcs2_interpreter);
-	if (elf_read_implies_exec(loc->elf_ex, have_pt_gnu_stack))
+	if (elf_read_implies_exec(loc->elf_ex, executable_stack))
 		current->personality |= READ_IMPLIES_EXEC;
 
 	arch_pick_mmap_layout(current->mm);
diff -purN linux-2.6.11-rc2-bk4/include/asm-i386/elf.h linux-foo/include/asm-i386/elf.h
--- linux-2.6.11-rc2-bk4/include/asm-i386/elf.h	2004-12-24 22:35:15.000000000 +0100
+++ linux-foo/include/asm-i386/elf.h	2005-02-06 12:29:55.000000000 +0100
@@ -123,7 +123,7 @@ typedef struct user_fxsr_struct elf_fpxr
  * An executable for which elf_read_implies_exec() returns TRUE will
  * have the READ_IMPLIES_EXEC personality flag set automatically.
  */
-#define elf_read_implies_exec(ex, have_pt_gnu_stack)	(!(have_pt_gnu_stack))
+#define elf_read_implies_exec(ex, executable_stack)	(executable_stack != EXSTACK_DISABLE_X)
 
 extern int dump_task_regs (struct task_struct *, elf_gregset_t *);
 extern int dump_task_fpu (struct task_struct *, elf_fpregset_t *);
diff -purN linux-2.6.11-rc2-bk4/include/asm-ia64/elf.h linux-foo/include/asm-ia64/elf.h
--- linux-2.6.11-rc2-bk4/include/asm-ia64/elf.h	2004-12-24 22:35:18.000000000 +0100
+++ linux-foo/include/asm-ia64/elf.h	2005-02-06 12:32:47.000000000 +0100
@@ -186,8 +186,8 @@ extern void ia64_elf_core_copy_regs (str
 
 #ifdef __KERNEL__
 #define SET_PERSONALITY(ex, ibcs2)	set_personality(PER_LINUX)
-#define elf_read_implies_exec(ex, have_pt_gnu_stack)					\
-	(!(have_pt_gnu_stack) && ((ex).e_flags & EF_IA_64_LINUX_EXECUTABLE_STACK) != 0)
+#define elf_read_implies_exec(ex, executable_stack)					\
+	((executable_stack!=EXSTACK_DISABLE_X) && ((ex).e_flags & EF_IA_64_LINUX_EXECUTABLE_STACK) != 0)
 
 struct task_struct;
 
diff -purN linux-2.6.11-rc2-bk4/include/asm-x86_64/elf.h linux-foo/include/asm-x86_64/elf.h
--- linux-2.6.11-rc2-bk4/include/asm-x86_64/elf.h	2004-12-24 22:35:24.000000000 +0100
+++ linux-foo/include/asm-x86_64/elf.h	2005-02-06 12:31:39.000000000 +0100
@@ -147,14 +147,7 @@ extern void set_personality_64bit(void);
  * An executable for which elf_read_implies_exec() returns TRUE will
  * have the READ_IMPLIES_EXEC personality flag set automatically.
  */
-#define elf_read_implies_exec(ex, have_pt_gnu_stack)	(!(have_pt_gnu_stack))
-	
-/*
- * An executable for which elf_read_implies_exec() returns TRUE will
- * have the READ_IMPLIES_EXEC personality flag set automatically.
- */
-#define elf_read_implies_exec_binary(ex, have_pt_gnu_stack)   \
-	 (!(have_pt_gnu_stack))
+#define elf_read_implies_exec(ex, executable_stack)	(executable_stack != EXSTACK_DISABLE_X)
 
 extern int dump_task_regs (struct task_struct *, elf_gregset_t *);
 extern int dump_task_fpu (struct task_struct *, elf_fpregset_t *);

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PROPOSAL/PATCH] Remove PT_GNU_STACK support before 2.6.11
  2005-02-06 11:47 ` Arjan van de Ven
@ 2005-02-06 12:02   ` Ingo Molnar
  2005-02-06 12:25     ` Ingo Molnar
                       ` (2 more replies)
  2005-02-06 12:33   ` Andi Kleen
  1 sibling, 3 replies; 36+ messages in thread
From: Ingo Molnar @ 2005-02-06 12:02 UTC (permalink / raw)
  To: Arjan van de Ven; +Cc: Andi Kleen, akpm, torvalds, linux-kernel, drepper


* Arjan van de Ven <arjan@infradead.org> wrote:

> > [...] when the program has trampolines and has PT_GNU_STACK
> > header with an E bit on the stack it still won't get an executable
> > heap by default  (this is what broke grub)
> 
> this I can fix easy, see the patch below
> 
> the problem is in the read_implies_exec() design, it passed in "does
> it have a PT_GNU_STACK flag" not the value. Easy fix.

> So I rather see the patch below merged instead; it fixes the worst
> problems (RWE not marking the heap executable) while keeping this
> useful feature enabled.
> 
> Signed-off-by: Arjan van de Ven <arjan@infradead.org>

looks good.

 Signed-off-by: Ingo Molnar <mingo@elte.hu>

(I'd like to stress that this problem only affects packages _recompiled_
with new gcc, running on NX capable CPUs - legacy apps or CPUs are in no
way affected. Also, even with a recompile, apps/kernels/distros have a
number of other options as well even without this kernel fix, of varying
granularity: to use the setarch utility, to set the READ_IMPLIES_EXEC
personality bit within the code, or to pass in the noexec=off kernel
commandline option, or to add a oneliner patch to their heap of 1500+
kernel patches, or to fix the application. Also, with Arjan's patch
applied, the execstack utility can be used to remark the binary
permanently, if needed.)

	Ingo

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PROPOSAL/PATCH] Remove PT_GNU_STACK support before 2.6.11
  2005-02-06 11:36 [PROPOSAL/PATCH] Remove PT_GNU_STACK support before 2.6.11 Andi Kleen
  2005-02-06 11:47 ` Arjan van de Ven
@ 2005-02-06 12:11 ` Paweł Sikora
       [not found] ` <200502061303.12377.pluto@pld-linux.org>
  2 siblings, 0 replies; 36+ messages in thread
From: Paweł Sikora @ 2005-02-06 12:11 UTC (permalink / raw)
  To: akpm, torvalds, mingo, linux-kernel

My proposal is to recompile broken software with cflags += -Wa,--noexecstack.
This works fine with e.g. 2.6.10+pax/i386.

-- 
/* Copyright (C) 2003, SCO, Inc. This is valuable Intellectual Property. */

                           #define say(x) lie(x)

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PROPOSAL/PATCH] Remove PT_GNU_STACK support before 2.6.11
  2005-02-06 12:02   ` Ingo Molnar
@ 2005-02-06 12:25     ` Ingo Molnar
  2005-02-06 12:36     ` Andi Kleen
  2005-02-06 12:45     ` Ingo Molnar
  2 siblings, 0 replies; 36+ messages in thread
From: Ingo Molnar @ 2005-02-06 12:25 UTC (permalink / raw)
  To: Arjan van de Ven; +Cc: Andi Kleen, akpm, torvalds, linux-kernel, drepper


* Ingo Molnar <mingo@elte.hu> wrote:

> > > [...] when the program has trampolines and has PT_GNU_STACK
> > > header with an E bit on the stack it still won't get an executable
> > > heap by default  (this is what broke grub)

> > So I rather see the patch below merged instead; it fixes the worst
> > problems (RWE not marking the heap executable) while keeping this
> > useful feature enabled.
> > 
> > Signed-off-by: Arjan van de Ven <arjan@infradead.org>
> 
> looks good.
> 
>  Signed-off-by: Ingo Molnar <mingo@elte.hu>
> 
> (I'd like to stress that this problem only affects packages
> _recompiled_ with new gcc, running on NX capable CPUs - legacy apps or
> CPUs are in no way affected. Also, even with a recompile,
> apps/kernels/distros have a number of other options as well even
> without this kernel fix, of varying granularity: to use the setarch
> utility, to set the READ_IMPLIES_EXEC personality bit within the code,
> or to pass in the noexec=off kernel commandline option, or to add a
> oneliner patch to their heap of 1500+ kernel patches, or to fix the
> application. Also, with Arjan's patch applied, the execstack utility
> can be used to remark the binary permanently, if needed.)

another, purely userspace solution is to add an execstack.c flag that
clears the PT_GNU_STACK ELF program header and changes it to e.g.
PT_NULL. That makes it a 'legacy' binary for the purposes of the kernel.

	Ingo

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PROPOSAL/PATCH] Remove PT_GNU_STACK support before 2.6.11
  2005-02-06 11:47 ` Arjan van de Ven
  2005-02-06 12:02   ` Ingo Molnar
@ 2005-02-06 12:33   ` Andi Kleen
  2005-02-06 12:40     ` Arjan van de Ven
  2005-02-06 17:05     ` Linus Torvalds
  1 sibling, 2 replies; 36+ messages in thread
From: Andi Kleen @ 2005-02-06 12:33 UTC (permalink / raw)
  To: Arjan van de Ven; +Cc: Andi Kleen, akpm, torvalds, mingo, linux-kernel, drepper

> Your main objection is that *incorrect* programs that assume they can
> execute malloc() code without PROT_EXEC protection. For legacy binaries
> keeping this behavior makes sense, no objection from me.
> 
> For newly compiled programs this is just wrong and incorrect.

That's not true as the grub/mono/... experience shows. 


> You mention grub (which has RWE and the patch below thus makes that work)
> and mono. mono has patches for this on their mailinglist and bugzilla since
> 2003 according to google, I'm surprised the novell mono guys haven't fixed
> this bug in their code.

There are probably more.

> FWIW all jvm's don't suffer from this. They are either legacy binaries or
> mprotect properly (only i386 traditionally had this behavior, all others
> already required PROT_EXEC anyway so any half portable app already did this,
> as well as any app portable to BSD since they enforce this on x86 as well)
> 
> So I rather see the patch below merged instead; it fixes the worst problems
> (RWE not marking the heap executable) while keeping this useful feature
> enabled.

It still keeps x86-64 32bit emulation as a proving point for experimental
i386 features which is really not its purpose.

-Andi

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PROPOSAL/PATCH] Remove PT_GNU_STACK support before 2.6.11
  2005-02-06 12:02   ` Ingo Molnar
  2005-02-06 12:25     ` Ingo Molnar
@ 2005-02-06 12:36     ` Andi Kleen
  2005-02-06 12:45     ` Ingo Molnar
  2 siblings, 0 replies; 36+ messages in thread
From: Andi Kleen @ 2005-02-06 12:36 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Arjan van de Ven, Andi Kleen, akpm, torvalds, linux-kernel,
	drepper

>  Signed-off-by: Ingo Molnar <mingo@elte.hu>
> 
> (I'd like to stress that this problem only affects packages _recompiled_
> with new gcc, running on NX capable CPUs - legacy apps or CPUs are in no

Yeah, but who did the auditing of all user land packages if they
don't need changes? And who told user land developers that they need
to do this all if they only want to recompile software.

That hasn't been done as far as I can figure out, and I'm not really
willing to serve as the frontend/coordinator for this process on the x86-64 
side.

I would request that this stuff is only turned on by default
until there is a reasonable process found for this.

> way affected. Also, even with a recompile, apps/kernels/distros have a
> number of other options as well even without this kernel fix, of varying
> granularity: to use the setarch utility, to set the READ_IMPLIES_EXEC
> personality bit within the code, or to pass in the noexec=off kernel
> commandline option, or to add a oneliner patch to their heap of 1500+
> kernel patches, or to fix the application. Also, with Arjan's patch
> applied, the execstack utility can be used to remark the binary
> permanently, if needed.)

That would still leave the mainline users who just update their
kernels from a kernel.org tarball out in the rain.

And guess who they will complain to if their favourite program doesn't
work anymore in x86-64 32bit emulation?

-Andi

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PROPOSAL/PATCH] Remove PT_GNU_STACK support before 2.6.11
  2005-02-06 12:33   ` Andi Kleen
@ 2005-02-06 12:40     ` Arjan van de Ven
  2005-02-06 12:48       ` Andi Kleen
  2005-02-06 17:05     ` Linus Torvalds
  1 sibling, 1 reply; 36+ messages in thread
From: Arjan van de Ven @ 2005-02-06 12:40 UTC (permalink / raw)
  To: Andi Kleen; +Cc: akpm, torvalds, mingo, linux-kernel, drepper

On Sun, 2005-02-06 at 13:33 +0100, Andi Kleen wrote:
> > Your main objection is that *incorrect* programs that assume they can
> > execute malloc() code without PROT_EXEC protection. For legacy binaries
> > keeping this behavior makes sense, no objection from me.
> > 
> > For newly compiled programs this is just wrong and incorrect.
> 
> That's not true as the grub/mono/... experience shows. 

both those apps are buggy and incorrect though and should be fixed.

> > You mention grub (which has RWE and the patch below thus makes that work)
> > and mono. mono has patches for this on their mailinglist and bugzilla since
> > 2003 according to google, I'm surprised the novell mono guys haven't fixed
> > this bug in their code.
> 
> There are probably more.

I consider that unlikely. Anything remotely portable (either to other
architectures which required since in linux since day one, or to OpenBSD
or similar which require this even for x86) already is correct. And this
is there since 2.6.6.. it's not like this is new

(and for the non-RWE case, fedora has had this behavior all along with
execshield, and that even lead to a patch to fix mono fwiw)



^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PROPOSAL/PATCH] Remove PT_GNU_STACK support before 2.6.11
  2005-02-06 12:02   ` Ingo Molnar
  2005-02-06 12:25     ` Ingo Molnar
  2005-02-06 12:36     ` Andi Kleen
@ 2005-02-06 12:45     ` Ingo Molnar
  2005-02-06 12:50       ` Andi Kleen
  2 siblings, 1 reply; 36+ messages in thread
From: Ingo Molnar @ 2005-02-06 12:45 UTC (permalink / raw)
  To: Arjan van de Ven; +Cc: Andi Kleen, akpm, torvalds, linux-kernel, drepper


* Ingo Molnar <mingo@elte.hu> wrote:

> * Arjan van de Ven <arjan@infradead.org> wrote:
> 
> > So I rather see the patch below merged instead; it fixes the worst
> > problems (RWE not marking the heap executable) while keeping this
> > useful feature enabled.
> > 
> > Signed-off-by: Arjan van de Ven <arjan@infradead.org>
> 
> looks good.
> 
>  Signed-off-by: Ingo Molnar <mingo@elte.hu>

tested it against BK-curr, on an NX-enabled x86 CPU, and it builds/boots
fine and works as expected. A PT_GNU_STACK RWE binary gets this layout:

 saturn:~/noexec> cat /proc/2983/maps
 00888000-0089d000 r-xp 00000000 03:41 3433999    /lib/ld-2.3.3.so
 0089d000-0089f000 rwxp 00014000 03:41 3433999    /lib/ld-2.3.3.so
 008a1000-009bf000 r-xp 00000000 03:41 3434007    /lib/tls/libc-2.3.3.so
 009bf000-009c1000 r-xp 0011d000 03:41 3434007    /lib/tls/libc-2.3.3.so
 009c1000-009c3000 rwxp 0011f000 03:41 3434007    /lib/tls/libc-2.3.3.so
 009c3000-009c5000 rwxp 009c3000 00:00 0
 08048000-08049000 r-xp 00000000 03:41 1046974    /home/mingo/noexec/test-stack
 08049000-0804a000 rwxp 00000000 03:41 1046974    /home/mingo/noexec/test-stack
 b7fe7000-b7fe8000 rwxp b7fe7000 00:00 0
 bffeb000-c0000000 rwxp bffeb000 00:00 0
 ffffe000-fffff000 ---p 00000000 00:00 0

i.e. all mappings are executable (i.e. READ_IMPLIES_EXEC effect) - the
intended change. (although i dont fully agree with PT_GNU_STACK being
about something else than the stack, from a security POV if the stack is
executable then all bets are off anyway. The heap and all mmaps being
executable too in that case makes little difference.)

	Ingo

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PROPOSAL/PATCH] Remove PT_GNU_STACK support before 2.6.11
  2005-02-06 12:40     ` Arjan van de Ven
@ 2005-02-06 12:48       ` Andi Kleen
  2005-02-06 15:54         ` Andreas Schwab
  0 siblings, 1 reply; 36+ messages in thread
From: Andi Kleen @ 2005-02-06 12:48 UTC (permalink / raw)
  To: Arjan van de Ven; +Cc: Andi Kleen, akpm, torvalds, mingo, linux-kernel, drepper

On Sun, Feb 06, 2005 at 01:40:22PM +0100, Arjan van de Ven wrote:
> On Sun, 2005-02-06 at 13:33 +0100, Andi Kleen wrote:
> > > Your main objection is that *incorrect* programs that assume they can
> > > execute malloc() code without PROT_EXEC protection. For legacy binaries
> > > keeping this behavior makes sense, no objection from me.
> > > 
> > > For newly compiled programs this is just wrong and incorrect.
> > 
> > That's not true as the grub/mono/... experience shows. 
> 
> both those apps are buggy and incorrect though and should be fixed.

They worked fine forever - and suddenly you define them as buggy.
This might be, but it's still quite bad to change the definition
of what is buggy and what is not so suddenly in a "mostly stable"
release series. And who tells the users what is considered buggy
this week?

-Andi

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PROPOSAL/PATCH] Remove PT_GNU_STACK support before 2.6.11
  2005-02-06 12:45     ` Ingo Molnar
@ 2005-02-06 12:50       ` Andi Kleen
  2005-02-06 12:59         ` Arjan van de Ven
  0 siblings, 1 reply; 36+ messages in thread
From: Andi Kleen @ 2005-02-06 12:50 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Arjan van de Ven, Andi Kleen, akpm, torvalds, linux-kernel,
	drepper

> i.e. all mappings are executable (i.e. READ_IMPLIES_EXEC effect) - the
> intended change. (although i dont fully agree with PT_GNU_STACK being
> about something else than the stack, from a security POV if the stack is
> executable then all bets are off anyway. The heap and all mmaps being
> executable too in that case makes little difference.)

Well, that won't fix mono (and i suspect wine) and the others
who don't use trampolines that the compiler can detect.

And breaking programs silently definitely doesn't make them secure!

-Andi

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PROPOSAL/PATCH] Remove PT_GNU_STACK support before 2.6.11
  2005-02-06 12:50       ` Andi Kleen
@ 2005-02-06 12:59         ` Arjan van de Ven
  2005-02-06 13:01           ` Andi Kleen
  0 siblings, 1 reply; 36+ messages in thread
From: Arjan van de Ven @ 2005-02-06 12:59 UTC (permalink / raw)
  To: Andi Kleen; +Cc: Ingo Molnar, akpm, torvalds, linux-kernel, drepper

On Sun, 2005-02-06 at 13:50 +0100, Andi Kleen wrote:
> > i.e. all mappings are executable (i.e. READ_IMPLIES_EXEC effect) - the
> > intended change. (although i dont fully agree with PT_GNU_STACK being
> > about something else than the stack, from a security POV if the stack is
> > executable then all bets are off anyway. The heap and all mmaps being
> > executable too in that case makes little difference.)
> 
> Well, that won't fix mono

correct,
http://lists.ximian.com/archives/public/mono-list/2004-June/021592.html

that fixes mono instead

>  (and i suspect wine) and the others

wine is ok, it uses PROT_EXEC correctly for things it's not sure about.



^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PROPOSAL/PATCH] Remove PT_GNU_STACK support before 2.6.11
  2005-02-06 12:59         ` Arjan van de Ven
@ 2005-02-06 13:01           ` Andi Kleen
  2005-02-06 13:04             ` Arjan van de Ven
  2005-02-06 13:06             ` Christoph Hellwig
  0 siblings, 2 replies; 36+ messages in thread
From: Andi Kleen @ 2005-02-06 13:01 UTC (permalink / raw)
  To: Arjan van de Ven
  Cc: Andi Kleen, Ingo Molnar, akpm, torvalds, linux-kernel, drepper

> correct,
> http://lists.ximian.com/archives/public/mono-list/2004-June/021592.html
> 
> that fixes mono instead

Silent breakage => bad.

> 
> >  (and i suspect wine) and the others
> 
> wine is ok, it uses PROT_EXEC correctly for things it's not sure about.

Hmm, I got a report that it doesn't work anymore with 
2.6.11rcs on x86-64. I haven't looked  closely yet,
but it wouldn't surprise me if this change isn't also involved.

-Andi

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PROPOSAL/PATCH] Remove PT_GNU_STACK support before 2.6.11
  2005-02-06 13:01           ` Andi Kleen
@ 2005-02-06 13:04             ` Arjan van de Ven
  2005-02-06 13:09               ` Andi Kleen
  2005-02-06 13:06             ` Christoph Hellwig
  1 sibling, 1 reply; 36+ messages in thread
From: Arjan van de Ven @ 2005-02-06 13:04 UTC (permalink / raw)
  To: Andi Kleen; +Cc: Ingo Molnar, akpm, torvalds, linux-kernel, drepper


> Hmm, I got a report that it doesn't work anymore with 
> 2.6.11rcs on x86-64. I haven't looked  closely yet,
> but it wouldn't surprise me if this change isn't also involved.

PT_GNU_STACK change is there since like 2.6.6 (and was put in by a suse person)
To me that is a strong indication that you are wrong on your
suspicion...




^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PROPOSAL/PATCH] Remove PT_GNU_STACK support before 2.6.11
  2005-02-06 13:01           ` Andi Kleen
  2005-02-06 13:04             ` Arjan van de Ven
@ 2005-02-06 13:06             ` Christoph Hellwig
  2005-02-06 13:11               ` Andi Kleen
  1 sibling, 1 reply; 36+ messages in thread
From: Christoph Hellwig @ 2005-02-06 13:06 UTC (permalink / raw)
  To: Andi Kleen
  Cc: Arjan van de Ven, Ingo Molnar, akpm, torvalds, linux-kernel,
	drepper

On Sun, Feb 06, 2005 at 02:01:52PM +0100, Andi Kleen wrote:
> > correct,
> > http://lists.ximian.com/archives/public/mono-list/2004-June/021592.html
> > 
> > that fixes mono instead
> 
> Silent breakage => bad.

silent breakage for newly compiled buggty and non-portable code.

Still not nice but certainly tolerable.  


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PROPOSAL/PATCH] Remove PT_GNU_STACK support before 2.6.11
  2005-02-06 13:04             ` Arjan van de Ven
@ 2005-02-06 13:09               ` Andi Kleen
  2005-02-06 13:31                 ` Ingo Molnar
  0 siblings, 1 reply; 36+ messages in thread
From: Andi Kleen @ 2005-02-06 13:09 UTC (permalink / raw)
  To: Arjan van de Ven
  Cc: Andi Kleen, Ingo Molnar, akpm, torvalds, linux-kernel, drepper

On Sun, Feb 06, 2005 at 02:04:57PM +0100, Arjan van de Ven wrote:
> 
> > Hmm, I got a report that it doesn't work anymore with 
> > 2.6.11rcs on x86-64. I haven't looked  closely yet,
> > but it wouldn't surprise me if this change isn't also involved.
> 
> PT_GNU_STACK change is there since like 2.6.6 (and was put in by a suse person)
> To me that is a strong indication that you are wrong on your
> suspicion...

Nah, the change to break 32bit userland mmap/mprotect like this was only 
put in after 2.6.10. 

64bit userland on the other hand always honored PROT_EXEC, but 
before PT_GNU_STACK it would run by default with executable stack. 

Hmm, I admit that my patch will break this case too, but fixing
that (setting READ_IMPLIES_EXEC only for 32bit) 
would require more complicated changes that may not be appropiate
for 2.6.11. If it's deemed appropiate I could do such a patch too
though.

-Andi

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PROPOSAL/PATCH] Remove PT_GNU_STACK support before 2.6.11
  2005-02-06 13:06             ` Christoph Hellwig
@ 2005-02-06 13:11               ` Andi Kleen
  2005-02-06 13:32                 ` Ingo Molnar
  0 siblings, 1 reply; 36+ messages in thread
From: Andi Kleen @ 2005-02-06 13:11 UTC (permalink / raw)
  To: Christoph Hellwig, Andi Kleen, Arjan van de Ven, Ingo Molnar,
	akpm, torvalds, linux-kernel, drepper

On Sun, Feb 06, 2005 at 01:06:50PM +0000, Christoph Hellwig wrote:
> On Sun, Feb 06, 2005 at 02:01:52PM +0100, Andi Kleen wrote:
> > > correct,
> > > http://lists.ximian.com/archives/public/mono-list/2004-June/021592.html
> > > 
> > > that fixes mono instead
> > 
> > Silent breakage => bad.
> 
> silent breakage for newly compiled buggty and non-portable code.

Executing custom code in mmap is by definition non portable, 
so this argument doesn't make very much sense.

> 
> Still not nice but certainly tolerable.  

I strongly disagree that breaking source level compatibility silently like
this is tolerable.

Especially since it won't even affect most users, so most developers
won't notice it, only the x86-64 users. This makes it extremly
silent for most people.

-Andi

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PROPOSAL/PATCH] Remove PT_GNU_STACK support before 2.6.11
  2005-02-06 13:09               ` Andi Kleen
@ 2005-02-06 13:31                 ` Ingo Molnar
  2005-02-06 13:43                   ` Andi Kleen
  0 siblings, 1 reply; 36+ messages in thread
From: Ingo Molnar @ 2005-02-06 13:31 UTC (permalink / raw)
  To: Andi Kleen; +Cc: Arjan van de Ven, akpm, torvalds, linux-kernel, drepper


* Andi Kleen <ak@suse.de> wrote:

> On Sun, Feb 06, 2005 at 02:04:57PM +0100, Arjan van de Ven wrote:
> > 
> > > Hmm, I got a report that it doesn't work anymore with 
> > > 2.6.11rcs on x86-64. I haven't looked  closely yet,
> > > but it wouldn't surprise me if this change isn't also involved.
> > 
> > PT_GNU_STACK change is there since like 2.6.6 (and was put in by a suse person)
> > To me that is a strong indication that you are wrong on your
> > suspicion...
> 
> Nah, the change to break 32bit userland mmap/mprotect like this was only 
> put in after 2.6.10. 

i suspect there may be some fundamental misunderstanding here. The
change to honor PT_GNU_STACK (on 64-bit) was added in April 2004 and
appeared in 2.6.6. The change to enforce the protection bits on x86 NX
CPUs (32-bit) was added in June 2004 and appeared in 2.6.8. I.e. it's
been part of the upstream kernel for more than half a year. Try it and
boot 2.6.8, you'll get NX protection. (I'm not sure about when it was
enabled for 32-bit emulation on the x64 kernel, you should be the one to
know that - but IIRC it was enabled prior 2.6.10.)

so i think you are on to the wrong victim :-)

	Ingo

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PROPOSAL/PATCH] Remove PT_GNU_STACK support before 2.6.11
  2005-02-06 13:11               ` Andi Kleen
@ 2005-02-06 13:32                 ` Ingo Molnar
  2005-02-06 13:46                   ` Andi Kleen
  0 siblings, 1 reply; 36+ messages in thread
From: Ingo Molnar @ 2005-02-06 13:32 UTC (permalink / raw)
  To: Andi Kleen
  Cc: Christoph Hellwig, Arjan van de Ven, akpm, torvalds, linux-kernel,
	drepper


* Andi Kleen <ak@suse.de> wrote:

> On Sun, Feb 06, 2005 at 01:06:50PM +0000, Christoph Hellwig wrote:
> > On Sun, Feb 06, 2005 at 02:01:52PM +0100, Andi Kleen wrote:
> > > > correct,
> > > > http://lists.ximian.com/archives/public/mono-list/2004-June/021592.html
> > > > 
> > > > that fixes mono instead
> > > 
> > > Silent breakage => bad.
> > 
> > silent breakage for newly compiled buggty and non-portable code.
> 
> Executing custom code in mmap is by definition non portable, 
> so this argument doesn't make very much sense.
> 
> > 
> > Still not nice but certainly tolerable.  
> 
> I strongly disagree that breaking source level compatibility silently
> like this is tolerable.
> 
> Especially since it won't even affect most users, so most developers
> won't notice it, only the x86-64 users. This makes it extremly silent
> for most people.

fortunately there's this 'NX-emulation' thing called exec-shield which
is part of Fedora (and has been part of it for almost 2 years) and did
all the testing for you on all x86 hardware, on thousands of packages
and on over a ten thousand binaries, well in advance of this going
upstream. It wasnt a bump-free ride, but it was worth it.

(the Mono bug was found this way, the Grub one wasnt, due to it being
RWE. But if it triggers it shows up immediately so it's not like you
have no sign that something wrong is going on. Only grub-install
triggers it and no boot/install kernel i know of defaults to
PAE-enabled, that's what caused grub-install being used in an NX
scenario so infrequently.)

anyway, this particular flamewar might have made more sense last Summer.

	Ingo

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PROPOSAL/PATCH] Remove PT_GNU_STACK support before 2.6.11
  2005-02-06 13:31                 ` Ingo Molnar
@ 2005-02-06 13:43                   ` Andi Kleen
  0 siblings, 0 replies; 36+ messages in thread
From: Andi Kleen @ 2005-02-06 13:43 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Andi Kleen, Arjan van de Ven, akpm, torvalds, linux-kernel,
	drepper

On Sun, Feb 06, 2005 at 02:31:33PM +0100, Ingo Molnar wrote:
> 
> * Andi Kleen <ak@suse.de> wrote:
> 
> > On Sun, Feb 06, 2005 at 02:04:57PM +0100, Arjan van de Ven wrote:
> > > 
> > > > Hmm, I got a report that it doesn't work anymore with 
> > > > 2.6.11rcs on x86-64. I haven't looked  closely yet,
> > > > but it wouldn't surprise me if this change isn't also involved.
> > > 
> > > PT_GNU_STACK change is there since like 2.6.6 (and was put in by a suse person)
> > > To me that is a strong indication that you are wrong on your
> > > suspicion...
> > 
> > Nah, the change to break 32bit userland mmap/mprotect like this was only 
> > put in after 2.6.10. 
> 
> i suspect there may be some fundamental misunderstanding here. The
> change to honor PT_GNU_STACK (on 64-bit) was added in April 2004 and
> appeared in 2.6.6. The change to enforce the protection bits on x86 NX
> CPUs (32-bit) was added in June 2004 and appeared in 2.6.8. I.e. it's
> been part of the upstream kernel for more than half a year. Try it and
> boot 2.6.8, you'll get NX protection. (I'm not sure about when it was
> enabled for 32-bit emulation on the x64 kernel, you should be the one to
> know that - but IIRC it was enabled prior 2.6.10.)

I suspect it was afterwards. All the reports about grub/mono/etc. not working
anymore etc. only appear now. It's possible that there wasn't
enough testing before or the testing happened with completely
different semantics (like Fedora on grub) 

Anyways, I don't care about the 32bit change much because nobody seems to 
test that one anyways. Basically all the NX capable machines
are 64bit capable and often run 64bit kernels, and even if people run
32bit kernels they tend to run non PAE kernels where the problem is hidden.

The main thing what annoys me is that the x86-64 32bit emulation
seems to effectively serve as a testing ground for 32bit changes now,
because the stuff is not tested on 32bit.

And it's already enough work to keep the 32bit emulation working,
without having to care about additional issues like this.

> so i think you are on to the wrong victim :-)

I don't think so, no.

Linus, can you please tell me what you prefer? If you think
it's better to only disable this on x86-64 I can change that too. 
But it would be probably the wrong thing to do, because
then the same issues will all reappear later.

-Andi

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PROPOSAL/PATCH] Remove PT_GNU_STACK support before 2.6.11
  2005-02-06 13:32                 ` Ingo Molnar
@ 2005-02-06 13:46                   ` Andi Kleen
  2005-02-06 14:08                     ` Ingo Molnar
  0 siblings, 1 reply; 36+ messages in thread
From: Andi Kleen @ 2005-02-06 13:46 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Andi Kleen, Christoph Hellwig, Arjan van de Ven, akpm, torvalds,
	linux-kernel, drepper

> fortunately there's this 'NX-emulation' thing called exec-shield which
> is part of Fedora (and has been part of it for almost 2 years) and did
> all the testing for you on all x86 hardware, on thousands of packages
> and on over a ten thousand binaries, well in advance of this going
> upstream. It wasnt a bump-free ride, but it was worth it.

But apparently a completely different patch was tested
with quite different semantics. 

> 
> (the Mono bug was found this way, the Grub one wasnt, due to it being

But not fixed in mainline mono.

And also as far as I can figure out there was no effort 
to warn user land people about this intrusive change,
and tell them what they need to do when they recompile.

> RWE. But if it triggers it shows up immediately so it's not like you
> have no sign that something wrong is going on. Only grub-install
> triggers it and no boot/install kernel i know of defaults to
> PAE-enabled, that's what caused grub-install being used in an NX
> scenario so infrequently.)
> 
> anyway, this particular flamewar might have made more sense last Summer.

Last summer nobody did change the 32bit ABI on x86-64.

I only started it because the bug reports are appearing now 
and it's clear now that we have a problem. 

-Andi

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PROPOSAL/PATCH] Remove PT_GNU_STACK support before 2.6.11
  2005-02-06 13:46                   ` Andi Kleen
@ 2005-02-06 14:08                     ` Ingo Molnar
  2005-02-06 14:22                       ` Ingo Molnar
  2005-02-06 14:29                       ` Andi Kleen
  0 siblings, 2 replies; 36+ messages in thread
From: Ingo Molnar @ 2005-02-06 14:08 UTC (permalink / raw)
  To: Andi Kleen
  Cc: Christoph Hellwig, Arjan van de Ven, akpm, torvalds, linux-kernel,
	drepper

* Andi Kleen <ak@suse.de> wrote:

> > RWE. But if it triggers it shows up immediately so it's not like you
> > have no sign that something wrong is going on. Only grub-install
> > triggers it and no boot/install kernel i know of defaults to
> > PAE-enabled, that's what caused grub-install being used in an NX
> > scenario so infrequently.)
> > 
> > anyway, this particular flamewar might have made more sense last Summer.
> 
> Last summer nobody did change the 32bit ABI on x86-64.
> 
> I only started it because the bug reports are appearing now and it's
> clear now that we have a problem. 

the vanilla 2.6.10 x64 kernel, using 32-bit fedora userland boots fine
here, and gives a noexec stack:

 $ ./test-stack
 Segmentation fault
 $
 $ uname -a
 Linux saturn 2.6.10 #1 Sun Feb 6 15:52:44 CET 2005 x86_64 x86_64 x86_64 GNU/Linux
 $
 $ readelf -l test-stack | grep STA
   GNU_STACK      0x000000 0x00000000 0x00000000 0x00000 0x00000 RW  0x4

	Ingo

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PROPOSAL/PATCH] Remove PT_GNU_STACK support before 2.6.11
  2005-02-06 14:08                     ` Ingo Molnar
@ 2005-02-06 14:22                       ` Ingo Molnar
  2005-02-06 14:29                       ` Andi Kleen
  1 sibling, 0 replies; 36+ messages in thread
From: Ingo Molnar @ 2005-02-06 14:22 UTC (permalink / raw)
  To: Andi Kleen
  Cc: Christoph Hellwig, Arjan van de Ven, akpm, torvalds, linux-kernel,
	drepper


* Ingo Molnar <mingo@elte.hu> wrote:

> > Last summer nobody did change the 32bit ABI on x86-64.
> > 
> > I only started it because the bug reports are appearing now and it's
> > clear now that we have a problem. 
> 
> the vanilla 2.6.10 x64 kernel, using 32-bit fedora userland boots fine
> here, and gives a noexec stack:

same with SuSE 9.1 32-bit user-space running a vanilla 2.6.10 x64 kernel
- PT_GNU_STACK is honored and the stack is noexec.

	Ingo

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PROPOSAL/PATCH] Remove PT_GNU_STACK support before 2.6.11
  2005-02-06 14:08                     ` Ingo Molnar
  2005-02-06 14:22                       ` Ingo Molnar
@ 2005-02-06 14:29                       ` Andi Kleen
  2005-02-06 17:08                         ` Linus Torvalds
  1 sibling, 1 reply; 36+ messages in thread
From: Andi Kleen @ 2005-02-06 14:29 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Andi Kleen, Christoph Hellwig, Arjan van de Ven, akpm, torvalds,
	linux-kernel, drepper

On Sun, Feb 06, 2005 at 03:08:02PM +0100, Ingo Molnar wrote:
> * Andi Kleen <ak@suse.de> wrote:
> 
> > > RWE. But if it triggers it shows up immediately so it's not like you
> > > have no sign that something wrong is going on. Only grub-install
> > > triggers it and no boot/install kernel i know of defaults to
> > > PAE-enabled, that's what caused grub-install being used in an NX
> > > scenario so infrequently.)
> > > 
> > > anyway, this particular flamewar might have made more sense last Summer.
> > 
> > Last summer nobody did change the 32bit ABI on x86-64.
> > 
> > I only started it because the bug reports are appearing now and it's
> > clear now that we have a problem. 
> 
> the vanilla 2.6.10 x64 kernel, using 32-bit fedora userland boots fine
> here, and gives a noexec stack:

I'm not too concerned about the noexec stack (PT_GNU_STACK seems 
to handle that well enough), more about the no exec heap which
seems to cause all the problems.  The stack change is not very
nice, but at least doesn't seem to cause wide spread breakage.

Anyways, you're right I double checked and the problem was already
in 2.6.10. I can just say that I didn't get any reports, maybe
nobody tried grub with 2.6.10 or they didn't report it the problem
properly.

Probably my original patch was a bit too radical too, just setting 
READ_IMPLIES_EXEC on all 32bit process unconditionally should be good enough. 

I still think it's something that needs to be addressed for 2.6.11.

Here's a new patch that just sets READ_IMPLIES_EXEC unconditionally.

It still has a bug that you cannot change it with personality()
without getting overwritten later
(that is a bug that was in the original changes, you would really 
need two independent bits for this). But I'm not going to change
that now so late in the game. It implies that you cannot 
use personality to force READ_IMPLIES_EXEC for a 64bit process.

-Andi


Force READ_IMPLIES_EXEC for all 32bit processes to fix
the 32bit source compatibility.

Signed-off-by: Andi Kleen <ak@suse.de>

diff -u linux-2.6.11rc3/include/asm-i386/elf.h-o linux-2.6.11rc3/include/asm-i386/elf.h
--- linux-2.6.11rc3/include/asm-i386/elf.h-o	2004-10-19 01:55:33.000000000 +0200
+++ linux-2.6.11rc3/include/asm-i386/elf.h	2005-02-06 15:27:13.000000000 +0100
@@ -117,7 +117,8 @@
 #define AT_SYSINFO_EHDR		33
 
 #ifdef __KERNEL__
-#define SET_PERSONALITY(ex, ibcs2) do { } while (0)
+#define SET_PERSONALITY(ex, ibcs2) \
+	do { current->personality |= READ_IMPLIES_EXEC; } while (0)
 
 /*
  * An executable for which elf_read_implies_exec() returns TRUE will
diff -u linux-2.6.11rc3/arch/x86_64/kernel/process.c-o linux-2.6.11rc3/arch/x86_64/kernel/process.c
--- linux-2.6.11rc3/arch/x86_64/kernel/process.c-o	2005-02-04 09:12:52.000000000 +0100
+++ linux-2.6.11rc3/arch/x86_64/kernel/process.c	2005-02-06 15:26:45.000000000 +0100
@@ -577,6 +577,11 @@
 
 	/* Make sure to be in 64bit mode */
 	clear_thread_flag(TIF_IA32); 
+
+	/* Clear in case it was set from a 32bit parent.
+	   Bug: this overwrites the user choice. Would need
+	   a second bit too. */
+	current->personality &= ~READ_IMPLIES_EXEC;
 }
 
 asmlinkage long sys_fork(struct pt_regs *regs)
diff -u linux-2.6.11rc3/arch/x86_64/ia32/ia32_binfmt.c-o linux-2.6.11rc3/arch/x86_64/ia32/ia32_binfmt.c
--- linux-2.6.11rc3/arch/x86_64/ia32/ia32_binfmt.c-o	2005-02-04 09:12:52.000000000 +0100
+++ linux-2.6.11rc3/arch/x86_64/ia32/ia32_binfmt.c	2005-02-06 15:23:33.000000000 +0100
@@ -262,6 +262,7 @@
 		set_thread_flag(TIF_ABI_PENDING);		\
 	else							\
 		clear_thread_flag(TIF_ABI_PENDING);		\
+	current->personality |= READ_IMPLIES_EXEC; 		\
 } while (0)
 
 /* Override some function names */



^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PROPOSAL/PATCH] Remove PT_GNU_STACK support before 2.6.11
  2005-02-06 12:48       ` Andi Kleen
@ 2005-02-06 15:54         ` Andreas Schwab
  0 siblings, 0 replies; 36+ messages in thread
From: Andreas Schwab @ 2005-02-06 15:54 UTC (permalink / raw)
  To: Andi Kleen; +Cc: Arjan van de Ven, akpm, torvalds, mingo, linux-kernel, drepper

Andi Kleen <ak@suse.de> writes:

> They worked fine forever - and suddenly you define them as buggy.

Working fine does not imply non-buggy, never has, never will.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PROPOSAL/PATCH] Remove PT_GNU_STACK support before 2.6.11
  2005-02-06 12:33   ` Andi Kleen
  2005-02-06 12:40     ` Arjan van de Ven
@ 2005-02-06 17:05     ` Linus Torvalds
  2005-02-06 17:58       ` Andi Kleen
  1 sibling, 1 reply; 36+ messages in thread
From: Linus Torvalds @ 2005-02-06 17:05 UTC (permalink / raw)
  To: Andi Kleen; +Cc: Arjan van de Ven, akpm, mingo, linux-kernel, drepper



On Sun, 6 Feb 2005, Andi Kleen wrote:
> 
> There are probably more.

So? Do you expect to never fix them, or what?

The fact is, a program that tries to execute without using PROT_EXEC is a 
buggy program. 

Are there buggy programs out there? Yes. We should fix them.

		Linus

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PROPOSAL/PATCH] Remove PT_GNU_STACK support before 2.6.11
  2005-02-06 14:29                       ` Andi Kleen
@ 2005-02-06 17:08                         ` Linus Torvalds
  2005-02-06 17:13                           ` Arjan van de Ven
  2005-02-06 17:56                           ` Andi Kleen
  0 siblings, 2 replies; 36+ messages in thread
From: Linus Torvalds @ 2005-02-06 17:08 UTC (permalink / raw)
  To: Andi Kleen
  Cc: Ingo Molnar, Christoph Hellwig, Arjan van de Ven, akpm,
	linux-kernel, drepper



On Sun, 6 Feb 2005, Andi Kleen wrote:
> 
> Force READ_IMPLIES_EXEC for all 32bit processes to fix
> the 32bit source compatibility.

Andi, stop this. We're _not_ going to say "32-bit executables don't need 
PROT_EXEC. The executables would need to be marked broken per-executable, 
not some kind of "we don't do this globally" setting.

		Linus

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PROPOSAL/PATCH] Remove PT_GNU_STACK support before 2.6.11
  2005-02-06 17:08                         ` Linus Torvalds
@ 2005-02-06 17:13                           ` Arjan van de Ven
  2005-02-06 17:31                             ` Linus Torvalds
  2005-02-06 17:56                           ` Andi Kleen
  1 sibling, 1 reply; 36+ messages in thread
From: Arjan van de Ven @ 2005-02-06 17:13 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Andi Kleen, Ingo Molnar, Christoph Hellwig, akpm, linux-kernel,
	drepper

On Sun, 2005-02-06 at 09:08 -0800, Linus Torvalds wrote:
> 
> On Sun, 6 Feb 2005, Andi Kleen wrote:
> > 
> > Force READ_IMPLIES_EXEC for all 32bit processes to fix
> > the 32bit source compatibility.
> 
> Andi, stop this. We're _not_ going to say "32-bit executables don't need 
> PROT_EXEC. The executables would need to be marked broken per-executable, 
> not some kind of "we don't do this globally" setting.


marking PT_GNU_STACK as RWE would be an acceptable marking imo; one can
insert such a marking post build (via the execstack tool), and during
the build with either an addition to the cflags or via a one line code
addition. 

In addition there are runtime markings; you can use setarch to start an
application with READ-IMPLIES-EXEC set (although you can't do that for
setuid binaries for obvious reasons)

Note that these techniques all exist today. The only issue is that the
current code doesn't do the RWE->READIMPLIESEXEC binding, which my patch
fixed. 




^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PROPOSAL/PATCH] Remove PT_GNU_STACK support before 2.6.11
  2005-02-06 17:13                           ` Arjan van de Ven
@ 2005-02-06 17:31                             ` Linus Torvalds
  2005-02-06 17:39                               ` Arjan van de Ven
  0 siblings, 1 reply; 36+ messages in thread
From: Linus Torvalds @ 2005-02-06 17:31 UTC (permalink / raw)
  To: Arjan van de Ven
  Cc: Andi Kleen, Ingo Molnar, Christoph Hellwig, akpm, linux-kernel,
	drepper



On Sun, 6 Feb 2005, Arjan van de Ven wrote:
> 
> Note that these techniques all exist today. The only issue is that the
> current code doesn't do the RWE->READIMPLIESEXEC binding, which my patch
> fixed. 

My main objection to your patch is the naming. If 'executable_stack' 
affects the heap, then why is it called "executable_STACK"?

Wouldn't it be much nicer to

 - get rid of "EXSTACK_DEFAULT" as a special case, and instead just have 
   the architecture _initialize_ the damn variable to what it wants? In 
   other words, make it a nice understandable binary value (or maybe a 
   bitmask, if you want to have separate flags for stack/heap/mmap), 
   rather than a ternary value where one of the values means something 
   arch-dependent.

 - just rename the dang thing to "read_implies_exec" and be done with it?

Hmm? Wouldn't that make a lot more sense?

And if you want to split things up, there's at least three flags there:  
"stack" vs "file mapping" vs "anonymous mapping". For example, it might
well make sense to enforce PROT_EXEC on real file mappings, but not on the
stack or heap..  So "read_implies_exec" might well be a collection of bits 
to enable these one by one (and make the "legacy app" thing make it enable 
read-implies-exec for all the cases).

		Linus

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PROPOSAL/PATCH] Remove PT_GNU_STACK support before 2.6.11
  2005-02-06 17:31                             ` Linus Torvalds
@ 2005-02-06 17:39                               ` Arjan van de Ven
  2005-02-06 18:04                                 ` Linus Torvalds
  0 siblings, 1 reply; 36+ messages in thread
From: Arjan van de Ven @ 2005-02-06 17:39 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Andi Kleen, Ingo Molnar, Christoph Hellwig, akpm, linux-kernel,
	drepper

On Sun, 2005-02-06 at 09:31 -0800, Linus Torvalds wrote:
> 
> On Sun, 6 Feb 2005, Arjan van de Ven wrote:
> > 
> > Note that these techniques all exist today. The only issue is that the
> > current code doesn't do the RWE->READIMPLIESEXEC binding, which my patch
> > fixed. 
> 
> My main objection to your patch is the naming. If 'executable_stack' 
> affects the heap, then why is it called "executable_STACK"?

fair point.

> 
> Wouldn't it be much nicer to
> 
>  - get rid of "EXSTACK_DEFAULT" as a special case, and instead just have 
>    the architecture _initialize_ the damn variable to what it wants? In 
>    other words, make it a nice understandable binary value (or maybe a 
>    bitmask, if you want to have separate flags for stack/heap/mmap), 
>    rather than a ternary value where one of the values means something 
>    arch-dependent.
> 
>  - just rename the dang thing to "read_implies_exec" and be done with it?
> 
> Hmm? Wouldn't that make a lot more sense?

it would. It'll be a bit bigger than I'd be comfortable with this late
in the 2.6.11 cycle though. 

> 
> And if you want to split things up, there's at least three flags there:  
> "stack" vs "file mapping" vs "anonymous mapping". For example, it might

lets add "brk" as 4th I guess.

Ok so what to do for 2.6.11... the setarch workaround is there; that
works. My patch patches the worst issues and is quite minimal. What you
propose will be more invasive and more suitable for 2.6.11-bk1... 
I can do such a patch no problem (although the next two days I won't
have time).


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PROPOSAL/PATCH] Remove PT_GNU_STACK support before 2.6.11
  2005-02-06 17:08                         ` Linus Torvalds
  2005-02-06 17:13                           ` Arjan van de Ven
@ 2005-02-06 17:56                           ` Andi Kleen
  1 sibling, 0 replies; 36+ messages in thread
From: Andi Kleen @ 2005-02-06 17:56 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Andi Kleen, Ingo Molnar, Christoph Hellwig, Arjan van de Ven,
	akpm, linux-kernel, drepper

On Sun, Feb 06, 2005 at 09:08:47AM -0800, Linus Torvalds wrote:
> 
> 
> On Sun, 6 Feb 2005, Andi Kleen wrote:
> > 
> > Force READ_IMPLIES_EXEC for all 32bit processes to fix
> > the 32bit source compatibility.
> 
> Andi, stop this. We're _not_ going to say "32-bit executables don't need 
> PROT_EXEC. The executables would need to be marked broken per-executable, 
> not some kind of "we don't do this globally" setting.

The thing I'm annoyed about is that all the testing for this
change seems to go towards the x86-64 32bit emulation
(because effectively near nobody uses 32bit PAE+NX right now) 

And the main job of the 32bit emulation is not to prove 
as a testing ground for experimental stuff, but to be compatible.

And changes like this break it and cause me a lot of additional work.

Here's a slightly different patch to only turn it off for 32bit x86-64.

If the 32bit experimental security people can get their stuff tested
properly and 32bit NX CPUs are actually used widely 
and all the third party sources fixed I can probably follow in a few months. 

But I really don't have the capacity to track third party software fixes for 
stuff that really has nothing to do with compatible 32bit emulation.

Please consider applying this patch. It only touches x86-64. Thanks: 

-Andi

Always enable PROT_READ implies PROT_EXEC for 32bit programs
running on x86-64.  This reverts behaviour back to what 2.6.9 did.

Signed-off-by: Andi Kleen <ak@suse.de>


diff -u linux-2.6.11rc3/arch/x86_64/kernel/process.c-o linux-2.6.11rc3/arch/x86_64/kernel/process.c
--- linux-2.6.11rc3/arch/x86_64/kernel/process.c-o	2005-02-04 09:12:52.000000000 +0100
+++ linux-2.6.11rc3/arch/x86_64/kernel/process.c	2005-02-06 15:26:45.000000000 +0100
@@ -577,6 +577,11 @@
 
 	/* Make sure to be in 64bit mode */
 	clear_thread_flag(TIF_IA32); 
+
+	/* Clear in case it was set from a 32bit parent.
+	   Bug: this overwrites the user choice. Would need
+	   a second bit too. */
+	current->personality &= ~READ_IMPLIES_EXEC;
 }
 
 asmlinkage long sys_fork(struct pt_regs *regs)
diff -u linux-2.6.11rc3/arch/x86_64/ia32/ia32_binfmt.c-o linux-2.6.11rc3/arch/x86_64/ia32/ia32_binfmt.c
--- linux-2.6.11rc3/arch/x86_64/ia32/ia32_binfmt.c-o	2005-02-04 09:12:52.000000000 +0100
+++ linux-2.6.11rc3/arch/x86_64/ia32/ia32_binfmt.c	2005-02-06 15:23:33.000000000 +0100
@@ -262,6 +262,7 @@
 		set_thread_flag(TIF_ABI_PENDING);		\
 	else							\
 		clear_thread_flag(TIF_ABI_PENDING);		\
+	current->personality |= READ_IMPLIES_EXEC; 		\
 } while (0)
 
 /* Override some function names */


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PROPOSAL/PATCH] Remove PT_GNU_STACK support before 2.6.11
  2005-02-06 17:05     ` Linus Torvalds
@ 2005-02-06 17:58       ` Andi Kleen
  0 siblings, 0 replies; 36+ messages in thread
From: Andi Kleen @ 2005-02-06 17:58 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Andi Kleen, Arjan van de Ven, akpm, mingo, linux-kernel, drepper

On Sun, Feb 06, 2005 at 09:05:05AM -0800, Linus Torvalds wrote:
> 
> 
> On Sun, 6 Feb 2005, Andi Kleen wrote:
> > 
> > There are probably more.
> 
> So? Do you expect to never fix them, or what?

Someone will fix them, but it's not the job of the 32bit emulation
of x86-64 to break compatibility. It's whole point is to be compatible,
not expose bugs.

If someone else wants to break existing programs they can 
do that if they think they have the capability to deal
with (rightfully) annoyed user space people. 

But not with x86-64 compat mode leading the front here.

-Andi

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PROPOSAL/PATCH] Remove PT_GNU_STACK support before 2.6.11
  2005-02-06 17:39                               ` Arjan van de Ven
@ 2005-02-06 18:04                                 ` Linus Torvalds
  2005-02-06 18:08                                   ` Arjan van de Ven
  0 siblings, 1 reply; 36+ messages in thread
From: Linus Torvalds @ 2005-02-06 18:04 UTC (permalink / raw)
  To: Arjan van de Ven
  Cc: Andi Kleen, Ingo Molnar, Christoph Hellwig, akpm, linux-kernel,
	drepper



On Sun, 6 Feb 2005, Arjan van de Ven wrote:
> > 
> > And if you want to split things up, there's at least three flags there:  
> > "stack" vs "file mapping" vs "anonymous mapping". For example, it might
> 
> lets add "brk" as 4th I guess.

I thought about that, but no normal user program uses brk() natively. They 
all just use "malloc()" and friends, and pretty much every implementation 
of those in turn just mixes brk/anon-mmap freely.

> Ok so what to do for 2.6.11... the setarch workaround is there; that
> works. My patch patches the worst issues and is quite minimal. What you
> propose will be more invasive and more suitable for 2.6.11-bk1... 
> I can do such a patch no problem (although the next two days I won't
> have time).

Hmm.. I can take your initial patch now. Can somebody explain why this 
hassn't come up before, though?

		Linus

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PROPOSAL/PATCH] Remove PT_GNU_STACK support before 2.6.11
       [not found]   ` <20050206124701.GD30109@wotan.suse.de>
@ 2005-02-06 18:07     ` Paweł Sikora
  2005-02-06 18:38       ` Andi Kleen
  0 siblings, 1 reply; 36+ messages in thread
From: Paweł Sikora @ 2005-02-06 18:07 UTC (permalink / raw)
  To: Andi Kleen; +Cc: linux-kernel

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

On Sunday 06 of February 2005 13:47, you wrote:
> On Sun, Feb 06, 2005 at 01:03:11PM +0100, Pawel Sikora wrote:
> > On Sunday 06 of February 2005 12:36, you wrote:
> > > Worse is that even when the program has trampolines and has
> > > PT_GNU_STACK header with an E bit on the stack it still won't get an
> > > executable heap by default  (this is what broke grub)
> > > (...)
> > > My proposal is to turn this all off at least for 2.6.11.
> >
> > My proposal is to recompile broken software with cflags +=
> > -Wa,--noexecstack
>
> By how do you detect broken software? There doesn't seem to be any
> fool proof way other than a extensive test on a NX capable system
> with correct kernel.

[1] glibc-2.3.4 kill buggy bins at the load time.
    (please look into: elf/dl-load.c, elf/dl-support.c, elf/rtld.c)
    This works on i386/PaX systems too (hardware NX isn't required).

[2] `readelf -Wl |grep GNU_STACK` shows RWE ;-)

Please look at this quick example.

# gcc tmp1.c tmp2-invalid.S -o tmp -s
# readelf -Wl tmp

(...)
  GNU_STACK      0x000000 0x00000000 0x00000000 0x00000 0x00000 RWE 0x4
                                                                  ^ execstack?
  PAX_FLAGS      0x000000 0x00000000 0x00000000 0x00000 0x00000     0x4
(...)

Now, let's add section note to the asm. file and rebuild.

# gcc tmp1.c tmp2-valid.S -o tmp -s
# readelf -Wl tmp

(...)
  GNU_STACK      0x000000 0x00000000 0x00000000 0x00000 0x00000 RW  0x4
  PAX_FLAGS      0x000000 0x00000000 0x00000000 0x00000 0x00000     0x4
(...)

The execstack req. disappeard (~99% of broken sources).
I get the same effect with fixed cflags and invalid source.

# gcc tmp1.c tmp2-invalid.S -o tmp -s -Wa,--noexecstack
# readelf -Wl tmp

(...)
  GNU_STACK      0x000000 0x00000000 0x00000000 0x00000 0x00000 RW  0x4
  PAX_FLAGS      0x000000 0x00000000 0x00000000 0x00000 0x00000     0x4
(...)

I known several apps that really need executable data/stack (eg. jvm, xorg).
The rest of RWE-marked binaries have IMHO buggy sources.

> It would be fine if there was a compile time error or something,
> but there isn't.

IMHO the `as` should warn about missed (.note.GNU-stack) section.

Regards,
Paweł.

-- 
/* Copyright (C) 2003, SCO, Inc. This is valuable Intellectual Property. */

                           #define say(x) lie(x)

[-- Attachment #2: tmp1.c --]
[-- Type: text/x-csrc, Size: 83 bytes --]

extern void test();

int main(int argc, char** argv)
{
    test();
    return 0;
}

[-- Attachment #3: tmp2-invalid.S --]
[-- Type: text/plain, Size: 50 bytes --]

    .text
    .global test
test:
    ret
    .end

[-- Attachment #4: tmp2-valid.S --]
[-- Type: text/plain, Size: 103 bytes --]

    .section .note.GNU-stack,"",@progbits; .previous
    .text
    .global test
test:
    ret
    .end

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PROPOSAL/PATCH] Remove PT_GNU_STACK support before 2.6.11
  2005-02-06 18:04                                 ` Linus Torvalds
@ 2005-02-06 18:08                                   ` Arjan van de Ven
  0 siblings, 0 replies; 36+ messages in thread
From: Arjan van de Ven @ 2005-02-06 18:08 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Andi Kleen, Ingo Molnar, Christoph Hellwig, akpm, linux-kernel,
	drepper

On Sun, 2005-02-06 at 10:04 -0800, Linus Torvalds wrote:
> > Ok so what to do for 2.6.11... the setarch workaround is there; that
> > works. My patch patches the worst issues and is quite minimal. What you
> > propose will be more invasive and more suitable for 2.6.11-bk1... 
> > I can do such a patch no problem (although the next two days I won't
> > have time).
> 
> Hmm.. I can take your initial patch now. Can somebody explain why this 
> hassn't come up before, though?

somehow things got "cleaned up" around 2.6.10-ish to change the
default... it's quite rare so few people noticed. It seems Andi got a
few reports in the last week or so and started investigating.





^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PROPOSAL/PATCH] Remove PT_GNU_STACK support before 2.6.11
  2005-02-06 18:07     ` Paweł Sikora
@ 2005-02-06 18:38       ` Andi Kleen
  0 siblings, 0 replies; 36+ messages in thread
From: Andi Kleen @ 2005-02-06 18:38 UTC (permalink / raw)
  To: Pawe?? Sikora; +Cc: Andi Kleen, linux-kernel

> [1] glibc-2.3.4 kill buggy bins at the load time.
>     (please look into: elf/dl-load.c, elf/dl-support.c, elf/rtld.c)

I don't see how that can work for arbitary code executed in some
arbitary mmap. 

Please explain.

>     This works on i386/PaX systems too (hardware NX isn't required).

But it's for the 99.99999% of other users who don't use such
weird third party patches.

> The execstack req. disappeard (~99% of broken sources).

My problem is basically that the effort of fixing these
sources seems to be shifted to x86-64 now in mainstream
linux. And I don't like that.

-Andi

^ permalink raw reply	[flat|nested] 36+ messages in thread

end of thread, other threads:[~2005-02-06 18:40 UTC | newest]

Thread overview: 36+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2005-02-06 11:36 [PROPOSAL/PATCH] Remove PT_GNU_STACK support before 2.6.11 Andi Kleen
2005-02-06 11:47 ` Arjan van de Ven
2005-02-06 12:02   ` Ingo Molnar
2005-02-06 12:25     ` Ingo Molnar
2005-02-06 12:36     ` Andi Kleen
2005-02-06 12:45     ` Ingo Molnar
2005-02-06 12:50       ` Andi Kleen
2005-02-06 12:59         ` Arjan van de Ven
2005-02-06 13:01           ` Andi Kleen
2005-02-06 13:04             ` Arjan van de Ven
2005-02-06 13:09               ` Andi Kleen
2005-02-06 13:31                 ` Ingo Molnar
2005-02-06 13:43                   ` Andi Kleen
2005-02-06 13:06             ` Christoph Hellwig
2005-02-06 13:11               ` Andi Kleen
2005-02-06 13:32                 ` Ingo Molnar
2005-02-06 13:46                   ` Andi Kleen
2005-02-06 14:08                     ` Ingo Molnar
2005-02-06 14:22                       ` Ingo Molnar
2005-02-06 14:29                       ` Andi Kleen
2005-02-06 17:08                         ` Linus Torvalds
2005-02-06 17:13                           ` Arjan van de Ven
2005-02-06 17:31                             ` Linus Torvalds
2005-02-06 17:39                               ` Arjan van de Ven
2005-02-06 18:04                                 ` Linus Torvalds
2005-02-06 18:08                                   ` Arjan van de Ven
2005-02-06 17:56                           ` Andi Kleen
2005-02-06 12:33   ` Andi Kleen
2005-02-06 12:40     ` Arjan van de Ven
2005-02-06 12:48       ` Andi Kleen
2005-02-06 15:54         ` Andreas Schwab
2005-02-06 17:05     ` Linus Torvalds
2005-02-06 17:58       ` Andi Kleen
2005-02-06 12:11 ` Paweł Sikora
     [not found] ` <200502061303.12377.pluto@pld-linux.org>
     [not found]   ` <20050206124701.GD30109@wotan.suse.de>
2005-02-06 18:07     ` Paweł Sikora
2005-02-06 18:38       ` Andi Kleen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox