From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932498AbcHWQcK (ORCPT ); Tue, 23 Aug 2016 12:32:10 -0400 Received: from mx1.redhat.com ([209.132.183.28]:23381 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753295AbcHWQcJ (ORCPT ); Tue, 23 Aug 2016 12:32:09 -0400 Date: Tue, 23 Aug 2016 11:21:50 -0500 From: Josh Poimboeuf To: Kees Cook Cc: Linus Torvalds , Thomas Gleixner , Ingo Molnar , "H . Peter Anvin" , the arch/x86 maintainers , Linux Kernel Mailing List , Andy Lutomirski , Steven Rostedt , Brian Gerst , Peter Zijlstra , Frederic Weisbecker , Byungchul Park , Nilay Vaish Subject: Re: [PATCH v4 54/57] x86/mm: convert arch_within_stack_frames() to use the new unwinder Message-ID: <20160823162150.k536j5wbrxvcujp3@treble> References: <62fab36288792edae0181274641d6b4c62157fea.1471525031.git.jpoimboe@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.6.0.1 (2016-04-01) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.25]); Tue, 23 Aug 2016 16:21:52 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Aug 22, 2016 at 06:27:28PM -0700, Kees Cook wrote: > On Mon, Aug 22, 2016 at 3:11 PM, Linus Torvalds > wrote: > > On Thu, Aug 18, 2016 at 6:06 AM, Josh Poimboeuf 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