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

[Josh Poimboeuf]

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@xxxxxxxxxxxxxxxxxxxx> wrote:
> > On Thu, Aug 18, 2016 at 6:06 AM, Josh Poimboeuf <jpoimboe@xxxxxxxxxx> 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