Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Sat, 25 Aug 2018 01:58:26 +0200
Message-Id: <20180824235826.62741-1-jannh@google.com>
Mime-Version: 1.0
Subject: [PATCH] x86/entry/64: wipe KASAN stack shadow in rewind_stack_do_exit()
From:   Jann Horn <jannh@google.com>
To:     Andy Lutomirski <luto@kernel.org>,
        "the arch/x86 maintainers" <x86@kernel.org>, jannh@google.com
Cc:     kernel list <linux-kernel@vger.kernel.org>,
        Dmitry Vyukov <dvyukov@google.com>, kasan-dev@googlegroups.com,
        Andrey Ryabinin <aryabinin@virtuozzo.com>,
        Alexander Potapenko <glider@google.com>,
        Kees Cook <keescook@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Reset the KASAN shadow state of the task stack when rewinding RSP.
Without this, a kernel oops will leave parts of the stack poisoned, and
code running under do_exit() can trip over such poisoned regions and cause
nonsensical false-positive KASAN reports about stack-out-of-bounds bugs.

This patch is 64-bit only because KASAN doesn't exist on 32-bit.

This patch does not wipe exception stacks; if you oops on an exception
stack, you might get random KASAN false-positives from other tasks
afterwards. This is probably relatively uninteresting, since if you're
oopsing on an exception stack, you likely have bigger things to worry
about. It'd be more interesting if vmapped stacks and KASAN were
compatible, since then handle_stack_overflow() would oops from exception
stack context.

Fixes: 2deb4be28077 ("x86/dumpstack: When OOPSing, rewind the stack before do_exit()")
Signed-off-by: Jann Horn <jannh@google.com>
---
I have manually tested that an oops that previously triggered this bug
doesn't trigger it anymore.

It would be possible to rewrite this assembly to use fewer instructions
in non-KASAN builds, but I think it's clearer this way.

If anyone thinks that this thing should also be wiping exception stacks:
I did write some (entirely untested) code that should take care of that
(before realizing that it's rather unlikely to occur in practice because
vmapped stacks and KASAN are mutually exclusive), but I'm not sure
whether it's worth complicating this code for that.
In case anyone's curious how that would look:
https://gist.github.com/thejh/c91f9b4e3cc4c58659bb3cd056c4fa40

 arch/x86/entry/entry_64.S | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index 957dfb693ecc..92d3ad5bd365 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -1673,9 +1673,25 @@ ENTRY(rewind_stack_do_exit)
 	/* Prevent any naive code from trying to unwind to our caller. */
 	xorl	%ebp, %ebp
 
+	movq	%rdi, %r14
+
 	movq	PER_CPU_VAR(cpu_current_top_of_stack), %rax
-	leaq	-PTREGS_SIZE(%rax), %rsp
+	leaq	-PTREGS_SIZE(%rax), %r15
+
+#ifdef CONFIG_KASAN
+	/*
+	 * Remove stack poisons left behind by our old stack.
+	 * Do this before updating RSP to avoid problems in case we get some
+	 * interrupt that is not handled on an exception stack before we're done
+	 * with the unpoisoning.
+	 */
+	movq	%r15, %rdi
+	call	kasan_unpoison_task_stack_below
+#endif
+
+	movq	%r15, %rsp
 	UNWIND_HINT_FUNC sp_offset=PTREGS_SIZE
 
+	movq	%r14, %rdi
 	call	do_exit
 END(rewind_stack_do_exit)
-- 
2.19.0.rc0.228.g281dcd1b4d0-goog