Date: Tue, 4 Jul 2017 19:22:47 +0200
From: Michal Hocko <mhocko@kernel.org>
To: Willy Tarreau <w@1wt.eu>
Cc: Ben Hutchings <ben@decadent.org.uk>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        Hugh Dickins <hughd@google.com>, Oleg Nesterov <oleg@redhat.com>,
        "Jason A. Donenfeld" <Jason@zx2c4.com>, Rik van Riel <riel@redhat.com>,
        Larry Woodman <lwoodman@redhat.com>,
        "Kirill A. Shutemov" <kirill@shutemov.name>,
        Tony Luck <tony.luck@intel.com>,
        "James E.J. Bottomley" <jejb@parisc-linux.org>,
        Helge Diller <deller@gmx.de>, James Hogan <james.hogan@imgtec.com>,
        Laura Abbott <labbott@redhat.com>, Greg KH <greg@kroah.com>,
        "security@kernel.org" <security@kernel.org>,
        linux-distros@vs.openwall.org,
        Qualys Security Advisory <qsa@qualys.com>,
        LKML <linux-kernel@vger.kernel.org>, Ximin Luo <infinity0@debian.org>
Subject: Re: [PATCH] mm: larger stack guard gap, between vmas
Message-ID: <20170704172247.GA6178@dhcp22.suse.cz>
References: <20170621092419.GA22051@dhcp22.suse.cz>
 <1498042057.2655.8.camel@decadent.org.uk>
 <1499126133.2707.20.camel@decadent.org.uk>
 <CA+55aFzMX72+Kb=zNgjCf6UfPt+C+e7WDp_rpbSLuOVx1k7iqg@mail.gmail.com>
 <20170704084122.GC14722@dhcp22.suse.cz>
 <20170704093538.GF14722@dhcp22.suse.cz>
 <20170704094728.GB22013@1wt.eu>
 <20170704104211.GG14722@dhcp22.suse.cz>
 <20170704113611.GA4732@decadent.org.uk>
 <20170704155140.GC22013@1wt.eu>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20170704155140.GC22013@1wt.eu>
User-Agent: Mutt/1.5.23 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1983
Lines: 48

On Tue 04-07-17 17:51:40, Willy Tarreau wrote:
> On Tue, Jul 04, 2017 at 12:36:11PM +0100, Ben Hutchings wrote:
> > > If anywhing this would require to have a loop over all PROT_NONE
> > > mappings to not hit into other weird usecases.
> > 
> > That's what I was thinking of.  Tried the following patch:
> (...)
> > -	next = vma->vm_next;
> > +	/*
> > +	 * Allow VM_NONE mappings in the gap as some applications try
> > +	 * to make their own stack guards
> > +	 */
> > +	for (next = vma->vm_next;
> > +	     next && !(next->vm_flags & (VM_READ | VM_WRITE | VM_EXEC));
> > +	     next = next->vm_next)
> > +		;
> 
> That's what I wanted to propose but I feared someone would scream at me
> for this loop :-)

Well, I've been thinking about this some more and the more I think about
it the less I am convinced we should try to be clever here. Why? Because
as soon as somebody tries to manage stacks explicitly you cannot simply
assume anything about the previous mapping. Say some interpret uses
[ mngmnt data][red zone]                 <--[- MAP_GROWSDOWN ]

Now if we consider the red zone's (PROT_NONE) prev mapping we would fail
the expansion even though we haven't hit the red zone and that is
essentially what the Java and rust bugs are about. So we just risk yet
another regression.

Now let's say another example
<--[- MAP_GROWSDOWN][red zone]   <--[- MAP_GROWSDOWN]
   thread 1                            thread 2

Does the more clever code prevent from smashing over unrelated stack?
No because of our VM_GROWS{DOWN,UP} checks which are needed for other
cases. Well we could special case those as well but...

That being said, I am not really convinced that mixing 2 different gap
implemetantions is sane. I guess it should be reasonable to assume that
a PROT_NONE mapping close to the stack is meant to be a red zone and at
this moment we should rather back off and rely on the userspace rather
than risk more weird cornercases and regressions.

-- 
Michal Hocko
SUSE Labs