arm64: fix dump_backtrace with NULL tsk

PATCH New Mark Rutland 2 participants 4 messages Sep 23, 2016 arm64

Share & tools

Mark Rutland

author reviewer

2016-09-23 14:56 UTC #0 lore raw

PATCH 1/1 arm64: fix dump_backtrace with NULL tsk

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 <[email protected]>

Fixes a80a0eb70c358f8c ("arm64: make irq_stack_ptr more robust")

Cc Catalin Marinas <[email protected]>

Cc James Morse <[email protected]>

Cc Will Deacon <[email protected]>

Cc Yang Shi <[email protected]>

---
 arch/arm64/kernel/traps.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

arch/arm64/kernel/traps.c +5 -5

index e04f838..df06750 100644
@@ -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;

Signature · 1 line

1.9.1

Mark Rutland

author reviewer

2016-09-23 15:52 UTC #1 lore raw

PATCH 1/1 arm64: fix dump_backtrace with NULL tsk

Quoted 14 lines · 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.

Quoted 8 lines

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.

James Morse

2016-09-23 15:58 UTC #2 lore raw

PATCH 1/1 arm64: fix dump_backtrace with NULL tsk

In reply to Mark Rutland

Hi Mark,

Quoted 5 lines · 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.

Quoted 8 lines

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 <[email protected]>

Thanks,

James

Mark Rutland

author reviewer

2016-09-23 16:10 UTC #3 lore raw

PATCH 1/1 arm64: fix dump_backtrace with NULL tsk

Quoted 12 lines · James Morse

Hi Mark,
Quoted 9 lines · 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.

Quoted 6 lines

Quoted 4 lines
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.

Quoted 3 lines

Thanks for catching this!
Acked-by James Morse <[email protected]>

Cheers!