Patchwork powerpc: Check end of stack canary at oops time

login
register
mail settings
Submitter Anton Blanchard
Date Aug. 24, 2010, 11:15 p.m.
Message ID <20100824231528.GC28360@kryten>
Download mbox | patch
Permalink /patch/62639/
State Accepted
Commit 28b549905b239357db7c249e261857c1716db05a
Delegated to: Benjamin Herrenschmidt
Headers show

Comments

Anton Blanchard - Aug. 24, 2010, 11:15 p.m.
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 <anton@samba.org>
---
Michael Ellerman - Aug. 25, 2010, 1:29 a.m.
On Wed, 2010-08-25 at 09:15 +1000, Anton Blanchard wrote:
> 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 <anton@samba.org>
> ---
> 
> Index: powerpc.git/arch/powerpc/mm/fault.c
> ===================================================================
> --- powerpc.git.orig/arch/powerpc/mm/fault.c	2010-08-25 08:41:08.230086186 +1000
> +++ powerpc.git/arch/powerpc/mm/fault.c	2010-08-25 09:12:38.276553103 +1000
> @@ -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");

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

cheers
Anton Blanchard - Aug. 25, 2010, 1:51 a.m.
Hi,

> 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 <JBeulich@novell.com>
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
Benjamin Herrenschmidt - Aug. 30, 2010, 12:18 a.m.
On Wed, 2010-08-25 at 09:15 +1000, Anton Blanchard wrote:

>  	/* 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.

Patch

Index: powerpc.git/arch/powerpc/mm/fault.c
===================================================================
--- powerpc.git.orig/arch/powerpc/mm/fault.c	2010-08-25 08:41:08.230086186 +1000
+++ powerpc.git/arch/powerpc/mm/fault.c	2010-08-25 09:12:38.276553103 +1000
@@ -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);
 }