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
            Re: [PATCH 2/2] mm/usercopy: enable usercopy size checking for  modern versions of gcc Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-25 23:00 +0200
              Re: [PATCH 2/2] mm/usercopy: enable usercopy size checking for modern  versions of gcc Kees Cook <keescook@chromium.org> - 2016-08-26 04:30 +0200
                Re: [PATCH 2/2] mm/usercopy: enable usercopy size checking for  modern versions of gcc Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-26 06:00 +0200
                  Re: [PATCH 2/2] mm/usercopy: enable usercopy size checking for modern  versions of gcc Kees Cook <keescook@chromium.org> - 2016-08-26 15:50 +0200
                    Re: [PATCH 2/2] mm/usercopy: enable usercopy size checking for  modern versions of gcc Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-26 16:00 +0200
                      Re: [PATCH 2/2] mm/usercopy: enable usercopy size checking for  modern versions of gcc Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-26 23:10 +0200
                        Re: [PATCH 2/2] mm/usercopy: enable usercopy size checking for  modern versions of gcc Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-26 23:10 +0200
                        Re: [PATCH 2/2] mm/usercopy: enable usercopy size checking for modern  versions of gcc Linus Torvalds <torvalds@linux-foundation.org> - 2016-08-27 02:40 +0200
                          Re: [PATCH 2/2] mm/usercopy: enable usercopy size checking for  modern versions of gcc Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-29 16:50 +0200
                            Re: [PATCH 2/2] mm/usercopy: enable usercopy size checking for modern  versions of gcc Linus Torvalds <torvalds@linux-foundation.org> - 2016-08-29 17:40 +0200
                              [PATCH v2] mm/usercopy: get rid of  CONFIG_DEBUG_STRICT_USER_COPY_CHECKS Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-29 19:10 +0200
                                Re: [PATCH v2] mm/usercopy: get rid of  CONFIG_DEBUG_STRICT_USER_COPY_CHECKS Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-29 20:10 +0200
                                [PATCH v3] mm/usercopy: get rid of CONFIG_DEBUG_STRICT_USER_COPY_CHECKS Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-30 15:10 +0200
                                  Re: [PATCH v3] mm/usercopy: get rid of CONFIG_DEBUG_STRICT_USER_COPY_CHECKS Linus Torvalds <torvalds@linux-foundation.org> - 2016-08-30 19:10 +0200
                                    Re: [PATCH v3] mm/usercopy: get rid of CONFIG_DEBUG_STRICT_USER_COPY_CHECKS Kees Cook <keescook@chromium.org> - 2016-08-30 20:20 +0200
                                      Re: [PATCH v3] mm/usercopy: get rid of  CONFIG_DEBUG_STRICT_USER_COPY_CHECKS Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-30 21:10 +0200
                                        Re: [PATCH v3] mm/usercopy: get rid of CONFIG_DEBUG_STRICT_USER_COPY_CHECKS Kees Cook <keescook@chromium.org> - 2016-08-30 21:30 +0200
                                      Re: [PATCH v3] mm/usercopy: get rid of  CONFIG_DEBUG_STRICT_USER_COPY_CHECKS Al Viro <viro@ZenIV.linux.org.uk> - 2016-08-30 22:20 +0200
                                        Re: [PATCH v3] mm/usercopy: get rid of CONFIG_DEBUG_STRICT_USER_COPY_CHECKS Kees Cook <keescook@chromium.org> - 2016-08-31 00:30 +0200
                                        Re: [PATCH v3] mm/usercopy: get rid of  CONFIG_DEBUG_STRICT_USER_COPY_CHECKS Mark Rutland <mark.rutland@arm.com> - 2016-08-31 11:50 +0200
                                    Re: [PATCH v3] mm/usercopy: get rid of CONFIG_DEBUG_STRICT_USER_COPY_CHECKS Linus Torvalds <torvalds@linux-foundation.org> - 2016-08-30 20:20 +0200
                                    Re: [PATCH v3] mm/usercopy: get rid of  CONFIG_DEBUG_STRICT_USER_COPY_CHECKS Al Viro <viro@ZenIV.linux.org.uk> - 2016-08-30 20:20 +0200
                            Re: [PATCH 2/2] mm/usercopy: enable usercopy size checking for modern  versions of gcc Kees Cook <keescook@chromium.org> - 2016-08-30 20:40 +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

Page 1 of 3  [1] 2 3  Next page →

#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]

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

On Tue, Aug 23, 2016 at 10:37:43PM -0400, Kees Cook wrote:
> 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?

What specifically is buggy with __builtin_object_size()?  Looking at the
generated code for a few of the "provably correct" warning sites, the
values generated by __builtin_object_size() are correct.

I think the problem is really related to the compile-time warning
function attribute used by __copy_to_user_overflow().  The warning is
printed when gcc *can* determine the object size but it *can't*
determine the copy size.  The warning just means that, even though the
object has a const size, gcc isn't able to prove that the overflow won't
happen.

As an example, here's one of the warnings:

  In file included from /home/jpoimboe/git/linux/include/linux/uaccess.h:5:0,
                   from /home/jpoimboe/git/linux/arch/x86/include/asm/stacktrace.h:9,
                   from /home/jpoimboe/git/linux/arch/x86/include/asm/perf_event.h:246,
                   from /home/jpoimboe/git/linux/include/linux/perf_event.h:24,
                   from /home/jpoimboe/git/linux/kernel/sys.c:16:
  In function ‘copy_to_user.part.10’,
      inlined from ‘copy_to_user’,
      inlined from ‘override_release.part.11’ at /home/jpoimboe/git/linux/kernel/sys.c:1136:9:
  /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
   #define __copy_to_user_overflow(size, count) __copy_to_user_overflow()
                                                ^~~~~~~~~~~~~~~~~~~~~~~~~
  /home/jpoimboe/git/linux/arch/x86/include/asm/uaccess.h:791:3: note: in expansion of macro ‘__copy_to_user_overflow’
     __copy_to_user_overflow(sz, n);
     ^~~~~~~~~~~~~~~~~~~~~~~

This is from override_release()'s use of copy_to_user().  The object
code shows that __builtin_object_size() correctly reports 65 bytes for
the 'buf' object size.  But the copy size ('copy + 1') isn't known at
compile-time.  Thus the (bogus) warning.

Maybe I'm missing something but I don't even see a gcc bug.  To me it
looks like a mismatch in expectations between the code and the compiler.

-- 
Josh

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

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

On Thu, Aug 25, 2016 at 4:47 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> On Tue, Aug 23, 2016 at 10:37:43PM -0400, Kees Cook wrote:
>> 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?
>
> What specifically is buggy with __builtin_object_size()?  Looking at the
> generated code for a few of the "provably correct" warning sites, the
> values generated by __builtin_object_size() are correct.
>
> I think the problem is really related to the compile-time warning
> function attribute used by __copy_to_user_overflow().  The warning is
> printed when gcc *can* determine the object size but it *can't*
> determine the copy size.  The warning just means that, even though the
> object has a const size, gcc isn't able to prove that the overflow won't
> happen.
>
> As an example, here's one of the warnings:
>
>   In file included from /home/jpoimboe/git/linux/include/linux/uaccess.h:5:0,
>                    from /home/jpoimboe/git/linux/arch/x86/include/asm/stacktrace.h:9,
>                    from /home/jpoimboe/git/linux/arch/x86/include/asm/perf_event.h:246,
>                    from /home/jpoimboe/git/linux/include/linux/perf_event.h:24,
>                    from /home/jpoimboe/git/linux/kernel/sys.c:16:
>   In function ‘copy_to_user.part.10’,
>       inlined from ‘copy_to_user’,
>       inlined from ‘override_release.part.11’ at /home/jpoimboe/git/linux/kernel/sys.c:1136:9:
>   /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
>    #define __copy_to_user_overflow(size, count) __copy_to_user_overflow()
>                                                 ^~~~~~~~~~~~~~~~~~~~~~~~~
>   /home/jpoimboe/git/linux/arch/x86/include/asm/uaccess.h:791:3: note: in expansion of macro ‘__copy_to_user_overflow’
>      __copy_to_user_overflow(sz, n);
>      ^~~~~~~~~~~~~~~~~~~~~~~
>
> This is from override_release()'s use of copy_to_user().  The object
> code shows that __builtin_object_size() correctly reports 65 bytes for
> the 'buf' object size.  But the copy size ('copy + 1') isn't known at
> compile-time.  Thus the (bogus) warning.
>
> Maybe I'm missing something but I don't even see a gcc bug.  To me it
> looks like a mismatch in expectations between the code and the compiler.

Ah, yes, I had a total brain failure. This is what I get trying to do
email between sessions at a conference. :)

Okay, right. __builtin_object_size() is totally fine, I absolutely
misspoke: it's the resolution of const value ranges. I wouldn't expect
gcc to warn here, though, since "copy + 1" isn't a const value...

-Kees

-- 
Kees Cook
Nexus Security

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

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

On Thu, Aug 25, 2016 at 10:14:36PM -0400, Kees Cook wrote:
> On Thu, Aug 25, 2016 at 4:47 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > On Tue, Aug 23, 2016 at 10:37:43PM -0400, Kees Cook wrote:
> >> 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?
> >
> > What specifically is buggy with __builtin_object_size()?  Looking at the
> > generated code for a few of the "provably correct" warning sites, the
> > values generated by __builtin_object_size() are correct.
> >
> > I think the problem is really related to the compile-time warning
> > function attribute used by __copy_to_user_overflow().  The warning is
> > printed when gcc *can* determine the object size but it *can't*
> > determine the copy size.  The warning just means that, even though the
> > object has a const size, gcc isn't able to prove that the overflow won't
> > happen.
> >
> > As an example, here's one of the warnings:
> >
> >   In file included from /home/jpoimboe/git/linux/include/linux/uaccess.h:5:0,
> >                    from /home/jpoimboe/git/linux/arch/x86/include/asm/stacktrace.h:9,
> >                    from /home/jpoimboe/git/linux/arch/x86/include/asm/perf_event.h:246,
> >                    from /home/jpoimboe/git/linux/include/linux/perf_event.h:24,
> >                    from /home/jpoimboe/git/linux/kernel/sys.c:16:
> >   In function ‘copy_to_user.part.10’,
> >       inlined from ‘copy_to_user’,
> >       inlined from ‘override_release.part.11’ at /home/jpoimboe/git/linux/kernel/sys.c:1136:9:
> >   /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
> >    #define __copy_to_user_overflow(size, count) __copy_to_user_overflow()
> >                                                 ^~~~~~~~~~~~~~~~~~~~~~~~~
> >   /home/jpoimboe/git/linux/arch/x86/include/asm/uaccess.h:791:3: note: in expansion of macro ‘__copy_to_user_overflow’
> >      __copy_to_user_overflow(sz, n);
> >      ^~~~~~~~~~~~~~~~~~~~~~~
> >
> > This is from override_release()'s use of copy_to_user().  The object
> > code shows that __builtin_object_size() correctly reports 65 bytes for
> > the 'buf' object size.  But the copy size ('copy + 1') isn't known at
> > compile-time.  Thus the (bogus) warning.
> >
> > Maybe I'm missing something but I don't even see a gcc bug.  To me it
> > looks like a mismatch in expectations between the code and the compiler.
> 
> Ah, yes, I had a total brain failure. This is what I get trying to do
> email between sessions at a conference. :)
> 
> Okay, right. __builtin_object_size() is totally fine, I absolutely
> misspoke: it's the resolution of const value ranges. I wouldn't expect
> gcc to warn here, though, since "copy + 1" isn't a const value...

Look at the code again :-)

__copy_to_user_overflow(), which does the "provably correct" warning, is
"called" when the copy size is non-const (and the object size is const).
So "copy + 1" being non-const is consistent with the warning.

-- 
Josh

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

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

On Thu, Aug 25, 2016 at 11:27 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> On Thu, Aug 25, 2016 at 10:14:36PM -0400, Kees Cook wrote:
>> Okay, right. __builtin_object_size() is totally fine, I absolutely
>> misspoke: it's the resolution of const value ranges. I wouldn't expect
>> gcc to warn here, though, since "copy + 1" isn't a const value...
>
> Look at the code again :-)
>
> __copy_to_user_overflow(), which does the "provably correct" warning, is
> "called" when the copy size is non-const (and the object size is const).
> So "copy + 1" being non-const is consistent with the warning.

Right, yes. Man, this is hard to read. All the names are the same. ;)

So this will trigger when the object size is known but the copy length
is non-const?

When I played with re-enabling this in the past, I didn't hit very
many false positives. I sent a bunch of patches a few months back for
legitimate problems that this warning pointed out, so I'm a bit
cautious to just entirely drop it.

-Kees

-- 
Kees Cook
Nexus Security

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

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

On Fri, Aug 26, 2016 at 09:42:42AM -0400, Kees Cook wrote:
> On Thu, Aug 25, 2016 at 11:27 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > On Thu, Aug 25, 2016 at 10:14:36PM -0400, Kees Cook wrote:
> >> Okay, right. __builtin_object_size() is totally fine, I absolutely
> >> misspoke: it's the resolution of const value ranges. I wouldn't expect
> >> gcc to warn here, though, since "copy + 1" isn't a const value...
> >
> > Look at the code again :-)
> >
> > __copy_to_user_overflow(), which does the "provably correct" warning, is
> > "called" when the copy size is non-const (and the object size is const).
> > So "copy + 1" being non-const is consistent with the warning.
> 
> Right, yes. Man, this is hard to read. All the names are the same. ;)

Yeah, agreed.  The code is way too cryptic.

> So this will trigger when the object size is known but the copy length
> is non-const?

Right.

> When I played with re-enabling this in the past, I didn't hit very
> many false positives. I sent a bunch of patches a few months back for
> legitimate problems that this warning pointed out, so I'm a bit
> cautious to just entirely drop it.

Ah, I didn't realize that.  We should definitely keep
DEBUG_STRICT_USER_COPY_CHECKS then.  Though it would be *really* nice to
find a way to associate some kind of whitelist with it to separate the
wheat from all the chaff.

-- 
Josh

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

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

On Fri, Aug 26, 2016 at 08:55:33AM -0500, Josh Poimboeuf wrote:
> On Fri, Aug 26, 2016 at 09:42:42AM -0400, Kees Cook wrote:
> > On Thu, Aug 25, 2016 at 11:27 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > > On Thu, Aug 25, 2016 at 10:14:36PM -0400, Kees Cook wrote:
> > >> Okay, right. __builtin_object_size() is totally fine, I absolutely
> > >> misspoke: it's the resolution of const value ranges. I wouldn't expect
> > >> gcc to warn here, though, since "copy + 1" isn't a const value...
> > >
> > > Look at the code again :-)
> > >
> > > __copy_to_user_overflow(), which does the "provably correct" warning, is
> > > "called" when the copy size is non-const (and the object size is const).
> > > So "copy + 1" being non-const is consistent with the warning.
> > 
> > Right, yes. Man, this is hard to read. All the names are the same. ;)
> 
> Yeah, agreed.  The code is way too cryptic.
> 
> > So this will trigger when the object size is known but the copy length
> > is non-const?
> 
> Right.
> 
> > When I played with re-enabling this in the past, I didn't hit very
> > many false positives. I sent a bunch of patches a few months back for
> > legitimate problems that this warning pointed out, so I'm a bit
> > cautious to just entirely drop it.
> 
> Ah, I didn't realize that.  We should definitely keep
> DEBUG_STRICT_USER_COPY_CHECKS then.  Though it would be *really* nice to
> find a way to associate some kind of whitelist with it to separate the
> wheat from all the chaff.

Ok, so I could drop patch 1/2 and then resubmit 2/2 with an updated
patch header.

There's one problem with that though.  It's going to annoy a lot of
people who do allyesconfig/allmodconfig builds because
DEBUG_STRICT_USER_COPY_CHECKS adds several fake warnings.

Anybody know if there's a way to disable an option for
allyesconfig/allmodconfig?

-- 
Josh

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

Page 1 of 3  [1] 2 3  Next page →

Back to top | Article view | linux.kernel

csiph-web