[2/2] block: fix use after free for bd_holder_dir
Commit Message
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().
Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
block/holder.c | 23 ++++++++++++++++-------
1 file changed, 16 insertions(+), 7 deletions(-)
Comments
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;
> 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.
> 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.
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.
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
>
> .
>
On Thu, Nov 03, 2022 at 05:45:25PM +0800, Yu Kuai wrote:
> This lock is different from current 'out_unlock', add a new lable will
> make the code more complex, I think.
Of course, same mistake as last time..
@@ -78,19 +78,32 @@ int bd_link_disk_holder(struct block_device *bdev, struct gendisk *disk)
if (bdev->bd_disk == disk)
return -EINVAL;
- mutex_lock(&disk->open_mutex);
+ /*
+ * 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.
+ */
+ mutex_lock(&bdev->bd_disk->open_mutex);
+ if (!disk_live(bdev->bd_disk)) {
+ mutex_unlock(&bdev->bd_disk->open_mutex);
+ return -ENODEV;
+ }
- WARN_ON_ONCE(!bdev->bd_holder);
+ kobject_get(bdev->bd_holder_dir);
+ mutex_unlock(&bdev->bd_disk->open_mutex);
+ mutex_lock(&disk->open_mutex);
+ WARN_ON_ONCE(!bdev->bd_holder);
holder = bd_find_holder_disk(bdev, disk);
if (holder) {
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);
goto out_unlock;
}
}
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);
out_unlock:
mutex_unlock(&disk->open_mutex);