Re: [PATCH v3] mm/usercopy: get rid of CONFIG_DEBUG_STRICT_USER_COPY_CHECKS

[Al Viro <viro@ZenIV.linux.org.uk>]

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
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.
	* 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.

	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.