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.
Add the actual compiler options to the CFLAGS
Signed-off-by: Arjan van de Ven <[email protected]>
---
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
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 <[email protected]>
---
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;
This patch adds the config options for -fstack-protector
Signed-off-by: Arjan van de Ven <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
--- 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")
+
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 <[email protected]>
---
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);
Finish annotating the PDA members with offsets
Signed-off-by: Arjan van de Ven <[email protected]>
---
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[];
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 <[email protected]>
---
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 */
Hi,
On 17/03/06, Arjan van de Ven <[email protected]> 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/)
Michal Piotrowski wrote:
> Hi,
>
> On 17/03/06, Arjan van de Ven <[email protected]> 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.
* Arjan van de Ven <[email protected]> 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
* Arjan van de Ven <[email protected]> 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
Ingo Molnar wrote:
> * Arjan van de Ven <[email protected]> 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
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.'
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)
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.'