Re: [PATCH v7 02/13] iov_iter: add iterator type for dmabuf maps

[sashiko-bot@kernel.org]

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 <asml.silence@gmail.com>
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/cover.1790602223.git.asml.silence@gmail.com?part=2