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

[Kees Cook <keescook@chromium.org>]

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