[vz7,5/5] ms/x86/unwind: Disable KASAN checks for non-current tasks

Submitted by Konstantin Khorenko on Oct. 31, 2018, 9:43 a.m.

Details

Message ID 20181031094336.21989-6-khorenko@virtuozzo.com
State New
Series "kasan: avoid false positive reports related to stack handling"
Headers show

Commit Message

Konstantin Khorenko Oct. 31, 2018, 9:43 a.m.
From: Josh Poimboeuf <jpoimboe@redhat.com>

There are a handful of callers to save_stack_trace_tsk() and
show_stack() which try to unwind the stack of a task other than current.
In such cases, it's remotely possible that the task is running on one
CPU while the unwinder is reading its stack from another CPU, causing
the unwinder to see stack corruption.

These cases seem to be mostly harmless.  The unwinder has checks which
prevent it from following bad pointers beyond the bounds of the stack.
So it's not really a bug as long as the caller understands that
unwinding another task will not always succeed.

In such cases, it's possible that the unwinder may read a KASAN-poisoned
region of the stack.  Account for that by using READ_ONCE_NOCHECK() when
reading the stack of another task.

Use READ_ONCE() when reading the stack of the current task, since KASAN
warnings can still be useful for finding bugs in that case.

Reported-by: Dmitry Vyukov <dvyukov@google.com>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Dave Jones <davej@codemonkey.org.uk>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Miroslav Benes <mbenes@suse.cz>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/4c575eb288ba9f73d498dfe0acde2f58674598f1.1483978430.git.jpoimboe@redhat.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>

(cherry picked from commit 84936118bdf37bda513d4a361c38181a216427e0)
https://jira.sw.ru/browse/HCI-171
https://pmc.acronis.com/browse/VSTOR-16798

Signed-off-by: Konstantin Khorenko <khorenko@virtuozzo.com>
---
 arch/x86/include/asm/stacktrace.h |  5 ++++-
 arch/x86/kernel/unwind_frame.c    | 19 +++++++++++++++++--
 2 files changed, 21 insertions(+), 3 deletions(-)

Patch hide | download patch | download mbox

diff --git a/arch/x86/include/asm/stacktrace.h b/arch/x86/include/asm/stacktrace.h
index a4fe84fd293c..2af5f0bac060 100644
--- a/arch/x86/include/asm/stacktrace.h
+++ b/arch/x86/include/asm/stacktrace.h
@@ -53,13 +53,16 @@  static inline bool on_stack(struct stack_info *info, void *addr, size_t len)
 static inline unsigned long *
 get_frame_pointer(struct task_struct *task, struct pt_regs *regs)
 {
+	struct inactive_task_frame *frame;
+
 	if (regs)
 		return (unsigned long *)regs->bp;
 
 	if (task == current)
 		return __builtin_frame_address(0);
 
-	return ((struct inactive_task_frame *)task->thread.sp)->bp;
+	frame = (struct inactive_task_frame *)task->thread.sp;
+	return (unsigned long *)READ_ONCE_NOCHECK(frame->bp);
 }
 #else
 static inline unsigned long *
diff --git a/arch/x86/kernel/unwind_frame.c b/arch/x86/kernel/unwind_frame.c
index e74cb1cceda2..97458ab6db69 100644
--- a/arch/x86/kernel/unwind_frame.c
+++ b/arch/x86/kernel/unwind_frame.c
@@ -6,6 +6,21 @@ 
 
 #define FRAME_HEADER_SIZE (sizeof(long) * 2)
 
+/*
+ * This disables KASAN checking when reading a value from another task's stack,
+ * since the other task could be running on another CPU and could have poisoned
+ * the stack in the meantime.
+ */
+#define READ_ONCE_TASK_STACK(task, x)			\
+({							\
+	unsigned long val;				\
+	if (task == current)				\
+		val = READ_ONCE(x);			\
+	else						\
+		val = READ_ONCE_NOCHECK(x);		\
+	val;						\
+})
+
 unsigned long unwind_get_return_address(struct unwind_state *state)
 {
 	if (unwind_done(state))
@@ -36,7 +51,7 @@  static bool update_stack_state(struct unwind_state *state,
 	/* Move state to the next frame: */
 	state->bp = next_bp;
 	addr_p = state->bp + 1;
-	addr = READ_ONCE(*addr_p);
+	addr = READ_ONCE_TASK_STACK(state->task, *addr_p);
 	state->ip = ftrace_graph_ret_addr(state->task, &state->graph_idx,
 					  addr, addr_p);
 
@@ -96,7 +111,7 @@  bool unwind_next_frame(struct unwind_state *state)
 	if (unwind_end(state))
 		goto the_end;
 
-	next_bp = (unsigned long *)READ_ONCE(*state->bp);
+	next_bp = (unsigned long *)READ_ONCE_TASK_STACK(state->task,*state->bp);
 
 	/* make sure the next frame's data is accessible */
 	if (!update_stack_state(state, next_bp)) {