On 03/28/2016 04:28 PM, Al Viro wrote:
> 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.
Hm, right. I guess I got confused on the copy_to_iter() case since it
can't fail when copying into kernel buffers. It would have be nice if
the semantics of all those iov iter functions had been documented.
> E.g. do read() into an mmapped buffer, with one page somewhere in the
> middle munmapped. Or the first page munmapped, for that matter...
Yeah, I actually wondered how that could possibly work...
>
> 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.
Yes. It's a bit of a shame that the copy_{to,from}_iter() functions
don't allow you to easily distinguish between target buffer is to small
and a fault occurred mid way through. This can't be the only place which
wants to know this.
--
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
