Skip to contentSkip to conversation

powerpc: Check end of stack canary at oops time

PATCH In review Anton Blanchard 3 participants 4 messages Aug 24, 2010 – Aug 30, 2010 PowerPC

lore
Share & tools
lore

Anton Blanchard

author
#0 lore raw

PATCH 1/1 powerpc: Check end of stack canary at oops time

Add a check for the stack canary when we oops, similar to x86. This should make it clear that we overran our stack:

Unable to handle kernel paging request for data at address 0x24652f63700ac689

Faulting instruction address: 0xc000000000063d24
Thread overran stack, or stack corrupted

Signed-off-by Anton Blanchard <[email protected]>

---

arch/powerpc/mm/fault.c +6 -0
Index: powerpc.git/arch/powerpc/mm/fault.c
===================================================================
@@ -30,6 +30,7 @@
 #include <linux/kprobes.h>
 #include <linux/kdebug.h>
 #include <linux/perf_event.h>
+#include <linux/magic.h>
 
 #include <asm/firmware.h>
 #include <asm/page.h>

@@ -385,6 +386,7 @@ do_sigbus:
 void bad_page_fault(struct pt_regs *regs, unsigned long address, int sig)
 {
 	const struct exception_table_entry *entry;
+	unsigned long *stackend;
 
 	/* Are we prepared to handle this fault?  */
 	if ((entry = search_exception_tables(regs->nip)) != NULL) {

@@ -413,5 +415,9 @@ void bad_page_fault(struct pt_regs *regs
 	printk(KERN_ALERT "Faulting instruction address: 0x%08lx\n",
 		regs->nip);
 
+	stackend = end_of_stack(current);
+	if (current != &init_task && *stackend != STACK_END_MAGIC)
+		printk(KERN_ALERT "Thread overran stack, or stack corrupted\n");
+
 	die("Kernel access of bad area", regs, sig);
 }
#1 lore raw
Quoted 31 lines · Anton Blanchard

Add a check for the stack canary when we oops, similar to x86. This should make it clear that we overran our stack:

Unable to handle kernel paging request for data at address 0x24652f63700ac689

Faulting instruction address: 0xc000000000063d24
Thread overran stack, or stack corrupted

Signed-off-by Anton Blanchard <[email protected]>

---

Index: powerpc.git/arch/powerpc/mm/fault.c
===================================================================

arch/powerpc/mm/fault.c +2 -0
@@ -30,6 +30,7 @@
 #include <linux/kprobes.h>
 #include <linux/kdebug.h>
 #include <linux/perf_event.h>
+#include <linux/magic.h>
 
 #include <asm/firmware.h>
 #include <asm/page.h>

@@ -385,6 +386,7 @@ do_sigbus:
 void bad_page_fault(struct pt_regs *regs, unsigned long address, int sig)
 {
 	const struct exception_table_entry *entry;
+	unsigned long *stackend;
 
 	/* Are we prepared to handle this fault?  */
 	if ((entry = search_exception_tables(regs->nip)) != NULL) {

arch/powerpc/mm/fault.c void bad_page_fault(struct pt_regs *regs · line 420

Quoted 6 lines
arch/powerpc/mm/fault.c +3 -0
@@ -413,5 +415,9 @@ void bad_page_fault(struct pt_regs *regs
 	printk(KERN_ALERT "Faulting instruction address: 0x%08lx\n",
 		regs->nip);
 
+	stackend = end_of_stack(current);
+	if (current != &init_task && *stackend != STACK_END_MAGIC)
+		printk(KERN_ALERT "Thread overran stack, or stack corrupted\n");

The check for init is just because we haven't set the magic value for init's stack right? But we could.

cheers

author
#2 lore raw

Hi,

Quoted 2 lines

The check for init is just because we haven't set the magic value for init's stack right? But we could.

Yeah, it's similar to what x86 are doing now:

commit 0e7810be30f66e9f430c4ce2cd3b14634211690f

Author: Jan Beulich <[email protected]>
Date:   Fri Nov 20 14:00:14 2009 +0000

x86: Suppress stack overrun message for init_task

init_task doesn't get its stack end location set to STACK_END_MAGIC, and hence the message is confusing rather than helpful in this case.

Adding it directly to init_task would be nice but I suspect we'd either have to make assumptions about end_of_stack in our code or move the canary into the thread_info (so we can statically allocate it via INIT_THREAD_INFO()) or do it at runtime somewhere, hopefully early enough that we couldn't take an oops.

Anton

#3 lore raw
In reply to Anton Blanchard

arch/powerpc/mm/fault.c void bad_page_fault(struct pt_regs *regs · line 423

Quoted 12 lines · Anton Blanchard
arch/powerpc/mm/fault.c +4 -0
 	/* Are we prepared to handle this fault?  */
 	if ((entry = search_exception_tables(regs->nip)) != NULL) {

@@ -413,5 +415,9 @@ void bad_page_fault(struct pt_regs *regs
 	printk(KERN_ALERT "Faulting instruction address: 0x%08lx\n",
 		regs->nip);
 
+	stackend = end_of_stack(current);
+	if (current != &init_task && *stackend != STACK_END_MAGIC)
+		printk(KERN_ALERT "Thread overran stack, or stack corrupted\n");
+
 	die("Kernel access of bad area", regs, sig);
 }

Is it worth making it a __get_user or similar just in case ? I suppose if the stack is really busted we wouldn't even have reached that code ...

Cheers,
Ben.