On Mon 01-04-19 17:32:32, yuyufen wrote:
> add Cc
>
> On 2019/4/1 17:34, Yufen Yu wrote:
> > commit 2da78092dda "block: Fix dev_t minor allocation lifetime"
> > specifically moved blk_free_devt(dev->devt) call to part_release()
> > to avoid reallocating device number before the device is fully
> > shutdown.
> >
> > However, it can cause use-after-free on gendisk in get_gendisk().
> > We use md device as example to show the race scenes:
> >
> > Process1 Worker Process2
> > md_free
> > blkdev_open
> > del_gendisk
> > add delete_partition_work_fn() to wq
> > __blkdev_get
> > get_gendisk
> > put_disk
> > disk_release
> > kfree(disk)
> > find part from ext_devt_idr
> > get_disk_and_module(disk)
> > cause use after free
> >
> > delete_partition_work_fn
> > put_device(part)
> > part_release
> > remove part from ext_devt_idr
> >
> > Before <devt, hd_struct pointer> is removed from ext_devt_idr by
> > delete_partition_work_fn(), we can find the devt and then access
> > gendisk by hd_struct pointer. But, if we access the gendisk after
> > it have been freed, it can cause in use-after-freeon gendisk in
> > get_gendisk().
> >
> > We fix this by adding a new helper blk_invalidate_devt() in
> > delete_partition() and del_gendisk(). It replaces hd_struct
> > pointer in idr with value 'NULL', and deletes the entry from
> > idr in part_release() as we do now.
> >
> > Fixes: 2da78092dda1 ("block: Fix dev_t minor allocation lifetime")
> > Cc: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> > Cc: Bart Van Assche <bart.vanassche@xxxxxxx>
> > Cc: Jan Kara <jack@xxxxxxx>
> > Cc: Keith Busch <keith.busch@xxxxxxxxx>
> > Signed-off-by: Yufen Yu <yuyufen@xxxxxxxxxx>
Thanks for the patch! Just two nits below:
> > diff --git a/block/genhd.c b/block/genhd.c
> > index 961b2bc4634f..7144153c6bf1 100644
> > --- a/block/genhd.c
> > +++ b/block/genhd.c
> > @@ -529,6 +529,18 @@ void blk_free_devt(dev_t devt)
> > }
> > }
> > +/**
> > + * We invalidate devt by assigning NULL pointer for devt in idr.
> > + */
> > +void blk_invalidate_devt(dev_t devt)
> > +{
> > + if (MAJOR(devt) == BLOCK_EXT_MAJOR) {
> > + spin_lock_bh(&ext_devt_lock);
> > + idr_replace(&ext_devt_idr, NULL, blk_mangle_minor(MINOR(devt)));
> > + spin_unlock_bh(&ext_devt_lock);
> > + }
> > +}
> > +
> > static char *bdevt_str(dev_t devt, char *buf)
> > {
> > if (MAJOR(devt) <= 0xff && MINOR(devt) <= 0xff) {
> > @@ -801,6 +813,11 @@ void del_gendisk(struct gendisk *disk)
> > sysfs_remove_link(block_depr, dev_name(disk_to_dev(disk)));
> > pm_runtime_set_memalloc_noio(disk_to_dev(disk), false);
> > device_del(disk_to_dev(disk));
> > +
> > + /*
> > + * we need to invalidate devt before remove it from idr.
> > + */
> > + blk_invalidate_devt(disk_devt(disk));
I would move this slightly up to where blk_unregister_region() is called
because these two are just different means to lookup the gendisk (idr or
bdev_map depending of major number used). Also I'd update the comment to
something like:
/*
* Remove gendisk pointer from idr so that it cannot be looked up
* while RCU period before freeing gendisk is running to prevent
* use-after-free issues. Note that the device number stays
* "in-use" until we really free the gendisk.
*/
> > }
> > EXPORT_SYMBOL(del_gendisk);
> > diff --git a/block/partition-generic.c b/block/partition-generic.c
> > index 1ee3e1d1bc2a..922230b5a907 100644
> > --- a/block/partition-generic.c
> > +++ b/block/partition-generic.c
> > @@ -288,6 +288,12 @@ void delete_partition(struct gendisk *disk, int partno)
> > kobject_put(part->holder_dir);
> > device_del(part_to_dev(part));
> > + /*
> > + * We need to invalidate devt by assigning NULL pointer for devt
> > + * before remove it from ext_devt_idr, which can avoid use-after-free
> > + * on gendisk.
> > + */
> > + blk_invalidate_devt(part_devt(part));
> > hd_struct_kill(part);
> > }
And here I'd just use the same comment as above.
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
