Commit 5c79d2a5 authored by Tejun Heo's avatar Tejun Heo Committed by Ingo Molnar

x86: fix x86_32 stack protector bugs

Impact: fix x86_32 stack protector

Brian Gerst found out that %gs was being initialized to stack_canary
instead of stack_canary - 20, which basically gave the same canary
value for all threads.  Fixing this also exposed the following bugs.

* cpu_idle() didn't call boot_init_stack_canary()

* stack canary switching in switch_to() was being done too late making
  the initial run of a new thread use the old stack canary value.

Fix all of them and while at it update comment in cpu_idle() about
calling boot_init_stack_canary().
Reported-by: default avatarBrian Gerst <brgerst@gmail.com>
Signed-off-by: default avatarTejun Heo <tj@kernel.org>
Signed-off-by: default avatarIngo Molnar <mingo@elte.hu>
parent 60a5317f
...@@ -85,7 +85,7 @@ static __always_inline void boot_init_stack_canary(void) ...@@ -85,7 +85,7 @@ static __always_inline void boot_init_stack_canary(void)
static inline void setup_stack_canary_segment(int cpu) static inline void setup_stack_canary_segment(int cpu)
{ {
#ifdef CONFIG_X86_32 #ifdef CONFIG_X86_32
unsigned long canary = (unsigned long)&per_cpu(stack_canary, cpu); unsigned long canary = (unsigned long)&per_cpu(stack_canary, cpu) - 20;
struct desc_struct *gdt_table = get_cpu_gdt_table(cpu); struct desc_struct *gdt_table = get_cpu_gdt_table(cpu);
struct desc_struct desc; struct desc_struct desc;
......
...@@ -25,13 +25,11 @@ struct task_struct *__switch_to(struct task_struct *prev, ...@@ -25,13 +25,11 @@ struct task_struct *__switch_to(struct task_struct *prev,
#ifdef CONFIG_CC_STACKPROTECTOR #ifdef CONFIG_CC_STACKPROTECTOR
#define __switch_canary \ #define __switch_canary \
"movl "__percpu_arg([current_task])",%%ebx\n\t" \ "movl %P[task_canary](%[next]), %%ebx\n\t" \
"movl %P[task_canary](%%ebx),%%ebx\n\t" \ "movl %%ebx, "__percpu_arg([stack_canary])"\n\t"
"movl %%ebx,"__percpu_arg([stack_canary])"\n\t"
#define __switch_canary_oparam \ #define __switch_canary_oparam \
, [stack_canary] "=m" (per_cpu_var(stack_canary)) , [stack_canary] "=m" (per_cpu_var(stack_canary))
#define __switch_canary_iparam \ #define __switch_canary_iparam \
, [current_task] "m" (per_cpu_var(current_task)) \
, [task_canary] "i" (offsetof(struct task_struct, stack_canary)) , [task_canary] "i" (offsetof(struct task_struct, stack_canary))
#else /* CC_STACKPROTECTOR */ #else /* CC_STACKPROTECTOR */
#define __switch_canary #define __switch_canary
...@@ -60,9 +58,9 @@ do { \ ...@@ -60,9 +58,9 @@ do { \
"movl %[next_sp],%%esp\n\t" /* restore ESP */ \ "movl %[next_sp],%%esp\n\t" /* restore ESP */ \
"movl $1f,%[prev_ip]\n\t" /* save EIP */ \ "movl $1f,%[prev_ip]\n\t" /* save EIP */ \
"pushl %[next_ip]\n\t" /* restore EIP */ \ "pushl %[next_ip]\n\t" /* restore EIP */ \
__switch_canary \
"jmp __switch_to\n" /* regparm call */ \ "jmp __switch_to\n" /* regparm call */ \
"1:\t" \ "1:\t" \
__switch_canary \
"popl %%ebp\n\t" /* restore EBP */ \ "popl %%ebp\n\t" /* restore EBP */ \
"popfl\n" /* restore flags */ \ "popfl\n" /* restore flags */ \
\ \
......
...@@ -447,6 +447,7 @@ is386: movl $2,%ecx # set MP ...@@ -447,6 +447,7 @@ is386: movl $2,%ecx # set MP
jne 1f jne 1f
movl $per_cpu__gdt_page,%eax movl $per_cpu__gdt_page,%eax
movl $per_cpu__stack_canary,%ecx movl $per_cpu__stack_canary,%ecx
subl $20, %ecx
movw %cx, 8 * GDT_ENTRY_STACK_CANARY + 2(%eax) movw %cx, 8 * GDT_ENTRY_STACK_CANARY + 2(%eax)
shrl $16, %ecx shrl $16, %ecx
movb %cl, 8 * GDT_ENTRY_STACK_CANARY + 4(%eax) movb %cl, 8 * GDT_ENTRY_STACK_CANARY + 4(%eax)
......
...@@ -11,6 +11,7 @@ ...@@ -11,6 +11,7 @@
#include <stdarg.h> #include <stdarg.h>
#include <linux/stackprotector.h>
#include <linux/cpu.h> #include <linux/cpu.h>
#include <linux/errno.h> #include <linux/errno.h>
#include <linux/sched.h> #include <linux/sched.h>
...@@ -91,6 +92,15 @@ void cpu_idle(void) ...@@ -91,6 +92,15 @@ void cpu_idle(void)
{ {
int cpu = smp_processor_id(); int cpu = smp_processor_id();
/*
* If we're the non-boot CPU, nothing set the stack canary up
* for us. CPU0 already has it initialized but no harm in
* doing it again. This is a good place for updating it, as
* we wont ever return from this function (so the invalid
* canaries already on the stack wont ever trigger).
*/
boot_init_stack_canary();
current_thread_info()->status |= TS_POLLING; current_thread_info()->status |= TS_POLLING;
/* endless idle loop with no priority at all */ /* endless idle loop with no priority at all */
......
...@@ -120,12 +120,11 @@ void cpu_idle(void) ...@@ -120,12 +120,11 @@ void cpu_idle(void)
current_thread_info()->status |= TS_POLLING; current_thread_info()->status |= TS_POLLING;
/* /*
* If we're the non-boot CPU, nothing set the PDA stack * If we're the non-boot CPU, nothing set the stack canary up
* canary up for us - and if we are the boot CPU we have * for us. CPU0 already has it initialized but no harm in
* a 0 stack canary. This is a good place for updating * doing it again. This is a good place for updating it, as
* it, as we wont ever return from this function (so the * we wont ever return from this function (so the invalid
* invalid canaries already on the stack wont ever * canaries already on the stack wont ever trigger).
* trigger):
*/ */
boot_init_stack_canary(); boot_init_stack_canary();
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment