Re: [PATCH] io_uring: fix out-of-bounds bvec access in io_vec_fill_kern_bvec

[Gabriel Krisman Bertazi <gabriel@krisman.be>]

Ömer Mete Kaya <omermetekaya0@gmail.com> writes:

> for_each_mp_bvec() dereferences src_bvec[bi_idx] in the loop condition
> before the body is entered, with no guard against bi_idx reaching
> imu->nr_bvecs. iov_kern_bvec_size() stops iterating when i reaches
> imu->nr_bvecs even if bi_size is still non-zero, so the fill loop can
> walk past the end of the bvec array and overrun res_bvec[].
>
> Open-code the loop with an explicit bi_idx < imu->nr_bvecs check before
> the dereference, matching the termination condition in
> iov_kern_bvec_size().

Do you have a reproducer?  This should be checked in iov_kern_bvec_size.
We make sure it doesn't go through imu->len which should match bv_len,
IIUC.

>
> Fixes: 1045afae4b88 ("io_uring: support vectored kernel fixed buffer")
> Signed-off-by: Ömer Mete Kaya <omermetekaya0@gmail.com>
> ---
>  io_uring/rsrc.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/io_uring/rsrc.c b/io_uring/rsrc.c
> index 51b46e624..3d29a0c7b 100644
> --- a/io_uring/rsrc.c
> +++ b/io_uring/rsrc.c
> @@ -1614,8 +1614,13 @@ static int io_vec_fill_kern_bvec(int ddir, struct iov_iter *iter,
>  		struct bio_vec bv;
>
>  		bvec_iter_advance(src_bvec, &bi, offset);
> -		for_each_mp_bvec(bv, src_bvec, bi, bi)
> +		while (bi.bi_size) {

bi.bi_size is the first condition of for_each_mp_bvec.  Do you really
need an open coded loop?  If there is an issue, can we just add the
check below?

> +			if (bi.bi_idx >= imu->nr_bvecs)
> +				return -EFAULT;

> +			bv = mp_bvec_iter_bvec(src_bvec, bi);
>  			res_bvec[res_idx++] = bv;
> +			bvec_iter_advance_single(src_bvec, &bi, bv.bv_len);
> +		}
>  		total_len += iov_len;
>  	}
>  	iov_iter_bvec(iter, ddir, res_bvec, res_idx, total_len);
> --
> 2.55.0
>

-- 
Gabriel Krisman Bertazi