Message-ID: <CA+55aFx2EqYjCbQwFo4cwMLkTxwdNvLcJxWD2+WAq6i3RgKPug@mail.gmail.com>
Date:   Tue, 23 Aug 2016 17:06:04 -0400
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Andy Lutomirski <luto@...nel.org>
Cc:     Frederic Weisbecker <fweisbec@...il.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Kees Cook <keescook@...omium.org>,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        "H . Peter Anvin" <hpa@...or.com>,
        Nilay Vaish <nilayvaish@...il.com>,
        "the arch/x86 maintainers" <x86@...nel.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Ingo Molnar <mingo@...nel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Brian Gerst <brgerst@...il.com>,
        Byungchul Park <byungchul.park@....com>,
        Peter Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH v4 54/57] x86/mm: convert arch_within_stack_frames() to
 use the new unwinder

On Tue, Aug 23, 2016 at 4:31 PM, Andy Lutomirski <luto@...nel.org> wrote:
>
> I'm a bit confused by what you're objecting to.  If I write:
>
> char buf[123];
>
> func(buf, size);
>
> And func eventually does some usercopy to buf, the idea is to check
> that size is in bounds.

That's the *IDEA*.

That's not what the code actually does.

The code will follow arbitrary stack frames, which seems silly since
it's expensive. At least the old code only checked within one page
(looking at the "stackend" thing), and aborted whenever the trivla
frame pointer chasing didn't. The new code may be a nice abstraction,
but also seems to not do that, and just follow the frame in general.

Should we have nested stacks and copy_to_user()? No. But why have
generic frame following code when we don't want the generic case to
ever trigger? If the code is slower - and Josh said it was quite
noticeably slower, then what's the advantage?

But my *real* objection is that I suspect that in 99% of all cases we
shouldn't do any of this, and the user access hardening should be made
smart enough that we don't need to worry about it. Right now the
hardening is not that smart. It tries to handle the case you mention,
but it does so by *also* handling the case _I_ mentioned, which is the
"trivially statically correct at build time", where the code is

    struct xyz tmp;

    .. fill in tmo ..

     copy_to_user(ptr, &tmp, sizeof(tmp));

where wasting cycles to see if it's on the stack is just stupid.

And quite frankly, I suspect that *most* situations where you copy
from or to the stack are very obvious constant sizes like the above.

Can  you find a _single_ case of a non-constant buffer on the stack?
It's rare. If it's a variably-sized area, 99% of all time it's a
dynamic allocation, not a stack variable.

So I actually suspect that we could just say "let's make it entirely
invalid to copy variably-sized things to/from the stack". Get rid of
this "follow frames" code _entirely_, and just make the rule be that a
variable copy_to/from_user had better not be on the stack. And the
static constant sizes are clearly not about overflows, so if those are
wrong, it's because somebody uses the wrong type entirely, and gcc
should be catching them statically (or we should catch them with other
tools).

Because the fs/stat.c copies really have been some of the hottest
user-copy code examples we have under certain loads. Do we really want
to have stupid code that makes them slower for no possibly valid
reason?

At some point somebody has to just say "That's just TOO STUPID TO LIVE!".

Checking those fs/stat.c copies dynamically is one such case.

              Linus

Powered by blists - more mailing lists