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

Groups > linux.kernel > #1468223

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

Show all headers | View raw

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

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[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

csiph-web