All of lore.kernel.org
 help / color / mirror / Atom feed
* Re: [PATCH v2] block: fix use-after-free on gendisk
  2019-04-01  9:34 [PATCH v2] block: fix use-after-free on gendisk Yufen Yu
@ 2019-04-01  9:32 ` yuyufen
  2019-04-01 10:12   ` Jan Kara
  0 siblings, 1 reply; 4+ messages in thread
From: yuyufen @ 2019-04-01  9:32 UTC (permalink / raw)
  To: axboe, linux-block; +Cc: viro, bart.vanassche, Jan Kara, Keith Busch

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@zeniv.linux.org.uk>
> Cc: Bart Van Assche <bart.vanassche@wdc.com>
> Cc: Jan Kara <jack@suse.cz>
> Cc: Keith Busch <keith.busch@intel.com>
> Signed-off-by: Yufen Yu <yuyufen@huawei.com>
> ---
>   block/genhd.c             | 17 +++++++++++++++++
>   block/partition-generic.c |  6 ++++++
>   include/linux/genhd.h     |  1 +
>   3 files changed, 24 insertions(+)
>
> 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));
>   }
>   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);
>   }
>   
> diff --git a/include/linux/genhd.h b/include/linux/genhd.h
> index 06c0fd594097..69db1affedb0 100644
> --- a/include/linux/genhd.h
> +++ b/include/linux/genhd.h
> @@ -610,6 +610,7 @@ struct unixware_disklabel {
>   
>   extern int blk_alloc_devt(struct hd_struct *part, dev_t *devt);
>   extern void blk_free_devt(dev_t devt);
> +extern void blk_invalidate_devt(dev_t devt);
>   extern dev_t blk_lookup_devt(const char *name, int partno);
>   extern char *disk_name (struct gendisk *hd, int partno, char *buf);
>   



^ permalink raw reply	[flat|nested] 4+ messages in thread
* [PATCH v2] block: fix use-after-free on gendisk
@ 2019-04-01  9:34 Yufen Yu
  2019-04-01  9:32 ` yuyufen
  0 siblings, 1 reply; 4+ messages in thread
From: Yufen Yu @ 2019-04-01  9:34 UTC (permalink / raw)
  To: axboe, linux-block

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@zeniv.linux.org.uk>
Cc: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Jan Kara <jack@suse.cz>
Cc: Keith Busch <keith.busch@intel.com>
Signed-off-by: Yufen Yu <yuyufen@huawei.com>
---
 block/genhd.c             | 17 +++++++++++++++++
 block/partition-generic.c |  6 ++++++
 include/linux/genhd.h     |  1 +
 3 files changed, 24 insertions(+)

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));
 }
 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);
 }
 
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index 06c0fd594097..69db1affedb0 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -610,6 +610,7 @@ struct unixware_disklabel {
 
 extern int blk_alloc_devt(struct hd_struct *part, dev_t *devt);
 extern void blk_free_devt(dev_t devt);
+extern void blk_invalidate_devt(dev_t devt);
 extern dev_t blk_lookup_devt(const char *name, int partno);
 extern char *disk_name (struct gendisk *hd, int partno, char *buf);
 
-- 
2.16.2.dirty


^ permalink raw reply related	[flat|nested] 4+ messages in thread
* Re: [PATCH v2] block: fix use-after-free on gendisk
  2019-04-01  9:32 ` yuyufen
@ 2019-04-01 10:12   ` Jan Kara
  2019-04-02 11:07     ` yuyufen
  0 siblings, 1 reply; 4+ messages in thread
From: Jan Kara @ 2019-04-01 10:12 UTC (permalink / raw)
  To: yuyufen; +Cc: axboe, linux-block, viro, bart.vanassche, Jan Kara, Keith Busch

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@zeniv.linux.org.uk>
> > Cc: Bart Van Assche <bart.vanassche@wdc.com>
> > Cc: Jan Kara <jack@suse.cz>
> > Cc: Keith Busch <keith.busch@intel.com>
> > Signed-off-by: Yufen Yu <yuyufen@huawei.com>

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@suse.com>
SUSE Labs, CR

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH v2] block: fix use-after-free on gendisk
  2019-04-01 10:12   ` Jan Kara
@ 2019-04-02 11:07     ` yuyufen
  0 siblings, 0 replies; 4+ messages in thread
From: yuyufen @ 2019-04-02 11:07 UTC (permalink / raw)
  To: Jan Kara; +Cc: axboe, linux-block, viro, bart.vanassche, Keith Busch



On 2019/4/1 18:12, Jan Kara wrote:
> 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@zeniv.linux.org.uk>
>>> Cc: Bart Van Assche <bart.vanassche@wdc.com>
>>> Cc: Jan Kara <jack@suse.cz>
>>> Cc: Keith Busch <keith.busch@intel.com>
>>> Signed-off-by: Yufen Yu <yuyufen@huawei.com>
> 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.
> 	 */

Thanks a lot for your nice suggestion. As you say, they are just two 
different means to lookup
the gendisk. So, moving blk_invalidate_devt() up to 
blk_unregister_region() is more reasonable
and also improve the code readability.

Yufen
Thanks

>>>    }
>>>    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



^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-04-02 11:07 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-01  9:34 [PATCH v2] block: fix use-after-free on gendisk Yufen Yu
2019-04-01  9:32 ` yuyufen
2019-04-01 10:12   ` Jan Kara
2019-04-02 11:07     ` yuyufen

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.