From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1752205AbdGDLgn (ORCPT <rfc822;w@1wt.eu>);
        Tue, 4 Jul 2017 07:36:43 -0400
Received: from shadbolt.e.decadent.org.uk ([88.96.1.126]:48141 "EHLO
        shadbolt.e.decadent.org.uk" rhost-flags-OK-OK-OK-OK)
        by vger.kernel.org with ESMTP id S1751910AbdGDLgm (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Tue, 4 Jul 2017 07:36:42 -0400
Date: Tue, 4 Jul 2017 12:36:11 +0100
From: Ben Hutchings <ben@decadent.org.uk>
To: Michal Hocko <mhocko@kernel.org>, Willy Tarreau <w@1wt.eu>
Cc: 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>
Message-ID: <20170704113611.GA4732@decadent.org.uk>
References: <20170619142358.GA32654@1wt.eu>
 <1498009101.2655.6.camel@decadent.org.uk>
 <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>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="tKW2IUtsqtDRztdT"
Content-Disposition: inline
In-Reply-To: <20170704104211.GG14722@dhcp22.suse.cz>
User-Agent: Mutt/1.5.23 (2014-03-12)
X-SA-Exim-Connect-IP: <locally generated>
X-SA-Exim-Mail-From: ben@decadent.org.uk
Subject: Re: [PATCH] mm: larger stack guard gap, between vmas
X-SA-Exim-Version: 4.2.1 (built Mon, 26 Dec 2011 16:24:06 +0000)
X-SA-Exim-Scanned: Yes (on shadbolt.decadent.org.uk)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


--tKW2IUtsqtDRztdT
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Tue, 2017-07-04 at 12:42 +0200, Michal Hocko wrote:
> On Tue 04-07-17 11:47:28, Willy Tarreau wrote:
> > On Tue, Jul 04, 2017 at 11:35:38AM +0200, Michal Hocko wrote:
[...]
> > But wouldn't this completely disable the check in case such a guard page
> > is installed, and possibly continue to allow the collision when the sta=
ck
> > allocation is large enough to skip this guard page ?
>=20
> Yes and but a PROT_NONE would fault and as the changelog says, we _hope_
> that userspace does the right thing.

It may well not be large enough, because of the same wrong assumptions
that resulted in the kernel's guard page not being large enough.  We
should count it as part of the guard gap but not a substitute.

> > Shouldn't we instead
> > "skip" such a vma and look for the next one ?
>=20
> Yeah, that would be possible, I am not sure it is worth it though. The
> gap as it is implemented now prevents regular mappings to get close to
> the stack. So we only care about those with MAP_FIXED and those can
> screw things already so we really have to rely on userspace doing some
> semi reasonable.
>=20
> > I was thinking about something more like :
> >=20
> > 	prev =3D vma->vm_prev;
> > +	/* Don't consider a possible user-space stack guard page */
> > +	if (prev && !(prev->vm_flags & VM_GROWSDOWN) &&
> > +	=A0=A0=A0=A0!(prev->vm_flags & (VM_WRITE|VM_READ|VM_EXEC)))
> > +		prev =3D prev->vm_prev;
> > +
>=20
> 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:

Subject: mmap: Ignore VM_NONE mappings when checking for space to
 expand the stack

Some user-space run-times (in particular, Java and Rust) allocate
their own guard pages in the main stack.  This didn't work well
before, but it can now block stack expansion where it is safe and would
previously have been allowed.  Ignore such mappings when checking the
size of the gap before expanding.

Reported-by: Ximin Luo <infinity0@debian.org>
References: https://bugs.debian.org/865416
Fixes: 1be7107fbe18 ("mm: larger stack guard gap, between vmas")
Cc: stable@vger.kernel.org
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
 mm/mmap.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/mm/mmap.c b/mm/mmap.c
index a5e3dcd75e79..19f3ce04f24f 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -2243,7 +2243,14 @@ int expand_upwards(struct vm_area_struct *vma, unsig=
ned long address)
 	if (gap_addr < address || gap_addr > TASK_SIZE)
 		gap_addr =3D TASK_SIZE;
=20
-	next =3D vma->vm_next;
+	/*
+	 * Allow VM_NONE mappings in the gap as some applications try
+	 * to make their own stack guards
+	 */
+	for (next =3D vma->vm_next;
+	     next && !(next->vm_flags & (VM_READ | VM_WRITE | VM_EXEC));
+	     next =3D next->vm_next)
+		;
 	if (next && next->vm_start < gap_addr) {
 		if (!(next->vm_flags & VM_GROWSUP))
 			return -ENOMEM;
@@ -2323,11 +2330,17 @@ int expand_downwards(struct vm_area_struct *vma,
 	if (error)
 		return error;
=20
-	/* Enforce stack_guard_gap */
+	/*
+	 * Enforce stack_guard_gap, but allow VM_NONE mappings in the gap
+	 * as some applications try to make their own stack guards
+	 */
 	gap_addr =3D address - stack_guard_gap;
 	if (gap_addr > address)
 		return -ENOMEM;
-	prev =3D vma->vm_prev;
+	for (prev =3D vma->vm_prev;
+	     prev && !(prev->vm_flags & (VM_READ | VM_WRITE | VM_EXEC));
+	     prev =3D prev->vm_prev)
+		;
 	if (prev && prev->vm_end > gap_addr) {
 		if (!(prev->vm_flags & VM_GROWSDOWN))
 			return -ENOMEM;
--- END ---

I don't have a ppc64el machine where I can change the kernel, but I
tried this on x86_64 with the stack limit reduced to 1 MiB and Rust
is able to expand its stack where previously it would crash.

This *doesn't* fix the LibreOffice regression on i386.

Ben.

--=20
Ben Hutchings
The world is coming to an end.	Please log off.


--tKW2IUtsqtDRztdT
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: Digital signature

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQIVAwUBWVt9que/yOyVhhEJAQrE3BAAg7Q839gp6oxD2YzHFZZiiy09x3sXwsVh
VV+nxvmrdyAHR0H3bfflivKdA1lT5I9cB2ombmlkw1Aqv5YOsgVArLyyxK3KgFiX
DVYKAoIjmNVezUp32SPGfxeJIf4AkncRc3C6difVk7ZgTCynf8i2BFbKfwWzX0W4
VpoRqvljUJBVLczl5+Cnm4nTzFU2nD1bI00cO0cN0IynJ+JEWYSa4FLcTA+e2tRx
VOKhNuDQA+ruQq5bpYlvcH9uHFG1yKrrlXIyguaAQcwvRAjwfqQZctmI3Xxjojo+
KgLpW30HSH5owAQ6kSgHOF1ejO93iZjOXwm3886WLLxDp4LsOIkvAVBPsMaAHcg/
DhJJ19T/sevkYhTZskkak32aw6ZX5/26T29ZgjiwFUU6kytDPa5Pe4gkYCvMwk3y
ulu+fGWL6iiVaD2BN6YU18YnrU1ZgIW3jYUb5rrrlmcrCWlLB6zQD9goRRAXYjLx
WQ61KLN/F3JcH95o0Nbzwas2flPA712+uiyWRFQ4lKX7YsSLygmgn5/vZ+DDp+Qp
rGU19B7opKuyYys235QOeswP063dy9UfWmoDLO1DUQGFY6OuhJVQst+bk7JJoypJ
KQwfa7VT6ISGIZNSTdEft2SbPcyvSDNgDzJShjq1p6JN8JTOQAm5IwEYNe3WrUCl
45jhZzxcTzw=
=KpeZ
-----END PGP SIGNATURE-----

--tKW2IUtsqtDRztdT--