From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757451AbcH3TUe (ORCPT ); Tue, 30 Aug 2016 15:20:34 -0400 Received: from mail-wm0-f44.google.com ([74.125.82.44]:36414 "EHLO mail-wm0-f44.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750846AbcH3TUc (ORCPT ); Tue, 30 Aug 2016 15:20:32 -0400 MIME-Version: 1.0 In-Reply-To: <20160830190941.lgpm43qn4obf2i2u@treble> References: <20160829170813.l3jwu75ltu7tpryn@treble> <20160830190941.lgpm43qn4obf2i2u@treble> From: Kees Cook Date: Tue, 30 Aug 2016 15:20:29 -0400 X-Google-Sender-Auth: FoJda2cKjW-7UDr0LtrBmcfnEqQ Message-ID: Subject: Re: [PATCH v3] mm/usercopy: get rid of CONFIG_DEBUG_STRICT_USER_COPY_CHECKS To: Josh Poimboeuf 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 , Al Viro , Mark Rutland Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Aug 30, 2016 at 3:09 PM, Josh Poimboeuf wrote: > On Tue, Aug 30, 2016 at 02:15:58PM -0400, Kees Cook wrote: >> static inline __must_check unsigned long __copy_from_user(void *to, >> const void __user *from, unsigned long n) >> { >> int dest_size = __compiletime_object_size(to); >> >> might_fault(); >> /* KASan seems to want pre-check arguments, so run it first. */ >> kasan_check_write(to, n); >> >> if (likely(dest_size != -1)) { >> /* Destination object size is known at compile time. */ >> if (n > dest_size) { >> /* Copy size is too large for destination object. */ >> if (__builtin_constant_p(n)) { >> /* Copy size is known at compile time: abort the build. */ >> copy_user_compile_time_overflow(dest_size, n); >> } else { >> /* Copy size only known at runtime, abort copy with BUG. */ >> __bad_user_copy(); >> } >> } else { >> /* Copy size within size of destination object, perform copy. */ >> n = __arch_copy_from_user(to, from, n); >> } >> } else { >> /* Destination object size needs runtime checking. */ >> check_runtime_object_size(to, from, n); >> /* If we got here, runtime checks passed, perform copy. */ >> n = __arch_copy_from_user(to, from, n); >> } >> return n; >> } >> >> static inline __must_check unsigned long copy_from_user(void *to, >> const void __user * from, unsigned long n) >> { >> if (access_ok(VERIFY_READ, from, n)) { >> n = __copy_from_user(to, from, n); >> } else >> memset(to, 0, n); /* This is needed to avoid memory >> content leaks. */ >> return n; >> } >> >> Some notes, here: the __bad_user_copy() should be a BUG, not a WARN >> since we've landed on a provably bad situation. > > Looks good to me. One nit: I think the "likely" check for "dest_size != > -1" isn't needed. dest_size is known at compile-time, so gcc should be > able to optimize it accordingly. Yeah, good point. >> check_object_size() should probably be renamed >> "check_runtime_obj_size" or something to clarify its purpose, since >> it's intended to be called only when we have to go off and examine >> runtime object metadata to figure out how to correctly perform bounds >> checking. > > Personally I find having "size" in the name to be misleading, since the > function actually looks at much more than just size. Especially > considering the fact that we already have the other static and runtime > checks which do only check the size. > > I also don't really care for "runtime", since most functions are indeed > called at runtime. If anything I'd prefer the reverse, where any > built-in compile-time "functions" are specially named or annotated. > > My vote would be something like check_usercopy_object(). Sounds good to me. :) -Kees -- Kees Cook Nexus Security