Lifecycle of bd_holder_dir is problematic: t1: t2: // get bdev of lower disk blkdev_get_by_dev // remove lower disk del_gendisk // initial reference is released, and // bd_holder_dir can be freed kobject_put // uaf is triggered bd_link_disk_holder Fix the problem by protecting bd_holder_dir by open_mutex. Signed-off-by: Yu Kuai <yukuai3@xxxxxxxxxx> --- block/genhd.c | 6 +++++- block/holder.c | 19 +++++++++++++------ block/partitions/core.c | 5 ++++- 3 files changed, 22 insertions(+), 8 deletions(-) diff --git a/block/genhd.c b/block/genhd.c index 53f9c8b2690a..b3d04e79e854 100644 --- a/block/genhd.c +++ b/block/genhd.c @@ -499,6 +499,8 @@ int __must_check device_add_disk(struct device *parent, struct gendisk *disk, ret = -ENOMEM; goto out_del_integrity; } + disk->part0->holder_dir_ref = 1; + disk->slave_dir = kobject_create_and_add("slaves", &ddev->kobj); if (!disk->slave_dir) { ret = -ENOMEM; @@ -557,6 +559,8 @@ int __must_check device_add_disk(struct device *parent, struct gendisk *disk, kobject_put(disk->slave_dir); out_put_holder_dir: kobject_put(disk->part0->bd_holder_dir); + disk->part0->bd_holder_dir = NULL; + disk->part0->holder_dir_ref = 0; out_del_integrity: blk_integrity_del(disk); out_del_block_link: @@ -649,7 +653,7 @@ void del_gendisk(struct gendisk *disk) blk_unregister_queue(disk); - kobject_put(disk->part0->bd_holder_dir); + bd_put_holder_dir(disk->part0); kobject_put(disk->slave_dir); part_stat_set_all(disk->part0, 0); diff --git a/block/holder.c b/block/holder.c index 5283bc804cc1..2e28b472ba1a 100644 --- a/block/holder.c +++ b/block/holder.c @@ -1,6 +1,7 @@ // SPDX-License-Identifier: GPL-2.0-only #include <linux/blkdev.h> #include <linux/slab.h> +#include "blk.h" struct bd_holder_disk { struct list_head list; @@ -85,9 +86,19 @@ int bd_link_disk_holder(struct block_device *bdev, struct gendisk *disk) goto out_unlock; } + /* + * 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. + */ + if (!bd_try_get_holder_dir(bdev)) { + ret = -ENODEV; + goto out_unlock; + } + holder = kzalloc(sizeof(*holder), GFP_KERNEL); if (!holder) { ret = -ENOMEM; + bd_put_holder_dir(bdev); goto out_unlock; } @@ -98,16 +109,12 @@ int bd_link_disk_holder(struct block_device *bdev, struct gendisk *disk) ret = __link_disk_holder(bdev, disk); if (ret) { kfree(holder); + bd_put_holder_dir(bdev); 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); @@ -141,7 +148,7 @@ void bd_unlink_disk_holder(struct block_device *bdev, struct gendisk *disk) if (!WARN_ON_ONCE(holder == NULL) && !--holder->refcnt) { if (disk->slave_dir) __unlink_disk_holder(bdev, disk); - kobject_put(bdev->bd_holder_dir); + bd_put_holder_dir(bdev); list_del_init(&holder->list); kfree(holder); } diff --git a/block/partitions/core.c b/block/partitions/core.c index b8112f52d388..39a14f7c308e 100644 --- a/block/partitions/core.c +++ b/block/partitions/core.c @@ -279,7 +279,7 @@ static void delete_partition(struct block_device *part) __invalidate_device(part, true); xa_erase(&part->bd_disk->part_tbl, part->bd_partno); - kobject_put(part->bd_holder_dir); + bd_put_holder_dir(part); device_del(&part->bd_device); /* @@ -390,6 +390,7 @@ static struct block_device *add_partition(struct gendisk *disk, int partno, bdev->bd_holder_dir = kobject_create_and_add("holders", &pdev->kobj); if (!bdev->bd_holder_dir) goto out_del; + bdev->holder_dir_ref = 1; dev_set_uevent_suppress(pdev, 0); if (flags & ADDPART_FLAG_WHOLEDISK) { @@ -411,6 +412,8 @@ static struct block_device *add_partition(struct gendisk *disk, int partno, out_del: kobject_put(bdev->bd_holder_dir); + bdev->bd_holder_dir = NULL; + bdev->holder_dir_ref = 0; device_del(pdev); out_put: put_device(pdev); -- 2.31.1