[PATCH] arm64: fix dump_backtrace with NULL tsk

[PATCH] arm64: fix dump_backtrace with NULL tsk

[Mark Rutland]

In some places, dump_backtrace() is called with a NULL tsk parameter,
e.g. in bug_handler() in arch/arm64, or indirectly via show_stack() in
core code. The expectation is that this is treated as if current were
passed instead of NULL.

Commit a80a0eb70c358f8c ("arm64: make irq_stack_ptr more robust") didn't
take this into account, and compares tsk against current *before* we
check if tsk is NULL.

Due to this, we won't initialise irq_stack_ptr, and when we try to dump
the exception regs we may call dump_mem() for memory immediately above
the IRQ stack range, rather than for the relevant range on the task
stack.

This will result in misleading, though should not be otherwise
problematic. The initial percpu areas (including the IRQ stacks) are
allocated in the linear map, and dump_mem uses __get_user(), so we
shouldn't access anything with side-effects, and will handle holes
safely.

This patch fixes the issue by handling the NULL tsk case before we do
anything else with tsk.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Fixes: a80a0eb70c358f8c ("arm64: make irq_stack_ptr more robust")
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: James Morse <james.morse@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Yang Shi <yang.shi@linaro.org>
---
 arch/arm64/kernel/traps.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index e04f838..df06750 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -142,6 +142,11 @@ static void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
 	unsigned long irq_stack_ptr;
 	int skip;
 
+	pr_debug("%s(regs = %p tsk = %p)\n", __func__, regs, tsk);
+
+	if (!tsk)
+		tsk = current;
+
 	/*
 	 * Switching between stacks is valid when tracing current and in
 	 * non-preemptible context.
@@ -151,11 +156,6 @@ static void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
 	else
 		irq_stack_ptr = 0;
 
-	pr_debug("%s(regs = %p tsk = %p)\n", __func__, regs, tsk);
-
-	if (!tsk)
-		tsk = current;
-
 	if (tsk == current) {
 		frame.fp = (unsigned long)__builtin_frame_address(0);
 		frame.sp = current_stack_pointer;
-- 
1.9.1

[PATCH] arm64: fix dump_backtrace with NULL tsk

[Mark Rutland]

On Fri, Sep 23, 2016 at 03:56:27PM +0100, Mark Rutland wrote:
> In some places, dump_backtrace() is called with a NULL tsk parameter,
> e.g. in bug_handler() in arch/arm64, or indirectly via show_stack() in
> core code. The expectation is that this is treated as if current were
> passed instead of NULL.
> 
> Commit a80a0eb70c358f8c ("arm64: make irq_stack_ptr more robust") didn't
> take this into account, and compares tsk against current *before* we
> check if tsk is NULL.
> 
> Due to this, we won't initialise irq_stack_ptr, and when we try to dump
> the exception regs we may call dump_mem() for memory immediately above
> the IRQ stack range, rather than for the relevant range on the task
> stack.
> 
> This will result in misleading, though should not be otherwise
> problematic. The initial percpu areas (including the IRQ stacks) are
> allocated in the linear map, and dump_mem uses __get_user(), so we
> shouldn't access anything with side-effects, and will handle holes
> safely.
> 
> This patch fixes the issue by handling the NULL tsk case before we do
> anything else with tsk.

Looking again, it seems we have a similar issue in unwind_frame(), at
least when called by profile_pc(). In that case, we'll needlessly bail
out early with -EINVAL.

I'll spin a fix for that in v2...

Thanks,
Mark.

[PATCH] arm64: fix dump_backtrace with NULL tsk

[James Morse]

Hi Mark,

On 23/09/16 15:56, Mark Rutland wrote:
> In some places, dump_backtrace() is called with a NULL tsk parameter,
> e.g. in bug_handler() in arch/arm64, or indirectly via show_stack() in
> core code. The expectation is that this is treated as if current were
> passed instead of NULL.
> 
> Commit a80a0eb70c358f8c ("arm64: make irq_stack_ptr more robust") didn't
> take this into account, and compares tsk against current *before* we
> check if tsk is NULL.
> 
> Due to this, we won't initialise irq_stack_ptr, and when we try to dump
> the exception regs we may call dump_mem() for memory immediately above
> the IRQ stack range, rather than for the relevant range on the task
> stack.

Bother, I should have spotted that.
Thanks for catching this!

Acked-by: James Morse <james.morse@arm.com>



Thanks,

James

[PATCH] arm64: fix dump_backtrace with NULL tsk

[Mark Rutland]

On Fri, Sep 23, 2016 at 04:58:15PM +0100, James Morse wrote:
> Hi Mark,
> 
> On 23/09/16 15:56, Mark Rutland wrote:
> > In some places, dump_backtrace() is called with a NULL tsk parameter,
> > e.g. in bug_handler() in arch/arm64, or indirectly via show_stack() in
> > core code. The expectation is that this is treated as if current were
> > passed instead of NULL.
> > 
> > Commit a80a0eb70c358f8c ("arm64: make irq_stack_ptr more robust") didn't
> > take this into account, and compares tsk against current *before* we
> > check if tsk is NULL.
> > 
> > Due to this, we won't initialise irq_stack_ptr, and when we try to dump
> > the exception regs we may call dump_mem() for memory immediately above
> > the IRQ stack range, rather than for the relevant range on the task
> > stack.
> 
> Bother, I should have spotted that.

FWIW, it certainly wasn't obvious!

I only noticed because I had to vet all the callers for
try_get_task_stack() ... put_task_stack() correctness with
THREAD_INFO_IN_TASK.

> Thanks for catching this!
> 
> Acked-by: James Morse <james.morse@arm.com>

Cheers!