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 3 of 4 — ← Prev page 1 2 [3] 4  Next page →

#1681660

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

On Wed, Jul 05, 2017 at 04:25:00PM +0100, Ben Hutchings wrote:
[...]
> 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 ended up with the following two patches, which seem to deal with
both the Java and Rust regressions.  These don't touch the
stack-grows-up paths at all because Rust doesn't run on those
architectures and the Java weirdness is i386-specific.

They definitely need longer commit messages and comments, but aside
from that do these look reasonable?

Ben.

Subject: [1/2] mmap: Skip a single VM_NONE mapping when checking the stack gap

Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
 mm/mmap.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/mm/mmap.c b/mm/mmap.c
index a5e3dcd75e79..c7906ae1a7a1 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -2323,11 +2323,16 @@ int expand_downwards(struct vm_area_struct *vma,
 	if (error)
 		return error;
 
-	/* Enforce stack_guard_gap */
+	/*
+	 * Enforce stack_guard_gap.  Some applications allocate a VM_NONE
+	 * mapping just below the stack, which we can safely ignore.
+	 */
 	gap_addr = address - stack_guard_gap;
 	if (gap_addr > address)
 		return -ENOMEM;
 	prev = vma->vm_prev;
+	if (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;

Subject: [2/2] mmap: Avoid mapping anywhere within the full stack extent
 if finite

Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
 include/linux/mm.h |  9 ++++-----
 mm/mmap.c          | 19 +++++++++++++++++++
 2 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 6f543a47fc92..2240a0505072 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -2223,15 +2223,14 @@ static inline struct vm_area_struct * find_vma_intersection(struct mm_struct * m
 	return vma;
 }
 
+unsigned long __vm_start_gap(struct vm_area_struct *vma);
+
 static inline unsigned long vm_start_gap(struct vm_area_struct *vma)
 {
 	unsigned long vm_start = vma->vm_start;
 
-	if (vma->vm_flags & VM_GROWSDOWN) {
-		vm_start -= stack_guard_gap;
-		if (vm_start > vma->vm_start)
-			vm_start = 0;
-	}
+	if (vma->vm_flags & VM_GROWSDOWN)
+		vm_start = __vm_start_gap(vma);
 	return vm_start;
 }
 
diff --git a/mm/mmap.c b/mm/mmap.c
index c7906ae1a7a1..f8131a94e56e 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -2307,6 +2307,25 @@ int expand_upwards(struct vm_area_struct *vma, unsigned long address)
 }
 #endif /* CONFIG_STACK_GROWSUP || CONFIG_IA64 */
 
+unsigned long __vm_start_gap(struct vm_area_struct *vma)
+{
+	unsigned long stack_limit =
+		current->signal->rlim[RLIMIT_STACK].rlim_cur;
+	unsigned long vm_start;
+
+	if (stack_limit != RLIM_INFINITY &&
+	    vma->vm_end - vma->vm_start < stack_limit)
+		vm_start = vma->vm_end - PAGE_ALIGN(stack_limit);
+	else
+		vm_start = vma->vm_start;
+
+	vm_start -= stack_guard_gap;
+	if (vm_start > vma->vm_start)
+		vm_start = 0;
+
+	return vm_start;
+}
+
 /*
  * vma is the first one with address < vma->vm_start.  Have to extend vma.
  */

-- 
Ben Hutchings
For every complex problem
there is a solution that is simple, neat, and wrong.

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

#1681661

On Wed 05-07-17 17:58:45, Ben Hutchings wrote:
[...]
> diff --git a/mm/mmap.c b/mm/mmap.c
> index c7906ae1a7a1..f8131a94e56e 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -2307,6 +2307,25 @@ int expand_upwards(struct vm_area_struct *vma, unsigned long address)
>  }
>  #endif /* CONFIG_STACK_GROWSUP || CONFIG_IA64 */
>  
> +unsigned long __vm_start_gap(struct vm_area_struct *vma)
> +{
> +	unsigned long stack_limit =
> +		current->signal->rlim[RLIMIT_STACK].rlim_cur;
> +	unsigned long vm_start;
> +
> +	if (stack_limit != RLIM_INFINITY &&
> +	    vma->vm_end - vma->vm_start < stack_limit)
> +		vm_start = vma->vm_end - PAGE_ALIGN(stack_limit);

This is exactly what I was worried about in my previous email. Say
somebody sets stack ulimit to 1G or so. Should we reduce the available
address space that much? Say you are 32b and you have an application
with multiple stacks each doing its MAP_GROWSDOWN. You are quickly out
of address space. That's why I've said that we would need to find a cap
for the user defined limit. How much that should be though? Few (tens,
hundreds) megs. If we can figure that up I would be of course quite
happy about such a change because MAP_GROWSDOWN doesn't work really well
these days.

> +	else
> +		vm_start = vma->vm_start;
> +
> +	vm_start -= stack_guard_gap;
> +	if (vm_start > vma->vm_start)
> +		vm_start = 0;
> +
> +	return vm_start;
> +}
> +
>  /*
>   * vma is the first one with address < vma->vm_start.  Have to extend vma.
>   */
> 
> -- 
> Ben Hutchings
> For every complex problem
> there is a solution that is simple, neat, and wrong.



-- 
Michal Hocko
SUSE Labs

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

#1681677

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

On Wed, 2017-07-05 at 19:05 +0200, Michal Hocko wrote:
> On Wed 05-07-17 17:58:45, Ben Hutchings wrote:
> [...]
> > diff --git a/mm/mmap.c b/mm/mmap.c
> > index c7906ae1a7a1..f8131a94e56e 100644
> > --- a/mm/mmap.c
> > +++ b/mm/mmap.c
> > @@ -2307,6 +2307,25 @@ int expand_upwards(struct vm_area_struct *vma, unsigned long address)
> >  }
> >  #endif /* CONFIG_STACK_GROWSUP || CONFIG_IA64 */
> >  
> > +unsigned long __vm_start_gap(struct vm_area_struct *vma)
> > +{
> > +	unsigned long stack_limit =
> > +		current->signal->rlim[RLIMIT_STACK].rlim_cur;
> > +	unsigned long vm_start;
> > +
> > +	if (stack_limit != RLIM_INFINITY &&
> > +	    vma->vm_end - vma->vm_start < stack_limit)
> > +		vm_start = vma->vm_end - PAGE_ALIGN(stack_limit);
> 
> This is exactly what I was worried about in my previous email. Say
> somebody sets stack ulimit to 1G or so. Should we reduce the available
> address space that much?

It's not ideal, but why would someone set the stack limit that high
unless it's for an application that will actually use most of that
stack space?  Do you think that "increase the stack limit" has been
cargo-culted?

> Say you are 32b and you have an application
> with multiple stacks each doing its MAP_GROWSDOWN.
[...]

So this application is using dietlibc or uclibc?  glibc uses fixed-size 
mappings for new threads.

I suppose there's a risk that by doing this we would mamke
MAP_GROWSDOWN useful enough that it is more likely to be used for new
thread stacks in future.

Ben.

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

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

#1681666

On Wed, Jul 5, 2017 at 9:58 AM, Ben Hutchings <ben@decadent.org.uk> wrote:
>
> I ended up with the following two patches, which seem to deal with
> both the Java and Rust regressions.  These don't touch the
> stack-grows-up paths at all because Rust doesn't run on those
> architectures and the Java weirdness is i386-specific.
>
> They definitely need longer commit messages and comments, but aside
> from that do these look reasonable?

I thin kthey both look reasonable, but I think we might still want to
massage things a bit (cutting down the quoting to a minimum, hopefully
leaving enough context to still make sense):

> Subject: [1/2] mmap: Skip a single VM_NONE mapping when checking the stack gap
>
>         prev = vma->vm_prev;
> +       if (prev && !(prev->vm_flags & (VM_READ | VM_WRITE | VM_EXEC)))
> +               prev = prev->vm_prev;
>         if (prev && prev->vm_end > gap_addr) {

Do we just want to ignore the user-supplied guard mapping, or do we
want to say "if the user does a guard mapping, we use that *instead*
of our stack gap"?

IOW, instead of "prev = prev->vm_prev;" and continuing, maybe we want
to just return "ok".

> Subject: [2/2] mmap: Avoid mapping anywhere within the full stack extent  if finite

This is good thinking, but no, I don't think the "if finite" is right.

I've seen people use "really big values" as replacement for
RLIM_INIFITY, for various reasons.

We've had huge confusion about RLIM_INFINITY over the years - look for
things like COMPAT_RLIM_OLD_INFINITY to see the kinds of confusions
we've had.

Some people just use MAX_LONG etc, which is *not* the same as
RLIM_INFINITY, but in practice ends up doing the same thing. Yadda
yadda.

So I'm personally leery of checking and depending on "exactly
RLIM_INIFITY", because I've seen it go wrong so many times.

And I think your second patch breaks that "use a really large value to
approximate infinity" case that definitely has existed as a pattern.

              Linus

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

#1681940

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

On Wed, 2017-07-05 at 10:15 -0700, Linus Torvalds wrote:
> > On Wed, Jul 5, 2017 at 9:58 AM, Ben Hutchings <ben@decadent.org.uk> wrote:
> > 
> > I ended up with the following two patches, which seem to deal with
> > both the Java and Rust regressions.  These don't touch the
> > stack-grows-up paths at all because Rust doesn't run on those
> > architectures and the Java weirdness is i386-specific.
> > 
> > They definitely need longer commit messages and comments, but aside
> > from that do these look reasonable?
> 
> I thin kthey both look reasonable, but I think we might still want to
> massage things a bit (cutting down the quoting to a minimum, hopefully
> leaving enough context to still make sense):
> 
> > Subject: [1/2] mmap: Skip a single VM_NONE mapping when checking the stack gap
> > 
> >         prev = vma->vm_prev;
> > +       if (prev && !(prev->vm_flags & (VM_READ | VM_WRITE | VM_EXEC)))
> > +               prev = prev->vm_prev;
> >         if (prev && prev->vm_end > gap_addr) {
> 
> Do we just want to ignore the user-supplied guard mapping, or do we
> want to say "if the user does a guard mapping, we use that *instead*
> of our stack gap"?
> 
> IOW, instead of "prev = prev->vm_prev;" and continuing, maybe we want
> to just return "ok".

Rust effectively added a second guard page to the main thread stack.  
But it does not (yet) implement stack probing
(https://github.com/rust-lang/rust/issues/16012) so I think it will
benefit from the kernel's larger stack guard gap.

> > Subject: [2/2] mmap: Avoid mapping anywhere within the full stack extent  if finite
> 
> This is good thinking, but no, I don't think the "if finite" is right.
> 
> I've seen people use "really big values" as replacement for
> RLIM_INIFITY, for various reasons.
> 
> We've had huge confusion about RLIM_INFINITY over the years - look for
> things like COMPAT_RLIM_OLD_INFINITY to see the kinds of confusions
> we've had.

That sounds familiar...

> Some people just use MAX_LONG etc, which is *not* the same as
> RLIM_INFINITY, but in practice ends up doing the same thing. Yadda
> yadda.
> 
> So I'm personally leery of checking and depending on "exactly
> RLIM_INIFITY", because I've seen it go wrong so many times.
> 
> And I think your second patch breaks that "use a really large value to
> approximate infinity" case that definitely has existed as a pattern.

Right.  Well that seems to leave us with remembering the MAP_FIXED flag
and using that as the condition to ignore the previous mapping.

Ben.

-- 
Ben Hutchings
Man invented language to satisfy his deep need to complain. - Lily
Tomlin

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

#1681959

On Wed, Jul 5, 2017 at 4:35 PM, Ben Hutchings <ben@decadent.org.uk> wrote:
>>
>> And I think your second patch breaks that "use a really large value to
>> approximate infinity" case that definitely has existed as a pattern.
>
> Right.  Well that seems to leave us with remembering the MAP_FIXED flag
> and using that as the condition to ignore the previous mapping.

I'm not particularly happy about having a MAP_FIXED special case, but
yeah, I'm not seeing a lot of alternatives.

             Linus

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

#1682173

On Wed, Jul 05, 2017 at 04:51:06PM -0700, Linus Torvalds wrote:
> On Wed, Jul 5, 2017 at 4:35 PM, Ben Hutchings <ben@decadent.org.uk> wrote:
> >>
> >> And I think your second patch breaks that "use a really large value to
> >> approximate infinity" case that definitely has existed as a pattern.
> >
> > Right.  Well that seems to leave us with remembering the MAP_FIXED flag
> > and using that as the condition to ignore the previous mapping.
> 
> I'm not particularly happy about having a MAP_FIXED special case, but
> yeah, I'm not seeing a lot of alternatives.

We can possibly refine it like this :
  - use PROT_NONE as a mark for the end of the stack and consider the
    application doing this knows exactly what it's doing ;

  - use other MAP_FIXED as a limit for a shorter gap (ie 4kB), considering
    that 1) it used to work like this for many years, and 2) if an application
    is forcing a MAP_FIXED just below the stack and at the same time uses
    large alloca() or VLA it's definitely bogus and looking for unfixable
    trouble. Not allowing this means we break existing applications anyway.

Willy

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

#1682313

On Thu, Jul 06, 2017 at 10:24:06AM +0200, Willy Tarreau wrote:
> On Wed, Jul 05, 2017 at 04:51:06PM -0700, Linus Torvalds wrote:
> > On Wed, Jul 5, 2017 at 4:35 PM, Ben Hutchings <ben@decadent.org.uk> wrote:
> > >>
> > >> And I think your second patch breaks that "use a really large value to
> > >> approximate infinity" case that definitely has existed as a pattern.
> > >
> > > Right.  Well that seems to leave us with remembering the MAP_FIXED flag
> > > and using that as the condition to ignore the previous mapping.
> > 
> > I'm not particularly happy about having a MAP_FIXED special case, but
> > yeah, I'm not seeing a lot of alternatives.
> 
> We can possibly refine it like this :
>   - use PROT_NONE as a mark for the end of the stack and consider the
>     application doing this knows exactly what it's doing ;
> 
>   - use other MAP_FIXED as a limit for a shorter gap (ie 4kB), considering
>     that 1) it used to work like this for many years, and 2) if an application
>     is forcing a MAP_FIXED just below the stack and at the same time uses
>     large alloca() or VLA it's definitely bogus and looking for unfixable
>     trouble. Not allowing this means we break existing applications anyway.

That would probably give the following (only build-tested on x86_64). Do
you think it would make sense and/or be acceptable ? That would more
easily avoid the other options like adding sysctl + warnings or making
a special case of setuid.

Willy
---

From 56ae4e57e446bc92fd2647327da281e313930524 Mon Sep 17 00:00:00 2001
From: Willy Tarreau <w@1wt.eu>
Date: Thu, 6 Jul 2017 12:00:54 +0200
Subject: mm: mm, mmap: only apply a one page gap betwen the stack an
 MAP_FIXED

Some programs place a MAP_FIXED below the stack, not leaving enough room
for the stack guard. This patch keeps track of MAP_FIXED, mirroring it in
a new VM_FIXED flag and reduces the stack guard to a single page (as it
used to be) in such a situation, assuming that when an application places
a fixed map close to the stack, it very likely does it on purpose and is
taking the full responsibility for the risk of the stack blowing up.

Cc: Ben Hutchings <ben@decadent.org.uk>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Andy Lutomirski <luto@kernel.org>
Signed-off-by: Willy Tarreau <w@1wt.eu>
---
 include/linux/mm.h   |  1 +
 include/linux/mman.h |  1 +
 mm/mmap.c            | 30 ++++++++++++++++++++----------
 3 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 6f543a4..41492b9 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -188,6 +188,7 @@ extern int overcommit_kbytes_handler(struct ctl_table *, int, void __user *,
 #define VM_ACCOUNT	0x00100000	/* Is a VM accounted object */
 #define VM_NORESERVE	0x00200000	/* should the VM suppress accounting */
 #define VM_HUGETLB	0x00400000	/* Huge TLB Page VM */
+#define VM_FIXED	0x00800000	/* MAP_FIXED was used */
 #define VM_ARCH_1	0x01000000	/* Architecture-specific flag */
 #define VM_ARCH_2	0x02000000
 #define VM_DONTDUMP	0x04000000	/* Do not include in the core dump */
diff --git a/include/linux/mman.h b/include/linux/mman.h
index 634c4c5..3a29069 100644
--- a/include/linux/mman.h
+++ b/include/linux/mman.h
@@ -86,6 +86,7 @@ static inline bool 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    );
 }
 
diff --git a/mm/mmap.c b/mm/mmap.c
index ece0f6d..7fc1c29 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -2244,12 +2244,17 @@ int expand_upwards(struct vm_area_struct *vma, unsigned long address)
 		gap_addr = TASK_SIZE;
 
 	next = vma->vm_next;
+
+	/* PROT_NONE above a MAP_GROWSUP always serves as a mark and inhibits
+	 * the stack guard gap.
+	 * MAP_FIXED above a MAP_GROWSUP only requires a single page guard.
+	 */
 	if (next && next->vm_start < gap_addr &&
-			(next->vm_flags & (VM_WRITE|VM_READ|VM_EXEC))) {
-		if (!(next->vm_flags & VM_GROWSUP))
-			return -ENOMEM;
-		/* Check that both stack segments have the same anon_vma? */
-	}
+	    !(next->vm_flags & VM_GROWSUP) &&
+	    (next->vm_flags & (VM_WRITE|VM_READ|VM_EXEC)) &&
+	    (!(next->vm_flags & VM_FIXED) ||
+	     next->vm_start < address + PAGE_SIZE))
+		return -ENOMEM;
 
 	/* We must make sure the anon_vma is allocated. */
 	if (unlikely(anon_vma_prepare(vma)))
@@ -2329,12 +2334,17 @@ int expand_downwards(struct vm_area_struct *vma,
 	if (gap_addr > address)
 		return -ENOMEM;
 	prev = vma->vm_prev;
+
+	/* PROT_NONE below a MAP_GROWSDOWN always serves as a mark and inhibits
+	 * the stack guard gap.
+	 * MAP_FIXED below a MAP_GROWSDOWN only requires a single page guard.
+	 */
 	if (prev && prev->vm_end > gap_addr &&
-			(prev->vm_flags & (VM_WRITE|VM_READ|VM_EXEC))) {
-		if (!(prev->vm_flags & VM_GROWSDOWN))
-			return -ENOMEM;
-		/* Check that both stack segments have the same anon_vma? */
-	}
+	    !(prev->vm_flags & VM_GROWSDOWN) &&
+	    (prev->vm_flags & (VM_WRITE|VM_READ|VM_EXEC)) &&
+	    (!(prev->vm_flags & VM_FIXED) ||
+	     prev->vm_end > address - PAGE_SIZE))
+		return -ENOMEM;
 
 	/* We must make sure the anon_vma is allocated. */
 	if (unlikely(anon_vma_prepare(vma)))
-- 
1.7.12.1

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

#1681635

On Wed, Jul 5, 2017 at 7:23 AM, Michal Hocko <mhocko@kernel.org> 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?

Why inherit on exec?

I think that, if we add a new API, we should do it right rather than
making it even more hackish.  Specifically, we'd add a real VMA type
(via flag or whatever) that means "this is a modern stack".  A modern
stack wouldn't ever expand and would have no guard page at all.  It
would, however, properly account stack space by tracking the pages
used as stack space.  Users of the new VMA type would be responsible
for allocating their own guard pages, probably by mapping an extra
page and than mapping PROT_NONE over it.

Also, this doesn't even need a new API, I think.  What's wrong with
plain old mmap(2) with MAP_STACK and *without* MAP_GROWSDOWN?  Only
new kernels would get the accounting right, but I doubt that matters
much in practice.

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

#1681642

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 *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.

                 Linus

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

#1681667

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.

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

#1681735

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

On Wed, 2017-07-05 at 10:23 -0700, Andy Lutomirski wrote:
[...]
> 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.

Given that in the following points you say that something sounding like
a stack gap would be "64k or whatever", what does "the stack gap" mean
in this first point?

>  - 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.

Having tested patches along these lines, I think the above would avoid
the reported regressions.

Ben.

> 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.
-- 
Ben Hutchings
Anthony's Law of Force: Don't force it, get a larger hammer.

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

#1681795

On Wed, Jul 05, 2017 at 08:32:43PM +0100, Ben Hutchings wrote:
> >  - 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.
> 
> Having tested patches along these lines, I think the above would avoid
> the reported regressions.

Stuff like this has already been proposed but Linus suspects that more
software than we imagine uses MAP_FIXED and could break. I cannot infirm
nor confirm, and that probably indicates that there's nothing fundamentally
wrong with this approach from the userland's perspective and that it could
indeed imply such software may be more common than we would like it.

Willy

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

#1681804

> On Jul 5, 2017, at 12:32 PM, Ben Hutchings <ben@decadent.org.uk> wrote:
> 
>> On Wed, 2017-07-05 at 10:23 -0700, Andy Lutomirski wrote:
>> [...]
>> 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.
> 
> Given that in the following points you say that something sounding like
> a stack gap would be "64k or whatever", what does "the stack gap" mean
> in this first point?

I mean one page, with semantics as close to previous (4.11) behavior as practical.

> 
>>  - 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.
> 
> Having tested patches along these lines, I think the above would avoid
> the reported regressions.
> 

FWIW, even this last part may be problematic.  It'll break anything that tries to allocate many small MAP_GROWSDOWN stacks on 32-bit.  Hopefully nothing does this, but maybe Java does.

> Ben.
> 
>> 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.
> -- 
> Ben Hutchings
> Anthony's Law of Force: Don't force it, get a larger hammer.
> 

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

#1681953

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

On Wed, 2017-07-05 at 13:53 -0700, Andy Lutomirski wrote:
> On Jul 5, 2017, at 12:32 PM, Ben Hutchings <ben@decadent.org.uk> wrote:
> > On Wed, 2017-07-05 at 10:23 -0700, Andy Lutomirski wrote:
[...]
> > >  - 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.
> > 
> > Having tested patches along these lines, I think the above would avoid
> > the reported regressions.
> > 
> 
> FWIW, even this last part may be problematic.  It'll break anything
> that tries to allocate many small MAP_GROWSDOWN stacks on 32-
> bit.  Hopefully nothing does this, but maybe Java does.

glibc (NPTL) does not.  Java (at least Hotspot in OpenJDK 6,7, 8) does
not.  LinuxThreads *does* and is used by uclibc.  dietlibc *does*.  I
would be surprised if either was used for applications with very many
threads, but then this issue has thrown up a lot of surprises.

Ben.

-- 
Ben Hutchings
Man invented language to satisfy his deep need to complain. - Lily
Tomlin

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

#1681969

On Wed, Jul 5, 2017 at 4:50 PM, Ben Hutchings <ben@decadent.org.uk> wrote:
> On Wed, 2017-07-05 at 13:53 -0700, Andy Lutomirski wrote:
>> On Jul 5, 2017, at 12:32 PM, Ben Hutchings <ben@decadent.org.uk> wrote:
>> > On Wed, 2017-07-05 at 10:23 -0700, Andy Lutomirski wrote:
> [...]
>> > >  - 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.
>> >
>> > Having tested patches along these lines, I think the above would avoid
>> > the reported regressions.
>> >
>>
>> FWIW, even this last part may be problematic.  It'll break anything
>> that tries to allocate many small MAP_GROWSDOWN stacks on 32-
>> bit.  Hopefully nothing does this, but maybe Java does.
>
> glibc (NPTL) does not.  Java (at least Hotspot in OpenJDK 6,7, 8) does
> not.  LinuxThreads *does* and is used by uclibc.  dietlibc *does*.  I
> would be surprised if either was used for applications with very many
> threads, but then this issue has thrown up a lot of surprises.
>

Ugh.  But yeah, I'd be a bit surprised to see heavily threaded apps
using LinuxThreads or dietlibc.

LinuxThreads still uses modify_ldt(), right?  modify_ldt() performance
is abysmal, and I have no intention of even trying to optimize it.
Anyhow, you *can't* have more than 8192 threads if you use
modify_ldt() for TLS because you run out of LDT slots.  8192 * 64k
fits in 32 bits with room to spare, so this is unlikely to be a
showstopper.

--Andy

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

#1681955

On Wed, Jul 5, 2017 at 10:23 AM, Andy Lutomirski <luto@kernel.org> wrote:
> 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.

As part of that should we put restrictions on the environment of
set*id exec too? Part of the risks demonstrated by Qualys was that
allowing a privilege-elevating binary to inherit rlimits can have lead
to the nasty memory layout side-effects. That would fall into the
"hardening" bucket as well. And if it turns out there is some set*id
binary out there that can't run with "only", e.g., 128MB of stack, we
can make it configurable...

-Kees

-- 
Kees Cook
Pixel Security

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

#1681956

On Wed, Jul 5, 2017 at 4:50 PM, Kees Cook <keescook@chromium.org> wrote:
>
> As part of that should we put restrictions on the environment of
> set*id exec too?

I'm not seeing what sane limits you could use.

I think the concept of "reset as much of the environment to sane
things when running suid binaries" is a good concepr.

But we simply don't have any sane values to reset things to.

                Linus

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

#1681972

On Wed, Jul 5, 2017 at 4:55 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Wed, Jul 5, 2017 at 4:50 PM, Kees Cook <keescook@chromium.org> wrote:
>>
>> As part of that should we put restrictions on the environment of
>> set*id exec too?
>
> I'm not seeing what sane limits you could use.
>
> I think the concept of "reset as much of the environment to sane
> things when running suid binaries" is a good concepr.
>
> But we simply don't have any sane values to reset things to.

I wonder if we could pull some "sane" values out of our arses and have
it work just fine.

It's worth noting that a lot of the rlimits don't meaningfully
restrict the use of any particular resource, so we could plausibly
drop requirements to have privilege to increase them if we really
cared to.  I don't see why we'd make such a change, but it means that,
if we reset on set*id and therefore poke a hole that allows a program
to do "sudo -u $me whatever" and thereby reset limits, it's not so
bad.  A tiny survey:

RLIMIT_AS: not a systemwide resource at all.
RLIMIT_CORE: more or less just a policy of what you do when you crash.
I don't see how you could do much damage here.
RLIMIT_CPU: unless you're not allowed to fork(), this doesn't restrict
anything systemwide.
RLIMIT_DATA: ***
RLIMIT_FSIZE: maybe?  but I can see this being quite dangerous across set*id
RLIMIT_LOCKS: gone
RLIMIT_MEMLOCK: this one matters, but it also seems nearly worthless
for exploits
RLIMIT_MSGQUEUE: privilege matters here
RLIMIT_NICE: maybe?  anyone who actually cares would use cgroups instead
RLIMIT_NOFILE: great for exploits.  Only sort of useful for resource management
RLIMIT_NPROC: privilege matters here
RLIMIT_RTTIME: privilege kind of matters.  Also dangerous for exploits
(a bit) since it lets you kill your children at controlled times.
RLIMIT_SIGPENDING: not sure
RLIMIT_STACK: ***

*** means that this is a half-arsed resource control.  It's half-arsed
because this stuff doesn't cover mmap(2), which seems to me like it
defeats the purpose.  This stuff feels like a throwback to the
eighties.

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

#1681978

On Wed, Jul 5, 2017 at 5:31 PM, Andy Lutomirski <luto@kernel.org> wrote:
>
> I wonder if we could pull some "sane" values out of our arses and have
> it work just fine.

That approach may work, but it's pretty nasty.

But together with at least some way for the distro to set the values
we pick, it would probably be fairly reasonable.

You're right that most of the rlimits are just not very useful.

           Linus

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

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

Back to top | Article view | linux.kernel

csiph-web