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;
Subject: Re: [PATCH] x86/entry/64: wipe KASAN stack shadow in
 rewind_stack_do_exit()
To:     Jann Horn <jannh@google.com>, Andy Lutomirski <luto@kernel.org>,
        the arch/x86 maintainers <x86@kernel.org>
Cc:     kernel list <linux-kernel@vger.kernel.org>,
        Dmitry Vyukov <dvyukov@google.com>, kasan-dev@googlegroups.com,
        Alexander Potapenko <glider@google.com>,
        Kees Cook <keescook@chromium.org>
References: <20180824235826.62741-1-jannh@google.com>
From:   Andrey Ryabinin <aryabinin@virtuozzo.com>
Message-ID: <0897d173-6a30-09df-f16a-76322384fe0d@virtuozzo.com>
Date:   Tue, 28 Aug 2018 12:04:59 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.9.1
MIME-Version: 1.0
In-Reply-To: <20180824235826.62741-1-jannh@google.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Received-SPF: None (protection.outlook.com: virtuozzo.com does not designate
 permitted sender hosts)
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 28 Aug 2018 09:04:46.3581 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 197062af-42c8-4389-16e7-08d60cc54d8b
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 0bc7f26d-0264-416e-a6fc-8352af79c58f
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM0PR08MB3249
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 08/25/2018 02:58 AM, Jann Horn wrote:
> 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


Why this has to be done in the rewind_stack_do_exit()?
Are there any problems with calling the kasan_unpoison_task_stack(current) from oops_end(), before the rewind_stack_do_exit()?

> +
> +	movq	%r15, %rsp
>  	UNWIND_HINT_FUNC sp_offset=PTREGS_SIZE
>  
> +	movq	%r14, %rdi
>  	call	do_exit
>  END(rewind_stack_do_exit)
>