lkml.org 
[lkml]   [2016]   [Aug]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v3] mm/usercopy: get rid of CONFIG_DEBUG_STRICT_USER_COPY_CHECKS
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

\
 
 \ /
  Last update: 2016-09-17 09:58    [W:0.250 / U:0.236 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site