[PATCH v3] block: fix use-after-free on gendisk

[PATCH v3] block: fix use-after-free on gendisk

[Yufen Yu]

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.

Thanks to Jan Kara for providing the solution and more clear comments
for the code.

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: Keith Busch <keith.busch@intel.com>
Suggested-by: Jan Kara <jack@suse.cz>
Signed-off-by: Yufen Yu <yuyufen@huawei.com>
---
 block/genhd.c             | 19 +++++++++++++++++++
 block/partition-generic.c |  7 +++++++
 include/linux/genhd.h     |  1 +
 3 files changed, 27 insertions(+)

diff --git a/block/genhd.c b/block/genhd.c
index 961b2bc4634f..a4ef0068dbb2 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) {
@@ -791,6 +803,13 @@ void del_gendisk(struct gendisk *disk)
 
 	if (!(disk->flags & GENHD_FL_HIDDEN))
 		blk_unregister_region(disk_devt(disk), disk->minors);
+	/*
+	 * 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.
+	 */
+	blk_invalidate_devt(disk_devt(disk));
 
 	kobject_put(disk->part0.holder_dir);
 	kobject_put(disk->slave_dir);
diff --git a/block/partition-generic.c b/block/partition-generic.c
index 1ee3e1d1bc2a..7cf769103a25 100644
--- a/block/partition-generic.c
+++ b/block/partition-generic.c
@@ -288,6 +288,13 @@ void delete_partition(struct gendisk *disk, int partno)
 	kobject_put(part->holder_dir);
 	device_del(part_to_dev(part));
 
+	/*
+	 * 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.
+	 */
+	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

Re: [PATCH v3] block: fix use-after-free on gendisk

[Jan Kara]

On Tue 02-04-19 20:06: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.
> 
> Thanks to Jan Kara for providing the solution and more clear comments
> for the code.
> 
> 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: Keith Busch <keith.busch@intel.com>
> Suggested-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Yufen Yu <yuyufen@huawei.com>

Thanks. The patch looks good to me. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  block/genhd.c             | 19 +++++++++++++++++++
>  block/partition-generic.c |  7 +++++++
>  include/linux/genhd.h     |  1 +
>  3 files changed, 27 insertions(+)
> 
> diff --git a/block/genhd.c b/block/genhd.c
> index 961b2bc4634f..a4ef0068dbb2 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) {
> @@ -791,6 +803,13 @@ void del_gendisk(struct gendisk *disk)
>  
>  	if (!(disk->flags & GENHD_FL_HIDDEN))
>  		blk_unregister_region(disk_devt(disk), disk->minors);
> +	/*
> +	 * 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.
> +	 */
> +	blk_invalidate_devt(disk_devt(disk));
>  
>  	kobject_put(disk->part0.holder_dir);
>  	kobject_put(disk->slave_dir);
> diff --git a/block/partition-generic.c b/block/partition-generic.c
> index 1ee3e1d1bc2a..7cf769103a25 100644
> --- a/block/partition-generic.c
> +++ b/block/partition-generic.c
> @@ -288,6 +288,13 @@ void delete_partition(struct gendisk *disk, int partno)
>  	kobject_put(part->holder_dir);
>  	device_del(part_to_dev(part));
>  
> +	/*
> +	 * 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.
> +	 */
> +	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
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v3] block: fix use-after-free on gendisk

[yuyufen]

ping ?


On 2019/4/2 23:16, Jan Kara wrote:
> On Tue 02-04-19 20:06: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.
>>
>> Thanks to Jan Kara for providing the solution and more clear comments
>> for the code.
>>
>> 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: Keith Busch <keith.busch@intel.com>
>> Suggested-by: Jan Kara <jack@suse.cz>
>> Signed-off-by: Yufen Yu <yuyufen@huawei.com>
> Thanks. The patch looks good to me. You can add:
>
> Reviewed-by: Jan Kara <jack@suse.cz>
>
> 								Honza
>
>> ---
>>   block/genhd.c             | 19 +++++++++++++++++++
>>   block/partition-generic.c |  7 +++++++
>>   include/linux/genhd.h     |  1 +
>>   3 files changed, 27 insertions(+)
>>
>> diff --git a/block/genhd.c b/block/genhd.c
>> index 961b2bc4634f..a4ef0068dbb2 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) {
>> @@ -791,6 +803,13 @@ void del_gendisk(struct gendisk *disk)
>>   
>>   	if (!(disk->flags & GENHD_FL_HIDDEN))
>>   		blk_unregister_region(disk_devt(disk), disk->minors);
>> +	/*
>> +	 * 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.
>> +	 */
>> +	blk_invalidate_devt(disk_devt(disk));
>>   
>>   	kobject_put(disk->part0.holder_dir);
>>   	kobject_put(disk->slave_dir);
>> diff --git a/block/partition-generic.c b/block/partition-generic.c
>> index 1ee3e1d1bc2a..7cf769103a25 100644
>> --- a/block/partition-generic.c
>> +++ b/block/partition-generic.c
>> @@ -288,6 +288,13 @@ void delete_partition(struct gendisk *disk, int partno)
>>   	kobject_put(part->holder_dir);
>>   	device_del(part_to_dev(part));
>>   
>> +	/*
>> +	 * 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.
>> +	 */
>> +	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
>>

Re: [PATCH v3] block: fix use-after-free on gendisk

[yuyufen]

ping again...


On 2019/4/2 23:16, Jan Kara wrote:
> On Tue 02-04-19 20:06: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.
>>
>> Thanks to Jan Kara for providing the solution and more clear comments
>> for the code.
>>
>> 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: Keith Busch <keith.busch@intel.com>
>> Suggested-by: Jan Kara <jack@suse.cz>
>> Signed-off-by: Yufen Yu <yuyufen@huawei.com>
> Thanks. The patch looks good to me. You can add:
>
> Reviewed-by: Jan Kara <jack@suse.cz>
>
> 								Honza
>
>> ---
>>   block/genhd.c             | 19 +++++++++++++++++++
>>   block/partition-generic.c |  7 +++++++
>>   include/linux/genhd.h     |  1 +
>>   3 files changed, 27 insertions(+)
>>
>> diff --git a/block/genhd.c b/block/genhd.c
>> index 961b2bc4634f..a4ef0068dbb2 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) {
>> @@ -791,6 +803,13 @@ void del_gendisk(struct gendisk *disk)
>>   
>>   	if (!(disk->flags & GENHD_FL_HIDDEN))
>>   		blk_unregister_region(disk_devt(disk), disk->minors);
>> +	/*
>> +	 * 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.
>> +	 */
>> +	blk_invalidate_devt(disk_devt(disk));
>>   
>>   	kobject_put(disk->part0.holder_dir);
>>   	kobject_put(disk->slave_dir);
>> diff --git a/block/partition-generic.c b/block/partition-generic.c
>> index 1ee3e1d1bc2a..7cf769103a25 100644
>> --- a/block/partition-generic.c
>> +++ b/block/partition-generic.c
>> @@ -288,6 +288,13 @@ void delete_partition(struct gendisk *disk, int partno)
>>   	kobject_put(part->holder_dir);
>>   	device_del(part_to_dev(part));
>>   
>> +	/*
>> +	 * 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.
>> +	 */
>> +	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
>>

Re: [PATCH v3] block: fix use-after-free on gendisk

[Keith Busch]

On Tue, Apr 02, 2019 at 05:06:34AM -0700, 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.
> 
> Thanks to Jan Kara for providing the solution and more clear comments
> for the code.
> 
> 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: Keith Busch <keith.busch@intel.com>
> Suggested-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Yufen Yu <yuyufen@huawei.com>

Looks good to me.

Reviewed-by: Keith Busch <keith.busch@intel.com>

Re: [PATCH v3] block: fix use-after-free on gendisk

[Bart Van Assche]

On Tue, 2019-04-02 at 20:06 +0800, Yufen Yu wrote:
> diff --git a/block/genhd.c b/block/genhd.c
> index 961b2bc4634f..a4ef0068dbb2 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);
> +       }
> +}

Did you perhaps copy the above code from blk_free_devt()? If so, please modify
blk_free_devt() such that it calls blk_invalidate_devt() instead of introducing a
copy of most of blk_free_devt().

Thanks,

Bart.

Re: [PATCH v3] block: fix use-after-free on gendisk

[Jan Kara]

On Mon 15-04-19 08:56:35, Bart Van Assche wrote:
> On Tue, 2019-04-02 at 20:06 +0800, Yufen Yu wrote:
> > diff --git a/block/genhd.c b/block/genhd.c
> > index 961b2bc4634f..a4ef0068dbb2 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);
> > +       }
> > +}
> 
> Did you perhaps copy the above code from blk_free_devt()? If so, please
> modify blk_free_devt() such that it calls blk_invalidate_devt() instead
> of introducing a copy of most of blk_free_devt().

I guess you've misread the patch. blk_free_devt() does idr_remove(). Here
we do idr_replace(). Subtle difference but an important one!

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v3] block: fix use-after-free on gendisk

[Bart Van Assche]

On Tue, 2019-04-02 at 20:06 +0800, 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.
> 
> Thanks to Jan Kara for providing the solution and more clear comments
> for the code.

Nice work.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH v3] block: fix use-after-free on gendisk

[Jens Axboe]

On 4/2/19 6:06 AM, 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.
> 
> Thanks to Jan Kara for providing the solution and more clear comments
> for the code.

Applied, thanks.

-- 
Jens Axboe