Re: [PATCH 2/2] block: fix use after free for bd_holder_dir

[Yu Kuai]

Hi,

在 2022/11/03 16:12, Christoph Hellwig 写道:
> On Thu, Nov 03, 2022 at 10:55:41AM +0800, Yu Kuai wrote:
> > From: Yu Kuai <yukuai3@xxxxxxxxxx>
> >
> > Currently, the caller of bd_link_disk_holer() get 'bdev' by
> > blkdev_get_by_dev(), which will look up 'bdev' by inode number 'dev'.
> > Howerver, it's possible that del_gendisk() can be called currently, and
> > 'bd_holder_dir' can be freed before bd_link_disk_holer() access it, thus
> > use after free is triggered.
> >
> > t1:				t2:
> > bdev = blkdev_get_by_dev
> > 				del_gendisk
> > 				 kobject_put(bd_holder_dir)
> > 				  kobject_free()
> > bd_link_disk_holder
> >
> > Fix the problem by checking disk is still live and grabbing a reference
> > to 'bd_holder_dir' first in bd_link_disk_holder().
>
> Looks good with some minor stilistic nipicks:
>
> > +	if (!disk_live(bdev->bd_disk)) {
> > +		mutex_unlock(&bdev->bd_disk->open_mutex);
> > +		return -ENODEV;
> > +	}
>
> This can use a goto out_unlock;

This lock is different from current 'out_unlock', add a new lable will
make the code more complex, I think.
> >   		holder->refcnt++;
> > +		kobject_put(bdev->bd_holder_dir);
> >   		goto out_unlock;
> >   	}
> >   
> >   	holder = kzalloc(sizeof(*holder), GFP_KERNEL);
> >   	if (!holder) {
> >   		ret = -ENOMEM;
> > +		kobject_put(bdev->bd_holder_dir);
> >   		goto out_unlock;
> >   	}
> >   
> > @@ -101,16 +114,12 @@ int bd_link_disk_holder(struct block_device *bdev, struct gendisk *disk)
> >   		ret = __link_disk_holder(bdev, disk);
> >   		if (ret) {
> >   			kfree(holder);
> > +			kobject_put(bdev->bd_holder_dir);
>
> And I think a goto out_put_holder and out_free_holder would clean this up
> nicely.

Yes, you're right.
> >   	list_add(&holder->list, &disk->slave_bdevs);
> > -	/*
> > -	 * del_gendisk drops the initial reference to bd_holder_dir, so we need
> > -	 * to keep our own here to allow for cleanup past that point.
> > -	 */
> > -	kobject_get(bdev->bd_holder_dir);
>
> .. with this then jumping straight to out_unlock.

Ok, I'll do that in next version.
> We should repost a series with my first 7 patches and your two.  I can do
> that, but it might take some time as I just got through (minor) knee
> surgery and am still at the hospital, so if you have spare cycles feel
> free to do it.

I'm glad to do that, and have a good rest 😄

Thanks,
Kuai
> .