* [PATCHv3 1/3] arm64: stacktrace: Constify stacktrace.h functions
2019-07-02 13:07 [PATCHv3 0/3] arm64: stacktrace: improve robustness Mark Rutland
@ 2019-07-02 13:07 ` Mark Rutland
2019-07-02 13:07 ` [PATCHv3 2/3] arm64: stacktrace: Factor out backtrace initialisation Mark Rutland
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Mark Rutland @ 2019-07-02 13:07 UTC (permalink / raw)
To: linux-arm-kernel
Cc: mark.rutland, tengfeif, catalin.marinas, will.deacon, james.morse,
Dave Martin
From: Dave Martin <Dave.Martin@arm.com>
on_accessible_stack() and on_task_stack() shouldn't (and don't)
modify their task argument, so it can be const.
This patch adds the appropriate modifiers. Whitespace violations in the
parameter lists are fixed at the same time.
No functional change.
Signed-off-by: Dave Martin <dave.martin@arm.com>
[Mark: fixup const location, whitespace]
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
---
arch/arm64/include/asm/stacktrace.h | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
index e86737b7c924..4dd569592e65 100644
--- a/arch/arm64/include/asm/stacktrace.h
+++ b/arch/arm64/include/asm/stacktrace.h
@@ -75,8 +75,9 @@ static inline bool on_irq_stack(unsigned long sp,
return true;
}
-static inline bool on_task_stack(struct task_struct *tsk, unsigned long sp,
- struct stack_info *info)
+static inline bool on_task_stack(const struct task_struct *tsk,
+ unsigned long sp,
+ struct stack_info *info)
{
unsigned long low = (unsigned long)task_stack_page(tsk);
unsigned long high = low + THREAD_SIZE;
@@ -123,9 +124,9 @@ static inline bool on_overflow_stack(unsigned long sp,
* We can only safely access per-cpu stacks from current in a non-preemptible
* context.
*/
-static inline bool on_accessible_stack(struct task_struct *tsk,
- unsigned long sp,
- struct stack_info *info)
+static inline bool on_accessible_stack(const struct task_struct *tsk,
+ unsigned long sp,
+ struct stack_info *info)
{
if (on_task_stack(tsk, sp, info))
return true;
--
2.11.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCHv3 2/3] arm64: stacktrace: Factor out backtrace initialisation
2019-07-02 13:07 [PATCHv3 0/3] arm64: stacktrace: improve robustness Mark Rutland
2019-07-02 13:07 ` [PATCHv3 1/3] arm64: stacktrace: Constify stacktrace.h functions Mark Rutland
@ 2019-07-02 13:07 ` Mark Rutland
2019-07-02 13:07 ` [PATCHv3 3/3] arm64: stacktrace: better handle corrupted stacks Mark Rutland
2019-07-22 10:41 ` [PATCHv3 0/3] arm64: stacktrace: improve robustness Catalin Marinas
3 siblings, 0 replies; 6+ messages in thread
From: Mark Rutland @ 2019-07-02 13:07 UTC (permalink / raw)
To: linux-arm-kernel
Cc: mark.rutland, tengfeif, catalin.marinas, will.deacon, james.morse,
Dave Martin
From: Dave Martin <Dave.Martin@arm.com>
Some common code is required by each stacktrace user to initialise
struct stackframe before the first call to unwind_frame().
In preparation for adding to the common code, this patch factors it
out into a separate function start_backtrace(), and modifies the
stacktrace callers appropriately.
No functional change.
Signed-off-by: Dave Martin <dave.martin@arm.com>
[Mark: drop tsk argument, update more callsites]
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Reviewed-by: James Morse <james.morse@arm.com>
---
arch/arm64/include/asm/stacktrace.h | 10 ++++++++++
arch/arm64/kernel/perf_callchain.c | 7 +------
arch/arm64/kernel/process.c | 7 ++-----
arch/arm64/kernel/return_address.c | 9 +++------
arch/arm64/kernel/stacktrace.c | 19 ++++++-------------
arch/arm64/kernel/time.c | 7 ++-----
arch/arm64/kernel/traps.c | 13 ++++++-------
7 files changed, 30 insertions(+), 42 deletions(-)
diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
index 4dd569592e65..18f90bf1385c 100644
--- a/arch/arm64/include/asm/stacktrace.h
+++ b/arch/arm64/include/asm/stacktrace.h
@@ -142,4 +142,14 @@ static inline bool on_accessible_stack(const struct task_struct *tsk,
return false;
}
+static inline void start_backtrace(struct stackframe *frame,
+ unsigned long fp, unsigned long pc)
+{
+ frame->fp = fp;
+ frame->pc = pc;
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+ frame->graph = 0;
+#endif
+}
+
#endif /* __ASM_STACKTRACE_H */
diff --git a/arch/arm64/kernel/perf_callchain.c b/arch/arm64/kernel/perf_callchain.c
index 61d983f5756f..35d024735869 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, regs->regs[29], regs->pc);
walk_stackframe(current, &frame, callchain_trace, entry);
}
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index 58efc3727778..46d6b21630a2 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -509,11 +509,8 @@ unsigned long get_wchan(struct task_struct *p)
if (!stack_page)
return 0;
- frame.fp = thread_saved_fp(p);
- frame.pc = thread_saved_pc(p);
-#ifdef CONFIG_FUNCTION_GRAPH_TRACER
- frame.graph = 0;
-#endif
+ start_backtrace(&frame, thread_saved_fp(p), thread_saved_pc(p));
+
do {
if (unwind_frame(p, &frame))
goto out;
diff --git a/arch/arm64/kernel/return_address.c b/arch/arm64/kernel/return_address.c
index 53c40196b607..1b25321c7bf3 100644
--- a/arch/arm64/kernel/return_address.c
+++ b/arch/arm64/kernel/return_address.c
@@ -41,12 +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,
+ (unsigned long)__builtin_frame_address(0),
+ (unsigned long)return_address);
walk_stackframe(current, &frame, save_return_addr, &data);
if (!data.level)
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index b00ec7d483d1..e5338216deaa 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -133,12 +133,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, regs->regs[29], regs->pc);
walk_stackframe(current, &frame, save_trace, &data);
}
EXPORT_SYMBOL_GPL(save_stack_trace_regs);
@@ -157,17 +152,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, 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,
+ (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);
diff --git a/arch/arm64/kernel/time.c b/arch/arm64/kernel/time.c
index a777ae90044d..6af8d976a2d8 100644
--- a/arch/arm64/kernel/time.c
+++ b/arch/arm64/kernel/time.c
@@ -49,11 +49,8 @@ unsigned long profile_pc(struct pt_regs *regs)
if (!in_lock_functions(regs->pc))
return regs->pc;
- frame.fp = regs->regs[29];
- frame.pc = regs->pc;
-#ifdef CONFIG_FUNCTION_GRAPH_TRACER
- frame.graph = 0;
-#endif
+ start_backtrace(&frame, regs->regs[29], regs->pc);
+
do {
int ret = unwind_frame(NULL, &frame);
if (ret < 0)
diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index 8a395523adcf..44810d5aab49 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -111,18 +111,17 @@ void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
return;
if (tsk == current) {
- frame.fp = (unsigned long)__builtin_frame_address(0);
- frame.pc = (unsigned long)dump_backtrace;
+ start_backtrace(&frame,
+ (unsigned long)__builtin_frame_address(0),
+ (unsigned long)dump_backtrace);
} else {
/*
* task blocked in __switch_to
*/
- frame.fp = thread_saved_fp(tsk);
- frame.pc = thread_saved_pc(tsk);
+ start_backtrace(&frame,
+ thread_saved_fp(tsk),
+ thread_saved_pc(tsk));
}
-#ifdef CONFIG_FUNCTION_GRAPH_TRACER
- frame.graph = 0;
-#endif
printk("Call trace:\n");
do {
--
2.11.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCHv3 3/3] arm64: stacktrace: better handle corrupted stacks
2019-07-02 13:07 [PATCHv3 0/3] arm64: stacktrace: improve robustness Mark Rutland
2019-07-02 13:07 ` [PATCHv3 1/3] arm64: stacktrace: Constify stacktrace.h functions Mark Rutland
2019-07-02 13:07 ` [PATCHv3 2/3] arm64: stacktrace: Factor out backtrace initialisation Mark Rutland
@ 2019-07-02 13:07 ` Mark Rutland
2019-07-03 9:13 ` Dave Martin
2019-07-22 10:41 ` [PATCHv3 0/3] arm64: stacktrace: improve robustness Catalin Marinas
3 siblings, 1 reply; 6+ messages in thread
From: Mark Rutland @ 2019-07-02 13:07 UTC (permalink / raw)
To: linux-arm-kernel
Cc: mark.rutland, tengfeif, catalin.marinas, will.deacon, james.morse,
dave.martin
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.
As this has turned out to be particularly subtle, comments are added to
explain the procedure.
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Reviewed-by: James Morse <james.morse@arm.com>
Tested-by: James Morse <james.morse@arm.com>
Acked-by: Dave Martin <Dave.Martin@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Tengfei Fan <tengfeif@codeaurora.org>
Cc: Will Deacon <will.deacon@arm.com>
---
arch/arm64/include/asm/stacktrace.h | 57 +++++++++++++++++++++++++++++++------
arch/arm64/kernel/stacktrace.c | 40 +++++++++++++++++++++++++-
2 files changed, 88 insertions(+), 9 deletions(-)
diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
index 18f90bf1385c..b3e03bbb69ea 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,37 @@ struct stack_info {
enum stack_type type;
};
+/*
+ * A snapshot of a frame record or fp/lr register values, along with some
+ * accounting information necessary for robust unwinding.
+ *
+ * @fp: The fp value in the frame record (or the real fp)
+ * @pc: The fp value in the frame record (or the real lr)
+ *
+ * @stacks_done: Stacks which have been entirely unwound, for which it is no
+ * longer valid to unwind to.
+ *
+ * @prev_fp: The fp that pointed to this frame record, or a synthetic value
+ * of 0. This is used to ensure that within a stack, each
+ * subsequent frame record is at an increasing address.
+ * @prev_type: The type of stack this frame record was on, or a synthetic
+ * value of STACK_TYPE_UNKNOWN. This is used to detect a
+ * transition from one stack to another.
+ *
+ * @graph: When FUNCTION_GRAPH_TRACER is selected, holds the index of a
+ * replacement lr value in the ftrace graph stack.
+ */
+struct stackframe {
+ unsigned long fp;
+ unsigned long pc;
+ DECLARE_BITMAP(stacks_done, __NR_STACK_TYPES);
+ unsigned long prev_fp;
+ enum stack_type prev_type;
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+ int graph;
+#endif
+};
+
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 +153,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 +178,19 @@ static inline void start_backtrace(struct stackframe *frame,
#ifdef CONFIG_FUNCTION_GRAPH_TRACER
frame->graph = 0;
#endif
+
+ /*
+ * Prime the first unwind.
+ *
+ * In unwind_frame() we'll check that the FP points to a valid stack,
+ * which can't be STACK_TYPE_UNKNOWN, and the first unwind will be
+ * treated as a transition to whichever stack that happens to be. The
+ * prev_fp value won't be used, but we set it to 0 such that it is
+ * definitely not an accessible stack address.
+ */
+ 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..8094bfe654f5 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -40,9 +40,18 @@
* ldp x29, x30, [sp]
* add sp, sp, #0x10
*/
+
+/*
+ * Unwind from one frame record (A) to the next frame record (B).
+ *
+ * We terminate early if the location of B indicates a malformed chain of frame
+ * records (e.g. a cycle), determined based on the location and fp value of A
+ * and the location (but not the fp value) of B.
+ */
int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
{
unsigned long fp = frame->fp;
+ struct stack_info info;
if (fp & 0xf)
return -EINVAL;
@@ -50,11 +59,40 @@ 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;
+
+ /*
+ * As stacks grow downward, any valid record on the same stack must be
+ * at a strictly higher address than the prior record.
+ *
+ * Stacks can nest in several valid orders, e.g.
+ *
+ * TASK -> IRQ -> OVERFLOW -> SDEI_NORMAL
+ * TASK -> SDEI_NORMAL -> SDEI_CRITICAL -> OVERFLOW
+ *
+ * ... but the nesting itself is strict. Once we transition from one
+ * stack to another, it's never valid to unwind back to that first
+ * stack.
+ */
+ if (info.type == frame->prev_type) {
+ if (fp <= frame->prev_fp)
+ return -EINVAL;
+ } else {
+ set_bit(frame->prev_type, frame->stacks_done);
+ }
+
+ /*
+ * Record this frame record's values and location. The prev_fp and
+ * prev_type are only meaningful to the next unwind_frame() invocation.
+ */
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;
#ifdef CONFIG_FUNCTION_GRAPH_TRACER
if (tsk->ret_stack &&
--
2.11.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCHv3 3/3] arm64: stacktrace: better handle corrupted stacks
2019-07-02 13:07 ` [PATCHv3 3/3] arm64: stacktrace: better handle corrupted stacks Mark Rutland
@ 2019-07-03 9:13 ` Dave Martin
0 siblings, 0 replies; 6+ messages in thread
From: Dave Martin @ 2019-07-03 9:13 UTC (permalink / raw)
To: Mark Rutland
Cc: catalin.marinas, tengfeif, will.deacon, james.morse,
linux-arm-kernel
On Tue, Jul 02, 2019 at 02:07:29PM +0100, 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.
>
> As this has turned out to be particularly subtle, comments are added to
> explain the procedure.
>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Reviewed-by: James Morse <james.morse@arm.com>
> Tested-by: James Morse <james.morse@arm.com>
> Acked-by: Dave Martin <Dave.Martin@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Tengfei Fan <tengfeif@codeaurora.org>
> Cc: Will Deacon <will.deacon@arm.com>
> ---
> arch/arm64/include/asm/stacktrace.h | 57 +++++++++++++++++++++++++++++++------
> arch/arm64/kernel/stacktrace.c | 40 +++++++++++++++++++++++++-
> 2 files changed, 88 insertions(+), 9 deletions(-)
>
> diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
> index 18f90bf1385c..b3e03bbb69ea 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,37 @@ struct stack_info {
> enum stack_type type;
> };
>
> +/*
> + * A snapshot of a frame record or fp/lr register values, along with some
> + * accounting information necessary for robust unwinding.
> + *
> + * @fp: The fp value in the frame record (or the real fp)
> + * @pc: The fp value in the frame record (or the real lr)
> + *
> + * @stacks_done: Stacks which have been entirely unwound, for which it is no
> + * longer valid to unwind to.
> + *
> + * @prev_fp: The fp that pointed to this frame record, or a synthetic value
> + * of 0. This is used to ensure that within a stack, each
> + * subsequent frame record is at an increasing address.
> + * @prev_type: The type of stack this frame record was on, or a synthetic
> + * value of STACK_TYPE_UNKNOWN. This is used to detect a
> + * transition from one stack to another.
> + *
> + * @graph: When FUNCTION_GRAPH_TRACER is selected, holds the index of a
> + * replacement lr value in the ftrace graph stack.
> + */
> +struct stackframe {
> + unsigned long fp;
> + unsigned long pc;
> + DECLARE_BITMAP(stacks_done, __NR_STACK_TYPES);
> + unsigned long prev_fp;
> + enum stack_type prev_type;
> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> + int graph;
> +#endif
> +};
> +
> 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 +153,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 +178,19 @@ static inline void start_backtrace(struct stackframe *frame,
> #ifdef CONFIG_FUNCTION_GRAPH_TRACER
> frame->graph = 0;
> #endif
> +
> + /*
> + * Prime the first unwind.
> + *
> + * In unwind_frame() we'll check that the FP points to a valid stack,
> + * which can't be STACK_TYPE_UNKNOWN, and the first unwind will be
> + * treated as a transition to whichever stack that happens to be. The
> + * prev_fp value won't be used, but we set it to 0 such that it is
> + * definitely not an accessible stack address.
> + */
> + 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..8094bfe654f5 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -40,9 +40,18 @@
> * ldp x29, x30, [sp]
> * add sp, sp, #0x10
> */
> +
> +/*
> + * Unwind from one frame record (A) to the next frame record (B).
> + *
> + * We terminate early if the location of B indicates a malformed chain of frame
> + * records (e.g. a cycle), determined based on the location and fp value of A
> + * and the location (but not the fp value) of B.
> + */
> int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
> {
> unsigned long fp = frame->fp;
> + struct stack_info info;
>
> if (fp & 0xf)
> return -EINVAL;
> @@ -50,11 +59,40 @@ 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;
> +
> + /*
> + * As stacks grow downward, any valid record on the same stack must be
> + * at a strictly higher address than the prior record.
> + *
> + * Stacks can nest in several valid orders, e.g.
> + *
> + * TASK -> IRQ -> OVERFLOW -> SDEI_NORMAL
> + * TASK -> SDEI_NORMAL -> SDEI_CRITICAL -> OVERFLOW
> + *
> + * ... but the nesting itself is strict. Once we transition from one
> + * stack to another, it's never valid to unwind back to that first
> + * stack.
> + */
> + if (info.type == frame->prev_type) {
> + if (fp <= frame->prev_fp)
> + return -EINVAL;
> + } else {
> + set_bit(frame->prev_type, frame->stacks_done);
> + }
> +
> + /*
> + * Record this frame record's values and location. The prev_fp and
> + * prev_type are only meaningful to the next unwind_frame() invocation.
> + */
> 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;
Thanks for the explanation.
This looks less complicated that I had feared.
Cheers
---Dave
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCHv3 0/3] arm64: stacktrace: improve robustness
2019-07-02 13:07 [PATCHv3 0/3] arm64: stacktrace: improve robustness Mark Rutland
` (2 preceding siblings ...)
2019-07-02 13:07 ` [PATCHv3 3/3] arm64: stacktrace: better handle corrupted stacks Mark Rutland
@ 2019-07-22 10:41 ` Catalin Marinas
3 siblings, 0 replies; 6+ messages in thread
From: Catalin Marinas @ 2019-07-22 10:41 UTC (permalink / raw)
To: Mark Rutland
Cc: will.deacon, tengfeif, james.morse, dave.martin, linux-arm-kernel
On Tue, Jul 02, 2019 at 02:07:26PM +0100, Mark Rutland wrote:
> Dave Martin (2):
> arm64: stacktrace: Constify stacktrace.h functions
> arm64: stacktrace: Factor out backtrace initialisation
>
> Mark Rutland (1):
> arm64: stacktrace: better handle corrupted stacks
It was pretty late to queue them for the 5.3 merging window but I think
Will can merge post -rc1. For the series:
Acked-by: Catalin Marinas <catalin.marinas@arm.com>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 6+ messages in thread