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

From	Kees Cook <keescook@chromium.org>
Newsgroups	linux.kernel
Subject	Re: [PATCH v3] mm/usercopy: get rid of CONFIG_DEBUG_STRICT_USER_COPY_CHECKS
Date	2016-08-30 21:30 +0200
Message-ID	<sbWTn-Dd-9@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> <sbWA2-tT-31@gated-at.bofh.it>
Organization	linux.* mail to news gateway

Show all headers | View raw

On Tue, Aug 30, 2016 at 3:09 PM, Josh Poimboeuf <jpoimboe@redhat.com> 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

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