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
