linux-kernel - Re: [PATCH v2 1/3] x86/entry: Clear extra registers beyond syscall arguments for 64bit kernels

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 5 Feb 2018 12:42:50 +0100
From:   Ingo Molnar <mingo@...nel.org>
To:     Dan Williams <dan.j.williams@...el.com>
Cc:     tglx@...utronix.de, Andi Kleen <ak@...ux.intel.com>,
        x86@...nel.org, linux-kernel@...r.kernel.org,
        Ingo Molnar <mingo@...hat.com>, luto@...nel.org,
        "H. Peter Anvin" <hpa@...or.com>, torvalds@...ux-foundation.org
Subject: Re: [PATCH v2 1/3] x86/entry: Clear extra registers beyond syscall
 arguments for 64bit kernels


* Dan Williams <dan.j.williams@...el.com> wrote:

> +	/*
> +	 * Sanitize extra registers of values that a speculation attack
> +	 * might want to exploit. In the CONFIG_FRAME_POINTER=y case,
> +	 * the expectation is that %ebp will be clobbered before it
> +	 * could be used.
> +	 */
> +	.macro CLEAR_EXTRA_REGS_NOSPEC
> +	xorq %r15, %r15
> +	xorq %r14, %r14
> +	xorq %r13, %r13
> +	xorq %r12, %r12
> +	xorl %ebx, %ebx
> +#ifndef CONFIG_FRAME_POINTER
> +	xorl %ebp, %ebp
> +#endif
> +	.endm

Yeah, so this series look pretty good to me, but there's one small detail: I think 
RBP should be cleared unconditionally here, even in the CONFIG_FRAME_POINTERS=y 
case, because:

 - It's much easier to think about the validity of this code if we _know_ that
   these particular registers are cleared, so they cannot be used for deep 
   speculation. While typically on frame-pointer kernels most regular C function
   entry sequences will set RBP, there's exceptions:

     - There's various conditional pieces of entry code that run before any 
       RBP-clobbering C function is called. While none of them has an exploitable 
       Spectre 'gadget' at the moment, we'd have to consider this for every future
       change.

     - There's various compiler instrumentation that might run before RBP 
       clobbering of the typical C function prologue: -mfentry is one such case 
       (used by all distro kernels) compiler plugins might be another case. The 
       instrumentation is often hand written in assembly - which would thus have 
       to be 'RBP safe' as well.

     - Sanitizing RBP is not a hard requirement on the compiler: there's versions 
       of GCC where it won't set RBP, such as leaf functions - and other 
       compilers might have different defaults. This fact makes it harder to 
       ascertain that various C functions from low level assembly that we are 
       verifying for 'RBP safety' do indeed sanitize RBP under all circumstances.

   I.e. we cannot universally rely on RBP being sanitized. In _practice_ it will 
   be sanitized, but we don't know for sure without expending quite some effort to 
   think through all the cases.

 - CONFIG_FRAME_POINTERS=y is a non-default debug option with a significant 
   runtime cost that will get less and less testing as time goes forward. So 
   we are complicating the code path and are micro-optimizing an already 
   significantly slower debug build of the kernel.

So all things considered removing the #ifndef would make this angle easier to 
think about: let's just clear all the extra registers.

If you agree then there's no need to resend the series for this reason alone, I'll 
remove the #ifndef when applying the patches.

Thanks,

	Ingo