Re: [PATCH v3] mm/usercopy: get rid of CONFIG_DEBUG_STRICT_USER_COPY_CHECKS

From	Josh Poimboeuf <jpoimboe@redhat.com>
Newsgroups	linux.kernel
Subject	Re: [PATCH v3] mm/usercopy: get rid of CONFIG_DEBUG_STRICT_USER_COPY_CHECKS
Date	2016-08-30 21:10 +0200
Message-ID	<sbWA2-tT-31@gated-at.bofh.it> (permalink)
References	<sbyel-1BC-11@gated-at.bofh.it> <sbQXD-5kV-23@gated-at.bofh.it> <sbUHT-7Jg-1@gated-at.bofh.it> <sbVND-8n7-9@gated-at.bofh.it>
Organization	linux.* mail to news gateway

Show all headers | View raw

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

Thread

Re: [PATCH 2/2] mm/usercopy: enable usercopy size checking for  modern versions of gcc Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-25 23:00 +0200
  Re: [PATCH 2/2] mm/usercopy: enable usercopy size checking for modern  versions of gcc Kees Cook <keescook@chromium.org> - 2016-08-26 04:30 +0200
    Re: [PATCH 2/2] mm/usercopy: enable usercopy size checking for  modern versions of gcc Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-26 06:00 +0200
      Re: [PATCH 2/2] mm/usercopy: enable usercopy size checking for modern  versions of gcc Kees Cook <keescook@chromium.org> - 2016-08-26 15:50 +0200
        Re: [PATCH 2/2] mm/usercopy: enable usercopy size checking for  modern versions of gcc Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-26 16:00 +0200
          Re: [PATCH 2/2] mm/usercopy: enable usercopy size checking for  modern versions of gcc Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-26 23:10 +0200
            Re: [PATCH 2/2] mm/usercopy: enable usercopy size checking for  modern versions of gcc Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-26 23:10 +0200
            Re: [PATCH 2/2] mm/usercopy: enable usercopy size checking for modern  versions of gcc Linus Torvalds <torvalds@linux-foundation.org> - 2016-08-27 02:40 +0200
              Re: [PATCH 2/2] mm/usercopy: enable usercopy size checking for  modern versions of gcc Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-29 16:50 +0200
                Re: [PATCH 2/2] mm/usercopy: enable usercopy size checking for modern  versions of gcc Linus Torvalds <torvalds@linux-foundation.org> - 2016-08-29 17:40 +0200
                [PATCH v2] mm/usercopy: get rid of  CONFIG_DEBUG_STRICT_USER_COPY_CHECKS Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-29 19:10 +0200
                Re: [PATCH v2] mm/usercopy: get rid of  CONFIG_DEBUG_STRICT_USER_COPY_CHECKS Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-29 20:10 +0200
                [PATCH v3] mm/usercopy: get rid of CONFIG_DEBUG_STRICT_USER_COPY_CHECKS Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-30 15:10 +0200
                Re: [PATCH v3] mm/usercopy: get rid of CONFIG_DEBUG_STRICT_USER_COPY_CHECKS Linus Torvalds <torvalds@linux-foundation.org> - 2016-08-30 19:10 +0200
                Re: [PATCH v3] mm/usercopy: get rid of CONFIG_DEBUG_STRICT_USER_COPY_CHECKS Kees Cook <keescook@chromium.org> - 2016-08-30 20:20 +0200
                Re: [PATCH v3] mm/usercopy: get rid of  CONFIG_DEBUG_STRICT_USER_COPY_CHECKS Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-30 21:10 +0200
                Re: [PATCH v3] mm/usercopy: get rid of CONFIG_DEBUG_STRICT_USER_COPY_CHECKS Kees Cook <keescook@chromium.org> - 2016-08-30 21:30 +0200
                Re: [PATCH v3] mm/usercopy: get rid of  CONFIG_DEBUG_STRICT_USER_COPY_CHECKS Al Viro <viro@ZenIV.linux.org.uk> - 2016-08-30 22:20 +0200
                Re: [PATCH v3] mm/usercopy: get rid of CONFIG_DEBUG_STRICT_USER_COPY_CHECKS Kees Cook <keescook@chromium.org> - 2016-08-31 00:30 +0200
                Re: [PATCH v3] mm/usercopy: get rid of  CONFIG_DEBUG_STRICT_USER_COPY_CHECKS Mark Rutland <mark.rutland@arm.com> - 2016-08-31 11:50 +0200
                Re: [PATCH v3] mm/usercopy: get rid of CONFIG_DEBUG_STRICT_USER_COPY_CHECKS Linus Torvalds <torvalds@linux-foundation.org> - 2016-08-30 20:20 +0200
                Re: [PATCH v3] mm/usercopy: get rid of  CONFIG_DEBUG_STRICT_USER_COPY_CHECKS Al Viro <viro@ZenIV.linux.org.uk> - 2016-08-30 20:20 +0200
                Re: [PATCH 2/2] mm/usercopy: enable usercopy size checking for modern  versions of gcc Kees Cook <keescook@chromium.org> - 2016-08-30 20:40 +0200

csiph-web