linux-kernel - Re: [PATCH v4 54/57] x86/mm: convert arch_within_stack

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CALCETrWM25AzbFu9G4-cTU_HRZikdvtwvybrkpJYn7byRAO+Bw@mail.gmail.com>
Date:   Tue, 23 Aug 2016 13:31:20 -0700
From:   Andy Lutomirski <luto@...nel.org>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Frederic Weisbecker <fweisbec@...il.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Kees Cook <keescook@...omium.org>,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        "H . Peter Anvin" <hpa@...or.com>,
        Nilay Vaish <nilayvaish@...il.com>,
        "the arch/x86 maintainers" <x86@...nel.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Ingo Molnar <mingo@...nel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Brian Gerst <brgerst@...il.com>,
        Byungchul Park <byungchul.park@....com>,
        Peter Zijlstra <peterz@...radead.org>
Subject: 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@...ux-foundation.org> wrote:
>
> On Thu, Aug 18, 2016 at 6:06 AM, Josh Poimboeuf <jpoimboe@...hat.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