[PATCH 0/2] fix use-after-free of gendisk

[PATCH 0/2] fix use-after-free of gendisk

[Yufen Yu]

Patch 1: fix use after free in get_gendisk, when open device partition.
Patch 2: remove unneccessary if statement

Yufen Yu (2):
  block: remove devt from ext_devt_idr when delete partition
  block: remove unnecessary statement in blk_free_devt

 block/genhd.c             | 3 ---
 block/partition-generic.c | 5 +++++
 2 files changed, 5 insertions(+), 3 deletions(-)

-- 
2.16.2.dirty

[PATCH 1/2] block: remove devt from ext_devt_idr when delete partition

[Yufen Yu]

In part_release(), it will remove devt from ext_devt_idr and
get_gendisk cannot find it. But, if disk_release() works before
part_release, open device partition may cause use-after-free of
disk in get_gendisk(). We use md device as example, the race sence:

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 Woker thread removes part from ext_devt_idr, Process2 can find
the part and access the disk, resulting use-after-free.

We fix this by removing the devt from ext_devt_idr when delete partition.

Signed-off-by: Yufen Yu <yuyufen@huawei.com>
---
 block/partition-generic.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/block/partition-generic.c b/block/partition-generic.c
index 1ee3e1d1bc2a..30d1039d5e8d 100644
--- a/block/partition-generic.c
+++ b/block/partition-generic.c
@@ -288,6 +288,11 @@ void delete_partition(struct gendisk *disk, int partno)
 	kobject_put(part->holder_dir);
 	device_del(part_to_dev(part));
 
+	/*
+	 * We should ensuere to delete part from idr before kfree(disk),
+	 * avoiding use-after-free of disk.
+	 */
+	blk_free_devt(part_devt(part));
 	hd_struct_kill(part);
 }
 
-- 
2.16.2.dirty

[PATCH 2/2] block: remove unnecessary statement in blk_free_devt

[Yufen Yu]

If devt is MKDEV(0, 0), its MAJOR cannot be BLOCK_EXT_MAJOR,
so, I think we can remove the statement.

Signed-off-by: Yufen Yu <yuyufen@huawei.com>
---
 block/genhd.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/block/genhd.c b/block/genhd.c
index f00198e6758a..961b2bc4634f 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -522,9 +522,6 @@ int blk_alloc_devt(struct hd_struct *part, dev_t *devt)
  */
 void blk_free_devt(dev_t devt)
 {
-	if (devt == MKDEV(0, 0))
-		return;
-
 	if (MAJOR(devt) == BLOCK_EXT_MAJOR) {
 		spin_lock_bh(&ext_devt_lock);
 		idr_remove(&ext_devt_idr, blk_mangle_minor(MINOR(devt)));
-- 
2.16.2.dirty

Re: [PATCH 1/2] block: remove devt from ext_devt_idr when delete partition

[yuyufen]

This patch fix use-after-free on gendisk when open the disk partition.

Ping and Cc

Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Jan Kara <jack@suse.cz>

Yufen
Thanks

On 2019/3/18 22:07, Yufen Yu wrote:
> In part_release(), it will remove devt from ext_devt_idr and
> get_gendisk cannot find it. But, if disk_release() works before
> part_release, open device partition may cause use-after-free of
> disk in get_gendisk(). We use md device as example, the race sence:
>
> 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 Woker thread removes part from ext_devt_idr, Process2 can find
> the part and access the disk, resulting use-after-free.
>
> We fix this by removing the devt from ext_devt_idr when delete partition.
>
> Signed-off-by: Yufen Yu <yuyufen@huawei.com>
> ---
>   block/partition-generic.c | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/block/partition-generic.c b/block/partition-generic.c
> index 1ee3e1d1bc2a..30d1039d5e8d 100644
> --- a/block/partition-generic.c
> +++ b/block/partition-generic.c
> @@ -288,6 +288,11 @@ void delete_partition(struct gendisk *disk, int partno)
>   	kobject_put(part->holder_dir);
>   	device_del(part_to_dev(part));
>   
> +	/*
> +	 * We should ensuere to delete part from idr before kfree(disk),
> +	 * avoiding use-after-free of disk.
> +	 */
> +	blk_free_devt(part_devt(part));
>   	hd_struct_kill(part);
>   }
>   

Re: [PATCH 1/2] block: remove devt from ext_devt_idr when delete partition

[Jan Kara]

On Sat 23-03-19 20:51:45, yuyufen wrote:
> This patch fix use-after-free on gendisk when open the disk partition.
> 
> Ping and Cc
> 
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Bart Van Assche <bart.vanassche@wdc.com>
> Cc: Jan Kara <jack@suse.cz>
> 
> Yufen
> Thanks
> 
> On 2019/3/18 22:07, Yufen Yu wrote:
> > In part_release(), it will remove devt from ext_devt_idr and
> > get_gendisk cannot find it. But, if disk_release() works before
> > part_release, open device partition may cause use-after-free of
> > disk in get_gendisk(). We use md device as example, the race sence:
> > 
> > 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 Woker thread removes part from ext_devt_idr, Process2 can find
> > the part and access the disk, resulting use-after-free.
> > 
> > We fix this by removing the devt from ext_devt_idr when delete partition.
> > 
> > Signed-off-by: Yufen Yu <yuyufen@huawei.com>

Thanks for your analysis and the patch! I agree with our analysis of the
problem but I'm afraid the fix won't be so straightforward. The problem is
that once you remove device number from idr, it can get reallocated for
another device. So if nothing else, the idr_remove() call you've left in
part_release() could delete *newly created device* from idr which is not
what you want. And even if this is fixed, there are other possible issues
with reallocating device number before the device is fully shutdown -
commit 2da78092dda "block: Fix dev_t minor allocation lifetime"
specifically moved blk_free_devt(dev->devt) call to part_release() to avoid
such issues (sadly without explaining more details). Adding Keith to CC
just in case he remembers.

Anyway, you are right that it is fundamentally wrong to expose a structure
(hd_struct in our case) for lookup in idr when it is essentially scheduled
for RCU-delayed freeing. So what we can do is to replace hd_struct pointer
in idr with some placeholder value - NULL seems to be OK with idr - and
delete the entry from idr in part_release() as we do now. We can call this
new helper blk_invalidate_devt() or something like that.

								Honza


> > ---
> >   block/partition-generic.c | 5 +++++
> >   1 file changed, 5 insertions(+)
> > 
> > diff --git a/block/partition-generic.c b/block/partition-generic.c
> > index 1ee3e1d1bc2a..30d1039d5e8d 100644
> > --- a/block/partition-generic.c
> > +++ b/block/partition-generic.c
> > @@ -288,6 +288,11 @@ void delete_partition(struct gendisk *disk, int partno)
> >   	kobject_put(part->holder_dir);
> >   	device_del(part_to_dev(part));
> > +	/*
> > +	 * We should ensuere to delete part from idr before kfree(disk),
> > +	 * avoiding use-after-free of disk.
> > +	 */
> > +	blk_free_devt(part_devt(part));
> >   	hd_struct_kill(part);
> >   }
> 
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 1/2] block: remove devt from ext_devt_idr when delete partition

[yuyufen]

Hi


On 2019/3/30 6:42, Jan Kara wrote:
> On Sat 23-03-19 20:51:45, yuyufen wrote:
>> This patch fix use-after-free on gendisk when open the disk partition.
>>
>> Ping and Cc
>>
>> Cc: Al Viro <viro@zeniv.linux.org.uk>
>> Cc: Bart Van Assche <bart.vanassche@wdc.com>
>> Cc: Jan Kara <jack@suse.cz>
>>
>> Yufen
>> Thanks
>>
>> On 2019/3/18 22:07, Yufen Yu wrote:
>>> In part_release(), it will remove devt from ext_devt_idr and
>>> get_gendisk cannot find it. But, if disk_release() works before
>>> part_release, open device partition may cause use-after-free of
>>> disk in get_gendisk(). We use md device as example, the race sence:
>>>
>>> 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 Woker thread removes part from ext_devt_idr, Process2 can find
>>> the part and access the disk, resulting use-after-free.
>>>
>>> We fix this by removing the devt from ext_devt_idr when delete partition.
>>>
>>> Signed-off-by: Yufen Yu <yuyufen@huawei.com>
> Thanks for your analysis and the patch! I agree with our analysis of the
> problem but I'm afraid the fix won't be so straightforward. The problem is
> that once you remove device number from idr, it can get reallocated for
> another device. So if nothing else, the idr_remove() call you've left in
> part_release() could delete *newly created device* from idr which is not
> what you want. And even if this is fixed, there are other possible issues
> with reallocating device number before the device is fully shutdown -
> commit 2da78092dda "block: Fix dev_t minor allocation lifetime"
> specifically moved blk_free_devt(dev->devt) call to part_release() to avoid
> such issues (sadly without explaining more details). Adding Keith to CC
> just in case he remembers.
>
> Anyway, you are right that it is fundamentally wrong to expose a structure
> (hd_struct in our case) for lookup in idr when it is essentially scheduled
> for RCU-delayed freeing. So what we can do is to replace hd_struct pointer
> in idr with some placeholder value - NULL seems to be OK with idr - and
> delete the entry from idr in part_release() as we do now. We can call this
> new helper blk_invalidate_devt() or something like that.
>
> 								Honza
>

Thanks a lot for your detailed explanation:-). And replacing hd_struct 
pointer by
NULL in idr is a good idea. I will send v2 for this problem.

Yufen
Thanks

>>> ---
>>>    block/partition-generic.c | 5 +++++
>>>    1 file changed, 5 insertions(+)
>>>
>>> diff --git a/block/partition-generic.c b/block/partition-generic.c
>>> index 1ee3e1d1bc2a..30d1039d5e8d 100644
>>> --- a/block/partition-generic.c
>>> +++ b/block/partition-generic.c
>>> @@ -288,6 +288,11 @@ void delete_partition(struct gendisk *disk, int partno)
>>>    	kobject_put(part->holder_dir);
>>>    	device_del(part_to_dev(part));
>>> +	/*
>>> +	 * We should ensuere to delete part from idr before kfree(disk),
>>> +	 * avoiding use-after-free of disk.
>>> +	 */
>>> +	blk_free_devt(part_devt(part));
>>>    	hd_struct_kill(part);
>>>    }
>>