[PATCH v2] block: Fix start and length check added to iov_iter_extract_bvecs()

David Howells posted 1 patch 1 month ago
There is a newer version of this series
lib/iov_iter.c |   11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
[PATCH v2] block: Fix start and length check added to iov_iter_extract_bvecs()
Posted by David Howells 1 month ago
Commit 14b007e17881 added an address check using iter_iov_addr() and a
length check using iter_iov_len() to iov_iter_extract_bvecs(), but these
cannot be used so and are unsafe in this circumstance as the functions have
hardwired assumptions about the iterator type.  They should only be used
with ITER_UBUF or ITER_IOVEC-type iterators and work with ITER_KVEC; they
should not be used with ITER_BVEC, ITER_FOLIOQ, ITER_XARRAY or ITER_DISCARD
iterators.

This change proved to be a problem for cachefiles as an iterator of type
ITER_FOLIOQ is passed and iter_iov_addr() and iter_iov_len() both
malfunction because iter->__iov in iter_iov() is not pointing to an iovec
array.

Fix this by using iov_iter_alignment() instead for anything other than
ITER_UBUF, ITER_IOVEC or ITER_KVEC.

Fixes: 14b007e17881 ("block: validate user space vectors during extraction")
Suggested-by: Keith Busch <kbusch@kernel.org>
cc: Keith Busch <kbusch@kernel.org>
cc: Jens Axboe <axboe@kernel.dk>
cc: Hannes Reinecke <hare@kernel.org>
cc: Christoph Hellwig <hch@lst.de>
cc: Alexander Viro <viro@zeniv.linux.org.uk>
cc: Paulo Alcantara <pc@manguebit.org>
cc: netfs@lists.linux.dev
cc: linux-block@vger.kernel.org
cc: linux-fsdevel@vger.kernel.org
---
 lib/iov_iter.c |   11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/lib/iov_iter.c b/lib/iov_iter.c
index 6665372ecf71..c489569a5815 100644
--- a/lib/iov_iter.c
+++ b/lib/iov_iter.c
@@ -1921,15 +1921,22 @@ ssize_t iov_iter_extract_bvecs(struct iov_iter *iter, struct bio_vec *bv,
 		unsigned short max_vecs, unsigned mem_align_mask,
 		iov_iter_extraction_t extraction_flags)
 {
-	unsigned long start = (unsigned long)iter_iov_addr(iter);
 	unsigned short entries_left = max_vecs - *nr_vecs;
 	unsigned short nr_pages, i = 0;
 	size_t left, offset, len;
 	struct page **pages;
 	ssize_t size;
 
-	if ((start | iter_iov_len(iter)) & mem_align_mask)
+	if (likely(iter_is_ubuf(iter) ||
+		   iter_is_iovec(iter) ||
+		   iov_iter_is_kvec(iter))) {
+		unsigned long start = (unsigned long)iter_iov_addr(iter);
+
+		if ((start | iter_iov_len(iter)) & mem_align_mask)
+			return -EINVAL;
+	} else if (iov_iter_alignment(iter) & mem_align_mask) {
 		return -EINVAL;
+	}
 
 	/*
 	 * Move page array up in the allocated memory for the bio vecs as far as
Re: [PATCH v2] block: Fix start and length check added to iov_iter_extract_bvecs()
Posted by Christoph Hellwig 3 weeks, 4 days ago
Should this grow a comment explaining why for non-iov/kvec/ubuf
we check the alignment of every sector instead of just the
start alignment?
Re: [PATCH v2] block: Fix start and length check added to iov_iter_extract_bvecs()
Posted by David Howells 3 weeks, 2 days ago
Christoph Hellwig <hch@lst.de> wrote:

> Should this grow a comment explaining why for non-iov/kvec/ubuf
> we check the alignment of every sector instead of just the
> start alignment?

There should possibly be a comment explaining the need for an address
alignment at all.  Is it mandatory for all devices?  Or are we being a bit
over restrictive?  I don't think NICs capable of RDMA are so restricted, for
example.

Note that iov_iter_alignment() for ITER_XARRAY and ITER_FOLIOQ basically just
check iov_offset and count anyway as they refer to whole pages/folios only,
with just the start and end reduced.

David
Re: [PATCH v2] block: Fix start and length check added to iov_iter_extract_bvecs()
Posted by Christoph Hellwig 2 weeks, 6 days ago
On Fri, Sep 04, 2026 at 10:42:17AM +0100, David Howells wrote:
> Christoph Hellwig <hch@lst.de> wrote:
> 
> > Should this grow a comment explaining why for non-iov/kvec/ubuf
> > we check the alignment of every sector instead of just the
> > start alignment?
> 
> There should possibly be a comment explaining the need for an address
> alignment at all.

Sure.
Re: [PATCH v2] block: Fix start and length check added to iov_iter_extract_bvecs()
Posted by Keith Busch 3 weeks, 2 days ago
On Fri, Sep 04, 2026 at 10:42:17AM +0100, David Howells wrote:
> Christoph Hellwig <hch@lst.de> wrote:
> 
> > Should this grow a comment explaining why for non-iov/kvec/ubuf
> > we check the alignment of every sector instead of just the
> > start alignment?
> 
> There should possibly be a comment explaining the need for an address
> alignment at all.  Is it mandatory for all devices?  Or are we being a bit
> over restrictive?  I don't think NICs capable of RDMA are so restricted, for
> example.

If a block device behind an RDMA NIC really wants to DMA a single byte
at a time, we can't describe that capability today. That would require a
0 mask, which is currently treated as "unset" and overridden with a
default 512b mask.
Re: [PATCH v2] block: Fix start and length check added to iov_iter_extract_bvecs()
Posted by David Howells 3 weeks, 1 day ago
Keith Busch <kbusch@kernel.org> wrote:

> If a block device behind an RDMA NIC really wants to DMA a single byte
> at a time, we can't describe that capability today. That would require a
> 0 mask, which is currently treated as "unset" and overridden with a
> default 512b mask.

The thing is, Christoph said:

    Massage __bio_iov_iter_get_pages so that it doesn't need the bio, and
    move it to lib/iov_iter.c so that it can be used by block code for
    other things than filling a bio and by other subsystems like netfs.

but unless it can have a 1-byte alignment, it's not actually much use for
netfslib (I have to be able to support "unbuffered writes", which are almost
exactly like DIO writes but without alignment restrictions).

Do block devices really need an aligned "start memory address" or just an
aligned "start file position" (whatever that means for a blockdev).

David
Re: [PATCH v2] block: Fix start and length check added to iov_iter_extract_bvecs()
Posted by Christoph Hellwig 2 weeks, 6 days ago
On Sat, Sep 05, 2026 at 05:26:40PM +0100, David Howells wrote:
> Keith Busch <kbusch@kernel.org> wrote:
> 
> > If a block device behind an RDMA NIC really wants to DMA a single byte
> > at a time, we can't describe that capability today. That would require a
> > 0 mask, which is currently treated as "unset" and overridden with a
> > default 512b mask.
> 
> The thing is, Christoph said:
> 
>     Massage __bio_iov_iter_get_pages so that it doesn't need the bio, and
>     move it to lib/iov_iter.c so that it can be used by block code for
>     other things than filling a bio and by other subsystems like netfs.
> 
> but unless it can have a 1-byte alignment, it's not actually much use for
> netfslib (I have to be able to support "unbuffered writes", which are almost
> exactly like DIO writes but without alignment restrictions).
> 
> Do block devices really need an aligned "start memory address" or just an
> aligned "start file position" (whatever that means for a blockdev).

The file position is totally irrelevant, the memory address, or in case
of IOMMUs the IOVA (which keeps the lower bits of the offset aligned)
matter.  Most hardware, bother storage and networking (and lots of
others) require at least 4-byte aka DWORD alignment.  But if we
want to support event less, we'll need to adjust the interface, which
shouldn't be too horrible.

> 
> David
---end quoted text---
Re: [PATCH v2] block: Fix start and length check added to iov_iter_extract_bvecs()
Posted by David Howells 1 month ago
I forgot to add:

Signed-off-by: David Howells <dhowells@redhat.com>
Re: [PATCH v2] block: Fix start and length check added to iov_iter_extract_bvecs()
Posted by Keith Busch 1 month ago
On Wed, Aug 26, 2026 at 09:46:32PM +0100, David Howells wrote:
> Fixes: 14b007e17881 ("block: validate user space vectors during extraction")
> Suggested-by: Keith Busch <kbusch@kernel.org>

Thanks, looks good.

Reviewed-by: Keith Busch <kbusch@kernel.org>

Missing your Signed-off-by?
 
> cc: Keith Busch <kbusch@kernel.org>
> cc: Jens Axboe <axboe@kernel.dk>
> cc: Hannes Reinecke <hare@kernel.org>
> cc: Christoph Hellwig <hch@lst.de>
> cc: Alexander Viro <viro@zeniv.linux.org.uk>
> cc: Paulo Alcantara <pc@manguebit.org>
> cc: netfs@lists.linux.dev
> cc: linux-block@vger.kernel.org
> cc: linux-fsdevel@vger.kernel.org
> ---
>  lib/iov_iter.c |   11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/iov_iter.c b/lib/iov_iter.c
> index 6665372ecf71..c489569a5815 100644
> --- a/lib/iov_iter.c
> +++ b/lib/iov_iter.c
> @@ -1921,15 +1921,22 @@ ssize_t iov_iter_extract_bvecs(struct iov_iter *iter, struct bio_vec *bv,
>  		unsigned short max_vecs, unsigned mem_align_mask,
>  		iov_iter_extraction_t extraction_flags)
>  {
> -	unsigned long start = (unsigned long)iter_iov_addr(iter);
>  	unsigned short entries_left = max_vecs - *nr_vecs;
>  	unsigned short nr_pages, i = 0;
>  	size_t left, offset, len;
>  	struct page **pages;
>  	ssize_t size;
>  
> -	if ((start | iter_iov_len(iter)) & mem_align_mask)
> +	if (likely(iter_is_ubuf(iter) ||
> +		   iter_is_iovec(iter) ||
> +		   iov_iter_is_kvec(iter))) {
> +		unsigned long start = (unsigned long)iter_iov_addr(iter);
> +
> +		if ((start | iter_iov_len(iter)) & mem_align_mask)
> +			return -EINVAL;
> +	} else if (iov_iter_alignment(iter) & mem_align_mask) {
>  		return -EINVAL;
> +	}
>  
>  	/*
>  	 * Move page array up in the allocated memory for the bio vecs as far as
>