[bug report] block: bio-integrity: directly map user buffers

[bug report] block: bio-integrity: directly map user buffers

[Dan Carpenter]

Hello Keith Busch,

The patch 492c5d455969: "block: bio-integrity: directly map user
buffers" from Nov 30, 2023 (linux-next), leads to the following
Smatch static checker warning:

	block/bio-integrity.c:350 bio_integrity_map_user()
	error: uninitialized symbol 'offset'.

block/bio-integrity.c
    340                 if (!bvec)
    341                         return -ENOMEM;
    342                 pages = NULL;
    343         }
    344 
    345         copy = !iov_iter_is_aligned(&iter, align, align);
    346         ret = iov_iter_extract_pages(&iter, &pages, bytes, nr_vecs, 0, &offset);

Smatch is concerned about the first "return 0;" if bytes or iter.count
is zero.  In that situation then offset is uninitialized.

    347         if (unlikely(ret < 0))
    348                 goto free_bvec;
    349 
--> 350         nr_bvecs = bvec_from_pages(bvec, pages, nr_vecs, bytes, offset);
                                                                        ^^^^^^

    351         if (pages != stack_pages)
    352                 kvfree(pages);
    353         if (nr_bvecs > queue_max_integrity_segments(q))
    354                 copy = true;
    355 
    356         if (copy)
    357                 ret = bio_integrity_copy_user(bio, bvec, nr_bvecs, bytes,
    358                                               direction, seed);
    359         else

regards,
dan carpenter

Re: [bug report] block: bio-integrity: directly map user buffers

[Keith Busch]

On Tue, Dec 05, 2023 at 12:35:30PM +0300, Dan Carpenter wrote:
> Hello Keith Busch,
> 
> The patch 492c5d455969: "block: bio-integrity: directly map user
> buffers" from Nov 30, 2023 (linux-next), leads to the following
> Smatch static checker warning:
> 
> 	block/bio-integrity.c:350 bio_integrity_map_user()
> 	error: uninitialized symbol 'offset'.
> 
> block/bio-integrity.c
>     340                 if (!bvec)
>     341                         return -ENOMEM;
>     342                 pages = NULL;
>     343         }
>     344 
>     345         copy = !iov_iter_is_aligned(&iter, align, align);
>     346         ret = iov_iter_extract_pages(&iter, &pages, bytes, nr_vecs, 0, &offset);
> 
> Smatch is concerned about the first "return 0;" if bytes or iter.count
> is zero.  In that situation then offset is uninitialized.
>
>     347         if (unlikely(ret < 0))
>     348                 goto free_bvec;
>     349 
> --> 350         nr_bvecs = bvec_from_pages(bvec, pages, nr_vecs, bytes, offset);
>                                                                         ^^^^^^

Thanks for the report! I don't think there's any scenario where someone
would purposefully request a 0 length metadata mapping, so I'll have it
return EINVAL for that condition.

But ... bvec_from_pages() only reads 'offset' if nr_vecs > 0. nr_vecs
would have to be 0 in this case, so it's not really accessing an
uninitialized variable. Everything in fact appears to "work" if you do
request 0 length, though again, I don't think there's a legit reason to
ever do that.

Re: [bug report] block: bio-integrity: directly map user buffers

[Dan Carpenter]

On Tue, Dec 05, 2023 at 09:29:39AM -0700, Keith Busch wrote:
> On Tue, Dec 05, 2023 at 12:35:30PM +0300, Dan Carpenter wrote:
> > Hello Keith Busch,
> > 
> > The patch 492c5d455969: "block: bio-integrity: directly map user
> > buffers" from Nov 30, 2023 (linux-next), leads to the following
> > Smatch static checker warning:
> > 
> > 	block/bio-integrity.c:350 bio_integrity_map_user()
> > 	error: uninitialized symbol 'offset'.
> > 
> > block/bio-integrity.c
> >     340                 if (!bvec)
> >     341                         return -ENOMEM;
> >     342                 pages = NULL;
> >     343         }
> >     344 
> >     345         copy = !iov_iter_is_aligned(&iter, align, align);
> >     346         ret = iov_iter_extract_pages(&iter, &pages, bytes, nr_vecs, 0, &offset);
> > 
> > Smatch is concerned about the first "return 0;" if bytes or iter.count
> > is zero.  In that situation then offset is uninitialized.
> >
> >     347         if (unlikely(ret < 0))
> >     348                 goto free_bvec;
> >     349 
> > --> 350         nr_bvecs = bvec_from_pages(bvec, pages, nr_vecs, bytes, offset);
> >                                                                         ^^^^^^
> 
> Thanks for the report! I don't think there's any scenario where someone
> would purposefully request a 0 length metadata mapping, so I'll have it
> return EINVAL for that condition.
> 
> But ... bvec_from_pages() only reads 'offset' if nr_vecs > 0. nr_vecs
> would have to be 0 in this case, so it's not really accessing an
> uninitialized variable. Everything in fact appears to "work" if you do
> request 0 length, though again, I don't think there's a legit reason to
> ever do that.

When we pass an uninitialized variable to a function but it isn't used,
then we have to look at if the function is inlined or not.  If it's not
inlined then it's still a bug, but if it is inlined, then it's okay.

Passing an uninitialized variable is undefined behavior in C.  And if
someone is running KMemsan then it will trigger a runtime warning.

In this case, I think it's likely that the function will be inlined so
it's probably fine.

regards,
dan carpenter