lkml.org 
[lkml]   [2018]   [Feb]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2 1/3] x86/entry: Clear extra registers beyond syscall arguments for 64bit kernels

* Dan Williams <dan.j.williams@intel.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

\
 
 \ /
  Last update: 2018-02-05 12:43    [W:0.066 / U:2.800 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site