Re: [PATCH 3/3] arm64: stacktrace: better handle corrupted stacks

[James Morse <james.morse@arm.com>]

Hi Mark,

On 24/06/2019 12:34, James Morse wrote:
> On 06/06/2019 13:54, Mark Rutland wrote:
>> The arm64 stacktrace code is careful to only dereference frame records
>> in valid stack ranges, ensuring that a corrupted frame record won't
>> result in a faulting access.
>>
>> However, it's still possible for corrupt frame records to result in
>> infinite loops in the stacktrace code, which is also undesirable.
>>
>> This patch ensures that we complete a stacktrace in finite time, by
>> keeping track of which stacks we have already completed unwinding, and
>> verifying that if the next frame record is on the same stack, it is at a
>> higher address.
> 
> This looks good, I tried to take it for a spin to test SDEI stack tracing ... but it
> wouldn't boot, it panic()s before earlycon.
> 
> defconfig doesn't do this, defconfig+CONFIG_PROVE_LOCKING does.
> Toggling CONFIG_DEBUG_LOCK_ALLOC is the smallest config change to make this show up.
> 
> Its taking a translation fault:
> | <__ll_sc_arch_atomic64_or>:
> |        f9800031        prfm    pstl1strm, [x1]
> |        c85f7c31        ldxr    x17, [x1]		(faulting instruction)
> |        aa000231        orr     x17, x17, x0
> |        c8107c31        stxr    w16, x17, [x1]
> |        35ffffb0        cbnz    w16, ffff000010c7d19c <__ll_sc_a
> |        d65f03c0        ret
> 
> x0: 0x0000000000000100
> x1: 0xffff0000137399e8			(far_el2)
> 
> If x1 were part of 'frame' in __save_stack_trace it should be on the stack, but at
> fault-time sp is 0xffff0000114a3a50. This happens before the linear map has been set up....
> 
> The lr points just after the set_bit() call in unwind_frame().

frame->stack_current is uninitialized/junk, so the calculated bit to set is outside of
mapped memory.

Lockdep is relevant because it uses save_stack_trace() which doesn't use the
start_backtrace() helper that initialises the new metadata.
DEBUG_LOCK_ALLOC was a red-herring, it was perturbing the stack layout so this code ate a
pointer instead of a much more believable 0.

Patch below, this needs to come before patch 3.
I'll give this a spin with the SDEI firmware.


Thanks,

James

----------------------------------%<----------------------------------
From: James Morse <james.morse@arm.com>
Date:   Tue Jun 25 11:05:33 2019 +0100

arm64: stacktrace: use start_backtrace() consistently

unwind_frame() is about to get smart when it comes to validating
stack frames and stepping between stacks without going round in
circles.

All this relies on new parameters in struct stackframe being
initialised. Before we can do this, we need all users of
struct stackframe to use start_backtrace(), instead of packing
the values manually.

Signed-off-by: James Morse <james.morse@arm.com>
---
 arch/arm64/kernel/perf_callchain.c |  7 +------
 arch/arm64/kernel/return_address.c |  8 +++-----
 arch/arm64/kernel/stacktrace.c     | 20 ++++++--------------
 3 files changed, 10 insertions(+), 25 deletions(-)

---
diff --git a/arch/arm64/kernel/perf_callchain.c b/arch/arm64/kernel/perf_callcha
in.c
index 61d983f5756f..1510ccbd7cb2 100644
--- a/arch/arm64/kernel/perf_callchain.c
+++ b/arch/arm64/kernel/perf_callchain.c
@@ -165,12 +165,7 @@ void perf_callchain_kernel(struct perf_callchain_entry_ctx
*entry,
                return;
        }

-       frame.fp = regs->regs[29];
-       frame.pc = regs->pc;
-#ifdef CONFIG_FUNCTION_GRAPH_TRACER
-       frame.graph = 0;
-#endif
-
+       start_backtrace(&frame, current, regs->regs[29], regs->pc);
        walk_stackframe(current, &frame, callchain_trace, entry);
 }

diff --git a/arch/arm64/kernel/return_address.c b/arch/arm64/kernel/return_address.c
index 53c40196b607..36f8888a5e76 100644
--- a/arch/arm64/kernel/return_address.c
+++ b/arch/arm64/kernel/return_address.c
@@ -41,11 +41,9 @@ void *return_address(unsigned int level)
        data.level = level + 2;
        data.addr = NULL;

-       frame.fp = (unsigned long)__builtin_frame_address(0);
-       frame.pc = (unsigned long)return_address; /* dummy */
-#ifdef CONFIG_FUNCTION_GRAPH_TRACER
-       frame.graph = 0;
-#endif
+       start_backtrace(&frame, current,
+                       (unsigned long)__builtin_frame_address(0),
+                       (unsigned long)return_address);

        walk_stackframe(current, &frame, save_return_addr, &data);

diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index 1c45b33c7474..8a1fa90c784d 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -147,12 +147,7 @@ void save_stack_trace_regs(struct pt_regs *regs, struct stack_trace
*trace)
        data.skip = trace->skip;
        data.no_sched_functions = 0;

-       frame.fp = regs->regs[29];
-       frame.pc = regs->pc;
-#ifdef CONFIG_FUNCTION_GRAPH_TRACER
-       frame.graph = 0;
-#endif
-
+       start_backtrace(&frame, current, regs->regs[29], regs->pc);
        walk_stackframe(current, &frame, save_trace, &data);
 }
 EXPORT_SYMBOL_GPL(save_stack_trace_regs);
@@ -171,18 +166,15 @@ static noinline void __save_stack_trace(struct task_struct *tsk,
        data.no_sched_functions = nosched;

        if (tsk != current) {
-               frame.fp = thread_saved_fp(tsk);
-               frame.pc = thread_saved_pc(tsk);
+               start_backtrace(&frame, tsk, thread_saved_fp(tsk),
+                               thread_saved_pc(tsk));
        } else {
                /* We don't want this function nor the caller */
                data.skip += 2;
-               frame.fp = (unsigned long)__builtin_frame_address(0);
-               frame.pc = (unsigned long)__save_stack_trace;
+               start_backtrace(&frame, tsk,
+                               (unsigned long)__builtin_frame_address(0),
+                               (unsigned long)__save_stack_trace);
        }
-#ifdef CONFIG_FUNCTION_GRAPH_TRACER
-       frame.graph = 0;
-#endif
-
        walk_stackframe(tsk, &frame, save_trace, &data);

        put_task_stack(tsk);

----------------------------------%<----------------------------------

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel