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 14:09:41 -0500
From:   Josh Poimboeuf <jpoimboe@...hat.com>
To:     Kees Cook <keescook@...omium.org>
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 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