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

[Jan Kara <jack@suse.cz>]

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