[Patch 0/8] Port of -fstack-protector to the kernel

[Patch 0/8] Port of -fstack-protector to the kernel

[Arjan van de Ven]

This patch series adds support for the gcc 4.1 -fstack-protector feature to
the kernel. Unfortunately this needs a gcc patch before it can work, so at
this point these patches are just for comment, not for merging.

-fstack-protector is a security feature in gcc that causes "selected" functions
to store a special "canary" value at the start of the function, just below
the return address. At the end of the function, just before using this
return address with the "ret" instruction, this canary value is compared to
the reference value again. If the value of the stack canary has changed, it is a sign
that there has been some stack corruption (most likely due to a buffer overflow) that
has compromised the integrity of the return address.

Standard, the "selected" functions are those that actually have stack
buffers of at least 8 bytes, this selection is done to limit the overhead to
only those functions with the highest risk potential. There is an override to enable this
for all functions.

On first sight this would not be needed for the kernel, because the kernel
is "perfect" and "has no buffer overflows on the stack". I thought that too
for a long time, but the last year has shown a few cases where that would
have been overly naive.

[Patch 1 of 8] Pack the x86-64 PDA structure

[Arjan van de Ven]

This patch reorders the PDA on x86-64 such that there is optimal packing of the data structures
(patch previously sent to Andi already)

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
---
 include/asm-x86_64/pda.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6.16-rc6-stack-protector/include/asm-x86_64/pda.h
===================================================================
--- linux-2.6.16-rc6-stack-protector.orig/include/asm-x86_64/pda.h
+++ linux-2.6.16-rc6-stack-protector/include/asm-x86_64/pda.h
@@ -22,8 +22,8 @@ struct x8664_pda {
 	int nodenumber;		    /* number of current node */
 	unsigned int __softirq_pending;
 	unsigned int __nmi_count;	/* number of NMI on this CPUs */
-	struct mm_struct *active_mm;
 	int mmu_state;     
+	struct mm_struct *active_mm;
 	unsigned apic_timer_irqs;
 } ____cacheline_aligned_in_smp;
 

[Patch 3 of 8] Introduce a config option for stack-protector

[Arjan van de Ven]

This patch adds the config options for -fstack-protector

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>

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

Index: linux-2.6.16-rc6-stack-protector/arch/x86_64/Kconfig
===================================================================
--- linux-2.6.16-rc6-stack-protector.orig/arch/x86_64/Kconfig
+++ linux-2.6.16-rc6-stack-protector/arch/x86_64/Kconfig
@@ -462,6 +462,31 @@ config SECCOMP
 
 	  If unsure, say Y. Only embedded should say N here.
 
+config STACK_PROTECTOR
+	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 NOTE NOTE
+	  At this point this requires a special, patched GCC compiler!
+	  Do not enable this unless you are using such a compiler.
+
+config STACK_PROTECTOR_ALL
+	bool "Use stack-protector for all functions"
+	depends on STACK_PROTECTOR
+	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
 
 endmenu

[Patch 2 of 8] annotate the PDA structure with offsets

[Arjan van de Ven]

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>
---
 include/asm-x86_64/pda.h |   14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

Index: linux-2.6.16-rc6-stack-protector/include/asm-x86_64/pda.h
===================================================================
--- linux-2.6.16-rc6-stack-protector.orig/include/asm-x86_64/pda.h
+++ linux-2.6.16-rc6-stack-protector/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 */

Re: [Patch 2 of 8] annotate the PDA structure with offsets

[Ingo Molnar]

* Arjan van de Ven <arjan@linux.intel.com> wrote:

> 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

i think offset 40 should be build-time checked - then you dont have to 
do this annotation.

	Ingo 

Re: [Patch 2 of 8] annotate the PDA structure with offsets

[Arjan van de Ven]

Ingo Molnar wrote:
> * Arjan van de Ven <arjan@linux.intel.com> wrote:
> 
>> 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
> 
> i think offset 40 should be build-time checked - then you dont have to 
> do this annotation.
> 

ok good idea; I'll investigate

[Patch 4 of 8] Add the cookie field

[Arjan van de Ven]

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>

---
 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.16-rc6-stack-protector/arch/x86_64/kernel/process.c
===================================================================
--- linux-2.6.16-rc6-stack-protector.orig/arch/x86_64/kernel/process.c
+++ linux-2.6.16-rc6-stack-protector/arch/x86_64/kernel/process.c
@@ -593,6 +593,9 @@ __switch_to(struct task_struct *prev_p, 
 	write_pda(pcurrent, next_p); 
 	write_pda(kernelstack,
 		  task_stack_page(next_p) + THREAD_SIZE - PDA_STACKOFFSET);
+#ifdef CONFIG_STACK_PROTECTOR
+	write_pda(stack_canary, next_p->stack_canary);
+#endif
 
 	/*
 	 * Now maybe reload the debug registers
Index: linux-2.6.16-rc6-stack-protector/include/asm-x86_64/pda.h
===================================================================
--- linux-2.6.16-rc6-stack-protector.orig/include/asm-x86_64/pda.h
+++ linux-2.6.16-rc6-stack-protector/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_STACK_PROTECTOR
+	unsigned long stack_canary;	/* 40 */  /* stack canary value */
+					/* Note: 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.16-rc6-stack-protector/include/linux/sched.h
===================================================================
--- linux-2.6.16-rc6-stack-protector.orig/include/linux/sched.h
+++ linux-2.6.16-rc6-stack-protector/include/linux/sched.h
@@ -740,6 +740,11 @@ struct task_struct {
 	unsigned did_exec:1;
 	pid_t pid;
 	pid_t tgid;
+
+#ifdef CONFIG_STACK_PROTECTOR
+	/* 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.16-rc6-stack-protector/kernel/fork.c
===================================================================
--- linux-2.6.16-rc6-stack-protector.orig/kernel/fork.c
+++ linux-2.6.16-rc6-stack-protector/kernel/fork.c
@@ -178,6 +178,10 @@ static struct task_struct *dup_task_stru
 	tsk->thread_info = ti;
 	setup_thread_stack(tsk, orig);
 
+#ifdef CONFIG_STACK_PROTECTOR
+	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 5 of 8] Add the __stack_chk_fail() function

[Arjan van de Ven]

GCC emits a call to a __stack_chk_fail() function when the cookie is not 
matching the expected value. Since this is a bad security issue; lets panic
the kernel

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>

---
 kernel/panic.c |    8 ++++++++
 1 file changed, 8 insertions(+)

Index: linux-2.6.16-rc6-stack-protector/kernel/panic.c
===================================================================
--- linux-2.6.16-rc6-stack-protector.orig/kernel/panic.c
+++ linux-2.6.16-rc6-stack-protector/kernel/panic.c
@@ -174,3 +174,11 @@ void add_taint(unsigned flag)
 	tainted |= flag;
 }
 EXPORT_SYMBOL(add_taint);
+
+#ifdef CONFIG_STACK_PROTECTOR
+void __stack_chk_fail(void)
+{
+	panic("stack-protector: Stack is corrupted\n");
+}
+EXPORT_SYMBOL(__stack_chk_fail);
+#endif

Re: [Patch 5 of 8] Add the __stack_chk_fail() function

[Nix]

On 17 Mar 2006, Arjan van de Ven wrote:
> GCC emits a call to a __stack_chk_fail() function when the cookie is not 
> matching the expected value. Since this is a bad security issue; lets panic
> the kernel

This turns even minor buffer overflows into complete denials of service.
If we're running in process context and the process is currently
killable it might make more sense to printk() a message and zap the
process; that way we only halt whatever service it is the attacker
hit us through.

(I agree that this is much-needed: I'm doing the rough equivalent in UML
right now, where it's a good bit simpler, but having it for the real
kernel on bare metal will be great!)

-- 
`Come now, you should know that whenever you plan the duration of your
 unplanned downtime, you should add in padding for random management
 freakouts.'

Re: [Patch 5 of 8] Add the __stack_chk_fail() function

[Arjan van de Ven]

Nix wrote:
> On 17 Mar 2006, Arjan van de Ven wrote:
>> GCC emits a call to a __stack_chk_fail() function when the cookie is not 
>> matching the expected value. Since this is a bad security issue; lets panic
>> the kernel
> 
> This turns even minor buffer overflows into complete denials of service.

only those who otherwise would get to the return address. So it turns a "own the machine" into a panic.
Not a "no side effects" thing....


> If we're running in process context and the process is currently
> killable it might make more sense to printk() a message and zap the
> process; that way we only halt whatever service it is the attacker
> hit us through.

maybe. The big question is if you can still trust the machine. That is highly questionable...
(and to kill the process you again need to trust bits of the stack, to get to current for example;
and you just found that the stack was compromised)

Re: [Patch 5 of 8] Add the __stack_chk_fail() function

[Nix]

On Sun, 19 Mar 2006, Arjan van de Ven yowled:
> Nix wrote:
>> On 17 Mar 2006, Arjan van de Ven wrote:
>>> GCC emits a call to a __stack_chk_fail() function when the cookie is not matching the expected value. Since this is a bad
>>> security issue; lets panic
>>> the kernel
>> This turns even minor buffer overflows into complete denials of service.
> 
> only those who otherwise would get to the return address. So it turns a "own the machine" into a panic.
> Not a "no side effects" thing....

True. Also...

> maybe. The big question is if you can still trust the machine. That is highly questionable...
> (and to kill the process you again need to trust bits of the stack, to get to current for example;
> and you just found that the stack was compromised)

... that's true, and furthermore it allows the attack vector of
`compromise global state, then allow this process to die and allow
the global compromise to take over the box'.

Possibly for UML images you can core dump the entire UML for later
analysis. Can't do that with Xen, though, unless we can be sure that a
compromised guest can't affect any other guests or the hypervisor (which
we know to be untrue in the case of at least *one[ guest).

-- 
`Come now, you should know that whenever you plan the duration of your
 unplanned downtime, you should add in padding for random management
 freakouts.'

[Patch 6 of 8] Implement the CFLAGs side

[Arjan van de Ven]

Add the actual compiler options to the CFLAGS

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
---
 arch/x86_64/Makefile |    2 ++
 1 file changed, 2 insertions(+)

Index: linux-2.6.16-rc6-stack-protector/arch/x86_64/Makefile
===================================================================
--- linux-2.6.16-rc6-stack-protector.orig/arch/x86_64/Makefile
+++ linux-2.6.16-rc6-stack-protector/arch/x86_64/Makefile
@@ -29,6 +29,8 @@ CHECKFLAGS      += -D__x86_64__ -m64
 
 cflags-$(CONFIG_MK8) += $(call cc-option,-march=k8)
 cflags-$(CONFIG_MPSC) += $(call cc-option,-march=nocona)
+cflags-$(CONFIG_STACK_PROTECTOR) += $(call cc-option, -fstack-protector)
+cflags-$(CONFIG_STACK_PROTECTOR_ALL) += $(call cc-option, -fstack-protector-all)
 CFLAGS += $(cflags-y)
 
 CFLAGS += -m64

[Patch 7 of 8] Finish PDA offset annotations

[Arjan van de Ven]

Finish annotating the PDA members with offsets

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
---
 include/asm-x86_64/pda.h |   16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

Index: linux-2.6.16-rc6-stack-protector/include/asm-x86_64/pda.h
===================================================================
--- linux-2.6.16-rc6-stack-protector.orig/include/asm-x86_64/pda.h
+++ linux-2.6.16-rc6-stack-protector/include/asm-x86_64/pda.h
@@ -19,14 +19,14 @@ struct x8664_pda {
 					/* Note: 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 */
-	unsigned int __softirq_pending;
-	unsigned int __nmi_count;	/* number of NMI on this CPUs */
-	int mmu_state;     
-	struct mm_struct *active_mm;
-	unsigned apic_timer_irqs;
+	int cpunumber;			/* 52 */  /* Logical CPU number */
+	char *irqstackptr;		/* 56 */  /* top of irqstack */
+	int nodenumber;			/* 64 */  /* number of current node */
+	unsigned int __softirq_pending; /* 68 */
+	unsigned int __nmi_count;	/* 72 */  /* number of NMI on this CPUs */
+	int mmu_state;			/* 76 */
+	struct mm_struct *active_mm;	/* 80 */
+	unsigned apic_timer_irqs;	/* 88 */
 } ____cacheline_aligned_in_smp;
 
 extern struct x8664_pda *_cpu_pda[];

[Patch 8 of 8] GCC 4.1 patch for kernel stack-protector

[Arjan van de Ven]

This patch to gcc 4.1 makes the kernel use the gs segment for the canary value, rather than 
the customary fs segment for userspace

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>

--- gcc-4.1/gcc/config/i386/i386.md.org	2006-03-01 20:08:20.000000000 +0100
+++ gcc-4.1/gcc/config/i386/i386.md	2006-03-01 21:20:36.000000000 +0100
@@ -146,6 +146,9 @@
    (UNSPEC_SP_TEST		101)
    (UNSPEC_SP_TLS_SET		102)
    (UNSPEC_SP_TLS_TEST		103)
+   (UNSPEC_SP_TLS_SET_KERNEL	104)
+   (UNSPEC_SP_TLS_TEST_KERNEL	105)
+ 
   ])
 
 (define_constants
@@ -20138,10 +20141,14 @@
   ""
 {
 #ifdef TARGET_THREAD_SSP_OFFSET
-  if (TARGET_64BIT)
-    emit_insn (gen_stack_tls_protect_set_di (operands[0],
+  if (TARGET_64BIT) {
+	if (ix86_cmodel==CM_KERNEL) 
+	    emit_insn (gen_stack_tls_protect_set_di_kernel (operands[0],
+					GEN_INT (TARGET_THREAD_SSP_OFFSET)));	
+	else
+	    emit_insn (gen_stack_tls_protect_set_di (operands[0],
 					GEN_INT (TARGET_THREAD_SSP_OFFSET)));
-  else
+  } else
     emit_insn (gen_stack_tls_protect_set_si (operands[0],
 					GEN_INT (TARGET_THREAD_SSP_OFFSET)));
 #else
@@ -20189,6 +20196,15 @@
   "mov{q}\t{%%fs:%P1, %2|%2, QWORD PTR %%fs:%P1}\;mov{q}\t{%2, %0|%0, %2}\;xor{l}\t%k2, %k2"
   [(set_attr "type" "multi")])
 
+(define_insn "stack_tls_protect_set_di_kernel"
+  [(set (match_operand:DI 0 "memory_operand" "=m")
+	(unspec:DI [(match_operand:DI 1 "const_int_operand" "i")] UNSPEC_SP_TLS_SET_KERNEL))
+   (set (match_scratch:DI 2 "=&r") (const_int 0))
+   (clobber (reg:CC FLAGS_REG))]
+  "TARGET_64BIT"
+  "mov{q}\t{%%gs:%P1, %2|%2, QWORD PTR %%gs:%P1}\;mov{q}\t{%2, %0|%0, %2}\;xor{l}\t%k2, %k2"
+  [(set_attr "type" "multi")])
+
 (define_expand "stack_protect_test"
   [(match_operand 0 "memory_operand" "")
    (match_operand 1 "memory_operand" "")
@@ -20201,10 +20217,14 @@
   ix86_compare_emitted = flags;
 
 #ifdef TARGET_THREAD_SSP_OFFSET
-  if (TARGET_64BIT)
-    emit_insn (gen_stack_tls_protect_test_di (flags, operands[0],
+  if (TARGET_64BIT) {
+	if (ix86_cmodel==CM_KERNEL)
+	    emit_insn (gen_stack_tls_protect_test_di_kernel (flags, operands[0],
 					GEN_INT (TARGET_THREAD_SSP_OFFSET)));
-  else
+	else
+	    emit_insn (gen_stack_tls_protect_test_di (flags, operands[0],
+					GEN_INT (TARGET_THREAD_SSP_OFFSET)));
+  } else
     emit_insn (gen_stack_tls_protect_test_si (flags, operands[0],
 					GEN_INT (TARGET_THREAD_SSP_OFFSET)));
 #else
@@ -20257,6 +20277,17 @@
   "mov{q}\t{%1, %3|%3, %1}\;xor{q}\t{%%fs:%P2, %3|%3, QWORD PTR %%fs:%P2}"
   [(set_attr "type" "multi")])
 
+(define_insn "stack_tls_protect_test_di_kernel"
+  [(set (match_operand:CCZ 0 "flags_reg_operand" "")
+	(unspec:CCZ [(match_operand:DI 1 "memory_operand" "m")
+		     (match_operand:DI 2 "const_int_operand" "i")]
+		    UNSPEC_SP_TLS_TEST_KERNEL))
+   (clobber (match_scratch:DI 3 "=r"))]
+  "TARGET_64BIT"
+  "mov{q}\t{%1, %3|%3, %1}\;xor{q}\t{%%gs:%P2, %3|%3, QWORD PTR %%gs:%P2}"
+  [(set_attr "type" "multi")])
+
 (include "sse.md")
 (include "mmx.md")
 (include "sync.md")
+

Re: [Patch 0/8] Port of -fstack-protector to the kernel

[Michal Piotrowski]

Hi,

On 17/03/06, Arjan van de Ven <arjan@linux.intel.com> wrote:
> This patch series adds support for the gcc 4.1 -fstack-protector feature to
> the kernel. Unfortunately this needs a gcc patch before it can work, so at
> this point these patches are just for comment, not for merging.
>

It's x86_64 specific?

Regards,
Michal

--
Michal K. K. Piotrowski
LTG - Linux Testers Group
(http://www.stardust.webpages.pl/ltg/wiki/)

Re: [Patch 0/8] Port of -fstack-protector to the kernel

[Arjan van de Ven]

Michal Piotrowski wrote:
> Hi,
> 
> On 17/03/06, Arjan van de Ven <arjan@linux.intel.com> wrote:
>> This patch series adds support for the gcc 4.1 -fstack-protector feature to
>> the kernel. Unfortunately this needs a gcc patch before it can work, so at
>> this point these patches are just for comment, not for merging.
>>
> 
> It's x86_64 specific?

for now it is yes; x86-64 is the "easiest" one because there already is a
per-processor datastructure in line with what gcc expect (eg similar to the
userland per thread structure). For x86... that's not there in the kernel.

Re: [Patch 0/8] Port of -fstack-protector to the kernel

[Ingo Molnar]

* Arjan van de Ven <arjan@linux.intel.com> wrote:

> This patch series adds support for the gcc 4.1 -fstack-protector 
> feature to the kernel. Unfortunately this needs a gcc patch before it 
> can work, so at this point these patches are just for comment, not for 
> merging.

i think this is a neat feature, and it looks quite unintrusive. Would be 
nice to get the gcc patch merged.

	Ingo