Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1680619 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] mm: larger stack guard gap, between vmas Ben Hutchings <ben@decadent.org.uk> - 2017-07-04 02:00 +0200
    Re: [PATCH] mm: larger stack guard gap, between vmas Linus Torvalds <torvalds@linux-foundation.org> - 2017-07-04 02:10 +0200
      Re: [PATCH] mm: larger stack guard gap, between vmas Michal Hocko <mhocko@kernel.org> - 2017-07-04 10:50 +0200
        Re: [PATCH] mm: larger stack guard gap, between vmas Michal Hocko <mhocko@kernel.org> - 2017-07-04 11:40 +0200
          Re: [PATCH] mm: larger stack guard gap, between vmas Willy Tarreau <w@1wt.eu> - 2017-07-04 11:50 +0200
            Re: [PATCH] mm: larger stack guard gap, between vmas Michal Hocko <mhocko@kernel.org> - 2017-07-04 12:50 +0200
              Re: [PATCH] mm: larger stack guard gap, between vmas Ben Hutchings <ben@decadent.org.uk> - 2017-07-04 13:40 +0200
                Re: [PATCH] mm: larger stack guard gap, between vmas Michal Hocko <mhocko@kernel.org> - 2017-07-04 14:10 +0200
                  Re: [PATCH] mm: larger stack guard gap, between vmas Michal Hocko <mhocko@kernel.org> - 2017-07-04 14:20 +0200
                  Re: [PATCH] mm: larger stack guard gap, between vmas Ben Hutchings <ben@decadent.org.uk> - 2017-07-04 14:30 +0200
                    Re: [PATCH] mm: larger stack guard gap, between vmas Michal Hocko <mhocko@kernel.org> - 2017-07-04 14:40 +0200
                      Re: [PATCH] mm: larger stack guard gap, between vmas Ximin Luo <infinity0@debian.org> - 2017-07-04 16:30 +0200
                        Re: [PATCH] mm: larger stack guard gap, between vmas Michal Hocko <mhocko@kernel.org> - 2017-07-04 16:50 +0200
                Re: [PATCH] mm: larger stack guard gap, between vmas Willy Tarreau <w@1wt.eu> - 2017-07-04 18:00 +0200
                  Re: [PATCH] mm: larger stack guard gap, between vmas Michal Hocko <mhocko@kernel.org> - 2017-07-04 19:30 +0200
                    Re: [PATCH] mm: larger stack guard gap, between vmas Linus Torvalds <torvalds@linux-foundation.org> - 2017-07-04 20:40 +0200
                      Re: [PATCH] mm: larger stack guard gap, between vmas Linus Torvalds <torvalds@linux-foundation.org> - 2017-07-04 20:50 +0200
                        Re: [PATCH] mm: larger stack guard gap, between vmas Willy Tarreau <w@1wt.eu> - 2017-07-04 21:10 +0200
                      Re: [PATCH] mm: larger stack guard gap, between vmas Willy Tarreau <w@1wt.eu> - 2017-07-04 20:50 +0200
                Re: [PATCH] mm: larger stack guard gap, between vmas Linus Torvalds <torvalds@linux-foundation.org> - 2017-07-04 18:20 +0200
                  Re: [PATCH] mm: larger stack guard gap, between vmas John Haxby <john.haxby@oracle.com> - 2017-07-04 18:30 +0200
                    Re: [PATCH] mm: larger stack guard gap, between vmas Willy Tarreau <w@1wt.eu> - 2017-07-04 19:10 +0200
                  Re: [PATCH] mm: larger stack guard gap, between vmas Ben Hutchings <ben@decadent.org.uk> - 2017-07-05 14:30 +0200
                Re: [PATCH] mm: larger stack guard gap, between vmas Willy Tarreau <w@1wt.eu> - 2017-07-04 19:20 +0200
                  Re: [PATCH] mm: larger stack guard gap, between vmas Ben Hutchings <ben@decadent.org.uk> - 2017-07-05 14:30 +0200
                Re: [PATCH] mm: larger stack guard gap, between vmas Ben Hutchings <ben@decadent.org.uk> - 2017-07-05 01:10 +0200
                  Re: [PATCH] mm: larger stack guard gap, between vmas Linus Torvalds <torvalds@linux-foundation.org> - 2017-07-05 01:40 +0200
                    Re: [PATCH] mm: larger stack guard gap, between vmas Michal Hocko <mhocko@kernel.org> - 2017-07-05 08:40 +0200
                      Re: [PATCH] mm: larger stack guard gap, between vmas Willy Tarreau <w@1wt.eu> - 2017-07-05 10:20 +0200
                        Re: [PATCH] mm: larger stack guard gap, between vmas Michal Hocko <mhocko@kernel.org> - 2017-07-05 10:30 +0200
                          Re: [PATCH] mm: larger stack guard gap, between vmas Willy Tarreau <w@1wt.eu> - 2017-07-05 11:20 +0200
                        Re: [PATCH] mm: larger stack guard gap, between vmas Ben Hutchings <ben@decadent.org.uk> - 2017-07-05 14:30 +0200
                          Re: [PATCH] mm: larger stack guard gap, between vmas Willy Tarreau <w@1wt.eu> - 2017-07-05 16:00 +0200
                          Re: [PATCH] mm: larger stack guard gap, between vmas Michal Hocko <mhocko@kernel.org> - 2017-07-05 16:30 +0200
                          Re: [PATCH] mm: larger stack guard gap, between vmas Linus Torvalds <torvalds@linux-foundation.org> - 2017-07-05 18:10 +0200
                      Re: [PATCH] mm: larger stack guard gap, between vmas Michal Hocko <mhocko@kernel.org> - 2017-07-06 09:40 +0200
                    Re: [PATCH] mm: larger stack guard gap, between vmas Ben Hutchings <ben@decadent.org.uk> - 2017-07-05 14:30 +0200
                      Re: [PATCH] mm: larger stack guard gap, between vmas Michal Hocko <mhocko@kernel.org> - 2017-07-05 16:30 +0200
                        Re: [PATCH] mm: larger stack guard gap, between vmas Ben Hutchings <ben@decadent.org.uk> - 2017-07-05 17:30 +0200
                          Re: [PATCH] mm: larger stack guard gap, between vmas Michal Hocko <mhocko@kernel.org> - 2017-07-05 18:10 +0200
                          Re: [PATCH] mm: larger stack guard gap, between vmas Ben Hutchings <ben@decadent.org.uk> - 2017-07-05 19:00 +0200
                            Re: [PATCH] mm: larger stack guard gap, between vmas Michal Hocko <mhocko@kernel.org> - 2017-07-05 19:10 +0200
                              Re: [PATCH] mm: larger stack guard gap, between vmas Ben Hutchings <ben@decadent.org.uk> - 2017-07-05 19:30 +0200
                            Re: [PATCH] mm: larger stack guard gap, between vmas Linus Torvalds <torvalds@linux-foundation.org> - 2017-07-05 19:20 +0200
                              Re: [PATCH] mm: larger stack guard gap, between vmas Ben Hutchings <ben@decadent.org.uk> - 2017-07-06 01:40 +0200
                                Re: [PATCH] mm: larger stack guard gap, between vmas Linus Torvalds <torvalds@linux-foundation.org> - 2017-07-06 02:00 +0200
                                  Re: [PATCH] mm: larger stack guard gap, between vmas Willy Tarreau <w@1wt.eu> - 2017-07-06 10:30 +0200
                                    Re: [PATCH] mm: larger stack guard gap, between vmas Willy Tarreau <w@1wt.eu> - 2017-07-06 12:20 +0200
                        Re: [PATCH] mm: larger stack guard gap, between vmas Andy Lutomirski <luto@kernel.org> - 2017-07-05 18:20 +0200
                          Re: [PATCH] mm: larger stack guard gap, between vmas Linus Torvalds <torvalds@linux-foundation.org> - 2017-07-05 18:30 +0200
                            Re: [PATCH] mm: larger stack guard gap, between vmas Andy Lutomirski <luto@kernel.org> - 2017-07-05 19:30 +0200
                              Re: [PATCH] mm: larger stack guard gap, between vmas Ben Hutchings <ben@decadent.org.uk> - 2017-07-05 21:40 +0200
                                Re: [PATCH] mm: larger stack guard gap, between vmas Willy Tarreau <w@1wt.eu> - 2017-07-05 22:50 +0200
                                Re: [PATCH] mm: larger stack guard gap, between vmas Andy Lutomirski <luto@amacapital.net> - 2017-07-05 23:00 +0200
                                  Re: [PATCH] mm: larger stack guard gap, between vmas Ben Hutchings <ben@decadent.org.uk> - 2017-07-06 02:00 +0200
                                    Re: [PATCH] mm: larger stack guard gap, between vmas Andy Lutomirski <luto@kernel.org> - 2017-07-06 02:30 +0200
                              Re: [PATCH] mm: larger stack guard gap, between vmas Kees Cook <keescook@chromium.org> - 2017-07-06 02:00 +0200
                                Re: [PATCH] mm: larger stack guard gap, between vmas Linus Torvalds <torvalds@linux-foundation.org> - 2017-07-06 02:00 +0200
                                  Re: [PATCH] mm: larger stack guard gap, between vmas Andy Lutomirski <luto@kernel.org> - 2017-07-06 02:40 +0200
                                    Re: [PATCH] mm: larger stack guard gap, between vmas Linus Torvalds <torvalds@linux-foundation.org> - 2017-07-06 02:50 +0200
                                Re: [PATCH] mm: larger stack guard gap, between vmas Andy Lutomirski <luto@kernel.org> - 2017-07-06 02:30 +0200
                                  Re: [PATCH] mm: larger stack guard gap, between vmas Kees Cook <keescook@chromium.org> - 2017-07-06 04:50 +0200
                                  Re: [PATCH] mm: larger stack guard gap, between vmas Willy Tarreau <w@1wt.eu> - 2017-07-06 07:30 +0200
                        Re: [PATCH] mm: larger stack guard gap, between vmas Kevin Easton <kevin@guarana.org> - 2017-07-06 07:40 +0200
                      Re: [PATCH] mm: larger stack guard gap, between vmas Linus Torvalds <torvalds@linux-foundation.org> - 2017-07-05 18:20 +0200
                        Re: [PATCH] mm: larger stack guard gap, between vmas Willy Tarreau <w@1wt.eu> - 2017-07-05 21:10 +0200
                          Re: [PATCH] mm: larger stack guard gap, between vmas Willy Tarreau <w@1wt.eu> - 2017-07-05 21:20 +0200
                            Re: [PATCH] mm: larger stack guard gap, between vmas Linus Torvalds <torvalds@linux-foundation.org> - 2017-07-05 21:30 +0200
                          Re: [PATCH] mm: larger stack guard gap, between vmas Linus Torvalds <torvalds@linux-foundation.org> - 2017-07-05 21:20 +0200
                  Re: [vs-plain] Re: [PATCH] mm: larger stack guard gap, between vmas "kseifried@redhat.com" <kseifried@redhat.com> - 2017-07-05 03:20 +0200
                    Re: [vs-plain] Re: [PATCH] mm: larger stack guard gap, between vmas Solar Designer <solar@openwall.com> - 2017-07-05 16:20 +0200
          Re: [PATCH] mm: larger stack guard gap, between vmas Michal Hocko <mhocko@kernel.org> - 2017-07-04 12:50 +0200
            Re: [PATCH] mm: larger stack guard gap, between vmas Michal Hocko <mhocko@kernel.org> - 2017-07-04 13:00 +0200
    Re: [PATCH] mm: larger stack guard gap, between vmas Andy Lutomirski <luto@kernel.org> - 2017-07-04 02:30 +0200
    Re: [vs-plain] Re: [PATCH] mm: larger stack guard gap, between vmas John Haxby <john.haxby@oracle.com> - 2017-07-04 14:30 +0200

Page 2 of 4 — ← Prev page 1 [2] 3 4  Next page →

#1681081

On 04/07/17 17:18, Linus Torvalds wrote:
> Also, separately, John Haxby kind of implied that the LibreOffice
> regression on i386 is already fixed by commit f4cb767d76cf ("mm: fix
> new crash in unmapped_area_topdown()").

I'm not certain.   We had two distinct problems that were avoided by
Hugh's original patch together with f4cb767d76cf: the Oracle RDBMS
started and java programs worked.   In my mind I'm conflating the second
of these with problems in LibreOffice.

Alas, the people who could confirm this for me are getting ready to
watch fireworks and generally have a good time.

jch

[toc] | [prev] | [next] | [standalone]

#1681097

On Tue, Jul 04, 2017 at 05:27:55PM +0100, John Haxby wrote:
> Alas, the people who could confirm this for me are getting ready to
> watch fireworks and generally have a good time.

Let's hope the fireworks is controlled by Java with on up-to-date
kernel so that they can quickly get back to work :-)

Willy

[toc] | [prev] | [next] | [standalone]

#1681499

[Multipart message — attachments visible in raw view] — view raw

On Tue, 2017-07-04 at 09:18 -0700, Linus Torvalds wrote:
> > On Tue, Jul 4, 2017 at 4:36 AM, Ben Hutchings <ben@decadent.org.uk> wrote:
> > 
> > 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
> 
> This looks sane to me.
> 
> I'm going to ignore it in this thread, and assume that it gets sent as
> a patch separately, ok?
> 
> It would be good to have more acks on it.
> 
> Also, separately, John Haxby kind of implied that the LibreOffice
> regression on i386 is already fixed by commit f4cb767d76cf ("mm: fix
> new crash in unmapped_area_topdown()").
> 
> Or was that a separate issue?

They are separate issues.

Ben.

-- 
Ben Hutchings
Anthony's Law of Force: Don't force it, get a larger hammer.

[toc] | [prev] | [next] | [standalone]

#1681100

On Tue, Jul 04, 2017 at 12:36:11PM +0100, Ben Hutchings wrote:
> @@ -2323,11 +2330,17 @@ int expand_downwards(struct vm_area_struct *vma,
>  	if (error)
>  		return error;
>  
> -	/* 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 = address - stack_guard_gap;
>  	if (gap_addr > address)
>  		return -ENOMEM;
> -	prev = vma->vm_prev;
> +	for (prev = vma->vm_prev;
> +	     prev && !(prev->vm_flags & (VM_READ | VM_WRITE | VM_EXEC));
> +	     prev = prev->vm_prev)
> +		;
>  	if (prev && prev->vm_end > gap_addr) {
>  		if (!(prev->vm_flags & VM_GROWSDOWN))
>  			return -ENOMEM;

Hmmm shouldn't we also stop looping when we're out of the gap ? Something
like this :

	for (prev = vma->vm_prev;
	     prev && !(prev->vm_flags & (VM_READ | VM_WRITE | VM_EXEC)) &&
	        address - prev->vm_end < stack_guard_gap;
	     prev = prev->vm_prev)
		;

This would limit the risk of runaway loops if someone is having fun
allocating a lot of memory in small chunks (eg: 4 GB in 1 million
independant mmap() calls).

Willy

[toc] | [prev] | [next] | [standalone]

#1681508

[Multipart message — attachments visible in raw view] — view raw

On Tue, 2017-07-04 at 19:11 +0200, Willy Tarreau wrote:
> On Tue, Jul 04, 2017 at 12:36:11PM +0100, Ben Hutchings wrote:
> > @@ -2323,11 +2330,17 @@ int expand_downwards(struct vm_area_struct *vma,
> >  	if (error)
> >  		return error;
> >  
> > -	/* 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 = address - stack_guard_gap;
> >  	if (gap_addr > address)
> >  		return -ENOMEM;
> > -	prev = vma->vm_prev;
> > +	for (prev = vma->vm_prev;
> > +	     prev && !(prev->vm_flags & (VM_READ | VM_WRITE | VM_EXEC));
> > +	     prev = prev->vm_prev)
> > +		;
> >  	if (prev && prev->vm_end > gap_addr) {
> >  		if (!(prev->vm_flags & VM_GROWSDOWN))
> >  			return -ENOMEM;
> 
> Hmmm shouldn't we also stop looping when we're out of the gap ?

Yes, either that or only allow one such vma.

Ben.

> Something like this :
> 
> 	for (prev = vma->vm_prev;
> 	     prev && !(prev->vm_flags & (VM_READ | VM_WRITE | VM_EXEC)) &&
> 	        address - prev->vm_end < stack_guard_gap;
> 	     prev = prev->vm_prev)
> 		;
> 
> This would limit the risk of runaway loops if someone is having fun
> allocating a lot of memory in small chunks (eg: 4 GB in 1 million
> independant mmap() calls).

-- 
Ben Hutchings
Anthony's Law of Force: Don't force it, get a larger hammer.

[toc] | [prev] | [next] | [standalone]

#1681210

[Multipart message — attachments visible in raw view] — view raw

On Tue, 2017-07-04 at 12:36 +0100, Ben Hutchings wrote:
[...]
> This *doesn't* fix the LibreOffice regression on i386.

gdb shows me that the crash is at the last statement in this function:

static void _expand_stack_to(address bottom) {
  address sp;
  size_t size;
  volatile char *p;

  // Adjust bottom to point to the largest address within the same page, it
  // gives us a one-page buffer if alloca() allocates slightly more memory.
  bottom = (address)align_size_down((uintptr_t)bottom, os::Linux::page_size());
  bottom += os::Linux::page_size() - 1;

  // sp might be slightly above current stack pointer; if that's the case, we
  // will alloca() a little more space than necessary, which is OK. Don't use
  // os::current_stack_pointer(), as its result can be slightly below current
  // stack pointer, causing us to not alloca enough to reach "bottom".
  sp = (address)&sp;

  if (sp > bottom) {
    size = sp - bottom;
    p = (volatile char *)alloca(size);
    assert(p != NULL && p <= (volatile char *)bottom, "alloca problem?");
    p[0] = '\0';
  }
}

We have:

bottom = 0xff803fff
sp =     0xffffb178

The relevant mappings are:

ff7fc000-ff7fd000 rwxp 00000000 00:00 0
fffdd000-ffffe000 rw-p 00000000 00:00 0                                  [stack]

So instead of a useless guard page, we have a dangerous WX page
underneath the stack!  I suppose I should find out where and why that's
being allocated.

Ben.

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

[toc] | [prev] | [next] | [standalone]

#1681213

On Tue, Jul 4, 2017 at 4:01 PM, Ben Hutchings <ben@decadent.org.uk> wrote:
>
> We have:
>
> bottom = 0xff803fff
> sp =     0xffffb178
>
> The relevant mappings are:
>
> ff7fc000-ff7fd000 rwxp 00000000 00:00 0
> fffdd000-ffffe000 rw-p 00000000 00:00 0                                  [stack]

Ugh. So that stack is actually 8MB in size, but the alloca() is about
to use up almost all of it, and there's only about 28kB left between
"bottom" and that 'rwx' mapping.

Still, that rwx mapping is interesting: it is a single page, and it
really is almost exactly 8MB below the stack.

In fact, the top of stack (at 0xffffe000) is *exactly* 8MB+4kB from
the top of that odd one-page allocation (0xff7fd000).

Can you find out where that is allocated? Perhaps a breakpoint on
mmap, with a condition to catch that particular one?

Because I'm wondering if it was done explicitly as a 8MB stack
boundary allocation, with the "knowledge" that the kernel then adds a
one-page guard page.

I really don't know why somebody would do that (as opposed to just
limiting the stack with ulimit), but the 8MB+4kB distance is kind of
intriguing.

Maybe that one-page mapping is some hack to make sure that no random
mmap() will ever get too close to the stack, so it really is a "guard
mapping", except it's explicitly designed not so much to guard the
stack from growing down further (ulimit does that), but to guard the
brk() and other mmaps from growing *up* into the stack area..

Sometimes user mode does crazy things just because people are insane.
But sometimes there really is a method to the madness.

I would *not* be surprised if the way somebody allocared the stack was
to basically say:

 - let's use "mmap()" with a size of 8MB+2 pages to find a
sufficiently sized virtual memory area

 - once we've gotten that virtual address space range, let's over-map
the last page as the new stack using MAP_FIXED

 - finally, munmap the 8MB in between so that the new stack can grow
down into that gap the munmap creates.

Notice how you end up with exactly the above pattern of allocations,
and how it guarantees that you get a nice 8MB stack without having to
do any locking (you rely on the kernel to just find the 8MB+8kB areas,
and once one has been allocated, it will be "safe").

And yes, it would have been much nicer to just use PROT_NONE for that
initial sizing allocation, but for somebody who is only interested in
carving out a 8MB stack in virtual space, the protections are actually
kind of immaterial, so 'rwx' might be just their mental default.

                  Linus

[toc] | [prev] | [next] | [standalone]

#1681307

On Tue 04-07-17 16:31:52, Linus Torvalds wrote:
> On Tue, Jul 4, 2017 at 4:01 PM, Ben Hutchings <ben@decadent.org.uk> wrote:
> >
> > We have:
> >
> > bottom = 0xff803fff
> > sp =     0xffffb178
> >
> > The relevant mappings are:
> >
> > ff7fc000-ff7fd000 rwxp 00000000 00:00 0
> > fffdd000-ffffe000 rw-p 00000000 00:00 0                                  [stack]
> 
> Ugh. So that stack is actually 8MB in size, but the alloca() is about
> to use up almost all of it, and there's only about 28kB left between
> "bottom" and that 'rwx' mapping.
> 
> Still, that rwx mapping is interesting: it is a single page, and it
> really is almost exactly 8MB below the stack.
> 
> In fact, the top of stack (at 0xffffe000) is *exactly* 8MB+4kB from
> the top of that odd one-page allocation (0xff7fd000).

Very interesting! I would be really curious whether changing ulimit to
something bigger changes the picture. And if this is really the case
what we are going to do here. We can special case a single page mapping
under the stack but that sounds quite dangerous for something that is
dubious in itself. PROT_NONE would explicitly fault but we would simply
run over this mapping too easily and who knows what might end up below
it. So to me the guard gap does its job here.

Do you want me to post the earier patch to ignore PROT_NONE mapping
or we should rather wait for this one to get more details?
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1681359

On Wed, Jul 05, 2017 at 08:36:46AM +0200, Michal Hocko wrote:
> PROT_NONE would explicitly fault but we would simply
> run over this mapping too easily and who knows what might end up below
> it. So to me the guard gap does its job here.

I tend to think that applications that implement their own stack guard
using PROT_NONE also assume that they will never perfom unchecked stack
allocations larger than their own guard, thus the condition above should
never happen. Otherwise they're bogus and/or vulnerable by design and it
is their responsibility to fix it.

Thus maybe if that helps we could even relax some of the stack guard
checks as soon as we meet a PROT_NONE area, allowing VMAs to be tightly
packed if the application knows what it's doing. That wouldn't solve
the libreoffice issue though, given the lower page is RWX.

Willy

[toc] | [prev] | [next] | [standalone]

#1681363

On Wed 05-07-17 10:14:43, Willy Tarreau wrote:
> On Wed, Jul 05, 2017 at 08:36:46AM +0200, Michal Hocko wrote:
> > PROT_NONE would explicitly fault but we would simply
> > run over this mapping too easily and who knows what might end up below
> > it. So to me the guard gap does its job here.
> 
> I tend to think that applications that implement their own stack guard
> using PROT_NONE also assume that they will never perfom unchecked stack
> allocations larger than their own guard, thus the condition above should
> never happen. Otherwise they're bogus and/or vulnerable by design and it
> is their responsibility to fix it.
> 
> Thus maybe if that helps we could even relax some of the stack guard
> checks as soon as we meet a PROT_NONE area, allowing VMAs to be tightly
> packed if the application knows what it's doing.

Yes, this is what my patch does [1]. Or did I miss your point?

> That wouldn't solve the libreoffice issue though, given the lower page
> is RWX.

unfortunatelly yes. We only have limited room to address this issue
though. We could add per task (mm) stack_gap limit (controlled either
via proc or prctl) and revert back to 1 page for the specific program
but I would be really careful to add some more hack into the stack
expansion code.

[1] http://lkml.kernel.org/r/20170704093538.GF14722@dhcp22.suse.cz
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1681404

On Wed, Jul 05, 2017 at 10:24:41AM +0200, Michal Hocko wrote:
> > Thus maybe if that helps we could even relax some of the stack guard
> > checks as soon as we meet a PROT_NONE area, allowing VMAs to be tightly
> > packed if the application knows what it's doing.
> 
> Yes, this is what my patch does [1]. Or did I miss your point?

Sorry you're right, I got my mind confused when looking at the
libreoffice dump and for whatever reason ended up thinking we were
just considering that page as part of the gap and not being a marker
for the bottom. Never mind.

> > That wouldn't solve the libreoffice issue though, given the lower page
> > is RWX.
> 
> unfortunatelly yes. We only have limited room to address this issue
> though. We could add per task (mm) stack_gap limit (controlled either
> via proc or prctl) and revert back to 1 page for the specific program
> but I would be really careful to add some more hack into the stack
> expansion code.

Actually one option could be to have a sysctl causing a warning to be
emitted when hitting the stack guard instead of killing the process. We
could think for example that once this warning is emitted, the guard is
reduced to 64kB (I think it was the size before) and the application can
continue to run. That could help problematic applications getting fixed
quickly. And in environments with lots of local users it would be
dissuasive enough to avoid users trying their luck on setuid binaries.

Just my two cents,
Willy

[toc] | [prev] | [next] | [standalone]

#1681509

[Multipart message — attachments visible in raw view] — view raw

On Wed, 2017-07-05 at 10:14 +0200, Willy Tarreau wrote:
> On Wed, Jul 05, 2017 at 08:36:46AM +0200, Michal Hocko wrote:
> > PROT_NONE would explicitly fault but we would simply
> > run over this mapping too easily and who knows what might end up below
> > it. So to me the guard gap does its job here.
> 
> I tend to think that applications that implement their own stack guard
> using PROT_NONE also assume that they will never perfom unchecked stack
> allocations larger than their own guard, thus the condition above should
> never happen. Otherwise they're bogus and/or vulnerable by design and it
> is their responsibility to fix it.
> 
> Thus maybe if that helps we could even relax some of the stack guard
> checks as soon as we meet a PROT_NONE area, allowing VMAs to be tightly
> packed if the application knows what it's doing. That wouldn't solve
> the libreoffice issue though, given the lower page is RWX.

How about, instead of looking at permissions, we remember whether vmas
were allocated with MAP_FIXED and ignore those when evaluating the gap?

Ben.

-- 
Ben Hutchings
Anthony's Law of Force: Don't force it, get a larger hammer.

[toc] | [prev] | [next] | [standalone]

#1681547

On Wed, Jul 05, 2017 at 01:21:54PM +0100, Ben Hutchings wrote:
> On Wed, 2017-07-05 at 10:14 +0200, Willy Tarreau wrote:
> > On Wed, Jul 05, 2017 at 08:36:46AM +0200, Michal Hocko wrote:
> > > PROT_NONE would explicitly fault but we would simply
> > > run over this mapping too easily and who knows what might end up below
> > > it. So to me the guard gap does its job here.
> > 
> > I tend to think that applications that implement their own stack guard
> > using PROT_NONE also assume that they will never perfom unchecked stack
> > allocations larger than their own guard, thus the condition above should
> > never happen. Otherwise they're bogus and/or vulnerable by design and it
> > is their responsibility to fix it.
> > 
> > Thus maybe if that helps we could even relax some of the stack guard
> > checks as soon as we meet a PROT_NONE area, allowing VMAs to be tightly
> > packed if the application knows what it's doing. That wouldn't solve
> > the libreoffice issue though, given the lower page is RWX.
> 
> How about, instead of looking at permissions, we remember whether vmas
> were allocated with MAP_FIXED and ignore those when evaluating the gap?

I like this idea. It leaves complete control to the application. Our
usual principle of letting people shoot themselves in the foot if they
insist on doing so.

Do you think something like this could work (not even build tested) ?

Willy
--

diff --git a/include/linux/mm.h b/include/linux/mm.h
index d16f524..8ad7f40 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -90,6 +90,7 @@
 #define VM_PFNMAP	0x00000400	/* Page-ranges managed without "struct page", just pure PFN */
 #define VM_DENYWRITE	0x00000800	/* ETXTBSY on write attempts.. */
 
+#define VM_FIXED	0x00001000	/* MAP_FIXED was used */
 #define VM_LOCKED	0x00002000
 #define VM_IO           0x00004000	/* Memory mapped I/O or similar */
 
diff --git a/include/linux/mman.h b/include/linux/mman.h
index 9aa863d..4df2659 100644
--- a/include/linux/mman.h
+++ b/include/linux/mman.h
@@ -79,6 +79,7 @@ static inline int arch_validate_prot(unsigned long prot)
 {
 	return _calc_vm_trans(flags, MAP_GROWSDOWN,  VM_GROWSDOWN ) |
 	       _calc_vm_trans(flags, MAP_DENYWRITE,  VM_DENYWRITE ) |
+	       _calc_vm_trans(flags, MAP_FIXED,      VM_FIXED     ) |
 	       _calc_vm_trans(flags, MAP_LOCKED,     VM_LOCKED    );
 }
 #endif /* _LINUX_MMAN_H */
diff --git a/mm/mmap.c b/mm/mmap.c
index 3c4e4d7..b612868 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -2145,7 +2145,7 @@ int expand_upwards(struct vm_area_struct *vma, unsigned long address)
 
 	next = vma->vm_next;
 	if (next && next->vm_start < gap_addr) {
-		if (!(next->vm_flags & VM_GROWSUP))
+		if (!(next->vm_flags & (VM_GROWSUP|VM_FIXED)))
 			return -ENOMEM;
 		/* Check that both stack segments have the same anon_vma? */
 	}
@@ -2225,7 +2225,7 @@ int expand_downwards(struct vm_area_struct *vma,
 		return -ENOMEM;
 	prev = vma->vm_prev;
 	if (prev && prev->vm_end > gap_addr) {
-		if (!(prev->vm_flags & VM_GROWSDOWN))
+		if (!(prev->vm_flags & (VM_GROWSDOWN|VM_FIXED)))
 			return -ENOMEM;
 		/* Check that both stack segments have the same anon_vma? */
 	}

[toc] | [prev] | [next] | [standalone]

#1681558

On Wed 05-07-17 13:21:54, Ben Hutchings wrote:
> On Wed, 2017-07-05 at 10:14 +0200, Willy Tarreau wrote:
> > On Wed, Jul 05, 2017 at 08:36:46AM +0200, Michal Hocko wrote:
> > > PROT_NONE would explicitly fault but we would simply
> > > run over this mapping too easily and who knows what might end up below
> > > it. So to me the guard gap does its job here.
> > 
> > I tend to think that applications that implement their own stack guard
> > using PROT_NONE also assume that they will never perfom unchecked stack
> > allocations larger than their own guard, thus the condition above should
> > never happen. Otherwise they're bogus and/or vulnerable by design and it
> > is their responsibility to fix it.
> > 
> > Thus maybe if that helps we could even relax some of the stack guard
> > checks as soon as we meet a PROT_NONE area, allowing VMAs to be tightly
> > packed if the application knows what it's doing. That wouldn't solve
> > the libreoffice issue though, given the lower page is RWX.
> 
> How about, instead of looking at permissions, we remember whether vmas
> were allocated with MAP_FIXED and ignore those when evaluating the gap?

To be honest I really hate this. The same way as any other heuristics
where we try to guess the gap which will not fault to let userspace
know something is wrong. And the Java example just proves the point
AFAIU. The mapping we clash on is _not_ a gap. It is a real mapping we
should rather not scribble over. It contains a code to execute and that
is even more worrying. So I guess the _only_ sane way forward for this
case is to reduce stack gap for the particular code. 
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1681629

On Wed, Jul 5, 2017 at 5:21 AM, Ben Hutchings <ben@decadent.org.uk> wrote:
>
> How about, instead of looking at permissions, we remember whether vmas
> were allocated with MAP_FIXED and ignore those when evaluating the gap?

No, I think that's a bad idea. There's tons of good reasons to use
MAP_FIXED, and real programs do it all the time.

I'd much rather just do something special for the Java case, either
recognizing that particular pattern, or (and this is likely what we'll
have to do) just have a per-process stack limit that

 (a) will be reset by suid transitions etc security boundaries

 (b) you can just set back to 4kB for the specific Java case.

because I'd rather make this be a very conscious thing rather than a
random hack.

The PROT_NONE thing made tons of conceptual sense ("let people do
their own guard mappings"). The MAP_FIXED thing does not.

             Linus

[toc] | [prev] | [next] | [standalone]

#1682130

On Wed 05-07-17 08:36:45, Michal Hocko wrote:
> On Tue 04-07-17 16:31:52, Linus Torvalds wrote:
> > On Tue, Jul 4, 2017 at 4:01 PM, Ben Hutchings <ben@decadent.org.uk> wrote:
> > >
> > > We have:
> > >
> > > bottom = 0xff803fff
> > > sp =     0xffffb178
> > >
> > > The relevant mappings are:
> > >
> > > ff7fc000-ff7fd000 rwxp 00000000 00:00 0
> > > fffdd000-ffffe000 rw-p 00000000 00:00 0                                  [stack]
> > 
> > Ugh. So that stack is actually 8MB in size, but the alloca() is about
> > to use up almost all of it, and there's only about 28kB left between
> > "bottom" and that 'rwx' mapping.
> > 
> > Still, that rwx mapping is interesting: it is a single page, and it
> > really is almost exactly 8MB below the stack.
> > 
> > In fact, the top of stack (at 0xffffe000) is *exactly* 8MB+4kB from
> > the top of that odd one-page allocation (0xff7fd000).
> 
> Very interesting! I would be really curious whether changing ulimit to
> something bigger changes the picture.

It's public holiday today here and I haven't read all new emails and I
will be mostly offline today. I will catch up tomorrow. But before we go
to more tricky workarounds. Could you double check that simply
increasing the RLIMIT_STACK workarounds the problem here? Because if it
does and other workarounds require some manual intervention then
changing ulimit sounds like the least tricky one to me.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1681504

[Multipart message — attachments visible in raw view] — view raw

On Tue, 2017-07-04 at 16:31 -0700, Linus Torvalds wrote:
> On Tue, Jul 4, 2017 at 4:01 PM, Ben Hutchings <ben@decadent.org.uk>
> wrote:
> > 
> > We have:
> > 
> > bottom = 0xff803fff
> > sp =     0xffffb178
> > 
> > The relevant mappings are:
> > 
> > ff7fc000-ff7fd000 rwxp 00000000 00:00 0
> > fffdd000-ffffe000 rw-p 00000000 00:00
> > 0                                  [stack]
> 
> Ugh. So that stack is actually 8MB in size, but the alloca() is about
> to use up almost all of it, and there's only about 28kB left between
> "bottom" and that 'rwx' mapping.
> 
> Still, that rwx mapping is interesting: it is a single page, and it
> really is almost exactly 8MB below the stack.
> 
> In fact, the top of stack (at 0xffffe000) is *exactly* 8MB+4kB from
> the top of that odd one-page allocation (0xff7fd000).
> 
> Can you find out where that is allocated? Perhaps a breakpoint on
> mmap, with a condition to catch that particular one?
[...]

Found it, and it's now clear why only i386 is affected:
http://hg.openjdk.java.net/jdk8/jdk8/hotspot/file/tip/src/os/linux/vm/os_linux.cpp#l4852
http://hg.openjdk.java.net/jdk8/jdk8/hotspot/file/tip/src/os_cpu/linux_x86/vm/os_linux_x86.cpp#l881

Ben.

-- 
Ben Hutchings
Anthony's Law of Force: Don't force it, get a larger hammer.

[toc] | [prev] | [next] | [standalone]

#1681559

On Wed 05-07-17 13:19:40, Ben Hutchings wrote:
> On Tue, 2017-07-04 at 16:31 -0700, Linus Torvalds wrote:
> > On Tue, Jul 4, 2017 at 4:01 PM, Ben Hutchings <ben@decadent.org.uk>
> > wrote:
> > > 
> > > We have:
> > > 
> > > bottom = 0xff803fff
> > > sp =     0xffffb178
> > > 
> > > The relevant mappings are:
> > > 
> > > ff7fc000-ff7fd000 rwxp 00000000 00:00 0
> > > fffdd000-ffffe000 rw-p 00000000 00:00
> > > 0                                  [stack]
> > 
> > Ugh. So that stack is actually 8MB in size, but the alloca() is about
> > to use up almost all of it, and there's only about 28kB left between
> > "bottom" and that 'rwx' mapping.
> > 
> > Still, that rwx mapping is interesting: it is a single page, and it
> > really is almost exactly 8MB below the stack.
> > 
> > In fact, the top of stack (at 0xffffe000) is *exactly* 8MB+4kB from
> > the top of that odd one-page allocation (0xff7fd000).
> > 
> > Can you find out where that is allocated? Perhaps a breakpoint on
> > mmap, with a condition to catch that particular one?
> [...]
> 
> Found it, and it's now clear why only i386 is affected:
> http://hg.openjdk.java.net/jdk8/jdk8/hotspot/file/tip/src/os/linux/vm/os_linux.cpp#l4852
> http://hg.openjdk.java.net/jdk8/jdk8/hotspot/file/tip/src/os_cpu/linux_x86/vm/os_linux_x86.cpp#l881

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?
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1681598

[Multipart message — attachments visible in raw view] — view raw

On Wed, 2017-07-05 at 16:23 +0200, Michal Hocko wrote:
> On Wed 05-07-17 13:19:40, Ben Hutchings wrote:
> > On Tue, 2017-07-04 at 16:31 -0700, Linus Torvalds wrote:
> > > On Tue, Jul 4, 2017 at 4:01 PM, Ben Hutchings <ben@decadent.org.uk>
> > > wrote:
> > > > 
> > > > We have:
> > > > 
> > > > bottom = 0xff803fff
> > > > sp =     0xffffb178
> > > > 
> > > > The relevant mappings are:
> > > > 
> > > > ff7fc000-ff7fd000 rwxp 00000000 00:00 0
> > > > fffdd000-ffffe000 rw-p 00000000 00:00
> > > > 0                                  [stack]
> > > 
> > > Ugh. So that stack is actually 8MB in size, but the alloca() is about
> > > to use up almost all of it, and there's only about 28kB left between
> > > "bottom" and that 'rwx' mapping.
> > > 
> > > Still, that rwx mapping is interesting: it is a single page, and it
> > > really is almost exactly 8MB below the stack.
> > > 
> > > In fact, the top of stack (at 0xffffe000) is *exactly* 8MB+4kB from
> > > the top of that odd one-page allocation (0xff7fd000).
> > > 
> > > Can you find out where that is allocated? Perhaps a breakpoint on
> > > mmap, with a condition to catch that particular one?
> > 
> > [...]
> > 
> > Found it, and it's now clear why only i386 is affected:
> > http://hg.openjdk.java.net/jdk8/jdk8/hotspot/file/tip/src/os/linux/vm/os_linux.cpp#l4852
> > http://hg.openjdk.java.net/jdk8/jdk8/hotspot/file/tip/src/os_cpu/linux_x86/vm/os_linux_x86.cpp#l881
> 
> 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?

Distributions can do that, but what about all the other apps out there
using JNI and private copies of the JRE?

Soemthing I noticed is that Java doesn't immediately use MAP_FIXED. 
Look at os::pd_attempt_reserve_memory_at().  If the first, hinted,
mmap() doesn't return the hinted address it then attempts to allocate
huge areas (I'm not sure how intentional this is) and unmaps the
unwanted parts.  Then os::workaround_expand_exec_shield_cs_limit() re-
mmap()s the wanted part with MAP_FIXED.  If this fails at any point it
is not a fatal error.

So if we change vm_start_gap() to take the stack limit into account
(when it's finite) that should neutralise
os::workaround_expand_exec_shield_cs_limit().  I'll try this.

Ben.

-- 
Ben Hutchings
Anthony's Law of Force: Don't force it, get a larger hammer.

[toc] | [prev] | [next] | [standalone]

#1681619

On Wed 05-07-17 16:25:00, Ben Hutchings wrote:
> On Wed, 2017-07-05 at 16:23 +0200, Michal Hocko wrote:
> > On Wed 05-07-17 13:19:40, Ben Hutchings wrote:
> > > On Tue, 2017-07-04 at 16:31 -0700, Linus Torvalds wrote:
> > > > On Tue, Jul 4, 2017 at 4:01 PM, Ben Hutchings <ben@decadent.org.uk>
> > > > wrote:
> > > > > 
> > > > > We have:
> > > > > 
> > > > > bottom = 0xff803fff
> > > > > sp =     0xffffb178
> > > > > 
> > > > > The relevant mappings are:
> > > > > 
> > > > > ff7fc000-ff7fd000 rwxp 00000000 00:00 0
> > > > > fffdd000-ffffe000 rw-p 00000000 00:00
> > > > > 0                                  [stack]
> > > > 
> > > > Ugh. So that stack is actually 8MB in size, but the alloca() is about
> > > > to use up almost all of it, and there's only about 28kB left between
> > > > "bottom" and that 'rwx' mapping.
> > > > 
> > > > Still, that rwx mapping is interesting: it is a single page, and it
> > > > really is almost exactly 8MB below the stack.
> > > > 
> > > > In fact, the top of stack (at 0xffffe000) is *exactly* 8MB+4kB from
> > > > the top of that odd one-page allocation (0xff7fd000).
> > > > 
> > > > Can you find out where that is allocated? Perhaps a breakpoint on
> > > > mmap, with a condition to catch that particular one?
> > > 
> > > [...]
> > > 
> > > Found it, and it's now clear why only i386 is affected:
> > > http://hg.openjdk.java.net/jdk8/jdk8/hotspot/file/tip/src/os/linux/vm/os_linux.cpp#l4852
> > > http://hg.openjdk.java.net/jdk8/jdk8/hotspot/file/tip/src/os_cpu/linux_x86/vm/os_linux_x86.cpp#l881
> > 
> > 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?
> 
> Distributions can do that, but what about all the other apps out there
> using JNI and private copies of the JRE?

Yes this sucks. I was thinking about something like run_legacy_stack
which would do
	prctl(PR_SET_STACK_GAP, 1, 0, 0, 0);
	execve(argv[1], argv+1, environ);

so we would have a way to start applications that start crashing with
the new setup without changing the default for all other applications.
The question is what to do if the execed task is suid because we
definitely do not want to allow tricking anybody to have smaller gap.

Or maybe just start the java with increased stack rlimit?

> Soemthing I noticed is that Java doesn't immediately use MAP_FIXED. 
> Look at os::pd_attempt_reserve_memory_at().  If the first, hinted,
> mmap() doesn't return the hinted address it then attempts to allocate
> huge areas (I'm not sure how intentional this is) and unmaps the
> unwanted parts.  Then os::workaround_expand_exec_shield_cs_limit() re-
> mmap()s the wanted part with MAP_FIXED.  If this fails at any point it
> is not a fatal error.
> 
> So if we change vm_start_gap() to take the stack limit into account
> (when it's finite) that should neutralise
> os::workaround_expand_exec_shield_cs_limit().  I'll try this.

I was already thinking about doing something like that to have a better
support for MAP_GROWSDOWN but then I just gave up because this would
require to cap RLIMIT_STACK for large values in order to not break
userspace again. The max value is not really clear to me.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

Page 2 of 4 — ← Prev page 1 [2] 3 4  Next page →

Back to top | Article view | linux.kernel

csiph-web