[dm-devel] [PATCH] block: fix a crash when bio_for_each_folio_all iterates over an empty bio

[dm-devel] [PATCH] block: fix a crash when bio_for_each_folio_all iterates over an empty bio

[Mikulas Patocka]

If we use bio_for_each_folio_all on an empty bio, it will access the first
bio vector unconditionally (it is uninitialized) and it may crash
depending on the uninitialized data.

This patch fixes it by checking the parameter "i" against "bio->bi_vcnt"
and returning NULL fi->folio if it is out of range.

The patch also drops the test "if (fi->_i + 1 < bio->bi_vcnt)" from
bio_next_folio because the same condition is already being checked in
bio_first_folio.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 include/linux/bio.h |   24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

Index: linux-2.6/include/linux/bio.h
===================================================================
--- linux-2.6.orig/include/linux/bio.h
+++ linux-2.6/include/linux/bio.h
@@ -279,15 +279,19 @@ struct folio_iter {
 static inline void bio_first_folio(struct folio_iter *fi, struct bio *bio,
 				   int i)
 {
-	struct bio_vec *bvec = bio_first_bvec_all(bio) + i;
+	if (i < bio->bi_vcnt) {
+		struct bio_vec *bvec = bio_first_bvec_all(bio) + i;
 
-	fi->folio = page_folio(bvec->bv_page);
-	fi->offset = bvec->bv_offset +
-			PAGE_SIZE * (bvec->bv_page - &fi->folio->page);
-	fi->_seg_count = bvec->bv_len;
-	fi->length = min(folio_size(fi->folio) - fi->offset, fi->_seg_count);
-	fi->_next = folio_next(fi->folio);
-	fi->_i = i;
+		fi->folio = page_folio(bvec->bv_page);
+		fi->offset = bvec->bv_offset +
+				PAGE_SIZE * (bvec->bv_page - &fi->folio->page);
+		fi->_seg_count = bvec->bv_len;
+		fi->length = min(folio_size(fi->folio) - fi->offset, fi->_seg_count);
+		fi->_next = folio_next(fi->folio);
+		fi->_i = i;
+	} else {
+		fi->folio = NULL;
+	}
 }
 
 static inline void bio_next_folio(struct folio_iter *fi, struct bio *bio)
@@ -298,10 +302,8 @@ static inline void bio_next_folio(struct
 		fi->offset = 0;
 		fi->length = min(folio_size(fi->folio), fi->_seg_count);
 		fi->_next = folio_next(fi->folio);
-	} else if (fi->_i + 1 < bio->bi_vcnt) {
-		bio_first_folio(fi, bio, fi->_i + 1);
 	} else {
-		fi->folio = NULL;
+		bio_first_folio(fi, bio, fi->_i + 1);
 	}
 }
 
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[PATCH] block: fix a crash when bio_for_each_folio_all iterates over an empty bio

[Mikulas Patocka]

If we use bio_for_each_folio_all on an empty bio, it will access the first
bio vector unconditionally (it is uninitialized) and it may crash
depending on the uninitialized data.

This patch fixes it by checking the parameter "i" against "bio->bi_vcnt"
and returning NULL fi->folio if it is out of range.

The patch also drops the test "if (fi->_i + 1 < bio->bi_vcnt)" from
bio_next_folio because the same condition is already being checked in
bio_first_folio.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 include/linux/bio.h |   24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

Index: linux-2.6/include/linux/bio.h
===================================================================
--- linux-2.6.orig/include/linux/bio.h
+++ linux-2.6/include/linux/bio.h
@@ -279,15 +279,19 @@ struct folio_iter {
 static inline void bio_first_folio(struct folio_iter *fi, struct bio *bio,
 				   int i)
 {
-	struct bio_vec *bvec = bio_first_bvec_all(bio) + i;
+	if (i < bio->bi_vcnt) {
+		struct bio_vec *bvec = bio_first_bvec_all(bio) + i;
 
-	fi->folio = page_folio(bvec->bv_page);
-	fi->offset = bvec->bv_offset +
-			PAGE_SIZE * (bvec->bv_page - &fi->folio->page);
-	fi->_seg_count = bvec->bv_len;
-	fi->length = min(folio_size(fi->folio) - fi->offset, fi->_seg_count);
-	fi->_next = folio_next(fi->folio);
-	fi->_i = i;
+		fi->folio = page_folio(bvec->bv_page);
+		fi->offset = bvec->bv_offset +
+				PAGE_SIZE * (bvec->bv_page - &fi->folio->page);
+		fi->_seg_count = bvec->bv_len;
+		fi->length = min(folio_size(fi->folio) - fi->offset, fi->_seg_count);
+		fi->_next = folio_next(fi->folio);
+		fi->_i = i;
+	} else {
+		fi->folio = NULL;
+	}
 }
 
 static inline void bio_next_folio(struct folio_iter *fi, struct bio *bio)
@@ -298,10 +302,8 @@ static inline void bio_next_folio(struct
 		fi->offset = 0;
 		fi->length = min(folio_size(fi->folio), fi->_seg_count);
 		fi->_next = folio_next(fi->folio);
-	} else if (fi->_i + 1 < bio->bi_vcnt) {
-		bio_first_folio(fi, bio, fi->_i + 1);
 	} else {
-		fi->folio = NULL;
+		bio_first_folio(fi, bio, fi->_i + 1);
 	}
 }
 

Re: [dm-devel] [PATCH] block: fix a crash when bio_for_each_folio_all iterates over an empty bio

[Matthew Wilcox]

On Mon, Apr 17, 2023 at 03:11:57PM -0400, Mikulas Patocka wrote:
> If we use bio_for_each_folio_all on an empty bio, it will access the first
> bio vector unconditionally (it is uninitialized) and it may crash
> depending on the uninitialized data.

Wait, how do we have an empty bio in the first place?

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH] block: fix a crash when bio_for_each_folio_all iterates over an empty bio

[Matthew Wilcox]

On Mon, Apr 17, 2023 at 03:11:57PM -0400, Mikulas Patocka wrote:
> If we use bio_for_each_folio_all on an empty bio, it will access the first
> bio vector unconditionally (it is uninitialized) and it may crash
> depending on the uninitialized data.

Wait, how do we have an empty bio in the first place?

Re: [dm-devel] [PATCH] block: fix a crash when bio_for_each_folio_all iterates over an empty bio

[Mikulas Patocka]

On Mon, 17 Apr 2023, Matthew Wilcox wrote:

> On Mon, Apr 17, 2023 at 03:11:57PM -0400, Mikulas Patocka wrote:
> > If we use bio_for_each_folio_all on an empty bio, it will access the first
> > bio vector unconditionally (it is uninitialized) and it may crash
> > depending on the uninitialized data.
> 
> Wait, how do we have an empty bio in the first place?

dm-crypt (with the patch that you suggested here: 
https://www.spinics.net/lists/kernel/msg4691572.html
https://lore.kernel.org/linux-mm/alpine.LRH.2.21.2302161619430.5436@file01.intranet.prod.int.rdu2.redhat.com/T/
) calls:

gfp_t gfp_mask = GFP_NOWAIT | __GFP_HIGHMEM;
...
pages = mempool_alloc(&cc->page_pool, gfp_mask);
if (!pages) {
	crypt_free_buffer_pages(cc, clone);
	bio_put(clone);
	gfp_mask |= __GFP_DIRECT_RECLAIM;
	order = 0;
	goto retry;
}

If the mempool_alloc of the first page fails (it may happen because it 
uses GFP_NOWAIT), crypt_free_buffer_pages will be called with an empty 
bio.


I also hit this bug when fixing a dm-flakey target - it is triggered by 
this: 
https://people.redhat.com/~mpatocka/patches/kernel/dm-flakey/dm-flakey-02-clone-pages.patch


I think that this patch doesn't have to be backported to the stable 
kernels (because there is currently no user that calls 
bio_for_each_folio_all on an empty bio), but for the next merge window, 
dm-crypt and dm-flakey is going to use it.

Mikulas
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH] block: fix a crash when bio_for_each_folio_all iterates over an empty bio

[Mikulas Patocka]

On Mon, 17 Apr 2023, Matthew Wilcox wrote:

> On Mon, Apr 17, 2023 at 03:11:57PM -0400, Mikulas Patocka wrote:
> > If we use bio_for_each_folio_all on an empty bio, it will access the first
> > bio vector unconditionally (it is uninitialized) and it may crash
> > depending on the uninitialized data.
> 
> Wait, how do we have an empty bio in the first place?

dm-crypt (with the patch that you suggested here: 
https://www.spinics.net/lists/kernel/msg4691572.html
https://lore.kernel.org/linux-mm/alpine.LRH.2.21.2302161619430.5436@file01.intranet.prod.int.rdu2.redhat.com/T/
) calls:

gfp_t gfp_mask = GFP_NOWAIT | __GFP_HIGHMEM;
...
pages = mempool_alloc(&cc->page_pool, gfp_mask);
if (!pages) {
	crypt_free_buffer_pages(cc, clone);
	bio_put(clone);
	gfp_mask |= __GFP_DIRECT_RECLAIM;
	order = 0;
	goto retry;
}

If the mempool_alloc of the first page fails (it may happen because it 
uses GFP_NOWAIT), crypt_free_buffer_pages will be called with an empty 
bio.


I also hit this bug when fixing a dm-flakey target - it is triggered by 
this: 
https://people.redhat.com/~mpatocka/patches/kernel/dm-flakey/dm-flakey-02-clone-pages.patch


I think that this patch doesn't have to be backported to the stable 
kernels (because there is currently no user that calls 
bio_for_each_folio_all on an empty bio), but for the next merge window, 
dm-crypt and dm-flakey is going to use it.

Mikulas

Re: [dm-devel] [PATCH] block: fix a crash when bio_for_each_folio_all iterates over an empty bio

[Christoph Hellwig]

On Mon, Apr 17, 2023 at 08:32:54PM +0100, Matthew Wilcox wrote:
> On Mon, Apr 17, 2023 at 03:11:57PM -0400, Mikulas Patocka wrote:
> > If we use bio_for_each_folio_all on an empty bio, it will access the first
> > bio vector unconditionally (it is uninitialized) and it may crash
> > depending on the uninitialized data.
> 
> Wait, how do we have an empty bio in the first place?

flush bios are always empty.  Not sure iterating over them makes much
sense, though.

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH] block: fix a crash when bio_for_each_folio_all iterates over an empty bio

[Christoph Hellwig]

On Mon, Apr 17, 2023 at 08:32:54PM +0100, Matthew Wilcox wrote:
> On Mon, Apr 17, 2023 at 03:11:57PM -0400, Mikulas Patocka wrote:
> > If we use bio_for_each_folio_all on an empty bio, it will access the first
> > bio vector unconditionally (it is uninitialized) and it may crash
> > depending on the uninitialized data.
> 
> Wait, how do we have an empty bio in the first place?

flush bios are always empty.  Not sure iterating over them makes much
sense, though.

Re: [dm-devel] block: fix a crash when bio_for_each_folio_all iterates over an empty bio

[Mike Snitzer]

On Mon, Apr 17 2023 at  3:11P -0400,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> If we use bio_for_each_folio_all on an empty bio, it will access the first
> bio vector unconditionally (it is uninitialized) and it may crash
> depending on the uninitialized data.
> 
> This patch fixes it by checking the parameter "i" against "bio->bi_vcnt"
> and returning NULL fi->folio if it is out of range.
> 
> The patch also drops the test "if (fi->_i + 1 < bio->bi_vcnt)" from
> bio_next_folio because the same condition is already being checked in
> bio_first_folio.
>
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

This fix is a prereq for this dm-crypt patch to use folios:
https://patchwork.kernel.org/project/dm-devel/patch/alpine.LRH.2.21.2302161619430.5436@file01.intranet.prod.int.rdu2.redhat.com/
Mikulas explained why an empty bio is possible here:
https://listman.redhat.com/archives/dm-devel/2023-April/053916.html

Reviewed-by: Mike Snitzer <snitzer@kernel.org>

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: block: fix a crash when bio_for_each_folio_all iterates over an empty bio

[Mike Snitzer]

On Mon, Apr 17 2023 at  3:11P -0400,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> If we use bio_for_each_folio_all on an empty bio, it will access the first
> bio vector unconditionally (it is uninitialized) and it may crash
> depending on the uninitialized data.
> 
> This patch fixes it by checking the parameter "i" against "bio->bi_vcnt"
> and returning NULL fi->folio if it is out of range.
> 
> The patch also drops the test "if (fi->_i + 1 < bio->bi_vcnt)" from
> bio_next_folio because the same condition is already being checked in
> bio_first_folio.
>
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

This fix is a prereq for this dm-crypt patch to use folios:
https://patchwork.kernel.org/project/dm-devel/patch/alpine.LRH.2.21.2302161619430.5436@file01.intranet.prod.int.rdu2.redhat.com/
Mikulas explained why an empty bio is possible here:
https://listman.redhat.com/archives/dm-devel/2023-April/053916.html

Reviewed-by: Mike Snitzer <snitzer@kernel.org>

Re: [dm-devel] block: fix a crash when bio_for_each_folio_all iterates over an empty bio

[Mike Snitzer]

On Tue, Apr 18 2023 at 11:20P -0400,
Mike Snitzer <snitzer@kernel.org> wrote:

> On Mon, Apr 17 2023 at  3:11P -0400,
> Mikulas Patocka <mpatocka@redhat.com> wrote:
> 
> > If we use bio_for_each_folio_all on an empty bio, it will access the first
> > bio vector unconditionally (it is uninitialized) and it may crash
> > depending on the uninitialized data.
> > 
> > This patch fixes it by checking the parameter "i" against "bio->bi_vcnt"
> > and returning NULL fi->folio if it is out of range.
> > 
> > The patch also drops the test "if (fi->_i + 1 < bio->bi_vcnt)" from
> > bio_next_folio because the same condition is already being checked in
> > bio_first_folio.
> >
> > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> 
> This fix is a prereq for this dm-crypt patch to use folios:
> https://patchwork.kernel.org/project/dm-devel/patch/alpine.LRH.2.21.2302161619430.5436@file01.intranet.prod.int.rdu2.redhat.com/
> Mikulas explained why an empty bio is possible here:
> https://listman.redhat.com/archives/dm-devel/2023-April/053916.html
> 
> Reviewed-by: Mike Snitzer <snitzer@kernel.org>

Hey Jens (and Matthew),

Can you please pick this up?

Without it DM needs to do the checking (by open-coding a fixed variant
of bio_for_each_folio_all); while we _could_ do that: fixing
bio_first_folio seems best.

Thanks,
Mike

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: block: fix a crash when bio_for_each_folio_all iterates over an empty bio

[Mike Snitzer]

On Tue, Apr 18 2023 at 11:20P -0400,
Mike Snitzer <snitzer@kernel.org> wrote:

> On Mon, Apr 17 2023 at  3:11P -0400,
> Mikulas Patocka <mpatocka@redhat.com> wrote:
> 
> > If we use bio_for_each_folio_all on an empty bio, it will access the first
> > bio vector unconditionally (it is uninitialized) and it may crash
> > depending on the uninitialized data.
> > 
> > This patch fixes it by checking the parameter "i" against "bio->bi_vcnt"
> > and returning NULL fi->folio if it is out of range.
> > 
> > The patch also drops the test "if (fi->_i + 1 < bio->bi_vcnt)" from
> > bio_next_folio because the same condition is already being checked in
> > bio_first_folio.
> >
> > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> 
> This fix is a prereq for this dm-crypt patch to use folios:
> https://patchwork.kernel.org/project/dm-devel/patch/alpine.LRH.2.21.2302161619430.5436@file01.intranet.prod.int.rdu2.redhat.com/
> Mikulas explained why an empty bio is possible here:
> https://listman.redhat.com/archives/dm-devel/2023-April/053916.html
> 
> Reviewed-by: Mike Snitzer <snitzer@kernel.org>

Hey Jens (and Matthew),

Can you please pick this up?

Without it DM needs to do the checking (by open-coding a fixed variant
of bio_for_each_folio_all); while we _could_ do that: fixing
bio_first_folio seems best.

Thanks,
Mike