[PATCH v2 0/3] ARM: enable dumping stacks for CONFIG_SMP

[PATCH v2 0/3] ARM: enable dumping stacks for CONFIG_SMP

[Colin Cross]

This topic has come up before, see
http://comments.gmane.org/gmane.linux.ports.arm.kernel/102458
for the previous discussion.

SMP is now the norm for new ARM systems, and the limitation that
CONFIG_STACKTRACE doesn't work for tasks besides 'current' causes
problems.  The particular case I'm dealing with is automated
debugging information collected from /proc/<pid>/stack when
userspace detects a process is not responding.

Dumping stacktraces is currently disabled due to the worry that the
task may be running on another CPU and that the unwinder may be
unstable when presented with a stack that is being modified.  I have
attempted to harden the frame pointer based unwinder and the unwind
table based unwinder against invalid stacks.  I separated the two
into individual patches, as I expect the patch to the table unwinder
to be more controversial than the frame pointer unwinder.

Even without the hardening, unwinding a stack for a running process
is not completely untested.  When CONFIG_ARM_UNWIND is enabled,
sysrq-t calls unwind_backtrace for all tasks including running ones.
In addition, any callers to unwind_frame with preemption enabled,
including proc_pid_stack, could see a modified stack even on a UP
system (pointed out by Rabin Vincent the last time this topic came
up).

v2 of the patchset adds additional checks to avoid starting the
stacktrace in an unmapped region or looping forever when unwinding
a bad stack.

v2:
   add depth parameter to unwind_frame
   verify that sp changes for non-leaf frames
   verify that initial sp value is in mapped lowmem
   verify that stack offsets are in the range
      [sizeof(struct thread_info), THREAD_START_SP)

 arch/arm/include/asm/stacktrace.h |    6 ++-
 arch/arm/kernel/process.c         |    2 +-
 arch/arm/kernel/stacktrace.c      |  122 +++++++++++++++++++++++++++++++------
 arch/arm/kernel/time.c            |    3 +-
 arch/arm/kernel/unwind.c          |   46 ++++++++++----
 5 files changed, 145 insertions(+), 34 deletions(-)

[PATCH v2 1/3] ARM: stacktrace: harden FP stacktraces against invalid stacks

[Colin Cross]

Dumping stacktraces is currently disabled in ARM SMP for all tasks
except the current task due to the worry that the task may be running
on another CPU and that the unwinder may be unstable when presented
with a stack that is being modified.

Unwinding with CONFIG_FRAME_POINTER is fairly simple compared to
when CONFIG_ARM_UNWIND is set.  The next frame's FP and SP registers
are read from the stack and can be validated against the current
values to ensure that they do not leave the stack and make progress
towards the upper end of the stack.  This guarantees that accesses
do not fault and that execution is bounded.

Add additional validations to the version of unwind_frame used when
CONFIG_FRAME_POINTER=y:

Verify that the stack is in a mapped region of kernel memory.
Fixes crashes seen in unwind_frame on real systems, although
stack corruption caused by memory instability is likely the cause
of the invalid sp and fp values.

Fix address comparison to catch fp >= 0xfffffffc correctly.
Fixes crash reported by Todd Poynor.

Ensure the stack pointer moves to a higher address between each
frame to make sure repeated calls to unwind_frame can't loop
forever.

Includes ideas from Dave Martin.

Signed-off-by: Colin Cross <ccross@android.com>
---
v2:
   verify that initial sp value is in mapped lowmem
   verify that stack offsets are in the range
      [sizeof(struct thread_info), THREAD_START_SP)
   export stack validation functions for use by unwind

 arch/arm/include/asm/stacktrace.h |    4 ++
 arch/arm/kernel/stacktrace.c      |  105 ++++++++++++++++++++++++++++++++++--
 2 files changed, 103 insertions(+), 6 deletions(-)

diff --git a/arch/arm/include/asm/stacktrace.h b/arch/arm/include/asm/stacktrace.h
index 4d0a164..6909056 100644
--- a/arch/arm/include/asm/stacktrace.h
+++ b/arch/arm/include/asm/stacktrace.h
@@ -12,4 +12,8 @@ struct stackframe {
 extern void walk_stackframe(struct stackframe *frame,
 			    int (*fn)(struct stackframe *, void *), void *data);
 
+bool sp_addr_valid(unsigned long sp);
+bool addr_in_stack(unsigned long orig_sp, unsigned long vsp);
+bool sp_in_stack(unsigned long orig_sp, unsigned long vsp);
+
 #endif	/* __ASM_STACKTRACE_H */
diff --git a/arch/arm/kernel/stacktrace.c b/arch/arm/kernel/stacktrace.c
index 00f79e5..ca7e71b 100644
--- a/arch/arm/kernel/stacktrace.c
+++ b/arch/arm/kernel/stacktrace.c
@@ -1,9 +1,89 @@
 #include <linux/export.h>
+#include <linux/mm.h>
 #include <linux/sched.h>
 #include <linux/stacktrace.h>
 
 #include <asm/stacktrace.h>
 
+#define STACK_MAX(sp) (round_down(sp, THREAD_SIZE) + THREAD_START_SP)
+
+/**
+ * sp_addr_valid - verify a stack pointer
+ * @sp: current stack pointer
+ *
+ * Returns true if sp is a pointer inside a memory area that could be a stack.
+ * Does not verify that sp is inside an actual stack (i.e. does not check for
+ * STACK_MAGIC).
+ *
+ * If sp_addr_valid(sp) returns true, then the kernel will not fault if it
+ * accesses memory in the range
+ * [sp, round_down(sp, THREAD_SIZE) + THREAD_START_SP)
+ */
+bool sp_addr_valid(unsigned long sp)
+{
+	unsigned long high;
+	unsigned long offset;
+	unsigned int pfn;
+	unsigned int start_pfn;
+	unsigned int end_pfn;
+
+	if (!IS_ALIGNED(sp, 4))
+		return false;
+
+	offset = sp & (THREAD_SIZE - 1);
+
+	if (offset > THREAD_START_SP)
+		return false;
+
+	if (offset < sizeof(struct thread_info))
+		return false;
+
+	high = STACK_MAX(sp);
+
+	if (!virt_addr_valid(sp) || !virt_addr_valid(high))
+		return false;
+
+	start_pfn = page_to_pfn(virt_to_page(sp));
+	end_pfn = page_to_pfn(virt_to_page(high));
+	for (pfn = start_pfn; pfn <= end_pfn; pfn++)
+		if (!pfn_valid(pfn))
+			return false;
+
+	return true;
+}
+
+/**
+ * addr_in_stack - verify a pointer is inside a specified stack
+ * @orig_sp: stack pointer at the bottom of the stack
+ * @sp: address to be verified
+ *
+ * Returns true if sp is in the stack bounded@the bottom by orig_sp, in the
+ * range [orig_sp, round_down(orig_sp, THREAD_SIZE) + THREAD_START_SP)
+ *
+ * If orig_sp is valid (see sp_addr_valid), then the kernel will not fault if it
+ * accesses a pointer where ptr_in_stack returns true.
+ */
+bool addr_in_stack(unsigned long orig_sp, unsigned long sp)
+{
+	return (sp >= orig_sp && sp < STACK_MAX(orig_sp) && IS_ALIGNED(sp, 4));
+}
+
+/**
+ * sp_in_stack - verify a stack pointer is inside a specified stack
+ * @orig_sp: stack pointer at the bottom of the stack
+ * @sp: stack pointer to be verified
+ *
+ * Returns true if sp is in the stack bounded@the bottom by orig_sp, in the
+ * range [orig_sp, round_down(orig_sp, THREAD_SIZE) + THREAD_START_SP]
+ *
+ * If sp_in_stack returns true,
+ * addr_in_stack(vsp, x) == addr_in_stack(orig_sp, x)
+ */
+bool sp_in_stack(unsigned long orig_sp, unsigned long sp)
+{
+	return (sp >= orig_sp && sp <= STACK_MAX(orig_sp) && IS_ALIGNED(sp, 4));
+}
+
 #if defined(CONFIG_FRAME_POINTER) && !defined(CONFIG_ARM_UNWIND)
 /*
  * Unwind the current stack frame and store the new register values in the
@@ -23,15 +103,17 @@
  */
 int notrace unwind_frame(struct stackframe *frame)
 {
-	unsigned long high, low;
 	unsigned long fp = frame->fp;
+	unsigned long sp = frame->sp;
+
+	if (!sp_addr_valid(sp))
+		return -EINVAL;
 
-	/* only go to a higher address on the stack */
-	low = frame->sp;
-	high = ALIGN(low, THREAD_SIZE);
+	/* Check current frame pointer is within the stack bounds. */
+	if (!addr_in_stack(sp, fp))
+		return -EINVAL;
 
-	/* check current frame pointer is within bounds */
-	if (fp < (low + 12) || fp + 4 >= high)
+	if (fp < 12 || !addr_in_stack(sp, fp - 12))
 		return -EINVAL;
 
 	/* restore the registers from the stack frame */
@@ -39,6 +121,17 @@ int notrace unwind_frame(struct stackframe *frame)
 	frame->sp = *(unsigned long *)(fp - 8);
 	frame->pc = *(unsigned long *)(fp - 4);
 
+	/* Ensure the next stack pointer is in the same stack */
+	if (!sp_in_stack(sp, frame->sp))
+		return -EINVAL;
+
+	/*
+	 * Ensure the next stack pointer is above this frame to guarantee
+	 * bounded execution.
+	 */
+	if (frame->sp < fp)
+		return -EINVAL;
+
 	return 0;
 }
 #endif
-- 
1.7.7.3

[PATCH v2 1/3] ARM: stacktrace: harden FP stacktraces against invalid stacks

[Laura Abbott]

On 11/29/2012 3:00 PM, Colin Cross wrote:

> +bool sp_addr_valid(unsigned long sp)
> +{
> +	unsigned long high;
> +	unsigned long offset;
> +	unsigned int pfn;
> +	unsigned int start_pfn;
> +	unsigned int end_pfn;
> +
> +	if (!IS_ALIGNED(sp, 4))
> +		return false;
> +
> +	offset = sp & (THREAD_SIZE - 1);
> +
> +	if (offset > THREAD_START_SP)
> +		return false;
> +
> +	if (offset < sizeof(struct thread_info))
> +		return false;
> +
> +	high = STACK_MAX(sp);
> +
> +	if (!virt_addr_valid(sp) || !virt_addr_valid(high))
> +		return false;
> +
> +	start_pfn = page_to_pfn(virt_to_page(sp));
> +	end_pfn = page_to_pfn(virt_to_page(high));
> +	for (pfn = start_pfn; pfn <= end_pfn; pfn++)
> +		if (!pfn_valid(pfn))
> +			return false;
> +
> +	return true;
> +}

I get crashes on bootup with CONFIG_SPARSEMEM enabled if a stacktrace 
needs to be saved before the sections are setup:

<1>[    0.000000] Unable to handle kernel NULL pointer dereference at 
virtual address 00000000
<1>[    0.000000] pgd = c0004000
<1>[    0.000000] [00000000] *pgd=00000000
<0>[    0.000000] Internal error: Oops: 5 [#1] PREEMPT SMP ARM
<4>[    0.000000] Modules linked in:
<4>[    0.000000] CPU: 0    Not tainted 
(3.4.0-ga472ec0-00007-g6479b9e-dirty #10)
<4>[    0.000000] PC is at sp_addr_valid+0xb0/0x1bc
<4>[    0.000000] LR is at unwind_frame+0x4c/0x5b0
...
<1>[    0.000000] Unable to handle kernel NULL pointer dereference at 
virtual address 00000000
<1>[    0.000000] pgd = c0004000
<1>[    0.000000] [00000000] *pgd=00000000
<0>[    0.000000] BUG: spinlock lockup on CPU#0, swapper/0
<0>[    0.000000]  lock: die_lock+0x0/0x10, .magic: dead4ead, .owner: 
swapper/0, .owner_cpu: 0
<1>[    0.000000] Unable to handle kernel NULL pointer dereference at 
virtual address 00000000
<1>[    0.000000] pgd = c0004000
<1>[    0.000000] [00000000] *pgd=00000000
<0>[    0.000000] BUG: spinlock lockup on CPU#0, swapper/0
<0>[    0.000000]  lock: die_lock+0x0/0x10, .magic: dead4ead, .owner: 
swapper/0, .owner_cpu: 0
[repeat several more times]

In this case, the stacktrace is being saved via a call to kmemleak_free 
in free_bootmem. The sections have not yet been initialized so there is 
a crash in virt_to_page when accessing the section data.

I don't see an easy workaround for this right now unless we want to 
restrict sp_addr_valid until later in bootup.

Laura

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[PATCH v2 1/3] ARM: stacktrace: harden FP stacktraces against invalid stacks

[Colin Cross]

On Thu, Jan 24, 2013 at 1:07 PM, Laura Abbott <lauraa@codeaurora.org> wrote:
> On 11/29/2012 3:00 PM, Colin Cross wrote:
>
>> +bool sp_addr_valid(unsigned long sp)
>> +{
>> +       unsigned long high;
>> +       unsigned long offset;
>> +       unsigned int pfn;
>> +       unsigned int start_pfn;
>> +       unsigned int end_pfn;
>> +
>> +       if (!IS_ALIGNED(sp, 4))
>> +               return false;
>> +
>> +       offset = sp & (THREAD_SIZE - 1);
>> +
>> +       if (offset > THREAD_START_SP)
>> +               return false;
>> +
>> +       if (offset < sizeof(struct thread_info))
>> +               return false;
>> +
>> +       high = STACK_MAX(sp);
>> +
>> +       if (!virt_addr_valid(sp) || !virt_addr_valid(high))
>> +               return false;
>> +
>> +       start_pfn = page_to_pfn(virt_to_page(sp));
>> +       end_pfn = page_to_pfn(virt_to_page(high));
>> +       for (pfn = start_pfn; pfn <= end_pfn; pfn++)
>> +               if (!pfn_valid(pfn))
>> +                       return false;
>> +
>> +       return true;
>> +}
>
>
> I get crashes on bootup with CONFIG_SPARSEMEM enabled if a stacktrace needs
> to be saved before the sections are setup:
>
> <1>[    0.000000] Unable to handle kernel NULL pointer dereference at
> virtual address 00000000
> <1>[    0.000000] pgd = c0004000
> <1>[    0.000000] [00000000] *pgd=00000000
> <0>[    0.000000] Internal error: Oops: 5 [#1] PREEMPT SMP ARM
> <4>[    0.000000] Modules linked in:
> <4>[    0.000000] CPU: 0    Not tainted (3.4.0-ga472ec0-00007-g6479b9e-dirty
> #10)
> <4>[    0.000000] PC is at sp_addr_valid+0xb0/0x1bc
> <4>[    0.000000] LR is at unwind_frame+0x4c/0x5b0
> ...
> <1>[    0.000000] Unable to handle kernel NULL pointer dereference at
> virtual address 00000000
> <1>[    0.000000] pgd = c0004000
> <1>[    0.000000] [00000000] *pgd=00000000
> <0>[    0.000000] BUG: spinlock lockup on CPU#0, swapper/0
> <0>[    0.000000]  lock: die_lock+0x0/0x10, .magic: dead4ead, .owner:
> swapper/0, .owner_cpu: 0
> <1>[    0.000000] Unable to handle kernel NULL pointer dereference at
> virtual address 00000000
> <1>[    0.000000] pgd = c0004000
> <1>[    0.000000] [00000000] *pgd=00000000
> <0>[    0.000000] BUG: spinlock lockup on CPU#0, swapper/0
> <0>[    0.000000]  lock: die_lock+0x0/0x10, .magic: dead4ead, .owner:
> swapper/0, .owner_cpu: 0
> [repeat several more times]
>
> In this case, the stacktrace is being saved via a call to kmemleak_free in
> free_bootmem. The sections have not yet been initialized so there is a crash
> in virt_to_page when accessing the section data.
>
> I don't see an easy workaround for this right now unless we want to restrict
> sp_addr_valid until later in bootup.

Thanks for testing, I was able to reproduce the problem.  It is easily
fixed by replacing page_to_pfn(virt_to_page(x)) with
__phys_to_pfn(__virt_to_phys(x)) to avoid touching the page structs at
all.

[PATCH v2 2/3] ARM: unwind: harden unwinding against invalid stacks

[Colin Cross]

Unwinding with CONFIG_ARM_UNWIND is much more complicated than
unwinding with CONFIG_FRAME_POINTER, but there are only a few points
that require validation in order to avoid faults or infinite loops.
Avoiding faults is easy by adding checks to verify that the stack
bounds are in lowmem, and that all accesses relative to the stack
pointer remain inside the stack.

When CONFIG_FRAME_POINTER is not set it is possible for a leaf frame
to have the same SP as its caller, but otherwise the SP of the caller
should always be higher than the SP of the callee.  Add a depth
parameter to unwind_frame to allow it to distinguish leaf frames
from non-leaf frames, and return an error if a caller has the same
sp as a non-leaf frame callee.

Signed-off-by: Colin Cross <ccross@android.com>
---
v2:
   add depth parameter to unwind_frame
   verify that sp changes for non-leaf frames
   verify that initial sp value is in mapped lowmem
   verify that stack offsets are in the range
      [sizeof(struct thread_info), THREAD_START_SP)

 arch/arm/include/asm/stacktrace.h |    2 +-
 arch/arm/kernel/process.c         |    2 +-
 arch/arm/kernel/stacktrace.c      |    6 +++-
 arch/arm/kernel/time.c            |    3 +-
 arch/arm/kernel/unwind.c          |   46 +++++++++++++++++++++++++++---------
 5 files changed, 42 insertions(+), 17 deletions(-)

diff --git a/arch/arm/include/asm/stacktrace.h b/arch/arm/include/asm/stacktrace.h
index 6909056..cc48971 100644
--- a/arch/arm/include/asm/stacktrace.h
+++ b/arch/arm/include/asm/stacktrace.h
@@ -8,7 +8,7 @@ struct stackframe {
 	unsigned long pc;
 };
 
-extern int unwind_frame(struct stackframe *frame);
+extern int unwind_frame(struct stackframe *frame, int depth);
 extern void walk_stackframe(struct stackframe *frame,
 			    int (*fn)(struct stackframe *, void *), void *data);
 
diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c
index 41aad9e..10ea483 100644
--- a/arch/arm/kernel/process.c
+++ b/arch/arm/kernel/process.c
@@ -633,7 +633,7 @@ unsigned long get_wchan(struct task_struct *p)
 	frame.lr = 0;			/* recovered from the stack */
 	frame.pc = thread_saved_pc(p);
 	do {
-		int ret = unwind_frame(&frame);
+		int ret = unwind_frame(&frame, count);
 		if (ret < 0)
 			return 0;
 		if (!in_sched_functions(frame.pc))
diff --git a/arch/arm/kernel/stacktrace.c b/arch/arm/kernel/stacktrace.c
index ca7e71b..c5ae9be 100644
--- a/arch/arm/kernel/stacktrace.c
+++ b/arch/arm/kernel/stacktrace.c
@@ -101,7 +101,7 @@ bool sp_in_stack(unsigned long orig_sp, unsigned long sp)
  * Note that with framepointer enabled, even the leaf functions have the same
  * prologue and epilogue, therefore we can ignore the LR value in this case.
  */
-int notrace unwind_frame(struct stackframe *frame)
+int notrace unwind_frame(struct stackframe *frame, int depth)
 {
 	unsigned long fp = frame->fp;
 	unsigned long sp = frame->sp;
@@ -139,12 +139,14 @@ int notrace unwind_frame(struct stackframe *frame)
 void notrace walk_stackframe(struct stackframe *frame,
 		     int (*fn)(struct stackframe *, void *), void *data)
 {
+	int depth = 0;
+
 	while (1) {
 		int ret;
 
 		if (fn(frame, data))
 			break;
-		ret = unwind_frame(frame);
+		ret = unwind_frame(frame, depth++);
 		if (ret < 0)
 			break;
 	}
diff --git a/arch/arm/kernel/time.c b/arch/arm/kernel/time.c
index fe31b22..5f0bf17 100644
--- a/arch/arm/kernel/time.c
+++ b/arch/arm/kernel/time.c
@@ -51,6 +51,7 @@
 unsigned long profile_pc(struct pt_regs *regs)
 {
 	struct stackframe frame;
+	int depth = 0;
 
 	if (!in_lock_functions(regs->ARM_pc))
 		return regs->ARM_pc;
@@ -60,7 +61,7 @@ unsigned long profile_pc(struct pt_regs *regs)
 	frame.lr = regs->ARM_lr;
 	frame.pc = regs->ARM_pc;
 	do {
-		int ret = unwind_frame(&frame);
+		int ret = unwind_frame(&frame, depth++);
 		if (ret < 0)
 			return 0;
 	} while (in_lock_functions(frame.pc));
diff --git a/arch/arm/kernel/unwind.c b/arch/arm/kernel/unwind.c
index 00df012..529c36f 100644
--- a/arch/arm/kernel/unwind.c
+++ b/arch/arm/kernel/unwind.c
@@ -40,6 +40,7 @@
 #include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/export.h>
+#include <linux/mm.h>
 #include <linux/sched.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
@@ -235,12 +236,18 @@ static unsigned long unwind_get_byte(struct unwind_ctrl_block *ctrl)
 	return ret;
 }
 
+static bool ptr_in_stack(unsigned long orig_sp, void *vsp)
+{
+	return addr_in_stack(orig_sp, (unsigned long)vsp);
+}
+
 /*
  * Execute the current unwind instruction.
  */
 static int unwind_exec_insn(struct unwind_ctrl_block *ctrl)
 {
 	unsigned long insn = unwind_get_byte(ctrl);
+	unsigned long orig_sp = ctrl->vrs[SP];
 
 	pr_debug("%s: insn = %08lx\n", __func__, insn);
 
@@ -264,8 +271,11 @@ static int unwind_exec_insn(struct unwind_ctrl_block *ctrl)
 		/* pop R4-R15 according to mask */
 		load_sp = mask & (1 << (13 - 4));
 		while (mask) {
-			if (mask & 1)
+			if (mask & 1) {
+				if (!ptr_in_stack(orig_sp, vsp))
+					return -URC_FAILURE;
 				ctrl->vrs[reg] = *vsp++;
+			}
 			mask >>= 1;
 			reg++;
 		}
@@ -279,10 +289,16 @@ static int unwind_exec_insn(struct unwind_ctrl_block *ctrl)
 		int reg;
 
 		/* pop R4-R[4+bbb] */
-		for (reg = 4; reg <= 4 + (insn & 7); reg++)
+		for (reg = 4; reg <= 4 + (insn & 7); reg++) {
+			if (!ptr_in_stack(orig_sp, vsp))
+				return -URC_FAILURE;
 			ctrl->vrs[reg] = *vsp++;
-		if (insn & 0x80)
+		}
+		if (insn & 0x80) {
+			if (!ptr_in_stack(orig_sp, vsp))
+				return -URC_FAILURE;
 			ctrl->vrs[14] = *vsp++;
+		}
 		ctrl->vrs[SP] = (unsigned long)vsp;
 	} else if (insn == 0xb0) {
 		if (ctrl->vrs[PC] == 0)
@@ -302,8 +318,11 @@ static int unwind_exec_insn(struct unwind_ctrl_block *ctrl)
 
 		/* pop R0-R3 according to mask */
 		while (mask) {
-			if (mask & 1)
+			if (mask & 1) {
+				if (!ptr_in_stack(orig_sp, vsp))
+					return -URC_FAILURE;
 				ctrl->vrs[reg] = *vsp++;
+			}
 			mask >>= 1;
 			reg++;
 		}
@@ -327,19 +346,17 @@ static int unwind_exec_insn(struct unwind_ctrl_block *ctrl)
  * Unwind a single frame starting with *sp for the symbol at *pc. It
  * updates the *pc and *sp with the new values.
  */
-int unwind_frame(struct stackframe *frame)
+int unwind_frame(struct stackframe *frame, int depth)
 {
-	unsigned long high, low;
 	const struct unwind_idx *idx;
 	struct unwind_ctrl_block ctrl;
 
-	/* only go to a higher address on the stack */
-	low = frame->sp;
-	high = ALIGN(low, THREAD_SIZE);
-
 	pr_debug("%s(pc = %08lx lr = %08lx sp = %08lx)\n", __func__,
 		 frame->pc, frame->lr, frame->sp);
 
+	if (!sp_addr_valid(frame->sp))
+		return -URC_FAILURE;
+
 	if (!kernel_text_address(frame->pc))
 		return -URC_FAILURE;
 
@@ -386,7 +403,7 @@ int unwind_frame(struct stackframe *frame)
 		int urc = unwind_exec_insn(&ctrl);
 		if (urc < 0)
 			return urc;
-		if (ctrl.vrs[SP] < low || ctrl.vrs[SP] >= high)
+		if (!sp_in_stack(frame->sp, ctrl.vrs[SP]))
 			return -URC_FAILURE;
 	}
 
@@ -397,6 +414,10 @@ int unwind_frame(struct stackframe *frame)
 	if (frame->pc == ctrl.vrs[PC])
 		return -URC_FAILURE;
 
+	/* only leaf functions can possibly not modify sp */
+	if (depth != 0 && frame->sp == ctrl.vrs[SP])
+		return -URC_FAILURE;
+
 	frame->fp = ctrl.vrs[FP];
 	frame->sp = ctrl.vrs[SP];
 	frame->lr = ctrl.vrs[LR];
@@ -409,6 +430,7 @@ void unwind_backtrace(struct pt_regs *regs, struct task_struct *tsk)
 {
 	struct stackframe frame;
 	register unsigned long current_sp asm ("sp");
+	int depth = 0;
 
 	pr_debug("%s(regs = %p tsk = %p)\n", __func__, regs, tsk);
 
@@ -443,7 +465,7 @@ void unwind_backtrace(struct pt_regs *regs, struct task_struct *tsk)
 		int urc;
 		unsigned long where = frame.pc;
 
-		urc = unwind_frame(&frame);
+		urc = unwind_frame(&frame, depth++);
 		if (urc < 0)
 			break;
 		dump_backtrace_entry(where, frame.pc, frame.sp - 4);
-- 
1.7.7.3

[PATCH v2 3/3] ARM: stacktrace: enable save_stack_trace_tsk for CONFIG_SMP

[Colin Cross]

Now that unwind_frame has been hardened against invalid stacks
for CONFIG_ARM_UNWIND or CONFIG_FRAME_POINTER, enable dumping
the stack on non-current threads for CONFIG_SMP.

Signed-off-by: Colin Cross <ccross@android.com>
---
 arch/arm/kernel/stacktrace.c |   11 -----------
 1 files changed, 0 insertions(+), 11 deletions(-)

diff --git a/arch/arm/kernel/stacktrace.c b/arch/arm/kernel/stacktrace.c
index c5ae9be..6c00a9b 100644
--- a/arch/arm/kernel/stacktrace.c
+++ b/arch/arm/kernel/stacktrace.c
@@ -187,22 +187,11 @@ void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace)
 	data.skip = trace->skip;
 
 	if (tsk != current) {
-#ifdef CONFIG_SMP
-		/*
-		 * What guarantees do we have here that 'tsk' is not
-		 * running on another CPU?  For now, ignore it as we
-		 * can't guarantee we won't explode.
-		 */
-		if (trace->nr_entries < trace->max_entries)
-			trace->entries[trace->nr_entries++] = ULONG_MAX;
-		return;
-#else
 		data.no_sched_functions = 1;
 		frame.fp = thread_saved_fp(tsk);
 		frame.sp = thread_saved_sp(tsk);
 		frame.lr = 0;		/* recovered from the stack */
 		frame.pc = thread_saved_pc(tsk);
-#endif
 	} else {
 		register unsigned long current_sp asm ("sp");
 
-- 
1.7.7.3