On Tue, Aug 30, 2016 at 4:13 PM, Al Viro <v...@zeniv.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
