Messages in this thread | | | Subject | Re: [PATCH 2/2] block: fix use after free for bd_holder_dir | From | Yu Kuai <> | Date | Thu, 3 Nov 2022 17:45:25 +0800 |
| |
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@huawei.com> >> >> 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 > > . >
| |