Re: [RESEND][SECURITY] block: double unpin in bounced direct reads

[Christoph Hellwig <hch@lst.de>]

On Mon, Jul 13, 2026 at 06:20:13PM +0800, Neptune wrote:
> Hi Christoph,
> 
> On Mon, Jul 13, 2026 at 11:29 AM Christoph Hellwig <hch@lst.de> wrote:
> > Does fix the issue for you?
> 
> Yes, it fixes the reported double-unpin in my original reproducer. I
> applied the patch to the vulnerable parent, built an x86-64 KASAN
> kernel, and reran the test with the same QEMU NVMe PI namespace and
> XFS setup. The exploit now reports a stale page miss and the process
> remains uid 1000.
> 
> I did find a boundary bug in the rewritten alignment loop. If the bio
> contains a single one-byte bvec and nbytes is also one, the first
> iteration consumes that bvec, decrements bi_vcnt to zero, and moves bv
> before the vector array. The next while condition then dereferences
> the invalid bv before the later bi_vcnt check can run.
> 
> I reproduced this on the patched kernel with a 512-byte O_DIRECT pread
> from a raw virtio block device. The destination starts at the last
> byte of a mapped 4 KiB page and the following page is PROT_NONE,
> leaving one bvec with bv_len=1 and bv_offset=4095. Immediately before
> the loop, GEF showed bi_vcnt=1, nbytes=1, and bv_len=1. After that
> bvec was consumed, GEF caught another call to unpin_user_page() with
> page=0x100020000, bi_vcnt=0, and nbytes=0; KASAN then reported a
> general protection fault from unpin_user_page() through
> bio_iov_iter_get_pages().
> 
> This boundary test requires permission to open the raw block device.
> It is a correctness regression in the proposed loop and is separate
> from the original unprivileged XFS entry point.
> 
> Keeping the loop driven by nbytes, with the old partial-bvec check
> inside it, should avoid evaluating bv after the last vector has been
> consumed. The callers can still pass an explicit starting bvec, but
> the loop needs to stop as soon as nbytes reaches zero.

Yes, updated version below.

> I also noticed two read-bounce cleanup details while reviewing the
> error paths. If alignment removes every user bvec,
> bio_iov_iter_bounce_read() returns the error without dropping the
> allocated bounce folio. If alignment succeeds after trimming bytes,
> bi_io_vec[0].bv_len retains the pre-trim length even though
> bio_iov_iter_unbounce_read() later uses it as the copy length. The
> error path appears to need a folio_put(), and the successful path
> needs to synchronize vector zero's length with bio->bi_iter.bi_size.

I'll look into that next.

---
From 43e1b03b38264d8fb8c647c8a801bd1e81a4638a Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <hch@lst.de>
Date: Mon, 13 Jul 2026 10:42:34 +0200
Subject: block: fix aligning of bounced dio read bios

bio_iov_iter_align_down expects the "normal" biovec layout from vector 0,
while bio_iov_iter_bounce_read abuses vector 0 for a bounce buffer
allocation.  Pass an explicit bvec to bio_iov_iter_align_down to deal
with this case to avoid a double unpin.

Fixes: e7b8b3c5b2a6 ("block: align down bounces bios")
Reported-by: Neptune <z1281552865@gmail.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/bio.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/block/bio.c b/block/bio.c
index f2a5f4d0a967..cc50c603a9f8 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -1191,7 +1191,7 @@ void bio_iov_bvec_set(struct bio *bio, const struct iov_iter *iter)
  * for the next iteration.
  */
 static int bio_iov_iter_align_down(struct bio *bio, struct iov_iter *iter,
-			    unsigned len_align_mask)
+				   struct bio_vec *bv, unsigned len_align_mask)
 {
 	size_t nbytes = bio->bi_iter.bi_size & len_align_mask;
 
@@ -1200,9 +1200,7 @@ static int bio_iov_iter_align_down(struct bio *bio, struct iov_iter *iter,
 
 	iov_iter_revert(iter, nbytes);
 	bio->bi_iter.bi_size -= nbytes;
-	do {
-		struct bio_vec *bv = &bio->bi_io_vec[bio->bi_vcnt - 1];
-
+	for (;;) {
 		if (nbytes < bv->bv_len) {
 			bv->bv_len -= nbytes;
 			break;
@@ -1211,9 +1209,10 @@ static int bio_iov_iter_align_down(struct bio *bio, struct iov_iter *iter,
 		if (bio_flagged(bio, BIO_PAGE_PINNED))
 			unpin_user_page(bv->bv_page);
 
-		bio->bi_vcnt--;
 		nbytes -= bv->bv_len;
-	} while (nbytes);
+		bio->bi_vcnt--;
+		bv--;
+	}
 
 	if (!bio->bi_vcnt)
 		return -EFAULT;
@@ -1276,7 +1275,8 @@ int bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter,
 
 	if (is_pci_p2pdma_page(bio->bi_io_vec->bv_page))
 		bio->bi_opf |= REQ_NOMERGE;
-	return bio_iov_iter_align_down(bio, iter, len_align_mask);
+	return bio_iov_iter_align_down(bio, iter,
+			&bio->bi_io_vec[bio->bi_vcnt - 1], len_align_mask);
 }
 
 static struct folio *folio_alloc_greedy(gfp_t gfp, size_t *size,
@@ -1360,7 +1360,8 @@ static int bio_iov_iter_bounce_write(struct bio *bio, struct iov_iter *iter,
 
 	if (!bio->bi_iter.bi_size)
 		return -ENOMEM;
-	return bio_iov_iter_align_down(bio, iter, minsize - 1);
+	return bio_iov_iter_align_down(bio, iter,
+			&bio->bi_io_vec[bio->bi_vcnt - 1], minsize - 1);
 }
 
 static int bio_iov_iter_bounce_read(struct bio *bio, struct iov_iter *iter,
@@ -1398,7 +1399,10 @@ static int bio_iov_iter_bounce_read(struct bio *bio, struct iov_iter *iter,
 	bvec_set_folio(&bio->bi_io_vec[0], folio, bio->bi_iter.bi_size, 0);
 	if (iov_iter_extract_will_pin(iter))
 		bio_set_flag(bio, BIO_PAGE_PINNED);
-	return bio_iov_iter_align_down(bio, iter, minsize - 1);
+
+	/* The first vec stores the bounce buffer, so do not subtract 1 here. */
+	return bio_iov_iter_align_down(bio, iter,
+			&bio->bi_io_vec[bio->bi_vcnt], minsize - 1);
 }
 
 /**
-- 
2.53.0