[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <dadef634-9ec3-aa7b-f1c0-65a98a0fafb1@huaweicloud.com>
Date: Mon, 17 Oct 2022 17:24:20 +0800
From: Yu Kuai <yukuai1@...weicloud.com>
To: Christoph Hellwig <hch@...radead.org>,
Yu Kuai <yukuai1@...weicloud.com>
Cc: axboe@...nel.dk, gregkh@...uxfoundation.org, willy@...radead.org,
kch@...dia.com, martin.petersen@...cle.com,
johannes.thumshirn@....com, linux-block@...r.kernel.org,
linux-kernel@...r.kernel.org, yi.zhang@...wei.com,
"yukuai (C)" <yukuai3@...wei.com>
Subject: Re: [PATCH RFC] block: fix use after free for bd_holder_dir/slave_dir
Hi, christoph!
在 2022/10/17 15:59, Christoph Hellwig 写道:
> AFAICS the problem is the that pre-registered holders don't get
> unregistered for a late add_disk failure.
>
> Something like this should fix your error:
>
I agree that this patch do make sense, however, it seems to me this
patch should fix the problem that kobject is leaked, not uaf... And I
verified that the problem can still be reporduced with this patch.
Some details about our error:
while creating dm using sda, sda is removed. Thus device_add_disk() will
fail at bd_register_pending_holders() and and slave_dir is removed.
Then, later in dm error path, slave_dir is accessed again.
Perhaps refactor dm error path can fix this error, however, I still
think the root cause is lifecycle of slave_dir/bd_holder_dir, and there
are still probably other potential problems...
Thanks,
Kuai
> diff --git a/block/genhd.c b/block/genhd.c
> index 17b33c62423df..6123005154b2a 100644
> --- a/block/genhd.c
> +++ b/block/genhd.c
> @@ -484,7 +484,7 @@ int __must_check device_add_disk(struct device *parent, struct gendisk *disk,
>
> ret = blk_register_queue(disk);
> if (ret)
> - goto out_put_slave_dir;
> + goto out_unregister_holders;
>
> if (!(disk->flags & GENHD_FL_HIDDEN)) {
> ret = bdi_register(disk->bdi, "%u:%u",
> @@ -526,6 +526,8 @@ int __must_check device_add_disk(struct device *parent, struct gendisk *disk,
> bdi_unregister(disk->bdi);
> out_unregister_queue:
> blk_unregister_queue(disk);
> +out_unregister_holders:
> + bd_unregister_all_holders(disk);
> out_put_slave_dir:
> kobject_put(disk->slave_dir);
> out_put_holder_dir:
> diff --git a/block/holder.c b/block/holder.c
> index 5283bc804cc14..12c09d5c21280 100644
> --- a/block/holder.c
> +++ b/block/holder.c
> @@ -169,3 +169,13 @@ int bd_register_pending_holders(struct gendisk *disk)
> mutex_unlock(&disk->open_mutex);
> return ret;
> }
> +
> +void bd_unregister_all_holders(struct gendisk *disk)
> +{
> + struct bd_holder_disk *holder;
> +
> + mutex_lock(&disk->open_mutex);
> + list_for_each_entry(holder, &disk->slave_bdevs, list)
> + __unlink_disk_holder(holder->bdev, disk);
> + mutex_unlock(&disk->open_mutex);
> +}
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index 50e358a19d986..ccab9a2dae4bd 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -840,6 +840,7 @@ void set_capacity(struct gendisk *disk, sector_t size);
> int bd_link_disk_holder(struct block_device *bdev, struct gendisk *disk);
> void bd_unlink_disk_holder(struct block_device *bdev, struct gendisk *disk);
> int bd_register_pending_holders(struct gendisk *disk);
> +void bd_unregister_all_holders(struct gendisk *disk);
> #else
> static inline int bd_link_disk_holder(struct block_device *bdev,
> struct gendisk *disk)
> @@ -854,6 +855,9 @@ static inline int bd_register_pending_holders(struct gendisk *disk)
> {
> return 0;
> }
> +static inline void bd_unregister_all_holders(struct gendisk *disk)
> +{
> +}
> #endif /* CONFIG_BLOCK_HOLDER_DEPRECATED */
>
> dev_t part_devt(struct gendisk *disk, u8 partno);
> .
>
Powered by blists - more mailing lists