Messages in this thread | | | Subject | Re: [PATCH RFC] block: fix use after free for bd_holder_dir/slave_dir | From | Yu Kuai <> | Date | Mon, 17 Oct 2022 17:24:20 +0800 |
| |
Hi, christoph!
在 2022/10/17 15:59, Christoph Hellwig 写道: > AFAICS the problem is the that pre-registered holders don't get > unregistered for a late add_disk failure. > > Something like this should fix your error: >
I agree that this patch do make sense, however, it seems to me this patch should fix the problem that kobject is leaked, not uaf... And I verified that the problem can still be reporduced with this patch.
Some details about our error:
while creating dm using sda, sda is removed. Thus device_add_disk() will fail at bd_register_pending_holders() and and slave_dir is removed. Then, later in dm error path, slave_dir is accessed again.
Perhaps refactor dm error path can fix this error, however, I still think the root cause is lifecycle of slave_dir/bd_holder_dir, and there are still probably other potential problems...
Thanks, Kuai > diff --git a/block/genhd.c b/block/genhd.c > index 17b33c62423df..6123005154b2a 100644 > --- a/block/genhd.c > +++ b/block/genhd.c > @@ -484,7 +484,7 @@ int __must_check device_add_disk(struct device *parent, struct gendisk *disk, > > ret = blk_register_queue(disk); > if (ret) > - goto out_put_slave_dir; > + goto out_unregister_holders; > > if (!(disk->flags & GENHD_FL_HIDDEN)) { > ret = bdi_register(disk->bdi, "%u:%u", > @@ -526,6 +526,8 @@ int __must_check device_add_disk(struct device *parent, struct gendisk *disk, > bdi_unregister(disk->bdi); > out_unregister_queue: > blk_unregister_queue(disk); > +out_unregister_holders: > + bd_unregister_all_holders(disk); > out_put_slave_dir: > kobject_put(disk->slave_dir); > out_put_holder_dir: > diff --git a/block/holder.c b/block/holder.c > index 5283bc804cc14..12c09d5c21280 100644 > --- a/block/holder.c > +++ b/block/holder.c > @@ -169,3 +169,13 @@ int bd_register_pending_holders(struct gendisk *disk) > mutex_unlock(&disk->open_mutex); > return ret; > } > + > +void bd_unregister_all_holders(struct gendisk *disk) > +{ > + struct bd_holder_disk *holder; > + > + mutex_lock(&disk->open_mutex); > + list_for_each_entry(holder, &disk->slave_bdevs, list) > + __unlink_disk_holder(holder->bdev, disk); > + mutex_unlock(&disk->open_mutex); > +} > diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h > index 50e358a19d986..ccab9a2dae4bd 100644 > --- a/include/linux/blkdev.h > +++ b/include/linux/blkdev.h > @@ -840,6 +840,7 @@ void set_capacity(struct gendisk *disk, sector_t size); > int bd_link_disk_holder(struct block_device *bdev, struct gendisk *disk); > void bd_unlink_disk_holder(struct block_device *bdev, struct gendisk *disk); > int bd_register_pending_holders(struct gendisk *disk); > +void bd_unregister_all_holders(struct gendisk *disk); > #else > static inline int bd_link_disk_holder(struct block_device *bdev, > struct gendisk *disk) > @@ -854,6 +855,9 @@ static inline int bd_register_pending_holders(struct gendisk *disk) > { > return 0; > } > +static inline void bd_unregister_all_holders(struct gendisk *disk) > +{ > +} > #endif /* CONFIG_BLOCK_HOLDER_DEPRECATED */ > > dev_t part_devt(struct gendisk *disk, u8 partno); > . >
| |