Re: [PATCH RFC v9 4/7] x86/entry: Erase kernel stack in syscall_trace_enter()

[Kees Cook <keescook@chromium.org>]

On Mon, Mar 5, 2018 at 12:15 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> This is the first I see of any of this, it was apparently not actually
> posted to lkml or anything like that.

This series was still RFC and narrowly focused (e.g. we recently had
compiler folks reviewing it). I'd say we're now to the point where
it's getting mature enough for larger review (I commented on the 0/n
patch to this end earlier today).

> It doesn't actually seem to help *find* bugs at all. As such, it's
> another "paper over and forget" thing that just adds fairly high
> overhead when it's enabled.

This specific class of flaw ("uninitialized" stack contents being used
or leaked) is already being looked for by tons of tools like KASan.
There are teams of people working on it, and they still haven't found
all the cases where these flaws appear.

Luckily we don't have to pick one or the other: we can continue to
look for bugs while defending against them ever happening in the first
place. We've already seen multiple cases of this just with the by-ref
initialization plugin, where a stack content leak goes away because we
asked the compiler to please initialize the memory for us when we
forgot to do it ourselves. Getting the compiler to help us seems like
the obviously correct thing to do, since we're using such a
memory-safety-unfriendly language. :)

> I'm NAK'ing it sight-unseen (see above) just because I'm tired of
> these kinds of pointless things that don't actually strive to improve
> on the kernel, just add more and more overhead for nebulous "things
> may happen", and that just make the code uglier.

I hope you'll reconsider. In this case, I think it does improve the
kernel, especially if we can gain more complete coverage through
native compiler options (instead of just a plugin). Right now, for
example, the kernel is littered with memset()s because the compiler
can't be trusted to correctly zero-init padding, etc. This is an
endless source of bugs, and this patch series provides a comprehensive
and fast way to keep the stack cleared. (See the benchmark I asked for
that compared this 1% "clear only what was used" against the 30% to
wipe the entire stack every time. The latter is obviously insane, and
the former is quite clever and kills an entire bug class.)

-Kees

-- 
Kees Cook
Pixel Security