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

["Ömer Mete Kaya" <omermetekaya0@gmail.com>]

On 10/1/26 18:36, Gabriel Krisman Bertazi wrote:
> Ö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.

Yes I checked more and looks like the *bug* is unreachable via existing
in-tree callers.>>
>> Fixes: 1045afae4b88 ("io_uring: support vectored kernel fixed buffer")

It doesnt fix something broken actually, more like a defensive refactoring.

>> 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?

mp_bvec_iter_bvec() is called in the for-loop condition not the body:

    for (iter = (start);
        (iter).bi_size &&
            ((bvl = mp_bvec_iter_bvec((bio_vec), (iter))), 1); <-***
        bvec_iter_advance_single(...))

src_bvec[bi_idx] is dereferenced before the loop body is entered.
A check inside the body executes after the out-of-bounds read has
already occurred. The open-coded loop is the way to check bi_idx
before the dereference. The question is "should the kernel be defensive
itself or trust the callers?". If you find these kinds of defensive
controls unnecessary, happy to withdraw the patch instead of releasing v2.

kind regards,

Ömer