From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754490AbcH3TJr (ORCPT ); Tue, 30 Aug 2016 15:09:47 -0400 Received: from mx1.redhat.com ([209.132.183.28]:54674 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751148AbcH3TJp (ORCPT ); Tue, 30 Aug 2016 15:09:45 -0400 Date: Tue, 30 Aug 2016 14:09:41 -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 , Al Viro , Mark Rutland Subject: Re: [PATCH v3] mm/usercopy: get rid of CONFIG_DEBUG_STRICT_USER_COPY_CHECKS Message-ID: <20160830190941.lgpm43qn4obf2i2u@treble> References: <20160829170813.l3jwu75ltu7tpryn@treble> 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.38]); Tue, 30 Aug 2016 19:09:45 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. > 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(). -- Josh