lkml.org 
[lkml]   [2022]   [Oct]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH RFC] block: fix use after free for bd_holder_dir/slave_dir
From
Date
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);
> .
>

\
 
 \ /
  Last update: 2022-10-17 11:25    [W:0.073 / U:0.476 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site