From 18be37603de81674e41a0b0282326a0debc1696e Mon Sep 17 00:00:00 2001 From: Joe Lawrence Date: Tue, 22 Jan 2019 10:57:22 -0500 Subject: powerpc/livepatch: relax reliable stack tracer checks for first-frame The bottom-most stack frame (the first to be unwound) may be largely uninitialized, for the "Power Architecture 64-Bit ELF V2 ABI" only requires its backchain pointer to be set. The reliable stack tracer should be careful when verifying this frame: skip checks on STACK_FRAME_LR_SAVE and STACK_FRAME_MARKER offsets that may contain uninitialized residual data. Fixes: df78d3f61480 ("powerpc/livepatch: Implement reliable stack tracing for the consistency model") Signed-off-by: Joe Lawrence Signed-off-by: Michael Ellerman --- arch/powerpc/kernel/stacktrace.c | 32 ++++++++++++++++++++++++-------- 1 file changed, 24 insertions(+), 8 deletions(-) (limited to 'arch/powerpc/kernel/stacktrace.c') diff --git a/arch/powerpc/kernel/stacktrace.c b/arch/powerpc/kernel/stacktrace.c index e2c50b55138f..06688f4d557b 100644 --- a/arch/powerpc/kernel/stacktrace.c +++ b/arch/powerpc/kernel/stacktrace.c @@ -84,6 +84,12 @@ save_stack_trace_regs(struct pt_regs *regs, struct stack_trace *trace) EXPORT_SYMBOL_GPL(save_stack_trace_regs); #ifdef CONFIG_HAVE_RELIABLE_STACKTRACE +/* + * This function returns an error if it detects any unreliable features of the + * stack. Otherwise it guarantees that the stack trace is reliable. + * + * If the task is not 'current', the caller *must* ensure the task is inactive. + */ int save_stack_trace_tsk_reliable(struct task_struct *tsk, struct stack_trace *trace) @@ -142,12 +148,6 @@ save_stack_trace_tsk_reliable(struct task_struct *tsk, if (sp & 0xF) return 1; - /* Mark stacktraces with exception frames as unreliable. */ - if (sp <= stack_end - STACK_INT_FRAME_SIZE && - stack[STACK_FRAME_MARKER] == STACK_FRAME_REGS_MARKER) { - return 1; - } - newsp = stack[0]; /* Stack grows downwards; unwinder may only go up. */ if (newsp <= sp) @@ -158,11 +158,26 @@ save_stack_trace_tsk_reliable(struct task_struct *tsk, return 1; /* invalid backlink, too far up. */ } + /* + * We can only trust the bottom frame's backlink, the + * rest of the frame may be uninitialized, continue to + * the next. + */ + if (firstframe) { + firstframe = 0; + goto next; + } + + /* Mark stacktraces with exception frames as unreliable. */ + if (sp <= stack_end - STACK_INT_FRAME_SIZE && + stack[STACK_FRAME_MARKER] == STACK_FRAME_REGS_MARKER) { + return 1; + } + /* Examine the saved LR: it must point into kernel code. */ ip = stack[STACK_FRAME_LR_SAVE]; - if (!firstframe && !__kernel_text_address(ip)) + if (!__kernel_text_address(ip)) return 1; - firstframe = 0; /* * FIXME: IMHO these tests do not belong in @@ -183,6 +198,7 @@ save_stack_trace_tsk_reliable(struct task_struct *tsk, else trace->skip--; +next: if (newsp == stack_end) break; -- cgit From 29a77bbb0cf2cea41fa46f8fa176f6cb1e3182c4 Mon Sep 17 00:00:00 2001 From: Joe Lawrence Date: Tue, 22 Jan 2019 10:57:23 -0500 Subject: powerpc/livepatch: small cleanups in save_stack_trace_tsk_reliable() Mostly cosmetic changes: - Group common stack pointer code at the top - Simplify the first frame logic - Code stackframe iteration into for...loop construct - Check for trace->nr_entries overflow before adding any into the array Suggested-by: Nicolai Stange Signed-off-by: Joe Lawrence Signed-off-by: Michael Ellerman --- arch/powerpc/kernel/stacktrace.c | 40 ++++++++++++++-------------------------- 1 file changed, 14 insertions(+), 26 deletions(-) (limited to 'arch/powerpc/kernel/stacktrace.c') diff --git a/arch/powerpc/kernel/stacktrace.c b/arch/powerpc/kernel/stacktrace.c index 06688f4d557b..28c3c25755d7 100644 --- a/arch/powerpc/kernel/stacktrace.c +++ b/arch/powerpc/kernel/stacktrace.c @@ -95,20 +95,11 @@ save_stack_trace_tsk_reliable(struct task_struct *tsk, struct stack_trace *trace) { unsigned long sp; + unsigned long newsp; unsigned long stack_page = (unsigned long)task_stack_page(tsk); unsigned long stack_end; int graph_idx = 0; - - /* - * The last frame (unwinding first) may not yet have saved - * its LR onto the stack. - */ - int firstframe = 1; - - if (tsk == current) - sp = current_stack_pointer(); - else - sp = tsk->thread.ksp; + bool firstframe; stack_end = stack_page + THREAD_SIZE; if (!is_idle_task(tsk)) { @@ -135,14 +126,20 @@ save_stack_trace_tsk_reliable(struct task_struct *tsk, stack_end -= STACK_FRAME_OVERHEAD; } + if (tsk == current) + sp = current_stack_pointer(); + else + sp = tsk->thread.ksp; + if (sp < stack_page + sizeof(struct thread_struct) || sp > stack_end - STACK_FRAME_MIN_SIZE) { return 1; } - for (;;) { + for (firstframe = true; sp != stack_end; + firstframe = false, sp = newsp) { unsigned long *stack = (unsigned long *) sp; - unsigned long newsp, ip; + unsigned long ip; /* sanity check: ABI requires SP to be aligned 16 bytes. */ if (sp & 0xF) @@ -163,10 +160,8 @@ save_stack_trace_tsk_reliable(struct task_struct *tsk, * rest of the frame may be uninitialized, continue to * the next. */ - if (firstframe) { - firstframe = 0; - goto next; - } + if (firstframe) + continue; /* Mark stacktraces with exception frames as unreliable. */ if (sp <= stack_end - STACK_INT_FRAME_SIZE && @@ -193,19 +188,12 @@ save_stack_trace_tsk_reliable(struct task_struct *tsk, return 1; #endif + if (trace->nr_entries >= trace->max_entries) + return -E2BIG; if (!trace->skip) trace->entries[trace->nr_entries++] = ip; else trace->skip--; - -next: - if (newsp == stack_end) - break; - - if (trace->nr_entries >= trace->max_entries) - return -E2BIG; - - sp = newsp; } return 0; } -- cgit From 3de27dcf8121c2a710ab93dce23e0f5901c29783 Mon Sep 17 00:00:00 2001 From: Joe Lawrence Date: Tue, 22 Jan 2019 10:57:24 -0500 Subject: powerpc/livepatch: return -ERRNO values in save_stack_trace_tsk_reliable() To match its x86 counterpart, save_stack_trace_tsk_reliable() should return -EINVAL in cases that it is currently returning 1. No caller is currently differentiating non-zero error codes, but let's keep the arch-specific implementations consistent. Signed-off-by: Joe Lawrence Signed-off-by: Michael Ellerman --- arch/powerpc/kernel/stacktrace.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) (limited to 'arch/powerpc/kernel/stacktrace.c') diff --git a/arch/powerpc/kernel/stacktrace.c b/arch/powerpc/kernel/stacktrace.c index 28c3c25755d7..cf31ce6c1f53 100644 --- a/arch/powerpc/kernel/stacktrace.c +++ b/arch/powerpc/kernel/stacktrace.c @@ -133,7 +133,7 @@ save_stack_trace_tsk_reliable(struct task_struct *tsk, if (sp < stack_page + sizeof(struct thread_struct) || sp > stack_end - STACK_FRAME_MIN_SIZE) { - return 1; + return -EINVAL; } for (firstframe = true; sp != stack_end; @@ -143,16 +143,16 @@ save_stack_trace_tsk_reliable(struct task_struct *tsk, /* sanity check: ABI requires SP to be aligned 16 bytes. */ if (sp & 0xF) - return 1; + return -EINVAL; newsp = stack[0]; /* Stack grows downwards; unwinder may only go up. */ if (newsp <= sp) - return 1; + return -EINVAL; if (newsp != stack_end && newsp > stack_end - STACK_FRAME_MIN_SIZE) { - return 1; /* invalid backlink, too far up. */ + return -EINVAL; /* invalid backlink, too far up. */ } /* @@ -166,13 +166,13 @@ save_stack_trace_tsk_reliable(struct task_struct *tsk, /* Mark stacktraces with exception frames as unreliable. */ if (sp <= stack_end - STACK_INT_FRAME_SIZE && stack[STACK_FRAME_MARKER] == STACK_FRAME_REGS_MARKER) { - return 1; + return -EINVAL; } /* Examine the saved LR: it must point into kernel code. */ ip = stack[STACK_FRAME_LR_SAVE]; if (!__kernel_text_address(ip)) - return 1; + return -EINVAL; /* * FIXME: IMHO these tests do not belong in @@ -185,7 +185,7 @@ save_stack_trace_tsk_reliable(struct task_struct *tsk, * as unreliable. */ if (ip == (unsigned long)kretprobe_trampoline) - return 1; + return -EINVAL; #endif if (trace->nr_entries >= trace->max_entries) -- cgit From 018cce33c5e62dda265df8ae0ddf7f3a3357ad1f Mon Sep 17 00:00:00 2001 From: Christophe Leroy Date: Thu, 31 Jan 2019 10:08:52 +0000 Subject: powerpc: prep stack walkers for THREAD_INFO_IN_TASK [text copied from commit 9bbd4c56b0b6 ("arm64: prep stack walkers for THREAD_INFO_IN_TASK")] When CONFIG_THREAD_INFO_IN_TASK is selected, task stacks may be freed before a task is destroyed. To account for this, the stacks are refcounted, and when manipulating the stack of another task, it is necessary to get/put the stack to ensure it isn't freed and/or re-used while we do so. This patch reworks the powerpc stack walking code to account for this. When CONFIG_THREAD_INFO_IN_TASK is not selected these perform no refcounting, and this should only be a structural change that does not affect behaviour. Acked-by: Mark Rutland Signed-off-by: Christophe Leroy Reviewed-by: Nicholas Piggin [mpe: Move try_get_task_stack() below tsk == NULL check in show_stack()] Signed-off-by: Michael Ellerman --- arch/powerpc/kernel/stacktrace.c | 29 ++++++++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-) (limited to 'arch/powerpc/kernel/stacktrace.c') diff --git a/arch/powerpc/kernel/stacktrace.c b/arch/powerpc/kernel/stacktrace.c index cf31ce6c1f53..f958f3bcba04 100644 --- a/arch/powerpc/kernel/stacktrace.c +++ b/arch/powerpc/kernel/stacktrace.c @@ -67,12 +67,17 @@ void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace) { unsigned long sp; + if (!try_get_task_stack(tsk)) + return; + if (tsk == current) sp = current_stack_pointer(); else sp = tsk->thread.ksp; save_context_stack(trace, sp, tsk, 0); + + put_task_stack(tsk); } EXPORT_SYMBOL_GPL(save_stack_trace_tsk); @@ -90,9 +95,8 @@ EXPORT_SYMBOL_GPL(save_stack_trace_regs); * * If the task is not 'current', the caller *must* ensure the task is inactive. */ -int -save_stack_trace_tsk_reliable(struct task_struct *tsk, - struct stack_trace *trace) +static int __save_stack_trace_tsk_reliable(struct task_struct *tsk, + struct stack_trace *trace) { unsigned long sp; unsigned long newsp; @@ -197,6 +201,25 @@ save_stack_trace_tsk_reliable(struct task_struct *tsk, } return 0; } + +int save_stack_trace_tsk_reliable(struct task_struct *tsk, + struct stack_trace *trace) +{ + int ret; + + /* + * If the task doesn't have a stack (e.g., a zombie), the stack is + * "reliably" empty. + */ + if (!try_get_task_stack(tsk)) + return 0; + + ret = __save_stack_trace_tsk_reliable(tsk, trace); + + put_task_stack(tsk); + + return ret; +} EXPORT_SYMBOL_GPL(save_stack_trace_tsk_reliable); #endif /* CONFIG_HAVE_RELIABLE_STACKTRACE */ -- cgit From 39070a96a1c2c502b2f77972ba8c2eba3ca6cd3a Mon Sep 17 00:00:00 2001 From: Joe Lawrence Date: Fri, 1 Mar 2019 14:17:21 -0500 Subject: powerpc: Remove export of save_stack_trace_tsk_reliable() As tglx points out, there are no in-tree module users of save_stack_trace_tsk_reliable() and its x86 counterpart is not exported, so remove the powerpc symbol export. Suggested-by: Thomas Gleixner Signed-off-by: Joe Lawrence Signed-off-by: Michael Ellerman --- arch/powerpc/kernel/stacktrace.c | 1 - 1 file changed, 1 deletion(-) (limited to 'arch/powerpc/kernel/stacktrace.c') diff --git a/arch/powerpc/kernel/stacktrace.c b/arch/powerpc/kernel/stacktrace.c index f958f3bcba04..1e2276963f6d 100644 --- a/arch/powerpc/kernel/stacktrace.c +++ b/arch/powerpc/kernel/stacktrace.c @@ -220,7 +220,6 @@ int save_stack_trace_tsk_reliable(struct task_struct *tsk, return ret; } -EXPORT_SYMBOL_GPL(save_stack_trace_tsk_reliable); #endif /* CONFIG_HAVE_RELIABLE_STACKTRACE */ #if defined(CONFIG_PPC_BOOK3S_64) && defined(CONFIG_NMI_IPI) -- cgit