| Message ID | 20220411093838.1729001-1-damien.lemoal@opensource.wdc.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Mike Snitzer |
| Headers | show |
| Series | dm: dm-zone: Fix NULL pointer dereference in dm_zone_map_bio() | expand |
On Mon, Apr 11, 2022 at 06:38:38PM +0900, Damien Le Moal wrote: > Commit 0fbb4d93b38b ("dm: add dm_submit_bio_remap interface") changed > the alloc_io() function to delay the initialization of struct dm_io > orig_bio field, leaving this field as NULL until the first call to > __split_and_process_bio() is executed for the user submitted BIO. This > change causes a NULL pointer dereference in dm_zone_map_bio() when the > original user BIO is inspected to detect the need for zone append > command emulation. > > Avoid this problem by adding a struct clone_info *ci argument to the > __map_bio() function and a struct bio *orig_bio argument to > dm_zone_map_bio(). Doing so, the call to dm_zone_map_bio() can be passed > directly a pointer to the original user BIO using the bio field of > struct clone_info. > > Fixes: 0fbb4d93b38b ("dm: add dm_submit_bio_remap interface") > Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> > --- > drivers/md/dm-zone.c | 3 +-- > drivers/md/dm.c | 10 +++++----- > drivers/md/dm.h | 5 +++-- > 3 files changed, 9 insertions(+), 9 deletions(-) > > diff --git a/drivers/md/dm-zone.c b/drivers/md/dm-zone.c > index c1ca9be4b79e..772161f0b029 100644 > --- a/drivers/md/dm-zone.c > +++ b/drivers/md/dm-zone.c > @@ -513,13 +513,12 @@ static bool dm_need_zone_wp_tracking(struct bio *orig_bio) > /* > * Special IO mapping for targets needing zone append emulation. > */ > -int dm_zone_map_bio(struct dm_target_io *tio) > +int dm_zone_map_bio(struct dm_target_io *tio, struct bio *orig_bio) > { > struct dm_io *io = tio->io; > struct dm_target *ti = tio->ti; > struct mapped_device *md = io->md; > struct request_queue *q = md->queue; > - struct bio *orig_bio = io->orig_bio; > struct bio *clone = &tio->clone; > unsigned int zno; > blk_status_t sts; > diff --git a/drivers/md/dm.c b/drivers/md/dm.c > index 3c5fad7c4ee6..1d8f24f04c7d 100644 > --- a/drivers/md/dm.c > +++ b/drivers/md/dm.c > @@ -1258,7 +1258,7 @@ static noinline void __set_swap_bios_limit(struct mapped_device *md, int latch) > mutex_unlock(&md->swap_bios_lock); > } > > -static void __map_bio(struct bio *clone) > +static void __map_bio(struct clone_info *ci, struct bio *clone) > { > struct dm_target_io *tio = clone_to_tio(clone); > int r; > @@ -1287,7 +1287,7 @@ static void __map_bio(struct bio *clone) > * map operation. > */ > if (dm_emulate_zone_append(io->md)) > - r = dm_zone_map_bio(tio); > + r = dm_zone_map_bio(tio, ci->bio); It depends if bio_split() in dm_split_and_process_bio() can be triggered for dm-zone. If it can be triggered, here the actual original bio should be the one returned from bio_split(). Thanks, Ming -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On 4/11/22 22:02, Ming Lei wrote: > On Mon, Apr 11, 2022 at 06:38:38PM +0900, Damien Le Moal wrote: >> Commit 0fbb4d93b38b ("dm: add dm_submit_bio_remap interface") changed >> the alloc_io() function to delay the initialization of struct dm_io >> orig_bio field, leaving this field as NULL until the first call to >> __split_and_process_bio() is executed for the user submitted BIO. This >> change causes a NULL pointer dereference in dm_zone_map_bio() when the >> original user BIO is inspected to detect the need for zone append >> command emulation. >> >> Avoid this problem by adding a struct clone_info *ci argument to the >> __map_bio() function and a struct bio *orig_bio argument to >> dm_zone_map_bio(). Doing so, the call to dm_zone_map_bio() can be passed >> directly a pointer to the original user BIO using the bio field of >> struct clone_info. >> >> Fixes: 0fbb4d93b38b ("dm: add dm_submit_bio_remap interface") >> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> >> --- >> drivers/md/dm-zone.c | 3 +-- >> drivers/md/dm.c | 10 +++++----- >> drivers/md/dm.h | 5 +++-- >> 3 files changed, 9 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/md/dm-zone.c b/drivers/md/dm-zone.c >> index c1ca9be4b79e..772161f0b029 100644 >> --- a/drivers/md/dm-zone.c >> +++ b/drivers/md/dm-zone.c >> @@ -513,13 +513,12 @@ static bool dm_need_zone_wp_tracking(struct bio *orig_bio) >> /* >> * Special IO mapping for targets needing zone append emulation. >> */ >> -int dm_zone_map_bio(struct dm_target_io *tio) >> +int dm_zone_map_bio(struct dm_target_io *tio, struct bio *orig_bio) >> { >> struct dm_io *io = tio->io; >> struct dm_target *ti = tio->ti; >> struct mapped_device *md = io->md; >> struct request_queue *q = md->queue; >> - struct bio *orig_bio = io->orig_bio; >> struct bio *clone = &tio->clone; >> unsigned int zno; >> blk_status_t sts; >> diff --git a/drivers/md/dm.c b/drivers/md/dm.c >> index 3c5fad7c4ee6..1d8f24f04c7d 100644 >> --- a/drivers/md/dm.c >> +++ b/drivers/md/dm.c >> @@ -1258,7 +1258,7 @@ static noinline void __set_swap_bios_limit(struct mapped_device *md, int latch) >> mutex_unlock(&md->swap_bios_lock); >> } >> >> -static void __map_bio(struct bio *clone) >> +static void __map_bio(struct clone_info *ci, struct bio *clone) >> { >> struct dm_target_io *tio = clone_to_tio(clone); >> int r; >> @@ -1287,7 +1287,7 @@ static void __map_bio(struct bio *clone) >> * map operation. >> */ >> if (dm_emulate_zone_append(io->md)) >> - r = dm_zone_map_bio(tio); >> + r = dm_zone_map_bio(tio, ci->bio); > > It depends if bio_split() in dm_split_and_process_bio() can be triggered > for dm-zone. If it can be triggered, here the actual original bio should > be the one returned from bio_split(). It was like this before commit 0fbb4d93b38b... I will check again though. > > Thanks, > Ming >
diff --git a/drivers/md/dm-zone.c b/drivers/md/dm-zone.c index c1ca9be4b79e..772161f0b029 100644 --- a/drivers/md/dm-zone.c +++ b/drivers/md/dm-zone.c @@ -513,13 +513,12 @@ static bool dm_need_zone_wp_tracking(struct bio *orig_bio) /* * Special IO mapping for targets needing zone append emulation. */ -int dm_zone_map_bio(struct dm_target_io *tio) +int dm_zone_map_bio(struct dm_target_io *tio, struct bio *orig_bio) { struct dm_io *io = tio->io; struct dm_target *ti = tio->ti; struct mapped_device *md = io->md; struct request_queue *q = md->queue; - struct bio *orig_bio = io->orig_bio; struct bio *clone = &tio->clone; unsigned int zno; blk_status_t sts; diff --git a/drivers/md/dm.c b/drivers/md/dm.c index 3c5fad7c4ee6..1d8f24f04c7d 100644 --- a/drivers/md/dm.c +++ b/drivers/md/dm.c @@ -1258,7 +1258,7 @@ static noinline void __set_swap_bios_limit(struct mapped_device *md, int latch) mutex_unlock(&md->swap_bios_lock); } -static void __map_bio(struct bio *clone) +static void __map_bio(struct clone_info *ci, struct bio *clone) { struct dm_target_io *tio = clone_to_tio(clone); int r; @@ -1287,7 +1287,7 @@ static void __map_bio(struct bio *clone) * map operation. */ if (dm_emulate_zone_append(io->md)) - r = dm_zone_map_bio(tio); + r = dm_zone_map_bio(tio, ci->bio); else r = ti->type->map(ti, clone); @@ -1364,13 +1364,13 @@ static void __send_duplicate_bios(struct clone_info *ci, struct dm_target *ti, case 1: clone = alloc_tio(ci, ti, 0, len, GFP_NOIO); dm_tio_set_flag(clone_to_tio(clone), DM_TIO_IS_DUPLICATE_BIO); - __map_bio(clone); + __map_bio(ci, clone); break; default: alloc_multiple_bios(&blist, ci, ti, num_bios, len); while ((clone = bio_list_pop(&blist))) { dm_tio_set_flag(clone_to_tio(clone), DM_TIO_IS_DUPLICATE_BIO); - __map_bio(clone); + __map_bio(ci, clone); } break; } @@ -1532,7 +1532,7 @@ static int __split_and_process_bio(struct clone_info *ci) len = min_t(sector_t, max_io_len(ti, ci->sector), ci->sector_count); clone = alloc_tio(ci, ti, 0, &len, GFP_NOIO); - __map_bio(clone); + __map_bio(ci, clone); ci->sector += len; ci->sector_count -= len; diff --git a/drivers/md/dm.h b/drivers/md/dm.h index 9013dc1a7b00..6e148590ec9c 100644 --- a/drivers/md/dm.h +++ b/drivers/md/dm.h @@ -108,7 +108,7 @@ void dm_cleanup_zoned_dev(struct mapped_device *md); int dm_blk_report_zones(struct gendisk *disk, sector_t sector, unsigned int nr_zones, report_zones_cb cb, void *data); bool dm_is_zone_write(struct mapped_device *md, struct bio *bio); -int dm_zone_map_bio(struct dm_target_io *io); +int dm_zone_map_bio(struct dm_target_io *io, struct bio *orig_bio); #else static inline void dm_cleanup_zoned_dev(struct mapped_device *md) {} #define dm_blk_report_zones NULL @@ -116,7 +116,8 @@ static inline bool dm_is_zone_write(struct mapped_device *md, struct bio *bio) { return false; } -static inline int dm_zone_map_bio(struct dm_target_io *tio) +static inline int dm_zone_map_bio(struct dm_target_io *tio, + struct bio *orig_bio) { return DM_MAPIO_KILL; }
Commit 0fbb4d93b38b ("dm: add dm_submit_bio_remap interface") changed the alloc_io() function to delay the initialization of struct dm_io orig_bio field, leaving this field as NULL until the first call to __split_and_process_bio() is executed for the user submitted BIO. This change causes a NULL pointer dereference in dm_zone_map_bio() when the original user BIO is inspected to detect the need for zone append command emulation. Avoid this problem by adding a struct clone_info *ci argument to the __map_bio() function and a struct bio *orig_bio argument to dm_zone_map_bio(). Doing so, the call to dm_zone_map_bio() can be passed directly a pointer to the original user BIO using the bio field of struct clone_info. Fixes: 0fbb4d93b38b ("dm: add dm_submit_bio_remap interface") Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> --- drivers/md/dm-zone.c | 3 +-- drivers/md/dm.c | 10 +++++----- drivers/md/dm.h | 5 +++-- 3 files changed, 9 insertions(+), 9 deletions(-)