linux-kernel - Re: [PATCH v3] mm/usercopy: get rid of CONFIG_DEBUG_STRICT_USER_COPY

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 30 Aug 2016 15:20:29 -0400
From:   Kees Cook <keescook@...omium.org>
To:     Josh Poimboeuf <jpoimboe@...hat.com>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...nel.org>,
        "H . Peter Anvin" <hpa@...or.com>,
        "the arch/x86 maintainers" <x86@...nel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Andy Lutomirski <luto@...capital.net>,
        Steven Rostedt <rostedt@...dmis.org>,
        Brian Gerst <brgerst@...il.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Frederic Weisbecker <fweisbec@...il.com>,
        Byungchul Park <byungchul.park@....com>,
        Nilay Vaish <nilayvaish@...il.com>,
        Al Viro <viro@...iv.linux.org.uk>,
        Mark Rutland <mark.rutland@....com>
Subject: Re: [PATCH v3] mm/usercopy: get rid of CONFIG_DEBUG_STRICT_USER_COPY_CHECKS

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