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

[Yu Kuai <yukuai1@xxxxxxxxxxxxxxx>]

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);
> .