[patch 1/5] Add comments to the PDA structure to annotate offsets

[patch 1/5] Add comments to the PDA structure to annotate offsets

[Arjan van de Ven]

Subject: [patch 1/5] Add comments to the PDA structure to annotate offsets
From: Arjan van de Ven <arjan@linux.intel.com>

Change the comments in the pda structure to make the first fields to have
their offset documented (the rest of the fields follows in a later patch)
and to have the comments aligned.

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
CC: Andi Kleen <ak@suse.de>
---
 include/asm-x86_64/pda.h |   14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

Index: linux-2.6.18-rc2-git5-stackprot/include/asm-x86_64/pda.h
===================================================================
--- linux-2.6.18-rc2-git5-stackprot.orig/include/asm-x86_64/pda.h
+++ linux-2.6.18-rc2-git5-stackprot/include/asm-x86_64/pda.h
@@ -9,14 +9,12 @@
 
 /* Per processor datastructure. %gs points to it while the kernel runs */ 
 struct x8664_pda {
-	struct task_struct *pcurrent;	/* Current process */
-	unsigned long data_offset;	/* Per cpu data offset from linker address */
-	unsigned long kernelstack;  /* top of kernel stack for current */ 
-	unsigned long oldrsp; 	    /* user rsp for system call */
-#if DEBUG_STKSZ > EXCEPTION_STKSZ
-	unsigned long debugstack;   /* #DB/#BP stack. */
-#endif
-        int irqcount;		    /* Irq nesting counter. Starts with -1 */  	
+	struct task_struct *pcurrent;	/*  0 */  /* Current process */
+	unsigned long data_offset;	/*  8 */  /* Per cpu data offset from linker address */
+	unsigned long kernelstack;	/* 16 */  /* top of kernel stack for current */
+	unsigned long oldrsp;		/* 24 */  /* user rsp for system call */
+	unsigned long debugstack;	/* 32 */  /* #DB/#BP stack. */
+	int irqcount;			/* 40 */  /* Irq nesting counter. Starts with -1 */
 	int cpunumber;		    /* Logical CPU number */
 	char *irqstackptr;	/* top of irqstack */
 	int nodenumber;		    /* number of current node */

[patch 2/5] Add the Kconfig option for the stackprotector feature

[Arjan van de Ven]

Subject: [patch 2/5] Add the Kconfig option for the stackprotector feature
From: Arjan van de Ven <arjan@linux.intel.com>

This patch adds the config options for -fstack-protector.

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
CC: Andi Kleen <ak@suse.de>

---
 arch/x86_64/Kconfig |   25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

Index: linux-2.6.18-rc2-git5-stackprot/arch/x86_64/Kconfig
===================================================================
--- linux-2.6.18-rc2-git5-stackprot.orig/arch/x86_64/Kconfig
+++ linux-2.6.18-rc2-git5-stackprot/arch/x86_64/Kconfig
@@ -522,6 +522,31 @@ config SECCOMP
 
 	  If unsure, say Y. Only embedded should say N here.
 
+config CC_STACKPROTECTOR
+	bool "Enable -fstack-protector buffer overflow detection (EXPRIMENTAL)"
+	depends on EXPERIMENTAL
+	default n
+	help
+	  This option turns on the -fstack-protector GCC feature that is new
+	  in GCC version 4.1. This feature puts, at the beginning of
+	  critical functions, a canary value on the stack just before the return
+	  address, and validates the value just before actually returning.
+	  Stack based buffer overflows that need to overwrite this return
+	  address now also overwrite the canary, which gets detected.
+
+	  NOTE 
+	  This feature requires gcc version 4.2 or above, or a distribution
+	  gcc with the feature backported. For older gcc versions, this is a NOP.
+
+config CC_STACKPROTECTOR_ALL
+	bool "Use stack-protector for all functions"
+	depends on CC_STACKPROTECTOR
+	default n
+	help
+	  Normally, GCC only inserts the canary value protection for
+	  functions that use large-ish on-stack buffers. By enabling
+	  this option, GCC will be asked to do this for ALL functions.
+
 source kernel/Kconfig.hz
 
 config REORDER

[patch 3/5] Add the canary field to the PDA area and the task struct

[Arjan van de Ven]

Subject: [patch 3/5] Add the canary field to the PDA area and the task struct
From: Arjan van de Ven <arjan@linux.intel.com>

This patch adds the per thread cookie field to the task struct and the PDA.
Also it makes sure that the PDA value gets the new cookie value at context
switch, and that a new task gets a new cookie at task creation time.

Signed-off-by: Arjan van Ven <arjan@linux.intel.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
CC: Andi Kleen <ak@suse.de>
---
 arch/x86_64/kernel/process.c |    3 +++
 include/asm-x86_64/pda.h     |    6 +++++-
 include/linux/sched.h        |    5 +++++
 kernel/fork.c                |    4 ++++
 4 files changed, 17 insertions(+), 1 deletion(-)

Index: linux-2.6.18-rc2-git5-stackprot/arch/x86_64/kernel/process.c
===================================================================
--- linux-2.6.18-rc2-git5-stackprot.orig/arch/x86_64/kernel/process.c
+++ linux-2.6.18-rc2-git5-stackprot/arch/x86_64/kernel/process.c
@@ -584,6 +584,9 @@ __switch_to(struct task_struct *prev_p, 
 	unlazy_fpu(prev_p);
 	write_pda(kernelstack,
 		  task_stack_page(next_p) + THREAD_SIZE - PDA_STACKOFFSET);
+#ifdef CONFIG_CC_STACKPROTECTOR
+	write_pda(stack_canary, next_p->stack_canary);
+#endif
 
 	/*
 	 * Now maybe reload the debug registers
Index: linux-2.6.18-rc2-git5-stackprot/include/asm-x86_64/pda.h
===================================================================
--- linux-2.6.18-rc2-git5-stackprot.orig/include/asm-x86_64/pda.h
+++ linux-2.6.18-rc2-git5-stackprot/include/asm-x86_64/pda.h
@@ -14,7 +14,11 @@ struct x8664_pda {
 	unsigned long kernelstack;	/* 16 */  /* top of kernel stack for current */
 	unsigned long oldrsp;		/* 24 */  /* user rsp for system call */
 	unsigned long debugstack;	/* 32 */  /* #DB/#BP stack. */
-	int irqcount;			/* 40 */  /* Irq nesting counter. Starts with -1 */
+#ifdef CONFIG_CC_STACKPROTECTOR
+	unsigned long stack_canary;	/* 40 */  /* stack canary value */
+					/* gcc-ABI: this canary MUST be at offset 40!!! */
+#endif
+	int irqcount;			/* 48 */  /* Irq nesting counter. Starts with -1 */
 	int cpunumber;		    /* Logical CPU number */
 	char *irqstackptr;	/* top of irqstack */
 	int nodenumber;		    /* number of current node */
Index: linux-2.6.18-rc2-git5-stackprot/include/linux/sched.h
===================================================================
--- linux-2.6.18-rc2-git5-stackprot.orig/include/linux/sched.h
+++ linux-2.6.18-rc2-git5-stackprot/include/linux/sched.h
@@ -819,6 +819,11 @@ struct task_struct {
 	unsigned did_exec:1;
 	pid_t pid;
 	pid_t tgid;
+
+#ifdef CONFIG_CC_STACKPROTECTOR
+	/* Canary value for the -fstack-protector gcc feature */
+	unsigned long stack_canary;
+#endif
 	/* 
 	 * pointers to (original) parent process, youngest child, younger sibling,
 	 * older sibling, respectively.  (p->father can be replaced with 
Index: linux-2.6.18-rc2-git5-stackprot/kernel/fork.c
===================================================================
--- linux-2.6.18-rc2-git5-stackprot.orig/kernel/fork.c
+++ linux-2.6.18-rc2-git5-stackprot/kernel/fork.c
@@ -174,6 +174,10 @@ static struct task_struct *dup_task_stru
 	tsk->thread_info = ti;
 	setup_thread_stack(tsk, orig);
 
+#ifdef CONFIG_CC_STACKPROTECTOR
+	tsk->stack_canary = get_random_int();
+#endif
+
 	/* One for us, one for whoever does the "release_task()" (usually parent) */
 	atomic_set(&tsk->usage,2);
 	atomic_set(&tsk->fs_excl, 0);

[patch 4/5] Add the __stack_chk_fail() function

[Arjan van de Ven]

Subject: [patch 4/5] Add the __stack_chk_fail() function

GCC emits a call to a __stack_chk_fail() function when the stack canary is
not matching the expected value.

Since this is a bad security issue; lets panic the kernel rather than limping
along; the kernel really can't be trusted anymore when this happens.

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
CC: Andi Kleen <ak@suse.de>

Index: linux-2.6.18-rc2-git5-stackprot/kernel/panic.c
===================================================================
--- linux-2.6.18-rc2-git5-stackprot.orig/kernel/panic.c
+++ linux-2.6.18-rc2-git5-stackprot/kernel/panic.c
@@ -269,3 +269,15 @@ void oops_exit(void)
 {
 	do_oops_enter_exit();
 }
+
+#ifdef CONFIG_CC_STACKPROTECTOR
+/*
+ * Called when gcc's -fstack-protector feature is used, and
+ * gcc detects corruption of the on-stack canary value
+ */
+void __stack_chk_fail(void)
+{
+	panic("stack-protector: Kernel stack is corrupted");
+}
+EXPORT_SYMBOL(__stack_chk_fail);
+#endif

[patch 5/5] Add the -fstack-protector option to the CFLAGS

[Arjan van de Ven]

Subject: [patch 5/5] Add the -fstack-protector option to the CFLAGS
From: Arjan van de Ven <arjan@linux.intel.com>

Add the actual compiler options to the CFLAGS, but only for gcc 4.2
and later. Based on feedback from Sam Ravnborg.

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
CC: Sam Ravnborg <sam@ravnborg.org>
CC: Andi Kleen <ak@suse.de>
--
Index: linux-2.6.18-rc2-git5-stackprot/Makefile
===================================================================
--- linux-2.6.18-rc2-git5-stackprot.orig/Makefile
+++ linux-2.6.18-rc2-git5-stackprot/Makefile
@@ -487,6 +487,18 @@ ifdef CONFIG_DEBUG_INFO
 CFLAGS		+= -g
 endif
 
+ifdef CONFIG_CC_STACKPROTECTOR
+CFLAGS += $(call cc-ifversion, -lt, 0402, -fno-stack-protector)
+CFLAGS += $(call cc-ifversion, -ge, 0402, -fstack-protector)
+else
+CFLAGS += -fno-stack-protector
+endif
+
+ifdef CONFIG_CC_STACKPROTECTOR_ALL
+CFLAGS += $(call cc-ifversion, -ge, 0402, -fstack-protector-all)
+endif
+
+
 include $(srctree)/arch/$(ARCH)/Makefile
 
 # arch Makefile may override CC so keep this after arch Makefile is included

Re: [patch 2/5] Add the Kconfig option for the stackprotector feature

[Daniel Walker]

On Fri, 2006-07-28 at 18:03 +0200, Arjan van de Ven wrote:

> ---
>  arch/x86_64/Kconfig |   25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)

Could this be supported on more than just x86_64, it seems fairly
generic ? 

Daniel

Re: [patch 2/5] Add the Kconfig option for the stackprotector feature

[Arjan van de Ven]

On Fri, 2006-07-28 at 09:24 -0700, Daniel Walker wrote:
> On Fri, 2006-07-28 at 18:03 +0200, Arjan van de Ven wrote:
> 
> > ---
> >  arch/x86_64/Kconfig |   25 +++++++++++++++++++++++++
> >  1 file changed, 25 insertions(+)
> 
> Could this be supported on more than just x86_64, it seems fairly
> generic ? 

I won't rule anything out, but for some it'll be impossible (such as
i386). It'll depend on the exact architecture I suppose.. for x86_64 a
gcc patch was needed (which took MONTHS to get in), I don't know how ppc
and such deal with this, I'll be talking to the respective maintainers
about this in the near future.... but I would not call it "fairly
generic".

Re: [patch 2/5] Add the Kconfig option for the stackprotector feature

[Paweł Sikora]

On Friday 28 July 2006 18:24, Daniel Walker wrote:
> On Fri, 2006-07-28 at 18:03 +0200, Arjan van de Ven wrote:
> > ---
> >  arch/x86_64/Kconfig |   25 +++++++++++++++++++++++++
> >  1 file changed, 25 insertions(+)
>
> Could this be supported on more than just x86_64, it seems fairly
> generic ?

yes, it could.

gcc supports stack protection at so called tree-level (it means
it's architecture-independent). i've just tested a simple userland-code:

#include <stdlib.h>
#include <string.h>
int main()
{
	char c;
	memset( &c, 0, 512 );
	return 0;
}

and stack protection works fine on {ix86,x86-64,powerpc}-linux.
i can test it on {alpha,sparc}-linux later but i'm pretty sure
it'll work too on these archs.

Re: [patch 2/5] Add the Kconfig option for the stackprotector feature

[Arjan van de Ven]

On Fri, 2006-07-28 at 19:13 +0200, Paweł Sikora wrote:
> 
> yes, it could.
> 
> gcc supports stack protection at so called tree-level (it means
> it's architecture-independent). i've just tested a simple
> userland-code:

in userland it works, no question about that.
The problem comes when you want to use this userland abi in kernel
space; for x86-64 it could be done by a 20 line gcc patch, on some other
architectures... it's going to be really incredibly hard.

I'd have loved to make this more generic... but the mechanism behind
this technology on the gcc side is very architecture specific.

Re: [patch 2/5] Add the Kconfig option for the stackprotector feature

[Thierry Vignaud]

Paweł Sikora <pluto@agmk.net> writes:

> gcc supports stack protection at so called tree-level (it means it's
> architecture-independent). i've just tested a simple userland-code:
> 
> #include <stdlib.h>
> #include <string.h>
> int main()
> {
> 	char c;
> 	memset( &c, 0, 512 );
> 	return 0;
> }
> 
> and stack protection works fine on {ix86,x86-64,powerpc}-linux.
> i can test it on {alpha,sparc}-linux later but i'm pretty sure
> it'll work too on these archs.

$ gcc -v
Using built-in specs.
Target: x86_64-mandriva-linux-gnu
Configured with: ../configure --prefix=/usr --libexecdir=/usr/lib --with-slibdir=/lib64 --mandir=/usr/share/man --infodir=/usr/share/info --enable-shared --enable-threads=posix --enable-checking=release --enable-languages=c,c++,ada,fortran,objc,obj-c++,java --host=x86_64-mandriva-linux-gnu --with-cpu=generic --with-system-zlib --enable-long-long --enable-__cxa_atexit --enable-clocale=gnu --disable-libunwind-exceptions --enable-java-awt=gtk --with-java-home=/usr/lib/jvm/java-1.4.2-gcj-1.4.2.0/jre --enable-gtk-cairo --enable-ssp --disable-libssp
Thread model: posix
gcc version 4.1.1 20060724 (prerelease) (4.1.1-3mdk)
$ gcc  -fstack-protector t.c
$ ./a.out                   
zsh: segmentation fault  ./a.out

it segfaults if using "return 0" instead of "exit(0)" and only if
memset overwrote the stack

why? because, according to gcc man page, "This includes functions that
call alloca, and functions with buffers larger than 8 bytes."
once the stack is bigger, it does abort with "*** stack smashing
detected ***: <unknown> terminated" however.

thus this won't protect stacks of small functions... such as your
example...

Re: [patch 2/5] Add the Kconfig option for the stackprotector feature

[Paweł Sikora]

On Friday 28 July 2006 19:56, Thierry Vignaud wrote:

> thus this won't protect stacks of small functions... such as your
> example...

i've tested it with -fstack-protector-all

Re: [patch 1/5] Add comments to the PDA structure to annotate offsets

[Andi Kleen]

On Friday 28 July 2006 18:03, Arjan van de Ven wrote:
> Subject: [patch 1/5] Add comments to the PDA structure to annotate offsets
> From: Arjan van de Ven <arjan@linux.intel.com>

So why exactly do you think these numbers need to be documented?

If there is a reason there should be a comment in the code.

Nobody should use fixed numbers, but always get the current ones
from asm-offset

-Andi

> Change the comments in the pda structu

Re: [patch 2/5] Add the Kconfig option for the stackprotector feature

[Andi Kleen]

On Friday 28 July 2006 18:27, Arjan van de Ven wrote:

> I won't rule anything out, but for some it'll be impossible (such as
> i386). It'll depend on the exact architecture I suppose.. for x86_64 a
> gcc patch was needed 

Did the patch make 4.1.0? 

-Andi

Re: [patch 1/5] Add comments to the PDA structure to annotate offsets

[Arjan van de Ven]

On Fri, 2006-07-28 at 20:41 +0200, Andi Kleen wrote:
> On Friday 28 July 2006 18:03, Arjan van de Ven wrote:
> > Subject: [patch 1/5] Add comments to the PDA structure to annotate offsets
> > From: Arjan van de Ven <arjan@linux.intel.com>
> 
> So why exactly do you think these numbers need to be documented?
> 
> If there is a reason there should be a comment in the code.
> 
> Nobody should use fixed numbers, but always get the current ones
> from asm-offset

the 40 one is a gcc ABI one (same offset as userland); that is
documented in the later patch

Re: [patch 5/5] Add the -fstack-protector option to the CFLAGS

[Andi Kleen]

> +ifdef CONFIG_CC_STACKPROTECTOR
> +CFLAGS += $(call cc-ifversion, -lt, 0402, -fno-stack-protector)
> +CFLAGS += $(call cc-ifversion, -ge, 0402, -fstack-protector)

Why can't you just use the normal call cc-option for this?

-Andi

Re: [patch 5/5] Add the -fstack-protector option to the CFLAGS

[Arjan van de Ven]

On Fri, 2006-07-28 at 20:45 +0200, Andi Kleen wrote:
> > +ifdef CONFIG_CC_STACKPROTECTOR
> > +CFLAGS += $(call cc-ifversion, -lt, 0402, -fno-stack-protector)
> > +CFLAGS += $(call cc-ifversion, -ge, 0402, -fstack-protector)
> 
> Why can't you just use the normal call cc-option for this?

this requires gcc 4.2; cc-option is not useful for that.

Re: [patch 2/5] Add the Kconfig option for the stackprotector feature

[Arjan van de Ven]

On Fri, 2006-07-28 at 20:42 +0200, Andi Kleen wrote:
> On Friday 28 July 2006 18:27, Arjan van de Ven wrote:
> 
> > I won't rule anything out, but for some it'll be impossible (such as
> > i386). It'll depend on the exact architecture I suppose.. for x86_64 a
> > gcc patch was needed 
> 
> Did the patch make 4.1.0? 

nope; 4.2+ only (which is why the kernel Makefile checks for 4.2)

Re: [patch 1/5] Add comments to the PDA structure to annotate offsets

[Andi Kleen]

On Friday 28 July 2006 20:43, Arjan van de Ven wrote:
> On Fri, 2006-07-28 at 20:41 +0200, Andi Kleen wrote:
> > On Friday 28 July 2006 18:03, Arjan van de Ven wrote:
> > > Subject: [patch 1/5] Add comments to the PDA structure to annotate
> > > offsets From: Arjan van de Ven <arjan@linux.intel.com>
> >
> > So why exactly do you think these numbers need to be documented?
> >
> > If there is a reason there should be a comment in the code.
> >
> > Nobody should use fixed numbers, but always get the current ones
> > from asm-offset
>
> the 40 one is a gcc ABI one (same offset as userland); 

Ah sounds ugly. Wasn't it possible to pass that as an option
to gcc?

> that is 
> documented in the later patch

I still hate the numbers. Perhaps do them only before your canary.
Also you should have a BUILD_BUG_ON() for this somewhere

-Andi

Re: [patch 1/5] Add comments to the PDA structure to annotate offsets

[Arjan van de Ven]

On Fri, 2006-07-28 at 20:52 +0200, Andi Kleen wrote:
> On Friday 28 July 2006 20:43, Arjan van de Ven wrote:
> > On Fri, 2006-07-28 at 20:41 +0200, Andi Kleen wrote:
> > > On Friday 28 July 2006 18:03, Arjan van de Ven wrote:
> > > > Subject: [patch 1/5] Add comments to the PDA structure to annotate
> > > > offsets From: Arjan van de Ven <arjan@linux.intel.com>
> > >
> > > So why exactly do you think these numbers need to be documented?
> > >
> > > If there is a reason there should be a comment in the code.
> > >
> > > Nobody should use fixed numbers, but always get the current ones
> > > from asm-offset
> >
> > the 40 one is a gcc ABI one (same offset as userland); 
> 
> Ah sounds ugly. Wasn't it possible to pass that as an option
> to gcc?

nope :(

Re: [patch 5/5] Add the -fstack-protector option to the CFLAGS

[Andi Kleen]

On Friday 28 July 2006 20:48, Arjan van de Ven wrote:
> On Fri, 2006-07-28 at 20:45 +0200, Andi Kleen wrote:
> > > +ifdef CONFIG_CC_STACKPROTECTOR
> > > +CFLAGS += $(call cc-ifversion, -lt, 0402, -fno-stack-protector)
> > > +CFLAGS += $(call cc-ifversion, -ge, 0402, -fstack-protector)
> >
> > Why can't you just use the normal call cc-option for this?
>
> this requires gcc 4.2; cc-option is not useful for that.

This means nearly nobody can use it. The CC option thing is also
very ugly.  Perhaps you really need a Makefile time check like
configure would do for it. Generating a .s and grepping for %gs 
would be enough I guess.

-Andi

Re: [patch 5/5] Add the -fstack-protector option to the CFLAGS

[Arjan van de Ven]

On Fri, 2006-07-28 at 21:00 +0200, Andi Kleen wrote:
> On Friday 28 July 2006 20:48, Arjan van de Ven wrote:
> > On Fri, 2006-07-28 at 20:45 +0200, Andi Kleen wrote:
> > > > +ifdef CONFIG_CC_STACKPROTECTOR
> > > > +CFLAGS += $(call cc-ifversion, -lt, 0402, -fno-stack-protector)
> > > > +CFLAGS += $(call cc-ifversion, -ge, 0402, -fstack-protector)
> > >
> > > Why can't you just use the normal call cc-option for this?
> >
> > this requires gcc 4.2; cc-option is not useful for that.
> 
> This means nearly nobody can use it.

... today.

>  The CC option thing is also
> very ugly. 

It's what Sam suggested as kbuild guy.

>  Perhaps you really need a Makefile time check like
> configure would do for it. Generating a .s and grepping for %gs 
> would be enough I guess.
> 


Sam?

Re: [patch 1/5] Add comments to the PDA structure to annotate offsets

[Arjan van de Ven]

Andi Kleen wrote:
>> that is 
>> documented in the later patch
> 
> I still hate the numbers. Perhaps do them only before your canary.

that's what the patch does

> Also you should have a BUILD_BUG_ON() for this somewhere

fair enough

Re: [patch 5/5] Add the -fstack-protector option to the CFLAGS

[Sam Ravnborg]

On Fri, Jul 28, 2006 at 09:00:01PM +0200, Andi Kleen wrote:
> On Friday 28 July 2006 20:48, Arjan van de Ven wrote:
> > On Fri, 2006-07-28 at 20:45 +0200, Andi Kleen wrote:
> > > > +ifdef CONFIG_CC_STACKPROTECTOR
> > > > +CFLAGS += $(call cc-ifversion, -lt, 0402, -fno-stack-protector)
> > > > +CFLAGS += $(call cc-ifversion, -ge, 0402, -fstack-protector)
> > >
> > > Why can't you just use the normal call cc-option for this?
> >
> > this requires gcc 4.2; cc-option is not useful for that.
> 
> The CC option thing is also very ugly.
The check is executed once pr. kernel compile - or at least once pr.
line. The reson to use cc-ifversion is that we need to check for a
specific gcc version and not just support for a specific argument type.

That said - checking for a version is not as reliable as checking if a
certain feature is really supported but Arjan suggested testing for
version >= 4.2 should do it.
Also we do not have any helpers in kbuild to do so -that could be worked
out so we could do something almost as elegant as 
$(call cc-ifversion ...)

	Sam

Re: [patch 5/5] Add the -fstack-protector option to the CFLAGS

[Arjan van de Ven]

On Fri, 2006-07-28 at 23:26 +0200, Sam Ravnborg wrote:
> On Fri, Jul 28, 2006 at 09:00:01PM +0200, Andi Kleen wrote:
> > On Friday 28 July 2006 20:48, Arjan van de Ven wrote:
> > > On Fri, 2006-07-28 at 20:45 +0200, Andi Kleen wrote:
> > > > > +ifdef CONFIG_CC_STACKPROTECTOR
> > > > > +CFLAGS += $(call cc-ifversion, -lt, 0402, -fno-stack-protector)
> > > > > +CFLAGS += $(call cc-ifversion, -ge, 0402, -fstack-protector)
> > > >
> > > > Why can't you just use the normal call cc-option for this?
> > >
> > > this requires gcc 4.2; cc-option is not useful for that.
> > 
> > The CC option thing is also very ugly.
> The check is executed once pr. kernel compile - or at least once pr.
> line. The reson to use cc-ifversion is that we need to check for a
> specific gcc version and not just support for a specific argument type.
> 
> That said - checking for a version is not as reliable as checking if a
> certain feature is really supported but Arjan suggested testing for
> version >= 4.2 should do it.


it's not hard to run a shell script that returns supported or not. I can
do the shell script no problem... but I would prefer that you then do
the Makefile foo for it :)
Would that work?

Re: [patch 5/5] Add the -fstack-protector option to the CFLAGS

[Sam Ravnborg]

On Fri, Jul 28, 2006 at 11:40:45PM +0200, Arjan van de Ven wrote:
> On Fri, 2006-07-28 at 23:26 +0200, Sam Ravnborg wrote:
> > On Fri, Jul 28, 2006 at 09:00:01PM +0200, Andi Kleen wrote:
> > > On Friday 28 July 2006 20:48, Arjan van de Ven wrote:
> > > > On Fri, 2006-07-28 at 20:45 +0200, Andi Kleen wrote:
> > > > > > +ifdef CONFIG_CC_STACKPROTECTOR
> > > > > > +CFLAGS += $(call cc-ifversion, -lt, 0402, -fno-stack-protector)
> > > > > > +CFLAGS += $(call cc-ifversion, -ge, 0402, -fstack-protector)
> > > > >
> > > > > Why can't you just use the normal call cc-option for this?
> > > >
> > > > this requires gcc 4.2; cc-option is not useful for that.
> > > 
> > > The CC option thing is also very ugly.
> > The check is executed once pr. kernel compile - or at least once pr.
> > line. The reson to use cc-ifversion is that we need to check for a
> > specific gcc version and not just support for a specific argument type.
> > 
> > That said - checking for a version is not as reliable as checking if a
> > certain feature is really supported but Arjan suggested testing for
> > version >= 4.2 should do it.
> 
> 
> it's not hard to run a shell script that returns supported or not. I can
> do the shell script no problem... but I would prefer that you then do
> the Makefile foo for it :)
> Would that work?
Yep - no problem. If you give me a day or two to do it.

	Sam

Re: [patch 5/5] Add the -fstack-protector option to the CFLAGS

[Arjan van de Ven]

On Fri, 2006-07-28 at 23:58 +0200, Sam Ravnborg wrote:
> On Fri, Jul 28, 2006 at 11:40:45PM +0200, Arjan van de Ven wrote:
> > On Fri, 2006-07-28 at 23:26 +0200, Sam Ravnborg wrote:
> > > On Fri, Jul 28, 2006 at 09:00:01PM +0200, Andi Kleen wrote:
> > > > On Friday 28 July 2006 20:48, Arjan van de Ven wrote:
> > > > > On Fri, 2006-07-28 at 20:45 +0200, Andi Kleen wrote:
> > > > > > > +ifdef CONFIG_CC_STACKPROTECTOR
> > > > > > > +CFLAGS += $(call cc-ifversion, -lt, 0402, -fno-stack-protector)
> > > > > > > +CFLAGS += $(call cc-ifversion, -ge, 0402, -fstack-protector)
> > > > > >
> > > > > > Why can't you just use the normal call cc-option for this?
> > > > >
> > > > > this requires gcc 4.2; cc-option is not useful for that.
> > > > 
> > > > The CC option thing is also very ugly.
> > > The check is executed once pr. kernel compile - or at least once pr.
> > > line. The reson to use cc-ifversion is that we need to check for a
> > > specific gcc version and not just support for a specific argument type.
> > > 
> > > That said - checking for a version is not as reliable as checking if a
> > > certain feature is really supported but Arjan suggested testing for
> > > version >= 4.2 should do it.
> > 
> > 
> > it's not hard to run a shell script that returns supported or not. I can
> > do the shell script no problem... but I would prefer that you then do
> > the Makefile foo for it :)
> > Would that work?
> Yep - no problem. If you give me a day or two to do it.

sure no problem.

the following line is enough actually:

echo "int foo(void) { char X[200]; return 3; }" | gcc -S -xc -c -O0 -mcmodel=kernel -fstack-protector - -o - | grep -q "%gs"

echo $? (eg return value) gives 0 for the "works" case, "1" for the
"wrong gcc" case...

Re: [patch 5/5] Add the -fstack-protector option to the CFLAGS

[Valdis.Kletnieks]

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

On Fri, 28 Jul 2006 20:48:31 +0200, Arjan van de Ven said:
> On Fri, 2006-07-28 at 20:45 +0200, Andi Kleen wrote:
> > > +ifdef CONFIG_CC_STACKPROTECTOR
> > > +CFLAGS += $(call cc-ifversion, -lt, 0402, -fno-stack-protector)
> > > +CFLAGS += $(call cc-ifversion, -ge, 0402, -fstack-protector)
> > 
> > Why can't you just use the normal call cc-option for this?
> 
> this requires gcc 4.2; cc-option is not useful for that.

At least some things calling themselves 4.1.1 include it.  On a Fedora
Rawhide box:

% gcc -v -fstack-protector hello.c
Using built-in specs.
Target: i386-redhat-linux
Configured with: ../configure --prefix=/usr --mandir=/usr/share/man --infodir=/usr/share/info --enable-shared --enable-threads=posix --enable-checking=release --with-system-zlib --enable-__cxa_atexit --disable-libunwind-exceptions --enable-libgcj-multifile --enable-languages=c,c++,objc,obj-c++,java,fortran,ada --enable-java-awt=gtk --disable-dssi --enable-plugin --with-java-home=/usr/lib/jvm/java-1.4.2-gcj-1.4.2.0/jre --with-cpu=generic --host=i386-redhat-linux
Thread model: posix
gcc version 4.1.1 20060721 (Red Hat 4.1.1-13)
 /usr/libexec/gcc/i386-redhat-linux/4.1.1/cc1 -quiet -v hello.c -quiet -dumpbase hello.c -mtune=generic -auxbase hello -version -fstack-protector -o /tmp/ccLfn4gs.s
ignoring nonexistent directory "/usr/lib/gcc/i386-redhat-linux/4.1.1/../../../../i386-redhat-linux/include"
#include "..." search starts here:
#include <...> search starts here:
 /usr/local/include
 /usr/lib/gcc/i386-redhat-linux/4.1.1/include
 /usr/include
End of search list.
GNU C version 4.1.1 20060721 (Red Hat 4.1.1-13) (i386-redhat-linux)
        compiled by GNU C version 4.1.1 20060721 (Red Hat 4.1.1-13).
GGC heuristics: --param ggc-min-expand=81 --param ggc-min-heapsize=96852
Compiler executable checksum: cbc4b2991046c2b178d1ad5878ca2677
hello.c: In function 'main':
hello.c:1: warning: incompatible implicit declaration of built-in function 'printf'
 as -V -Qy -o /tmp/cceHv3eO.o /tmp/ccLfn4gs.s
GNU assembler version 2.17.50.0.3-1 (i386-redhat-linux) using BFD version 2.17.50.0.3-1 20060715
 /usr/libexec/gcc/i386-redhat-linux/4.1.1/collect2 --eh-frame-hdr -m elf_i386 --hash-style=gnu -dynamic-linker /lib/ld-linux.so.2 /usr/lib/gcc/i386-redhat-linux/4.1.1/../../../crt1.o /usr/lib/gcc/i386-redhat-linux/4.1.1/../../../crti.o /usr/lib/gcc/i386-redhat-linux/4.1.1/crtbegin.o -L/usr/lib/gcc/i386-redhat-linux/4.1.1 -L/usr/lib/gcc/i386-redhat-linux/4.1.1 -L/usr/lib/gcc/i386-redhat-linux/4.1.1/../../.. /tmp/cceHv3eO.o -lgcc --as-needed -lgcc_s --no-as-needed -lc -lgcc --as-needed -lgcc_s --no-as-needed /usr/lib/gcc/i386-redhat-linux/4.1.1/crtend.o /usr/lib/gcc/i386-redhat-linux/4.1.1/../../../crtn.o

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

Re: [patch 5/5] Add the -fstack-protector option to the CFLAGS

[David Miller]

From: Valdis.Kletnieks@vt.edu
Date: Fri, 28 Jul 2006 19:05:40 -0400

> On Fri, 28 Jul 2006 20:48:31 +0200, Arjan van de Ven said:
> > On Fri, 2006-07-28 at 20:45 +0200, Andi Kleen wrote:
> > > > +ifdef CONFIG_CC_STACKPROTECTOR
> > > > +CFLAGS += $(call cc-ifversion, -lt, 0402, -fno-stack-protector)
> > > > +CFLAGS += $(call cc-ifversion, -ge, 0402, -fstack-protector)
> > > 
> > > Why can't you just use the normal call cc-option for this?
> > 
> > this requires gcc 4.2; cc-option is not useful for that.
> 
> At least some things calling themselves 4.1.1 include it.  On a Fedora

Your gcc-4.1.1 includes the -fstack-protector feature, but it might
not have the gcc bug fix necessary to make that feature work on the
kernel compile, which is why the version check is necessary.

Re: [patch 5/5] Add the -fstack-protector option to the CFLAGS

[Valdis.Kletnieks]

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

On Fri, 28 Jul 2006 16:12:15 PDT, David Miller said:

> Your gcc-4.1.1 includes the -fstack-protector feature, but it might
> not have the gcc bug fix necessary to make that feature work on the
> kernel compile, which is why the version check is necessary.

Whee.  A busticated feature - how annoying.

Do you happen to know the exact PR# for that one?  Looking at the gcc RPM
changelog, there's a *lot* of backported fixes in the Fedora compiler, so it
may in fact be in there already.  I'm mentioning this mostly as a practical
"increase the number of testers" - as far as I can tell, what will ship in
Fedora Core 6 is going to call itself gcc 4.1.1, and I'm pretty sure I'm not
the only person who isn't ambitious enough to build a whole new gcc just to
test this.  So a lot of people won't be able to easily use this until FC7.

Having said that, I have *no* idea how best to code "gcc 4.2 or patched Fedora
4.1.1"...


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

Re: [patch 5/5] Add the -fstack-protector option to the CFLAGS

[Arjan van de Ven]

Valdis.Kletnieks@vt.edu wrote:
> On Fri, 28 Jul 2006 16:12:15 PDT, David Miller said:
> 
>> Your gcc-4.1.1 includes the -fstack-protector feature, but it might
>> not have the gcc bug fix necessary to make that feature work on the
>> kernel compile, which is why the version check is necessary.
> 
> Whee.  A busticated feature - how annoying.

actually it's the kernel that has a differnet ABI than userspace, so it's not entirely gcc
that is to blame.

> 
> Do you happen to know the exact PR# for that one? 

it's gcc PR 28281

Re: [patch 2/5] Add the Kconfig option for the stackprotector feature

[Adrian Bunk]

On Fri, Jul 28, 2006 at 06:03:46PM +0200, Arjan van de Ven wrote:
>...
> --- linux-2.6.18-rc2-git5-stackprot.orig/arch/x86_64/Kconfig
> +++ linux-2.6.18-rc2-git5-stackprot/arch/x86_64/Kconfig
> @@ -522,6 +522,31 @@ config SECCOMP
>  
>  	  If unsure, say Y. Only embedded should say N here.
>  
> +config CC_STACKPROTECTOR
> +	bool "Enable -fstack-protector buffer overflow detection (EXPRIMENTAL)"
> +	depends on EXPERIMENTAL
> +	default n

You can remove the "default n".

> +	help
> +	  This option turns on the -fstack-protector GCC feature that is new
> +	  in GCC version 4.1. This feature puts, at the beginning of
> +	  critical functions, a canary value on the stack just before the return
> +	  address, and validates the value just before actually returning.
> +	  Stack based buffer overflows that need to overwrite this return
> +	  address now also overwrite the canary, which gets detected.
> +
> +	  NOTE 
> +	  This feature requires gcc version 4.2 or above, or a distribution
> +	  gcc with the feature backported. For older gcc versions, this is a NOP.

After reading this thread, I do understand why you write once 
"GCC version 4.1" and once "gcc version 4.2".

But for the normal user this will be quite confusing.

What about simply removing the first sentence of the help text since 
it's anyway handled by the NOTE?

> +config CC_STACKPROTECTOR_ALL
> +	bool "Use stack-protector for all functions"
> +	depends on CC_STACKPROTECTOR
> +	default n

You can remove the "default n".

> +	help
> +	  Normally, GCC only inserts the canary value protection for
> +	  functions that use large-ish on-stack buffers. By enabling
> +	  this option, GCC will be asked to do this for ALL functions.
> +

cu
Adrian

-- 

    Gentoo kernels are 42 times more popular than SUSE kernels among
    KLive users  (a service by SUSE contractor Andrea Arcangeli that
    gathers data about kernels from many users worldwide).

       There are three kinds of lies: Lies, Damn Lies, and Statistics.
                                                    Benjamin Disraeli

Re: [patch 2/5] Add the Kconfig option for the stackprotector feature

[Andi Kleen]

> After reading this thread, I do understand why you write once 
> "GCC version 4.1" and once "gcc version 4.2".
> 
> But for the normal user this will be quite confusing.

Yes it's a mess.

> What about simply removing the first sentence of the help text since 
> it's anyway handled by the NOTE?

It should be obsolete with autoprobing for the feature as earlier discussed.

-Andi

Re: [patch 2/5] Add the Kconfig option for the stackprotector feature

[Adrian Bunk]

On Sat, Jul 29, 2006 at 08:50:37PM +0200, Andi Kleen wrote:
> 
> > After reading this thread, I do understand why you write once 
> > "GCC version 4.1" and once "gcc version 4.2".
> > 
> > But for the normal user this will be quite confusing.
> 
> Yes it's a mess.
> 
> > What about simply removing the first sentence of the help text since 
> > it's anyway handled by the NOTE?
> 
> It should be obsolete with autoprobing for the feature as earlier discussed.

That's not the point of the version information in the help text.

The point of the version information in the help text is to inform the 
user that the option has zero effect with older compilers.

> -Andi

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [patch 2/5] Add the Kconfig option for the stackprotector feature

[Andi Kleen]

> > It should be obsolete with autoprobing for the feature as earlier discussed.
> 
> That's not the point of the version information in the help text.

The point in the current option is to select or not select it - 
if the user gets it wrong it won't compile or worse miscompile.

Once it is auto selected the user could be still informed about 
it, but it doesn't matter much anymore (we don't inform the user
about every possible trade off based on compiler version everywhere)

-Andi

Re: [patch 2/5] Add the Kconfig option for the stackprotector feature

[Adrian Bunk]

On Sat, Jul 29, 2006 at 09:04:18PM +0200, Andi Kleen wrote:
> 
> > > It should be obsolete with autoprobing for the feature as earlier discussed.
> > 
> > That's not the point of the version information in the help text.
> 
> The point in the current option is to select or not select it - 
> if the user gets it wrong it won't compile or worse miscompile.

That was never true in Arjan's patches.

The only change is from a gcc version check to a feature check.

In both cases, a gcc 4.1 without the appropriate patch applied will 
result in this option not being set.

> Once it is auto selected the user could be still informed about 
> it, but it doesn't matter much anymore (we don't inform the user
> about every possible trade off based on compiler version everywhere)

If an option might possible have zero effect, we do always inform the 
user. If not, please tell me which options this are so we can fix them.

We don't inform users about internal compiler version dependent things 
like -fno-unit-at-a-time on i386 with neither a config option nor any 
user visible effect (except for kernel size and speed).

> -Andi

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [patch 2/5] Add the Kconfig option for the stackprotector feature

[Valdis.Kletnieks]

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

On Sat, 29 Jul 2006 21:19:38 +0200, Adrian Bunk said:

> That was never true in Arjan's patches.
> 
> The only change is from a gcc version check to a feature check.
> 
> In both cases, a gcc 4.1 without the appropriate patch applied will 
> result in this option not being set.

What do you get if you have a gcc 4.1.1. that has the stack protector option
(so a feature check works), but not the fix for gcc PR 28281?

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

Re: [patch 2/5] Add the Kconfig option for the stackprotector feature

[Adrian Bunk]

On Sun, Jul 30, 2006 at 12:14:51PM -0400, Valdis.Kletnieks@vt.edu wrote:
> On Sat, 29 Jul 2006 21:19:38 +0200, Adrian Bunk said:
> 
> > That was never true in Arjan's patches.
> > 
> > The only change is from a gcc version check to a feature check.
> > 
> > In both cases, a gcc 4.1 without the appropriate patch applied will 
> > result in this option not being set.
> 
> What do you get if you have a gcc 4.1.1. that has the stack protector option
> (so a feature check works), but not the fix for gcc PR 28281?

This is handled correctly in both cases.

Please read the patches in this thread for more information.

cu
Adrian

-- 

    Gentoo kernels are 42 times more popular than SUSE kernels among
    KLive users  (a service by SUSE contractor Andrea Arcangeli that
    gathers data about kernels from many users worldwide).

       There are three kinds of lies: Lies, Damn Lies, and Statistics.
                                                    Benjamin Disraeli

Re: [patch 2/5] Add the Kconfig option for the stackprotector feature

[Arjan van de Ven]

Valdis.Kletnieks@vt.edu wrote:
> On Sat, 29 Jul 2006 21:19:38 +0200, Adrian Bunk said:
> 
>> That was never true in Arjan's patches.
>>
>> The only change is from a gcc version check to a feature check.
>>
>> In both cases, a gcc 4.1 without the appropriate patch applied will 
>> result in this option not being set.
> 
> What do you get if you have a gcc 4.1.1. that has the stack protector option
> (so a feature check works), but not the fix for gcc PR 28281?

the feature check actually checks for a correctly operating gcc, not just for the "option exists"

Re: [patch 2/5] Add the Kconfig option for the stackprotector feature

[Valdis.Kletnieks]

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

On Sun, 30 Jul 2006 18:49:38 +0200, Adrian Bunk said:
> On Sun, Jul 30, 2006 at 12:14:51PM -0400, Valdis.Kletnieks@vt.edu wrote:
> > On Sat, 29 Jul 2006 21:19:38 +0200, Adrian Bunk said:
> > 
> > > That was never true in Arjan's patches.
> > > 
> > > The only change is from a gcc version check to a feature check.
> > > 
> > > In both cases, a gcc 4.1 without the appropriate patch applied will 
> > > result in this option not being set.
> > 
> > What do you get if you have a gcc 4.1.1. that has the stack protector option
> > (so a feature check works), but not the fix for gcc PR 28281?
> 
> This is handled correctly in both cases.
> 
> Please read the patches in this thread for more information.

Patches? I read the *patches*. :)  What I missed was this:

http://marc.theaimsgroup.com/?l=linux-kernel&m=115412601229175&w=2

was the only thing I found (over in the 5/5 thread) that remotely looked
like an actual workable test, and all Arjan said was:

> the following line is enough actually:

> echo "int foo(void) { char X[200]; return 3; }" | gcc -S -xc -c -O0 -mcmodel=kernel \ -fstack-protector - -o - | grep -q "%gs"

> echo $? (eg return value) gives 0 for the "works" case, "1" for the
> "wrong gcc" case...

I admit missing that one, because it wasn't actually a patch, but a commentary
I managed to not read and digest in detail (in particular, it wasn't at all
clear that his one-liner would DTRT re: PR28281...)


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