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

Groups > linux.kernel > #1465255 > unrolled thread

[PATCH v4 54/57] x86/mm: convert arch_within_stack_frames() to use the new unwinder

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

  [PATCH v4 54/57] x86/mm: convert arch_within_stack_frames() to use the new unwinder Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-18 15:10 +0200
    Re: [PATCH v4 54/57] x86/mm: convert arch_within_stack_frames() to  use the new unwinder Kees Cook <keescook@chromium.org> - 2016-08-19 20:30 +0200
      Re: [PATCH v4 54/57] x86/mm: convert arch_within_stack_frames() to  use the new unwinder Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-20 00:00 +0200
        Re: [PATCH v4 54/57] x86/mm: convert arch_within_stack_frames() to  use the new unwinder Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-22 22:40 +0200
          Re: [PATCH v4 54/57] x86/mm: convert arch_within_stack_frames() to  use the new unwinder Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-23 01:40 +0200
            Re: [PATCH v4 54/57] x86/mm: convert arch_within_stack_frames() to  use the new unwinder Kees Cook <keescook@chromium.org> - 2016-08-23 03:00 +0200
              Re: [PATCH v4 54/57] x86/mm: convert arch_within_stack_frames() to  use the new unwinder Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-23 06:30 +0200
    Re: [PATCH v4 54/57] x86/mm: convert arch_within_stack_frames() to  use the new unwinder Linus Torvalds <torvalds@linux-foundation.org> - 2016-08-23 00:20 +0200
      Re: [PATCH v4 54/57] x86/mm: convert arch_within_stack_frames() to  use the new unwinder Kees Cook <keescook@chromium.org> - 2016-08-23 03:30 +0200
        Re: [PATCH v4 54/57] x86/mm: convert arch_within_stack_frames() to  use the new unwinder Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-23 18:40 +0200
        Re: [PATCH v4 54/57] x86/mm: convert arch_within_stack_frames() to  use the new unwinder Linus Torvalds <torvalds@linux-foundation.org> - 2016-08-24 01:20 +0200
      Re: [PATCH v4 54/57] x86/mm: convert arch_within_stack_frames() to  use the new unwinder Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-23 18:10 +0200
        [PATCH 2/2] mm/usercopy: enable usercopy size checking for modern versions of gcc Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-23 21:30 +0200
          Re: [PATCH 2/2] mm/usercopy: enable usercopy size checking for modern  versions of gcc Kees Cook <keescook@chromium.org> - 2016-08-24 05:50 +0200
        [PATCH 1/2] mm/usercopy: get rid of "provably correct" warnings Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-23 21:30 +0200
          Re: [PATCH 1/2] mm/usercopy: get rid of "provably correct" warnings Kees Cook <keescook@chromium.org> - 2016-08-24 04:40 +0200
      Re: [PATCH v4 54/57] x86/mm: convert arch_within_stack_frames() to  use the new unwinder Andy Lutomirski <luto@kernel.org> - 2016-08-23 22:50 +0200
        Re: [PATCH v4 54/57] x86/mm: convert arch_within_stack_frames() to  use the new unwinder Linus Torvalds <torvalds@linux-foundation.org> - 2016-08-23 23:10 +0200
        Re: [PATCH v4 54/57] x86/mm: convert arch_within_stack_frames() to  use the new unwinder Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-23 23:20 +0200
          Re: [PATCH v4 54/57] x86/mm: convert arch_within_stack_frames() to  use the new unwinder Kees Cook <keescook@chromium.org> - 2016-08-24 03:50 +0200

#1465255 — [PATCH v4 54/57] x86/mm: convert arch_within_stack_frames() to use the new unwinder

Convert arch_within_stack_frames() to use the new unwinder.

This also changes some existing behavior:

- Skip checking of pt_regs frames.
- Warn if it can't reach the grandparent's stack frame.
- Warn if it doesn't unwind to the end of the stack.

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/lib/usercopy.c | 44 ++++++++++++++++++++++++++++----------------
 1 file changed, 28 insertions(+), 16 deletions(-)

diff --git a/arch/x86/lib/usercopy.c b/arch/x86/lib/usercopy.c
index 2492fa7..8fe0a9c 100644
--- a/arch/x86/lib/usercopy.c
+++ b/arch/x86/lib/usercopy.c
@@ -50,30 +50,42 @@ int arch_within_stack_frames(const void * const stack,
 			     const void * const stackend,
 			     const void *obj, unsigned long len)
 {
-	const void *frame = NULL;
-	const void *oldframe;
+	struct unwind_state state;
+	const void *frame, *frame_end;
+
+	/*
+	 * Start at the end of our grandparent's frame (beginning of
+	 * great-grandparent's frame).
+	 */
+	unwind_start(&state, current, NULL, NULL);
+	if (WARN_ON_ONCE(!unwind_next_frame(&state) ||
+			 !unwind_next_frame(&state)))
+		return 0;
+	frame = unwind_get_stack_ptr(&state);
 
-	oldframe = __builtin_frame_address(2);
-	if (oldframe)
-		frame = __builtin_frame_address(3);
 	/*
 	 * low ----------------------------------------------> high
 	 * [saved bp][saved ip][args][local vars][saved bp][saved ip]
 	 *                     ^----------------^
 	 *               allow copies only within here
 	 */
-	while (stack <= frame && frame < stackend) {
-		/*
-		 * If obj + len extends past the last frame, this
-		 * check won't pass and the next frame will be 0,
-		 * causing us to bail out and correctly report
-		 * the copy as invalid.
-		 */
-		if (obj + len <= frame)
-			return obj >= oldframe + 2 * sizeof(void *) ? 1 : -1;
-		oldframe = frame;
-		frame = *(const void * const *)frame;
+	frame += 2*sizeof(long);
+
+	while (unwind_next_frame(&state)) {
+		frame_end = unwind_get_stack_ptr(&state);
+
+		/* skip checking of pt_regs frames */
+		if (!unwind_get_entry_regs(&state) &&
+		    obj >= frame && obj + len <= frame_end)
+			return 1;
+
+		frame = frame_end + 2*sizeof(long);
 	}
+
+	/* make sure the unwinder reached the end of the task stack */
+	if (WARN_ON_ONCE(frame != (void *)task_pt_regs(current)))
+		return 0;
+
 	return -1;
 }
 #endif /* CONFIG_HARDENED_USERCOPY && CONFIG_FRAME_POINTER */
-- 
2.7.4

[toc] | [next] | [standalone]

#1466604 — Re: [PATCH v4 54/57] x86/mm: convert arch_within_stack_frames() to use the new unwinder

On Thu, Aug 18, 2016 at 6:06 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> Convert arch_within_stack_frames() to use the new unwinder.
>
> This also changes some existing behavior:
>
> - Skip checking of pt_regs frames.
> - Warn if it can't reach the grandparent's stack frame.
> - Warn if it doesn't unwind to the end of the stack.
>
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>

All the stuff touching usercopy looks good to me. One question,
though, in looking through the unwinder. It seems like it's much more
complex than just the frame-hopping that the old
arch_within_stack_frames() did, but I'm curious to hear what you think
about its performance. We'll be calling this with every usercopy that
touches the stack, so I'd like to be able to estimate the performance
impact of this replacement...

-Kees

> ---
>  arch/x86/lib/usercopy.c | 44 ++++++++++++++++++++++++++++----------------
>  1 file changed, 28 insertions(+), 16 deletions(-)
>
> diff --git a/arch/x86/lib/usercopy.c b/arch/x86/lib/usercopy.c
> index 2492fa7..8fe0a9c 100644
> --- a/arch/x86/lib/usercopy.c
> +++ b/arch/x86/lib/usercopy.c
> @@ -50,30 +50,42 @@ int arch_within_stack_frames(const void * const stack,
>                              const void * const stackend,
>                              const void *obj, unsigned long len)
>  {
> -       const void *frame = NULL;
> -       const void *oldframe;
> +       struct unwind_state state;
> +       const void *frame, *frame_end;
> +
> +       /*
> +        * Start at the end of our grandparent's frame (beginning of
> +        * great-grandparent's frame).
> +        */
> +       unwind_start(&state, current, NULL, NULL);
> +       if (WARN_ON_ONCE(!unwind_next_frame(&state) ||
> +                        !unwind_next_frame(&state)))
> +               return 0;
> +       frame = unwind_get_stack_ptr(&state);
>
> -       oldframe = __builtin_frame_address(2);
> -       if (oldframe)
> -               frame = __builtin_frame_address(3);
>         /*
>          * low ----------------------------------------------> high
>          * [saved bp][saved ip][args][local vars][saved bp][saved ip]
>          *                     ^----------------^
>          *               allow copies only within here
>          */
> -       while (stack <= frame && frame < stackend) {
> -               /*
> -                * If obj + len extends past the last frame, this
> -                * check won't pass and the next frame will be 0,
> -                * causing us to bail out and correctly report
> -                * the copy as invalid.
> -                */
> -               if (obj + len <= frame)
> -                       return obj >= oldframe + 2 * sizeof(void *) ? 1 : -1;
> -               oldframe = frame;
> -               frame = *(const void * const *)frame;
> +       frame += 2*sizeof(long);
> +
> +       while (unwind_next_frame(&state)) {
> +               frame_end = unwind_get_stack_ptr(&state);
> +
> +               /* skip checking of pt_regs frames */
> +               if (!unwind_get_entry_regs(&state) &&
> +                   obj >= frame && obj + len <= frame_end)
> +                       return 1;
> +
> +               frame = frame_end + 2*sizeof(long);
>         }
> +
> +       /* make sure the unwinder reached the end of the task stack */
> +       if (WARN_ON_ONCE(frame != (void *)task_pt_regs(current)))
> +               return 0;
> +
>         return -1;
>  }
>  #endif /* CONFIG_HARDENED_USERCOPY && CONFIG_FRAME_POINTER */
> --
> 2.7.4
>



-- 
Kees Cook
Nexus Security

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

#1466699 — Re: [PATCH v4 54/57] x86/mm: convert arch_within_stack_frames() to use the new unwinder

On Fri, Aug 19, 2016 at 11:27:18AM -0700, Kees Cook wrote:
> On Thu, Aug 18, 2016 at 6:06 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > Convert arch_within_stack_frames() to use the new unwinder.
> >
> > This also changes some existing behavior:
> >
> > - Skip checking of pt_regs frames.
> > - Warn if it can't reach the grandparent's stack frame.
> > - Warn if it doesn't unwind to the end of the stack.
> >
> > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> 
> All the stuff touching usercopy looks good to me. One question,
> though, in looking through the unwinder. It seems like it's much more
> complex than just the frame-hopping that the old
> arch_within_stack_frames() did, but I'm curious to hear what you think
> about its performance. We'll be calling this with every usercopy that
> touches the stack, so I'd like to be able to estimate the performance
> impact of this replacement...

Yeah, good point.  I'll take some measurements from before and after and
get back to you.

-- 
Josh

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

#1468033 — Re: [PATCH v4 54/57] x86/mm: convert arch_within_stack_frames() to use the new unwinder

On Fri, Aug 19, 2016 at 04:55:22PM -0500, Josh Poimboeuf wrote:
> On Fri, Aug 19, 2016 at 11:27:18AM -0700, Kees Cook wrote:
> > On Thu, Aug 18, 2016 at 6:06 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > > Convert arch_within_stack_frames() to use the new unwinder.
> > >
> > > This also changes some existing behavior:
> > >
> > > - Skip checking of pt_regs frames.
> > > - Warn if it can't reach the grandparent's stack frame.
> > > - Warn if it doesn't unwind to the end of the stack.
> > >
> > > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > 
> > All the stuff touching usercopy looks good to me. One question,
> > though, in looking through the unwinder. It seems like it's much more
> > complex than just the frame-hopping that the old
> > arch_within_stack_frames() did, but I'm curious to hear what you think
> > about its performance. We'll be calling this with every usercopy that
> > touches the stack, so I'd like to be able to estimate the performance
> > impact of this replacement...
> 
> Yeah, good point.  I'll take some measurements from before and after and
> get back to you.

I took some before/after measurements by enclosing the affected
functions with ktime calls to get the total time spent in each function,
and did a "find /usr >/dev/null" to trigger a bunch of user copies.

	copy_to/from_user	check_object_size	arch_within_stack_frames
before: 13ms			6.8ms			0.61ms
after: 	17ms			11ms			4.6ms

The unwinder port made arch_within_stack_frames() *much* (8x) slower
than its current simple implementation, and added about 30% (4ms) to the
total copy_to/from_user() run time.

Note that hardened usercopy itself is already quite slow: it made user
copies about 52% slower.  With the unwinder port, that worsened to ~65%.

"find /usr" took about 170ms of kernel time and 2.3s total.  So the
unwinder port added about 2% on the kernel side and 0.2% total for this
particular test case.  Though I'm sure there are more I/O-intensive
workloads out there which would be more adversely affected.

I haven't yet looked to see where the bottlenecks are and if there could
be any obvious performance improvements.

BTW, ignoring the performance issues, using the unwinder here would have
some benefits:

- It protects pt_regs frames from being changed.  For example, during a
  page fault operation, the saved regs->ip on the stack is protected.

- Unlike the existing code, it could potentially work with
  __copy_from_user_inatomic() and copy_from_user_nmi(), which can copy
  to/from an irq/exception stack.  (I think check_stack_object() would
  need to be rewritten a bit so that it doesn't always assume the task
  stack.)

- It complains loudly if there's stack corruption or something else goes
  wrong with walking the stack instead of just silently failing.

- The same code could also work with DWARF if we ever add a DWARF
  unwinder (with a possible tweak to the unwinder API to get the stack
  frame header size).

-- 
Josh

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

#1468180 — Re: [PATCH v4 54/57] x86/mm: convert arch_within_stack_frames() to use the new unwinder

On Mon, Aug 22, 2016 at 03:27:19PM -0500, Josh Poimboeuf wrote:
> On Fri, Aug 19, 2016 at 04:55:22PM -0500, Josh Poimboeuf wrote:
> > On Fri, Aug 19, 2016 at 11:27:18AM -0700, Kees Cook wrote:
> > > On Thu, Aug 18, 2016 at 6:06 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > > > Convert arch_within_stack_frames() to use the new unwinder.
> > > >
> > > > This also changes some existing behavior:
> > > >
> > > > - Skip checking of pt_regs frames.
> > > > - Warn if it can't reach the grandparent's stack frame.
> > > > - Warn if it doesn't unwind to the end of the stack.
> > > >
> > > > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > > 
> > > All the stuff touching usercopy looks good to me. One question,
> > > though, in looking through the unwinder. It seems like it's much more
> > > complex than just the frame-hopping that the old
> > > arch_within_stack_frames() did, but I'm curious to hear what you think
> > > about its performance. We'll be calling this with every usercopy that
> > > touches the stack, so I'd like to be able to estimate the performance
> > > impact of this replacement...
> > 
> > Yeah, good point.  I'll take some measurements from before and after and
> > get back to you.
> 
> I took some before/after measurements by enclosing the affected
> functions with ktime calls to get the total time spent in each function,
> and did a "find /usr >/dev/null" to trigger a bunch of user copies.
> 
> 	copy_to/from_user	check_object_size	arch_within_stack_frames
> before: 13ms			6.8ms			0.61ms
> after: 	17ms			11ms			4.6ms
> 
> The unwinder port made arch_within_stack_frames() *much* (8x) slower
> than its current simple implementation, and added about 30% (4ms) to the
> total copy_to/from_user() run time.
> 
> Note that hardened usercopy itself is already quite slow: it made user
> copies about 52% slower.  With the unwinder port, that worsened to ~65%.

FWIW, I think I messed up my math summary here.  Hardened usercopy was
roughly 110% slower than normal usercopy (i.e., it took more than twice
as long) with 52% of the usercopy time being consumed by
check_object_size().

With the unwinder, that worsened to 180% slower -- with 65% of the
usercopy time being consumed by check_object_size().

-- 
Josh

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

#1468214 — Re: [PATCH v4 54/57] x86/mm: convert arch_within_stack_frames() to use the new unwinder

On Mon, Aug 22, 2016 at 4:33 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> On Mon, Aug 22, 2016 at 03:27:19PM -0500, Josh Poimboeuf wrote:
>> On Fri, Aug 19, 2016 at 04:55:22PM -0500, Josh Poimboeuf wrote:
>> > On Fri, Aug 19, 2016 at 11:27:18AM -0700, Kees Cook wrote:
>> > > On Thu, Aug 18, 2016 at 6:06 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>> > > > Convert arch_within_stack_frames() to use the new unwinder.
>> > > >
>> > > > This also changes some existing behavior:
>> > > >
>> > > > - Skip checking of pt_regs frames.
>> > > > - Warn if it can't reach the grandparent's stack frame.
>> > > > - Warn if it doesn't unwind to the end of the stack.
>> > > >
>> > > > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
>> > >
>> > > All the stuff touching usercopy looks good to me. One question,
>> > > though, in looking through the unwinder. It seems like it's much more
>> > > complex than just the frame-hopping that the old
>> > > arch_within_stack_frames() did, but I'm curious to hear what you think
>> > > about its performance. We'll be calling this with every usercopy that
>> > > touches the stack, so I'd like to be able to estimate the performance
>> > > impact of this replacement...
>> >
>> > Yeah, good point.  I'll take some measurements from before and after and
>> > get back to you.
>>
>> I took some before/after measurements by enclosing the affected
>> functions with ktime calls to get the total time spent in each function,
>> and did a "find /usr >/dev/null" to trigger a bunch of user copies.
>>
>>       copy_to/from_user       check_object_size       arch_within_stack_frames
>> before: 13ms                  6.8ms                   0.61ms
>> after:        17ms                    11ms                    4.6ms
>>
>> The unwinder port made arch_within_stack_frames() *much* (8x) slower
>> than its current simple implementation, and added about 30% (4ms) to the
>> total copy_to/from_user() run time.
>>
>> Note that hardened usercopy itself is already quite slow: it made user
>> copies about 52% slower.  With the unwinder port, that worsened to ~65%.
>
> FWIW, I think I messed up my math summary here.  Hardened usercopy was
> roughly 110% slower than normal usercopy (i.e., it took more than twice
> as long) with 52% of the usercopy time being consumed by
> check_object_size().

And this is comparing usercopy to hardened usercopy, which isn't
expected to be super fast, it's just a cheap expense in comparison to
the rest of the work being done for a given syscall.

> With the unwinder, that worsened to 180% slower -- with 65% of the
> usercopy time being consumed by check_object_size().

That's quite a bit more than just a simple frame walk. You mentioned a
few benefits to using the unwinder, but I'm trying to make sure the
cases it covers can actually happen during a usercopy?

-Kees

-- 
Kees Cook
Nexus Security

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

#1468265 — Re: [PATCH v4 54/57] x86/mm: convert arch_within_stack_frames() to use the new unwinder

On Mon, Aug 22, 2016 at 05:59:18PM -0700, Kees Cook wrote:
> On Mon, Aug 22, 2016 at 4:33 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > On Mon, Aug 22, 2016 at 03:27:19PM -0500, Josh Poimboeuf wrote:
> >> On Fri, Aug 19, 2016 at 04:55:22PM -0500, Josh Poimboeuf wrote:
> >> > On Fri, Aug 19, 2016 at 11:27:18AM -0700, Kees Cook wrote:
> >> > > On Thu, Aug 18, 2016 at 6:06 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >> > > > Convert arch_within_stack_frames() to use the new unwinder.
> >> > > >
> >> > > > This also changes some existing behavior:
> >> > > >
> >> > > > - Skip checking of pt_regs frames.
> >> > > > - Warn if it can't reach the grandparent's stack frame.
> >> > > > - Warn if it doesn't unwind to the end of the stack.
> >> > > >
> >> > > > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> >> > >
> >> > > All the stuff touching usercopy looks good to me. One question,
> >> > > though, in looking through the unwinder. It seems like it's much more
> >> > > complex than just the frame-hopping that the old
> >> > > arch_within_stack_frames() did, but I'm curious to hear what you think
> >> > > about its performance. We'll be calling this with every usercopy that
> >> > > touches the stack, so I'd like to be able to estimate the performance
> >> > > impact of this replacement...
> >> >
> >> > Yeah, good point.  I'll take some measurements from before and after and
> >> > get back to you.
> >>
> >> I took some before/after measurements by enclosing the affected
> >> functions with ktime calls to get the total time spent in each function,
> >> and did a "find /usr >/dev/null" to trigger a bunch of user copies.
> >>
> >>       copy_to/from_user       check_object_size       arch_within_stack_frames
> >> before: 13ms                  6.8ms                   0.61ms
> >> after:        17ms                    11ms                    4.6ms
> >>
> >> The unwinder port made arch_within_stack_frames() *much* (8x) slower
> >> than its current simple implementation, and added about 30% (4ms) to the
> >> total copy_to/from_user() run time.
> >>
> >> Note that hardened usercopy itself is already quite slow: it made user
> >> copies about 52% slower.  With the unwinder port, that worsened to ~65%.
> >
> > FWIW, I think I messed up my math summary here.  Hardened usercopy was
> > roughly 110% slower than normal usercopy (i.e., it took more than twice
> > as long) with 52% of the usercopy time being consumed by
> > check_object_size().
> 
> And this is comparing usercopy to hardened usercopy, which isn't
> expected to be super fast, it's just a cheap expense in comparison to
> the rest of the work being done for a given syscall.
> 
> > With the unwinder, that worsened to 180% slower -- with 65% of the
> > usercopy time being consumed by check_object_size().
> 
> That's quite a bit more than just a simple frame walk. You mentioned a
> few benefits to using the unwinder, but I'm trying to make sure the
> cases it covers can actually happen during a usercopy?

Yeah, I really don't know.  And given Linus's objections, I think I'll
drop it, and the other usercopy patches.

Though I think "move arch_within_stack_frames() to usercopy.c" is still
a nice cleanup if you want to pick that one up.

-- 
Josh

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

#1468103 — Re: [PATCH v4 54/57] x86/mm: convert arch_within_stack_frames() to use the new unwinder

On Thu, Aug 18, 2016 at 6:06 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> Convert arch_within_stack_frames() to use the new unwinder.

Please don't do this.

There's no real reason to unwind the stack frame. If it's not on the
current stack page, it shouldn't be a valid source anyway, so
unwidning things just seems entirely pointless.

Quite frankly, I think the whole "look at the stack frames" logic
should be removed from this. It's classic crap that external patches
do. How many call-sites does it actually check, and how many of them
aren't already checked by the existing static checks for constant
addresses within existing objects?

It's entirely possible that there is simply no point what-so-ever to
this all, and it mostly triggers on things like the fs/stat.c code
that does

        struct stat tmp;
    ...
        return copy_to_user(statbuf,&tmp,sizeof(tmp)) ? -EFAULT : 0;

where the new useraccess.c code is pure masturbatory crap.

One of the reasons I had for merging that code was that I was hoping
that it would improve by being in the  kernel. And by "improve" I mean
"get rid of crap" rather than make it more expensive and even more
self-congratulatory stupidity.

Right now, I suspect 99% of all the stack checks in usercopy.c are
solidly in the "mindbogglingly stupid crap" camp.

             Linus

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

#1468223 — Re: [PATCH v4 54/57] x86/mm: convert arch_within_stack_frames() to use the new unwinder

On Mon, Aug 22, 2016 at 3:11 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Thu, Aug 18, 2016 at 6:06 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>> Convert arch_within_stack_frames() to use the new unwinder.
>
> Please don't do this.
>
> There's no real reason to unwind the stack frame. If it's not on the
> current stack page, it shouldn't be a valid source anyway, so
> unwidning things just seems entirely pointless.
>
> Quite frankly, I think the whole "look at the stack frames" logic
> should be removed from this. It's classic crap that external patches
> do. How many call-sites does it actually check, and how many of them
> aren't already checked by the existing static checks for constant
> addresses within existing objects?
>
> It's entirely possible that there is simply no point what-so-ever to
> this all, and it mostly triggers on things like the fs/stat.c code
> that does
>
>         struct stat tmp;
>     ...
>         return copy_to_user(statbuf,&tmp,sizeof(tmp)) ? -EFAULT : 0;
>
> where the new useraccess.c code is pure masturbatory crap.

I need to re-check the copy_*_user changes, but on several
architectures, the bounds checking is only triggered for non
built-in-const sizes, so these kinds of pointless checks shouldn't
happen. This should be done universally to avoid the needless
overhead. (And is why I'm hoping to consolidate the copy_*_user logic,
which Al appears to also be looking at recently.)

> One of the reasons I had for merging that code was that I was hoping
> that it would improve by being in the  kernel. And by "improve" I mean
> "get rid of crap" rather than make it more expensive and even more
> self-congratulatory stupidity.
>
> Right now, I suspect 99% of all the stack checks in usercopy.c are
> solidly in the "mindbogglingly stupid crap" camp.

The stack bounds checking makes sense to block writes to the saved
frame and instruction pointers, though in practice the stack canary
should resist that kind of attack. The improvement I'd like to see
would be for the canary to be excluded from the frame size calculation
(though I can't imagine how) so that canaries couldn't be exposed
during reads.

-Kees

-- 
Kees Cook
Nexus Security

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

#1468693 — Re: [PATCH v4 54/57] x86/mm: convert arch_within_stack_frames() to use the new unwinder

On Mon, Aug 22, 2016 at 06:27:28PM -0700, Kees Cook wrote:
> On Mon, Aug 22, 2016 at 3:11 PM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> > On Thu, Aug 18, 2016 at 6:06 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >> Convert arch_within_stack_frames() to use the new unwinder.
> >
> > Please don't do this.
> >
> > There's no real reason to unwind the stack frame. If it's not on the
> > current stack page, it shouldn't be a valid source anyway, so
> > unwidning things just seems entirely pointless.
> >
> > Quite frankly, I think the whole "look at the stack frames" logic
> > should be removed from this. It's classic crap that external patches
> > do. How many call-sites does it actually check, and how many of them
> > aren't already checked by the existing static checks for constant
> > addresses within existing objects?
> >
> > It's entirely possible that there is simply no point what-so-ever to
> > this all, and it mostly triggers on things like the fs/stat.c code
> > that does
> >
> >         struct stat tmp;
> >     ...
> >         return copy_to_user(statbuf,&tmp,sizeof(tmp)) ? -EFAULT : 0;
> >
> > where the new useraccess.c code is pure masturbatory crap.
> 
> I need to re-check the copy_*_user changes, but on several
> architectures, the bounds checking is only triggered for non
> built-in-const sizes, so these kinds of pointless checks shouldn't
> happen. This should be done universally to avoid the needless
> overhead. (And is why I'm hoping to consolidate the copy_*_user logic,
> which Al appears to also be looking at recently.)

I noticed you added this check for powerpc:

	if (!__builtin_constant_p(n))
		check_object_size(to, n, false);

But I don't see a similar check on x86 or any of the other arches I
looked at.  Was that an oversight or is there a specific reason for
doing it on some arches and not others?

> > One of the reasons I had for merging that code was that I was hoping
> > that it would improve by being in the  kernel. And by "improve" I mean
> > "get rid of crap" rather than make it more expensive and even more
> > self-congratulatory stupidity.
> >
> > Right now, I suspect 99% of all the stack checks in usercopy.c are
> > solidly in the "mindbogglingly stupid crap" camp.
> 
> The stack bounds checking makes sense to block writes to the saved
> frame and instruction pointers, though in practice the stack canary
> should resist that kind of attack. The improvement I'd like to see
> would be for the canary to be excluded from the frame size calculation
> (though I can't imagine how) so that canaries couldn't be exposed
> during reads.

Yeah, protecting the stack canary would be nice, but it would be hard
without DWARF.  The only way I can think of doing it would be with a gcc
plugin or an objtool extension which creates some kind of fast-access
table of per-function canary stack offsets for
arch_within_stack_frames() to consult.

-- 
Josh

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

#1468968 — Re: [PATCH v4 54/57] x86/mm: convert arch_within_stack_frames() to use the new unwinder

On Mon, Aug 22, 2016 at 9:27 PM, Kees Cook <keescook@chromium.org> wrote:
>
> I need to re-check the copy_*_user changes, but on several
> architectures, the bounds checking is only triggered for non
> built-in-const sizes, so these kinds of pointless checks shouldn't
> happen.

They definitely happen at least on x86.

"stat()" is one common user of fixed-sized structures being copied.
There are tons of others, but 'stat()' is the one I've seen in my
profiles before as being noticeable. It's been critical enough that I
have occasionally tried to play with making it avoid the "copy to
temporary struct, then copy_to_user() the whole struct" and just do it
field-by-field. But it gets nasty with the padding fields etc, so it's
never been done.

Not doing the access size checks for constant-sized copies (at least
when they are "sufficiently small" constants) would probably be the
right thing to do, and then depend on gcc just getting the static case
right warning-wise. Which isn't apparently getting done right now
either, but oh well..

            Linus

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

#1468674 — Re: [PATCH v4 54/57] x86/mm: convert arch_within_stack_frames() to use the new unwinder

On Mon, Aug 22, 2016 at 03:11:32PM -0700, Linus Torvalds wrote:
> On Thu, Aug 18, 2016 at 6:06 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > Convert arch_within_stack_frames() to use the new unwinder.
> 
> Please don't do this.
> 
> There's no real reason to unwind the stack frame. If it's not on the
> current stack page, it shouldn't be a valid source anyway, so
> unwidning things just seems entirely pointless.
> 
> Quite frankly, I think the whole "look at the stack frames" logic
> should be removed from this. It's classic crap that external patches
> do. How many call-sites does it actually check, and how many of them
> aren't already checked by the existing static checks for constant
> addresses within existing objects?

I noticed the __compiletime_object_size() check is completely disabled
for gcc >= 4.6, thanks to:

  2fb0815c9ee6 ("gcc4: disable __compiletime_object_size for GCC 4.6+")

AFAICT, that change went too far: it disabled both the compile-time
*and* the runtime checks, so copy_from_user_overflow() is never called.

Working on a couple of patches to try to fix that.

-- 
Josh

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

#1468792 — [PATCH 2/2] mm/usercopy: enable usercopy size checking for modern versions of gcc

This is a revert of:

  2fb0815c9ee6 ("gcc4: disable __compiletime_object_size for GCC 4.6+")

The goal of that commit was to silence the "provably correct" gcc
warnings.  But it went too far: it also disabled the runtime warnings.

Now that the pretty much useless gcc warnings have been properly
disposed of with the previous patch, re-enable this checking on modern
versions of gcc so we can get the runtime warnings again.

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 include/linux/compiler-gcc.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
index e294939..e7f7a68 100644
--- a/include/linux/compiler-gcc.h
+++ b/include/linux/compiler-gcc.h
@@ -158,7 +158,7 @@
 #define __compiler_offsetof(a, b)					\
 	__builtin_offsetof(a, b)
 
-#if GCC_VERSION >= 40100 && GCC_VERSION < 40600
+#if GCC_VERSION >= 40100
 # define __compiletime_object_size(obj) __builtin_object_size(obj, 0)
 #endif
 
-- 
2.7.4

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

#1469060 — Re: [PATCH 2/2] mm/usercopy: enable usercopy size checking for modern versions of gcc

On Tue, Aug 23, 2016 at 3:28 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> This is a revert of:
>
>   2fb0815c9ee6 ("gcc4: disable __compiletime_object_size for GCC 4.6+")
>
> The goal of that commit was to silence the "provably correct" gcc
> warnings.  But it went too far: it also disabled the runtime warnings.
>
> Now that the pretty much useless gcc warnings have been properly
> disposed of with the previous patch, re-enable this checking on modern
> versions of gcc so we can get the runtime warnings again.

As far as I know, this will still be broken since it's
__builtin_object_size() that is buggy. Maybe I'm misunderstanding
which piece is busted, though?

-Kees

>
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> ---
>  include/linux/compiler-gcc.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
> index e294939..e7f7a68 100644
> --- a/include/linux/compiler-gcc.h
> +++ b/include/linux/compiler-gcc.h
> @@ -158,7 +158,7 @@
>  #define __compiler_offsetof(a, b)                                      \
>         __builtin_offsetof(a, b)
>
> -#if GCC_VERSION >= 40100 && GCC_VERSION < 40600
> +#if GCC_VERSION >= 40100
>  # define __compiletime_object_size(obj) __builtin_object_size(obj, 0)
>  #endif
>
> --
> 2.7.4
>



-- 
Kees Cook
Nexus Security

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

#1468794 — [PATCH 1/2] mm/usercopy: get rid of "provably correct" warnings

With CONFIG_DEBUG_STRICT_USER_COPY_CHECKS=y, if I force enable the
__compiletime_object_size() macro with a recent compiler by removing the
"GCC_VERSION < 40600" check, I get a bunch of false positive warnings.
For example:

  In function ‘copy_to_user.part.8’,
      inlined from ‘copy_to_user’,
      inlined from ‘proc_put_long’ at /home/jpoimboe/git/linux/kernel/sysctl.c:2096:6:
  /home/jpoimboe/git/linux/arch/x86/include/asm/uaccess.h:723:46: warning: call to ‘__copy_to_user_overflow’ declared with attribute warning: copy_to_user() buffer size is not provably correct

The problem is that gcc can't always definitively tell whether
copy_from_user_overflow() will be called.  And when in doubt, it prints
the warning anyway.  So in practice, these warnings mostly just create a
lot of noise.  There might be a bug lurking in there somewhere, but the
signal to noise ratio is really low, and not worth the pain IMO.

So just remove the "provably correct" warnings altogether.  This also
lays the groundwork for re-enabling the copy_from_user_overflow()
runtime warnings for newer compilers.

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/parisc/include/asm/uaccess.h |  8 +-------
 arch/s390/include/asm/uaccess.h   |  6 +-----
 arch/tile/include/asm/uaccess.h   |  3 +--
 arch/x86/include/asm/uaccess.h    | 35 -----------------------------------
 4 files changed, 3 insertions(+), 49 deletions(-)

diff --git a/arch/parisc/include/asm/uaccess.h b/arch/parisc/include/asm/uaccess.h
index 0f59fd9..b34c022 100644
--- a/arch/parisc/include/asm/uaccess.h
+++ b/arch/parisc/include/asm/uaccess.h
@@ -208,13 +208,7 @@ unsigned long copy_in_user(void __user *dst, const void __user *src, unsigned lo
 #define __copy_to_user_inatomic __copy_to_user
 #define __copy_from_user_inatomic __copy_from_user
 
-extern void copy_from_user_overflow(void)
-#ifdef CONFIG_DEBUG_STRICT_USER_COPY_CHECKS
-        __compiletime_error("copy_from_user() buffer size is not provably correct")
-#else
-        __compiletime_warning("copy_from_user() buffer size is not provably correct")
-#endif
-;
+extern void copy_from_user_overflow(void);
 
 static inline unsigned long __must_check copy_from_user(void *to,
                                           const void __user *from,
diff --git a/arch/s390/include/asm/uaccess.h b/arch/s390/include/asm/uaccess.h
index 9b49cf1..6d36860 100644
--- a/arch/s390/include/asm/uaccess.h
+++ b/arch/s390/include/asm/uaccess.h
@@ -332,11 +332,7 @@ copy_to_user(void __user *to, const void *from, unsigned long n)
 	return __copy_to_user(to, from, n);
 }
 
-void copy_from_user_overflow(void)
-#ifdef CONFIG_DEBUG_STRICT_USER_COPY_CHECKS
-__compiletime_warning("copy_from_user() buffer size is not provably correct")
-#endif
-;
+void copy_from_user_overflow(void);
 
 /**
  * copy_from_user: - Copy a block of data from user space.
diff --git a/arch/tile/include/asm/uaccess.h b/arch/tile/include/asm/uaccess.h
index 0a9c4265..e0e313f 100644
--- a/arch/tile/include/asm/uaccess.h
+++ b/arch/tile/include/asm/uaccess.h
@@ -422,8 +422,7 @@ _copy_from_user(void *to, const void __user *from, unsigned long n)
  * option is not really compatible with -Werror, which is more useful in
  * general.
  */
-extern void copy_from_user_overflow(void)
-	__compiletime_warning("copy_from_user() size is not provably correct");
+extern void copy_from_user_overflow(void);
 
 static inline unsigned long __must_check copy_from_user(void *to,
 					  const void __user *from,
diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index a0ae610..89c12cb 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -710,20 +710,6 @@ copy_to_user_overflow(void) __asm__("copy_from_user_overflow");
 
 #undef copy_user_diag
 
-#ifdef CONFIG_DEBUG_STRICT_USER_COPY_CHECKS
-
-extern void
-__compiletime_warning("copy_from_user() buffer size is not provably correct")
-__copy_from_user_overflow(void) __asm__("copy_from_user_overflow");
-#define __copy_from_user_overflow(size, count) __copy_from_user_overflow()
-
-extern void
-__compiletime_warning("copy_to_user() buffer size is not provably correct")
-__copy_to_user_overflow(void) __asm__("copy_from_user_overflow");
-#define __copy_to_user_overflow(size, count) __copy_to_user_overflow()
-
-#else
-
 static inline void
 __copy_from_user_overflow(int size, unsigned long count)
 {
@@ -732,8 +718,6 @@ __copy_from_user_overflow(int size, unsigned long count)
 
 #define __copy_to_user_overflow __copy_from_user_overflow
 
-#endif
-
 static inline unsigned long __must_check
 copy_from_user(void *to, const void __user *from, unsigned long n)
 {
@@ -743,24 +727,6 @@ copy_from_user(void *to, const void __user *from, unsigned long n)
 
 	kasan_check_write(to, n);
 
-	/*
-	 * While we would like to have the compiler do the checking for us
-	 * even in the non-constant size case, any false positives there are
-	 * a problem (especially when DEBUG_STRICT_USER_COPY_CHECKS, but even
-	 * without - the [hopefully] dangerous looking nature of the warning
-	 * would make people go look at the respecitive call sites over and
-	 * over again just to find that there's no problem).
-	 *
-	 * And there are cases where it's just not realistic for the compiler
-	 * to prove the count to be in range. For example when multiple call
-	 * sites of a helper function - perhaps in different source files -
-	 * all doing proper range checking, yet the helper function not doing
-	 * so again.
-	 *
-	 * Therefore limit the compile time checking to the constant size
-	 * case, and do only runtime checking for non-constant sizes.
-	 */
-
 	if (likely(sz < 0 || sz >= n)) {
 		check_object_size(to, n, false);
 		n = _copy_from_user(to, from, n);
@@ -781,7 +747,6 @@ copy_to_user(void __user *to, const void *from, unsigned long n)
 
 	might_fault();
 
-	/* See the comment in copy_from_user() above. */
 	if (likely(sz < 0 || sz >= n)) {
 		check_object_size(from, n, true);
 		n = _copy_to_user(to, from, n);
-- 
2.7.4

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

#1469035 — Re: [PATCH 1/2] mm/usercopy: get rid of "provably correct" warnings

On Tue, Aug 23, 2016 at 3:28 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> With CONFIG_DEBUG_STRICT_USER_COPY_CHECKS=y, if I force enable the
> __compiletime_object_size() macro with a recent compiler by removing the
> "GCC_VERSION < 40600" check, I get a bunch of false positive warnings.
> For example:
>
>   In function ‘copy_to_user.part.8’,
>       inlined from ‘copy_to_user’,
>       inlined from ‘proc_put_long’ at /home/jpoimboe/git/linux/kernel/sysctl.c:2096:6:
>   /home/jpoimboe/git/linux/arch/x86/include/asm/uaccess.h:723:46: warning: call to ‘__copy_to_user_overflow’ declared with attribute warning: copy_to_user() buffer size is not provably correct
>
> The problem is that gcc can't always definitively tell whether
> copy_from_user_overflow() will be called.  And when in doubt, it prints
> the warning anyway.  So in practice, these warnings mostly just create a
> lot of noise.  There might be a bug lurking in there somewhere, but the
> signal to noise ratio is really low, and not worth the pain IMO.
>
> So just remove the "provably correct" warnings altogether.  This also
> lays the groundwork for re-enabling the copy_from_user_overflow()
> runtime warnings for newer compilers.

Hrrrm, I'd much rather split configs or something. This "probably
correct" warning is something gcc should be ABLE to do, but the
ability regressed and hasn't yet been fixed:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=46639
originally: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=48880

We should get that back at some point, and I'd like to have the
compile-time checks enabled again then without having to reintroduce
the code.

Jeff, any news on this front? It'd be really nice to get this back in.
One of your comments in 2014 on the bug make it sound like it might be
easy to fix?

-Kees

>
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> ---
>  arch/parisc/include/asm/uaccess.h |  8 +-------
>  arch/s390/include/asm/uaccess.h   |  6 +-----
>  arch/tile/include/asm/uaccess.h   |  3 +--
>  arch/x86/include/asm/uaccess.h    | 35 -----------------------------------
>  4 files changed, 3 insertions(+), 49 deletions(-)
>
> diff --git a/arch/parisc/include/asm/uaccess.h b/arch/parisc/include/asm/uaccess.h
> index 0f59fd9..b34c022 100644
> --- a/arch/parisc/include/asm/uaccess.h
> +++ b/arch/parisc/include/asm/uaccess.h
> @@ -208,13 +208,7 @@ unsigned long copy_in_user(void __user *dst, const void __user *src, unsigned lo
>  #define __copy_to_user_inatomic __copy_to_user
>  #define __copy_from_user_inatomic __copy_from_user
>
> -extern void copy_from_user_overflow(void)
> -#ifdef CONFIG_DEBUG_STRICT_USER_COPY_CHECKS
> -        __compiletime_error("copy_from_user() buffer size is not provably correct")
> -#else
> -        __compiletime_warning("copy_from_user() buffer size is not provably correct")
> -#endif
> -;
> +extern void copy_from_user_overflow(void);
>
>  static inline unsigned long __must_check copy_from_user(void *to,
>                                            const void __user *from,
> diff --git a/arch/s390/include/asm/uaccess.h b/arch/s390/include/asm/uaccess.h
> index 9b49cf1..6d36860 100644
> --- a/arch/s390/include/asm/uaccess.h
> +++ b/arch/s390/include/asm/uaccess.h
> @@ -332,11 +332,7 @@ copy_to_user(void __user *to, const void *from, unsigned long n)
>         return __copy_to_user(to, from, n);
>  }
>
> -void copy_from_user_overflow(void)
> -#ifdef CONFIG_DEBUG_STRICT_USER_COPY_CHECKS
> -__compiletime_warning("copy_from_user() buffer size is not provably correct")
> -#endif
> -;
> +void copy_from_user_overflow(void);
>
>  /**
>   * copy_from_user: - Copy a block of data from user space.
> diff --git a/arch/tile/include/asm/uaccess.h b/arch/tile/include/asm/uaccess.h
> index 0a9c4265..e0e313f 100644
> --- a/arch/tile/include/asm/uaccess.h
> +++ b/arch/tile/include/asm/uaccess.h
> @@ -422,8 +422,7 @@ _copy_from_user(void *to, const void __user *from, unsigned long n)
>   * option is not really compatible with -Werror, which is more useful in
>   * general.
>   */
> -extern void copy_from_user_overflow(void)
> -       __compiletime_warning("copy_from_user() size is not provably correct");
> +extern void copy_from_user_overflow(void);
>
>  static inline unsigned long __must_check copy_from_user(void *to,
>                                           const void __user *from,
> diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
> index a0ae610..89c12cb 100644
> --- a/arch/x86/include/asm/uaccess.h
> +++ b/arch/x86/include/asm/uaccess.h
> @@ -710,20 +710,6 @@ copy_to_user_overflow(void) __asm__("copy_from_user_overflow");
>
>  #undef copy_user_diag
>
> -#ifdef CONFIG_DEBUG_STRICT_USER_COPY_CHECKS
> -
> -extern void
> -__compiletime_warning("copy_from_user() buffer size is not provably correct")
> -__copy_from_user_overflow(void) __asm__("copy_from_user_overflow");
> -#define __copy_from_user_overflow(size, count) __copy_from_user_overflow()
> -
> -extern void
> -__compiletime_warning("copy_to_user() buffer size is not provably correct")
> -__copy_to_user_overflow(void) __asm__("copy_from_user_overflow");
> -#define __copy_to_user_overflow(size, count) __copy_to_user_overflow()
> -
> -#else
> -
>  static inline void
>  __copy_from_user_overflow(int size, unsigned long count)
>  {
> @@ -732,8 +718,6 @@ __copy_from_user_overflow(int size, unsigned long count)
>
>  #define __copy_to_user_overflow __copy_from_user_overflow
>
> -#endif
> -
>  static inline unsigned long __must_check
>  copy_from_user(void *to, const void __user *from, unsigned long n)
>  {
> @@ -743,24 +727,6 @@ copy_from_user(void *to, const void __user *from, unsigned long n)
>
>         kasan_check_write(to, n);
>
> -       /*
> -        * While we would like to have the compiler do the checking for us
> -        * even in the non-constant size case, any false positives there are
> -        * a problem (especially when DEBUG_STRICT_USER_COPY_CHECKS, but even
> -        * without - the [hopefully] dangerous looking nature of the warning
> -        * would make people go look at the respecitive call sites over and
> -        * over again just to find that there's no problem).
> -        *
> -        * And there are cases where it's just not realistic for the compiler
> -        * to prove the count to be in range. For example when multiple call
> -        * sites of a helper function - perhaps in different source files -
> -        * all doing proper range checking, yet the helper function not doing
> -        * so again.
> -        *
> -        * Therefore limit the compile time checking to the constant size
> -        * case, and do only runtime checking for non-constant sizes.
> -        */
> -
>         if (likely(sz < 0 || sz >= n)) {
>                 check_object_size(to, n, false);
>                 n = _copy_from_user(to, from, n);
> @@ -781,7 +747,6 @@ copy_to_user(void __user *to, const void *from, unsigned long n)
>
>         might_fault();
>
> -       /* See the comment in copy_from_user() above. */
>         if (likely(sz < 0 || sz >= n)) {
>                 check_object_size(from, n, true);
>                 n = _copy_to_user(to, from, n);
> --
> 2.7.4
>



-- 
Kees Cook
Nexus Security

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

#1468841 — Re: [PATCH v4 54/57] x86/mm: convert arch_within_stack_frames() to use the new unwinder

On Aug 23, 2016 12:11 AM, "Linus Torvalds"
<torvalds@linux-foundation.org> wrote:
>
> On Thu, Aug 18, 2016 at 6:06 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > Convert arch_within_stack_frames() to use the new unwinder.
>
> Please don't do this.
>
> There's no real reason to unwind the stack frame. If it's not on the
> current stack page, it shouldn't be a valid source anyway, so
> unwidning things just seems entirely pointless.
>
> Quite frankly, I think the whole "look at the stack frames" logic
> should be removed from this. It's classic crap that external patches
> do. How many call-sites does it actually check, and how many of them
> aren't already checked by the existing static checks for constant
> addresses within existing objects?
>

I'm a bit confused by what you're objecting to.  If I write:

char buf[123];

func(buf, size);

And func eventually does some usercopy to buf, the idea is to check
that size is in bounds.  Now admittedly this kind of code should be
quite rare in the kernel, and it should be even rarer for the buffer
to be more than a frame or two up the stack.

So the fact that this seems to have any significant effect on
performance suggests to me that it's being run unnecessarily or that
somehow we're walking all the way to the top of the stack in cases
where we shouldn't have done so.

Josh, can you see an example call site in a profile of your test to
find out what this code is doing?

All that being said, Linus, assuming that Josh's new unwinder can be
made reasonably performant, I don't understand your objection to this
patch in particular.  Josh isn't changing the way that usercopy
hardening works -- he's just changing the stack walking
implementation.  It seems that you're objecting to this code in
general, but that predates Josh's patch, no?

--Andy

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

#1468861 — Re: [PATCH v4 54/57] x86/mm: convert arch_within_stack_frames() to use the new unwinder

On Tue, Aug 23, 2016 at 4:31 PM, Andy Lutomirski <luto@kernel.org> wrote:
>
> I'm a bit confused by what you're objecting to.  If I write:
>
> char buf[123];
>
> func(buf, size);
>
> And func eventually does some usercopy to buf, the idea is to check
> that size is in bounds.

That's the *IDEA*.

That's not what the code actually does.

The code will follow arbitrary stack frames, which seems silly since
it's expensive. At least the old code only checked within one page
(looking at the "stackend" thing), and aborted whenever the trivla
frame pointer chasing didn't. The new code may be a nice abstraction,
but also seems to not do that, and just follow the frame in general.

Should we have nested stacks and copy_to_user()? No. But why have
generic frame following code when we don't want the generic case to
ever trigger? If the code is slower - and Josh said it was quite
noticeably slower, then what's the advantage?

But my *real* objection is that I suspect that in 99% of all cases we
shouldn't do any of this, and the user access hardening should be made
smart enough that we don't need to worry about it. Right now the
hardening is not that smart. It tries to handle the case you mention,
but it does so by *also* handling the case _I_ mentioned, which is the
"trivially statically correct at build time", where the code is

    struct xyz tmp;

    .. fill in tmo ..

     copy_to_user(ptr, &tmp, sizeof(tmp));

where wasting cycles to see if it's on the stack is just stupid.

And quite frankly, I suspect that *most* situations where you copy
from or to the stack are very obvious constant sizes like the above.

Can  you find a _single_ case of a non-constant buffer on the stack?
It's rare. If it's a variably-sized area, 99% of all time it's a
dynamic allocation, not a stack variable.

So I actually suspect that we could just say "let's make it entirely
invalid to copy variably-sized things to/from the stack". Get rid of
this "follow frames" code _entirely_, and just make the rule be that a
variable copy_to/from_user had better not be on the stack. And the
static constant sizes are clearly not about overflows, so if those are
wrong, it's because somebody uses the wrong type entirely, and gcc
should be catching them statically (or we should catch them with other
tools).

Because the fs/stat.c copies really have been some of the hottest
user-copy code examples we have under certain loads. Do we really want
to have stupid code that makes them slower for no possibly valid
reason?

At some point somebody has to just say "That's just TOO STUPID TO LIVE!".

Checking those fs/stat.c copies dynamically is one such case.

              Linus

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

#1468877 — Re: [PATCH v4 54/57] x86/mm: convert arch_within_stack_frames() to use the new unwinder

On Tue, Aug 23, 2016 at 01:31:20PM -0700, Andy Lutomirski wrote:
> On Aug 23, 2016 12:11 AM, "Linus Torvalds"
> So the fact that this seems to have any significant effect on
> performance suggests to me that it's being run unnecessarily

Yeah, I think check_object_size() is being run unnecessarily in a lot of
cases.  Calling it only when size is non-const would probably speed
things up a lot.

> or that somehow we're walking all the way to the top of the stack in
> cases where we shouldn't have done so.

I know that's not happening because this code would print a warning.

> Josh, can you see an example call site in a profile of your test to
> find out what this code is doing?

I can try to figure it out tomorrow.  But really it doesn't surprise me
much that this patch makes arch_within_stack_frames() an order of
magnitude slower.  The original code was very simple, whereas
__unwind_start() and unwind_next_frame() have a lot more code.

-- 
Josh

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

#1469020 — Re: [PATCH v4 54/57] x86/mm: convert arch_within_stack_frames() to use the new unwinder

On Tue, Aug 23, 2016 at 5:08 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> On Tue, Aug 23, 2016 at 01:31:20PM -0700, Andy Lutomirski wrote:
>> On Aug 23, 2016 12:11 AM, "Linus Torvalds"
>> So the fact that this seems to have any significant effect on
>> performance suggests to me that it's being run unnecessarily
>
> Yeah, I think check_object_size() is being run unnecessarily in a lot of
> cases.  Calling it only when size is non-const would probably speed
> things up a lot.

Yup, this is at the top of my list to fix. The non-const is only done
on a handful of architectures, and it needs to be done everywhere.

-Kees

-- 
Kees Cook
Nexus Security

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web