On Mon, Jul 01, 2019 at 11:56:56AM +0100, Dave Martin wrote:

On Fri, Jun 28, 2019 at 04:46:39PM +0100, Mark Rutland wrote:

quoted

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.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Dave Martin <dave.martin@arm.com>
Cc: James Morse <james.morse@arm.com>
Cc: Tengfei Fan <redacted>
Cc: Will Deacon <redacted>
---
 arch/arm64/include/asm/stacktrace.h | 32 ++++++++++++++++++++++++--------
 arch/arm64/kernel/stacktrace.c      | 15 ++++++++++++++-
 2 files changed, 38 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
index 18f90bf1385c..938b96ba1f0f 100644
--- a/arch/arm64/include/asm/stacktrace.h
+++ b/arch/arm64/include/asm/stacktrace.h

@@ -19,19 +19,12 @@
 #include <linux/percpu.h>
 #include <linux/sched.h>
 #include <linux/sched/task_stack.h>
+#include <linux/types.h>
 
 #include <asm/memory.h>
 #include <asm/ptrace.h>
 #include <asm/sdei.h>
 
-struct stackframe {
-	unsigned long fp;
-	unsigned long pc;
-#ifdef CONFIG_FUNCTION_GRAPH_TRACER
-	int graph;
-#endif
-};
-
 enum stack_type {
 	STACK_TYPE_UNKNOWN,
 	STACK_TYPE_TASK,

@@ -39,6 +32,7 @@ enum stack_type {
 	STACK_TYPE_OVERFLOW,
 	STACK_TYPE_SDEI_NORMAL,
 	STACK_TYPE_SDEI_CRITICAL,
+	__NR_STACK_TYPES
 };
 
 struct stack_info {

@@ -47,6 +41,17 @@ struct stack_info {
 	enum stack_type type;
 };
 
+struct stackframe {
+	unsigned long fp;
+	unsigned long pc;
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+	int graph;
+#endif
+	DECLARE_BITMAP(stacks_done, __NR_STACK_TYPES);
+	unsigned long prev_fp;
+	enum stack_type prev_type;
+};
+
 extern int unwind_frame(struct task_struct *tsk, struct stackframe *frame);
 extern void walk_stackframe(struct task_struct *tsk, struct stackframe *frame,
 			    int (*fn)(struct stackframe *, void *), void *data);

@@ -128,6 +133,9 @@ static inline bool on_accessible_stack(const struct task_struct *tsk,
 				       unsigned long sp,
 				       struct stack_info *info)
 {
+	if (info)
+		info->type = STACK_TYPE_UNKNOWN;
+
 	if (on_task_stack(tsk, sp, info))
 		return true;
 	if (tsk != current || preemptible())

@@ -150,6 +158,14 @@ static inline void start_backtrace(struct stackframe *frame,
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 	frame->graph = 0;
 #endif
+
+	/*
+	 * Prime the first unwind, which will be treated as a transition from
+	 * STACK_TYPE_UNKNOWN to some valid stack.
+	 */
+	bitmap_zero(frame->stacks_done, __NR_STACK_TYPES);
+	frame->prev_fp = 0;
+	frame->prev_type = STACK_TYPE_UNKNOWN;
 }
 
 #endif	/* __ASM_STACKTRACE_H */

diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index e5338216deaa..2e4b59e10e71 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c

@@ -43,6 +43,7 @@
 int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
 {
 	unsigned long fp = frame->fp;
+	struct stack_info info, prev_info;
 
 	if (fp & 0xf)
 		return -EINVAL;

@@ -50,11 +51,23 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
 	if (!tsk)
 		tsk = current;
 
-	if (!on_accessible_stack(tsk, fp, NULL))
+	if (!on_accessible_stack(tsk, fp, &info))
 		return -EINVAL;
 
+	if (test_bit(info.type, frame->stacks_done))
+		return -EINVAL;
+
+	if (info.type == frame->prev_type) {
+		if (fp <= frame->prev_fp)
+			return -EINVAL;
+	} else {
+		set_bit(prev_info.type, frame->stacks_done);
+	}
+
 	frame->fp = READ_ONCE_NOCHECK(*(unsigned long *)(fp));
 	frame->pc = READ_ONCE_NOCHECK(*(unsigned long *)(fp + 8));
+	frame->prev_fp = fp;
+	frame->prev_type = info.type;

As in my response on the last series, do we really need to track 2
frames at the same time in struct stackframe?

It's better to think of this as tracking the location and contents of
one stackframe.

If we back up a bit, I want to ensure that if we have a chain A->B->C
and the B->C transition is bogus, we report A and B in the backtrace.

The struct stackframe is a snapshot of the frame record A (which may
have been the FP and LR rather than an in-memory record). The contents
of A tells us where the B can be found, but we need the location of A so
that we can check intra-stack whether A < B.

The transition (i.e., where we came from and where we're going to should
be visible in unwind_frame().  I don't see why we need additional history
in order to detect stack changes or track which stacks have been visited.

Please see my prior reply to James, where I described this.

So, say (incomplete paste-o-code):

 
Let's consider this for the above A->B->C transition.

	unsigned long fp = frame->fp;
	enum stack_type fp_stack = frame->fp_stack;

When we enter with frame describing A, frame->fp points at B, and
frame->fp_stack describes where A is (i.e. it describes the same thing
as prev_type in my code).

  	frame->fp = READ_ONCE_NOCHECK(*(unsigned long *)(fp));
  	frame->pc = READ_ONCE_NOCHECK(*(unsigned long *)(fp + 8));

Here we update frame to be the contents of B.

	if (!on_accessible_stack(tsk, frame->fp, &info))
		return -EINVAL;

... and here we bail out if C is inaccessible, before we report B.

	if (test_bit(info.type, frame->stacks_done))
		return -EINVAL;

Likewise, here we check C's stack type, and bail out an entry earlier
than necessary.

	if (info.type == frame->fp_stack) {

We haven't updated frame->fp_stack  yet, so this is comparing A's
location to C's location, ignoring B.

		/* no stack change */
	} else {
		/* stack change */
	}


This would require stack_backtrace() to be tweaked, so that fp_stack
describes where frame->fp is and the corresponding bit is already set in
stacks_done, rather than starting out as STACK_UNKNOWN.

I haven't fleshed this out, so the idea may fall down somewhere else.

I think I've laid that out above.

Finally, can we enforce which stack transitions are valid?  (i.e.,
TASK -> SDEI or TASK -> IRQ should never be seen in backtraces).
There may be some gotchas here, so it might not be worth it...  I
vaguely remember some past discussion.

We could to some extent, but the valid transitions are a lattice, so
we end up needing some ad-hoc checks.

This isn't necessary to ensure termination so long as we have
intra-stack monotonicity checks and we only permit each stack transition
to occur once.

Thanks,
Mark.

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