On Thu, Mar 30, 2017 at 9:43 AM, Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
> On Thu, Mar 30, 2017 at 05:22:41PM +0100, Russell King - ARM Linux wrote:
> How would the following affect things?
>
> diff --git a/lib/iov_iter.c b/lib/iov_iter.c
> index e68604ae3ced..d24d338f0682 100644
> --- a/lib/iov_iter.c
> +++ b/lib/iov_iter.c
> @@ -184,7 +184,7 @@ static size_t copy_page_to_iter_iovec(struct page *page, size_t offset, size_t b
>
> kaddr = kmap(page);
> from = kaddr + offset;
> - left = __copy_to_user(buf, from, copy);
> + left = __copy_to_user_inatomic(buf, from, copy);
This is all going in the wrong direction entirely.
That "__copy_to_user()" code was bad from the beginning: it should
never have had the double underscores. I objected to it at the time.
Now you're making it go from bad to insane. You're apparently
mis-using "inatomic" because of subtle issues that have nothing to do
with "inatomic" - you want to get rid of a might_sleep() warning, but
you don't actuially want inatomic behavior, so the thing will still
sleep.
This all very subtle already depends on people having checked the
"struct iov_iter" beforehand. We should *remove* subtle stuff like
that, not add yet more layers of subtlety and possible future bugs
when somebody calls copy_page_to_iter() without having properly
validated the iter.
These are not theoretical issues. We've _had_ these exact bugs when
people didn't validate the stuff they created by hand and bypassed the
normal IO paths.
Trying to optimize away an access_ok() or a might_fault() is *not* a
valid reason to completely break our security model, and create code
that makes no sense (claiming it is atomic when it isn't).
Linus
