Re: [PATCH] mm: larger stack guard gap, between vmas

[Andy Lutomirski <luto@kernel.org>]

On Wed, Jul 5, 2017 at 9:20 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Wed, Jul 5, 2017 at 9:15 AM, Andy Lutomirski <luto@kernel.org> wrote:
>> On Wed, Jul 5, 2017 at 7:23 AM, Michal Hocko <mhocko@kernel.org> wrote:
>>>
>>> This is really worrying. This doesn't look like a gap at all. It is a
>>> mapping which actually contains a code and so we should absolutely not
>>> allow to scribble over it. So I am afraid the only way forward is to
>>> allow per process stack gap and run this particular program to have a
>>> smaller gap. We basically have two ways. Either /proc/<pid>/$file or
>>> a prctl inherited on exec. The later is a smaller code. What do you
>>> think?
>>
>> Why inherit on exec?
>
> .. because the whole point is that you have an existing binary that breaks.
>
> So you need to be able to wrap it in "let's lower the stack gap, then
> run that known-problematic binary".
>
> If you think the problem is solved by recompiling existing binaries,
> then why are we doing this kernel hack to begin with? The *real*
> solution was always to just fix the damn compiler and ABI.

That's not what I was suggesting at all.  I was suggesting that, if
we're going to suggest a new API, that the new API actually be sane.

>
> That *real* solution is simple and needs no kernel support at all.
>
> In other words, *ALL* of the kernel work in this area is purely to
> support existing binaries. Don't overlook that fact.

Right.  But I think the approach that we're all taking here is a bit
nutty.  We all realize that this issue is a longstanding *GCC* bug
[1], but we're acting like it's a Big Deal (tm) kernel bug that Must
Be Fixed (tm) and therefore is allowed to break ABI.  My security hat
is normally pretty hard-line, but I think it may be time to call BS.

Imagine if Kees had sent some symlink hardening patch that was
default-on and broke a stock distro.  Or if I had sent a vsyscall
hardening patch that broke real code.  It would get reverted right
away, probably along with a diatribe about how we should have known
better.  I think this stack gap stuff is the same thing.  It's not a
security fix -- it's a hardening patch.

Looking at it that way, I think a new inherited-on-exec flag is nucking futs.

I'm starting to think that the right approach is to mostly revert all
this stuff (the execve fixes are fine).  Then start over and think
about it as hardening.  I would suggest the following approach:

 - The stack gap is one page, just like it's been for years.
 - As a hardening feature, if the stack would expand within 64k or
whatever of a non-MAP_FIXED mapping, refuse to expand it.  (This might
have to be a non-hinted mapping, not just a non-MAP_FIXED mapping.)
The idea being that, if you deliberately place a mapping under the
stack, you know what you're doing.  If you're like LibreOffice and do
something daft and are thus exploitable, you're on your own.
 - As a hardening measure, don't let mmap without MAP_FIXED position
something within 64k or whatever of the bottom of the stack unless a
MAP_FIXED mapping is between them.

And that's all.  It's not like a 64k gap actually fixes these bugs for
real -- it just makes them harder to exploit.

[1] The code that GCC generates for char buf[bug number] and alloca()
is flat-out wrong.  Everyone who's ever thought about it all all knows
it and has known about it for years, but no one cared to fix it.