On Mon, Mar 28, 2016 at 02:42:43PM +0200, Lars-Peter Clausen wrote:
> In the current implementation functionfs generates a EFAULT for async read
> operations if the read buffer size is larger than the URB data size. Since
> a application does not necessarily know how much data the host side is
> going to send it typically supplies a buffer larger than the actual data,
> which will then result in a EFAULT error.
>
> This behaviour was introduced while refactoring the code to use iov_iter
> interface in commit c993c39b8639 ("gadget/function/f_fs.c: use put iov_iter
> into io_data"). The original code took the minimum over the URB size and
> the user buffer size and then attempt to copy that many bytes using
> copy_to_user(). If copy_to_user() returned less copied bytes the code would
> generate a EFAULT error. This has exactly the same behaviour as using
> copy_to_iter() except that copy_to_iter() can't fail since the check
> whether access to the user supplied buffer is OK is already done earlier
> and copy_to_iter() wont be called in that case. Restore the original
> behaviour by simply dropping the code that generates the EFAULT error.
"The check whether access to user supplied buffer is OK" is *not* done
before. access_ok() checked does not guarantee that copy_to_user() won't
fail.
access_ok() and its equivalents checks only one thing - that you are not
trying to pass kernel addresses for userland ones. It does not check
permissions you have for individual pages, it does not check if they are
present, or, if absent, that page fault will bring them in.
Both copy_to_user() and copy_to_iter() can bloody well fail halfway through
without access_ok() having any objections. E.g. do read() into an mmapped
buffer, with one page somewhere in the middle munmapped. Or the first page
munmapped, for that matter...
I see where the commit in question is buggy, but your patch doesn't really
fix it; original behaviour is not restored.
- ret = min_t(size_t, ret, io_data->len);
-
- if (unlikely(copy_to_user(io_data->buf,
- data, ret)))
+ ret = copy_to_iter(data, ret, &io_data->data);
+ if (unlikely(iov_iter_count(&io_data->data)))
ret = -EFAULT;
was wrong; what it should've done to preserve the original behaviour is
something like
copied = copy_to_iter(data, ret, &io_data->data)
if (unlikely(copied != ret))
ret = iov_iter_count(&io_data->data)
? EFAULT : copied;
After copy_to_iter(data, len, target) we could have three outcomes:
* everything copied; return value is equal to len
* copying stopped at the end of iterator; return value is less than len,
iov_iter_count(target) is zero.
* copying stopped at unmapped page/page that was readonly/etc;
return value is less than len, iov_iter_count(target) is *not* zero.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
