Date:   Tue, 30 Aug 2016 14:15:58 -0400
From:   Kees Cook <keescook@...omium.org>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Josh Poimboeuf <jpoimboe@...hat.com>,
        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 1:02 PM, Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
> On Tue, Aug 30, 2016 at 6:04 AM, Josh Poimboeuf <jpoimboe@...hat.com> wrote:
>> There are three usercopy warnings which are currently being silenced for
>> gcc 4.6 and newer:
>
> [.. snip snip ..]
>
> Ok, I'm not entirely happy with the timing, but I think the problem
> counts as a regression since it effectively made all the checks go
> away in practice for most people, so I'm going to apply this patch.

Yeah, for pragmatism, I'm a fan of this patch since it restores the
const checks. What gets lost here are the gcc dead-code optimization
situations where gcc can figure out the value range for a non-const
size, but that's currently broken anyway, so there's no point in
keeping it. We can add it back when gcc fixes their regression.

> I know Al Viro is working on some uaccess cleanups and trying to make
> a lot of this be generic, so there's hopefully cleanups coming in the
> not too distant future (I say "hopefully", because I worry that
> looking at the mess will make Al dig his eyes out), but this seems to
> be a clear improvement.

Yeah. Mark Rutland is also looking at this too.

> I still do wish we'd move the x86 __builtin_constant_p(n) check
> around, so that x86 wouldn't do the run-time check_object_size() for
> the trivially statically correct case, but I guess that's a separate
> issue from this patch anyway.

Yeah, I'm going to wait a bit for the dust to settle here, but it's
worth documenting the situation as I'd like to see it.

First, some current API usage which we'll need to maintain at least
for now: __copy_*_user() is just copy_*_user() without the access_ok()
checks. Unfortunately, some arch implement different copying methods
depending on if the entry is via copy...() or __copy..() (e.g. see
x86's use of _copy...() -- single underscore??) There doesn't seem to
be a good reason for this, and I think it would make sense to extract
the actual per-arch implementation that performs the real copy into
something like arm64's __arch_copy_*_user(), which only does the copy
itself and nothing else.

Once that's in place, we can do sanity-checking in __copy_*_user(),
leaving the access_ok() only in copy_*_user(). The logic should be
something like:

if const destination object size is known:
    if copy size is too large:
        if copy size is const:
            abort build
        else:
            runtime BUG
    else:
        perform copy
else:
    perform runtime object size sanity checks
    perform copy

For example, totally untested, put together based on Josh's updates,
and the arm64 code, and some variable name clarity changes:

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.

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.

> If somebody has objections to this patch, holler quickly, because it's
> about to get applied. 3.. 2.. 1..

Go for it! :)

-Kees

-- 
Kees Cook
Nexus Security

Powered by blists - more mailing lists