* Re: [PATCH v3] block: fix use-after-free on gendisk
2019-04-02 12:06 [PATCH v3] block: fix use-after-free on gendisk Yufen Yu
@ 2019-04-02 15:16 ` Jan Kara
2019-04-09 14:07 ` yuyufen
2019-04-15 14:32 ` yuyufen
2019-04-15 15:04 ` Keith Busch
` (3 subsequent siblings)
4 siblings, 2 replies; 9+ messages in thread
From: Jan Kara @ 2019-04-02 15:16 UTC (permalink / raw)
To: Yufen Yu; +Cc: axboe, jack, viro, bart.vanassche, linux-block
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v3] block: fix use-after-free on gendisk
2019-04-02 15:16 ` Jan Kara
@ 2019-04-09 14:07 ` yuyufen
2019-04-15 14:32 ` yuyufen
1 sibling, 0 replies; 9+ messages in thread
From: yuyufen @ 2019-04-09 14:07 UTC (permalink / raw)
To: axboe; +Cc: Jan Kara, viro, bart.vanassche, linux-block
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
>>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v3] block: fix use-after-free on gendisk
2019-04-02 15:16 ` Jan Kara
2019-04-09 14:07 ` yuyufen
@ 2019-04-15 14:32 ` yuyufen
1 sibling, 0 replies; 9+ messages in thread
From: yuyufen @ 2019-04-15 14:32 UTC (permalink / raw)
To: axboe; +Cc: Jan Kara, viro, bart.vanassche, linux-block
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
>>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] block: fix use-after-free on gendisk
2019-04-02 12:06 [PATCH v3] block: fix use-after-free on gendisk Yufen Yu
2019-04-02 15:16 ` Jan Kara
@ 2019-04-15 15:04 ` Keith Busch
2019-04-15 15:56 ` Bart Van Assche
` (2 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: Keith Busch @ 2019-04-15 15:04 UTC (permalink / raw)
To: Yufen Yu
Cc: axboe@kernel.dk, jack@suse.cz, viro@zeniv.linux.org.uk,
bart.vanassche@wdc.com, linux-block@vger.kernel.org
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>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v3] block: fix use-after-free on gendisk
2019-04-02 12:06 [PATCH v3] block: fix use-after-free on gendisk Yufen Yu
2019-04-02 15:16 ` Jan Kara
2019-04-15 15:04 ` Keith Busch
@ 2019-04-15 15:56 ` Bart Van Assche
2019-04-15 16:01 ` Jan Kara
2019-04-15 19:30 ` Bart Van Assche
2019-04-15 21:36 ` Jens Axboe
4 siblings, 1 reply; 9+ messages in thread
From: Bart Van Assche @ 2019-04-15 15:56 UTC (permalink / raw)
To: Yufen Yu, axboe, jack; +Cc: viro, bart.vanassche, linux-block
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.
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v3] block: fix use-after-free on gendisk
2019-04-15 15:56 ` Bart Van Assche
@ 2019-04-15 16:01 ` Jan Kara
0 siblings, 0 replies; 9+ messages in thread
From: Jan Kara @ 2019-04-15 16:01 UTC (permalink / raw)
To: Bart Van Assche; +Cc: Yufen Yu, axboe, jack, viro, bart.vanassche, linux-block
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] block: fix use-after-free on gendisk
2019-04-02 12:06 [PATCH v3] block: fix use-after-free on gendisk Yufen Yu
` (2 preceding siblings ...)
2019-04-15 15:56 ` Bart Van Assche
@ 2019-04-15 19:30 ` Bart Van Assche
2019-04-15 21:36 ` Jens Axboe
4 siblings, 0 replies; 9+ messages in thread
From: Bart Van Assche @ 2019-04-15 19:30 UTC (permalink / raw)
To: Yufen Yu, axboe, jack; +Cc: viro, bart.vanassche, linux-block
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>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v3] block: fix use-after-free on gendisk
2019-04-02 12:06 [PATCH v3] block: fix use-after-free on gendisk Yufen Yu
` (3 preceding siblings ...)
2019-04-15 19:30 ` Bart Van Assche
@ 2019-04-15 21:36 ` Jens Axboe
4 siblings, 0 replies; 9+ messages in thread
From: Jens Axboe @ 2019-04-15 21:36 UTC (permalink / raw)
To: Yufen Yu, jack; +Cc: viro, bart.vanassche, linux-block
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
^ permalink raw reply [flat|nested] 9+ messages in thread