Message-ID: <CAGXu5j+ecWKW4ZvY9BFsxj+vp1q5jyiGo72xY1jtpC7a0nPh2w@mail.gmail.com>
Date:   Tue, 30 Aug 2016 18:20:47 -0400
From:   Kees Cook <keescook@...omium.org>
To:     Al Viro <viro@...iv.linux.org.uk>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        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>,
        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 4:13 PM, Al Viro <viro@...iv.linux.org.uk> wrote:
> On Tue, Aug 30, 2016 at 02:15:58PM -0400, Kees Cook wrote:
>
>> 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.
>
> No.  __arch_copy_from_user() is a bloody bad idea; the real primitive
> is what's currently called __copy_from_user_inatomic(), and I'm planning
> to rename it to raw_copy_from_user().  Note that _this_ should not

I don't think the name is important, just as long as it's clear. We
both seem to agree: the arch-specific stuff should be separate from
the common API that has the sanity checking, etc, which it sounds like
you're already doing.

-Kees

> zero anything on fault; "inatomic" part is a misnomer.  I'm not sure
> if __copy_from_user() will survive long-term, actually; copy_from_user()
> should (size checks aside) be equivalent to
>         size_t res = size;
>         might_fault();
>         if (likely(access_ok(...)))
>                 res = __copy_from_user_inatomic(...);
>         if (unlikely(res))
>                 memset(to + (size - res), 0, res);
>         return res;
>
> Linus asked to take that to lib/* - at least the memset() part.
>
>         * get_user()/put_user()/clear_user()/copy_{from,to,in}_user() should
> check access_ok() (if non-degenerate on the architecture in question).
>         * failing get_user(x, p)/__get_user(x, p) should zero x
>         * short copy (for any reason, including access_ok() failure) in
> copy_from_user() should return the amount of bytes *not* copied and zero them.
> In no circumstances should it return -E...
>         * __copy_from_user_inatomic(to, from, size) should return exactly
> size - amount of bytes stored.  It does *not* need to copy as much as possible
> in case of fault.  It should not zero anything; as the matter of fact, zeroing
> does not belong in assembler part at all.
>         * iov_iter_copy_from_user_atomic(), copy_page_from_iter()
> and copy_page_from_iter() will not modify anything past the amount they
> return.  In particular, they will not zero anything at all.  Right now it's
> arch-dependent.
>         * iov_iter_fault_in_readable() will merge with
> iov_iter_fault_in_multipages_readable(), with the semantics of the latter.
> As the matter of fact, the same ought to happen to fault_in_pages_readable()
> and fault_in_multipages_readable().
>         * ->write_end() instances on short copy into uptodate page should
> not zero anything whatsoever; when page is not uptodate, they should only
> zero an area if readpage should've done the same (e.g. if it's something like
> ramfs, or if we know that we'd allocated new on-disk blocks and hadn't
> copied them over, etc.  Returning 0 and leaving a page !uptodate is always
> OK on a short copy; we might do something more intelligent, but that's
> up to specific ->write_end() instance.

Agreed on all these; and getting that documented in the final
uaccess.h seems like a very good idea too.

>         * includes of asm/uaccess.h are going away.  That's obviously not
> something we can afford as a prereq for fixes to be backported, but for
> the next window we definitely want a one-time tree-wide switch to
> linux/uaccess.h.  For *.c (and local .h) it's trivial, for general-purpose
> headers it'll take some massage.  Once we have linux/uaccess.h use, we
> can move duplicates over there.

How do you envision architectures gluing their copy implementation to
raw_copy_from_user()?

>         The above obviously doesn't go into exception/longjmp/asm-goto/etc.
> pile of joy; that needs more experiments and frankly, I want to finish
> separating the -stable fodder first.

Yup, cool.

-Kees

-- 
Kees Cook
Nexus Security

Powered by blists - more mailing lists