Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] iov_iter: silent data discard for ITER_DMABUF_MAP in iterate_and_advance_kernel - [Medium] iov_iter: iov_iter_revert() fails to prevent ITER_DMABUF_MAP from reverting past start --
commit 592c6072da9af2f29c973e9a7808406da6345041 Author: Pavel Begunkov <[email protected]> iov_iter: add iterator type for dmabuf maps Introduce a new iterator type for dmabuf maps. The map in an opaque object with internals and format specific to the subsystem / driver, and only it can use that subsystem / driver for issuing IO. > diff --git a/include/linux/iov_iter.h b/include/linux/iov_iter.h > index f9a17fbbd3980..2f91884d733bf 100644 > --- a/include/linux/iov_iter.h > +++ b/include/linux/iov_iter.h > @@ -310,6 +310,8 @@ size_t iterate_and_advance2(struct iov_iter *iter, size_t > len, void *priv, > return iterate_folioq(iter, len, priv, priv2, step); > if (iov_iter_is_xarray(iter)) > return iterate_xarray(iter, len, priv, priv2, step); > + if (WARN_ON_ONCE(iov_iter_is_dmabuf_map(iter))) > + return 0; > return iterate_discard(iter, len, priv, priv2, step); > } [Severity: High] This safely rejects ITER_DMABUF_MAP by returning 0, but is there a missing guard in iterate_and_advance_kernel() which is defined later in this file? If a kernel subsystem uses iterate_and_advance_kernel() on an ITER_DMABUF_MAP, it will unconditionally fall through to iterate_discard(): if (iov_iter_is_xarray(iter)) return iterate_xarray(iter, len, priv, priv2, step); return iterate_discard(iter, len, priv, priv2, step); iterate_discard() decrements count and returns the requested len, falsely indicating that the data was successfully processed without actually calling the step function. Can this lead to silent data discard or corruption (such as hashing missing data) without any warning or error propagation? > diff --git a/lib/iov_iter.c b/lib/iov_iter.c > index 2072c04e99d05..6831a5d9396e1 100644 > --- a/lib/iov_iter.c > +++ b/lib/iov_iter.c [ ... ] > @@ -631,7 +632,8 @@ void iov_iter_revert(struct iov_iter *i, size_t unroll) > return; > } > unroll -= i->iov_offset; > - if (iov_iter_is_xarray(i) || iter_is_ubuf(i)) { > + if (iov_iter_is_xarray(i) || iter_is_ubuf(i) || > + iov_iter_is_dmabuf_map(i)) { > BUG(); /* We should never go beyond the start of the specified > * range since we might then be straying into pages that > * aren't pinned. [Severity: Medium] Does this safely prevent ITER_DMABUF_MAP from reverting past its start? iov_iter_dmabuf_map() initializes i->iov_offset to the absolute offset (off) rather than 0. Earlier in iov_iter_revert(), the boundary check compares the revert amount against this absolute offset rather than the amount advanced: if (unroll <= i->iov_offset) { i->iov_offset -= unroll; return; } Because of this early return, the revert can rewind the iterator by up to off + advanced bytes without triggering the BUG() safeguard shown in this hunk. Can this allow a caller to silently rewind the iterator to an out-of-bounds offset before the mapped range? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
