Message-ID: <20160823162150.k536j5wbrxvcujp3@treble>
Date:   Tue, 23 Aug 2016 11:21:50 -0500
From:   Josh Poimboeuf <jpoimboe@...hat.com>
To:     Kees Cook <keescook@...omium.org>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...nel.org>,
        "H . Peter Anvin" <hpa@...or.com>,
        the arch/x86 maintainers <x86@...nel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Andy Lutomirski <luto@...capital.net>,
        Steven Rostedt <rostedt@...dmis.org>,
        Brian Gerst <brgerst@...il.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Frederic Weisbecker <fweisbec@...il.com>,
        Byungchul Park <byungchul.park@....com>,
        Nilay Vaish <nilayvaish@...il.com>
Subject: 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@...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?
> >
> > 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

Powered by blists - more mailing lists