Re: [PATCH v10 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: iterate_and_advance_kernel() silently discards ITER_DMABUF_MAP data
- [High] iov_iter: dmabuf absolute offset truncation and revert bounds bypass
--

commit 190aa53efd62f0f61a9121fb6b0b02899d01272d
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. The task of the
middle layers is to pass the map / iterator further down, maybe doing
basic splitting and length checking.

> 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 patch explicitly rejects ITER_DMABUF_MAP in iterate_and_advance2(), but
does iterate_and_advance_kernel() also need this check?

If an ITER_DMABUF_MAP iterator is passed to iterate_and_advance_kernel(), it
looks like execution will fall through directly to iterate_discard(), which
silently subtracts the requested length from iter->count and returns the
length.

Could this cause internal kernel components to silently bypass data processing
instead of failing loudly?

[ ... ]
> 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: High]
Does setting iov_offset to the absolute file offset in iov_iter_dmabuf_map()
break the bounds check in iov_iter_revert() prior to this BUG() guard?

In iov_iter_revert(), the check is:

	if (unroll <= i->iov_offset) {
		i->iov_offset -= unroll;
		return;
	}

If iov_offset contains an absolute offset rather than relative progress (0),
does this evaluate the relative unroll amount against the absolute offset,
allowing the iterator to rewind completely out of bounds without hitting
the BUG() safety check?

[ ... ]
> @@ -775,6 +777,20 @@ void iov_iter_xarray(struct iov_iter *i, unsigned int direction,
>  }
>  EXPORT_SYMBOL(iov_iter_xarray);
>  
> +void iov_iter_dmabuf_map(struct iov_iter *i, unsigned int direction,
> +			 struct dma_buf_io_map *map,
> +			 loff_t off, size_t count)
> +{
> +	WARN_ON(direction & ~(READ | WRITE));
> +	*i = (struct iov_iter){
> +		.iter_type = ITER_DMABUF_MAP,
> +		.data_source = direction,
> +		.dmabuf_map = map,
> +		.count = count,
> +		.iov_offset = off,
> +	};
> +}

[Severity: High]
Since iov_offset is defined as a size_t, does assigning the loff_t off
truncate the offset on 32-bit architectures?

Additionally, when this iterator is mapped into a bio in subsequent patches,
iter->iov_offset is copied into bio->bi_iter.bi_offset (an unsigned int).

Does this cause truncation on 64-bit architectures as well?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1791924581.git.asml.silence@gmail.com?part=2