On 12/4/23 10:02 AM, Arnaud POULIQUEN wrote:
Hi,
On 12/4/23 17:40, Jens Axboe wrote:
On 12/4/23 9:36 AM, Jens Axboe wrote:
On 12/4/23 5:42 AM, Sumit Garg wrote:
IMO, access_ok() should be the first thing that import_ubuf() or import_single_range() should do, something as follows:
diff --git a/lib/iov_iter.c b/lib/iov_iter.c index 8ff6824a1005..4aee0371824c 100644 --- a/lib/iov_iter.c +++ b/lib/iov_iter.c @@ -1384,10 +1384,10 @@ EXPORT_SYMBOL(import_single_range);
int import_ubuf(int rw, void __user *buf, size_t len, struct iov_iter *i) {
if (len > MAX_RW_COUNT)len = MAX_RW_COUNT; if (unlikely(!access_ok(buf, len))) return -EFAULT;
if (len > MAX_RW_COUNT)len = MAX_RW_COUNT; iov_iter_ubuf(i, rw, buf, len); return 0;Jens A., Al Viro,
Was there any particular reason which I am unaware of to perform access_ok() check on modified input length?
This change makes sense to me, and seems consistent with what is done elsewhere too.
For some reason I missed import_single_range(), which does it the same way as import_ubuf() currently does - cap the range before the access_ok() check. The vec variants sum as they go, but access_ok() before the range.
I think part of the issue here is that the single range imports return 0 for success and -ERROR otherwise. This means that the caller does not know if the full range was imported or not. OTOH, we always cap any data transfer at MAX_RW_COUNT, so may make more sense to fix up the caller here.
Should we limit to MAX_RW_COUNT or return an error? Seems to me that limiting could generate side effect later that could be not simple to debug.
We've traditionally just truncated the length, so principle of least surprise says we should continue doing that.