[PATCH -nect RFC v2 0/2] block: fix uaf in bd_link_disk_holder()

[PATCH -nect RFC v2 0/2] block: fix uaf in bd_link_disk_holder()

[Yu Kuai]

Changes in v2:
 - in order to know when bd_holder_dir will be freed, instead of changing
kobject_put(), add a new field in block_device.

Yu Kuai (2):
  block: add helpers for bd_holder_dir refcount management
  block: fix uaf for bd_holder_dir

 block/blk.h               |  3 +++
 block/genhd.c             | 33 ++++++++++++++++++++++++++++++++-
 block/holder.c            | 19 +++++++++++++------
 block/partitions/core.c   |  5 ++++-
 include/linux/blk_types.h |  1 +
 5 files changed, 53 insertions(+), 8 deletions(-)

-- 
2.31.1

[PATCH -nect RFC v2 1/2] block: add helpers for bd_holder_dir refcount management

[Yu Kuai]

Currently the lifecycle of bd_holder_dir is problematic, it can be freed
in del_gendisk() while it can still be accessed before disk_release().
And there in no way to know if the kobject if freed by kobject_put() for
now, thus add a new field in bloce_device to manage refcount of
bd_holder_dir seperately.

Prepare to fix the problem in following patch.

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 block/blk.h               |  3 +++
 block/genhd.c             | 27 +++++++++++++++++++++++++++
 include/linux/blk_types.h |  1 +
 3 files changed, 31 insertions(+)

diff --git a/block/blk.h b/block/blk.h
index d6ea0d1a6db0..fef868b4169c 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -458,6 +458,9 @@ extern const struct address_space_operations def_blk_aops;
 int disk_register_independent_access_ranges(struct gendisk *disk);
 void disk_unregister_independent_access_ranges(struct gendisk *disk);
 
+bool bd_try_get_holder_dir(struct block_device *bdev);
+void bd_put_holder_dir(struct block_device *bdev);
+
 #ifdef CONFIG_FAIL_MAKE_REQUEST
 bool should_fail_request(struct block_device *part, unsigned int bytes);
 #else /* CONFIG_FAIL_MAKE_REQUEST */
diff --git a/block/genhd.c b/block/genhd.c
index 17b33c62423d..53f9c8b2690a 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -375,6 +375,33 @@ int disk_scan_partitions(struct gendisk *disk, fmode_t mode)
 	return 0;
 }
 
+bool bd_try_get_holder_dir(struct block_device *bdev)
+{
+	mutex_lock(&bdev->bd_disk->open_mutex);
+	if (!bdev->holder_dir_ref) {
+		mutex_unlock(&bdev->bd_disk->open_mutex);
+		return false;
+	}
+
+	bdev->holder_dir_ref++;
+	mutex_unlock(&bdev->bd_disk->open_mutex);
+	return true;
+}
+
+void bd_put_holder_dir(struct block_device *bdev)
+{
+	mutex_lock(&bdev->bd_disk->open_mutex);
+	if (WARN_ON(bdev->holder_dir_ref <= 0))
+		goto out;
+
+	if (!(--bdev->holder_dir_ref)) {
+		kobject_put(bdev->bd_holder_dir);
+		bdev->bd_holder_dir = NULL;
+	}
+out:
+	mutex_unlock(&bdev->bd_disk->open_mutex);
+}
+
 /**
  * device_add_disk - add disk information to kernel list
  * @parent: parent device for the disk
diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index e0b098089ef2..550c8e15af49 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -53,6 +53,7 @@ struct block_device {
 	int			bd_holders;
 	bool			bd_write_holder;
 	struct kobject		*bd_holder_dir;
+	int			holder_dir_ref;
 	u8			bd_partno;
 	spinlock_t		bd_size_lock; /* for bd_inode->i_size updates */
 	struct gendisk *	bd_disk;
-- 
2.31.1

[PATCH -nect RFC v2 2/2] block: fix uaf for bd_holder_dir

[Yu Kuai]

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@huawei.com>
---
 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

Re: [PATCH -nect RFC v2 0/2] block: fix uaf in bd_link_disk_holder()

[Christoph Hellwig]

As mentioned before I don't think we should make this even more
crufty in the block layer.  See the series I just sent to move it int
dm.  

Re: [PATCH -nect RFC v2 0/2] block: fix uaf in bd_link_disk_holder()

[Yu Kuai]

Hi,

在 2022/10/21 0:47, Christoph Hellwig 写道:
> As mentioned before I don't think we should make this even more
> crufty in the block layer.  See the series I just sent to move it int
> dm.

It seems we had some misunderstanding, the problem I tried to fix here
should not just related to dm, but all the caller of
bd_link_disk_holder().

Thanks,
Kuai
> 
> .
> 

Re: [PATCH -nect RFC v2 0/2] block: fix uaf in bd_link_disk_holder()

[Yu Kuai]

Hi, Christoph

在 2022/10/21 11:15, Yu Kuai 写道:
> Hi,
> 
> 在 2022/10/21 0:47, Christoph Hellwig 写道:
>> As mentioned before I don't think we should make this even more
>> crufty in the block layer.  See the series I just sent to move it int
>> dm.
> 
> It seems we had some misunderstanding, the problem I tried to fix here
> should not just related to dm, but all the caller of
> bd_link_disk_holder().

Any suggestions about how to fix this problem?

Thanks,
Kuai
> 
> Thanks,
> Kuai
>>
>> .
>>
> 
> .
> 

Re: [PATCH -nect RFC v2 0/2] block: fix uaf in bd_link_disk_holder()

[Christoph Hellwig]

On Fri, Oct 21, 2022 at 11:15:34AM +0800, Yu Kuai wrote:
> Hi,
>
> 在 2022/10/21 0:47, Christoph Hellwig 写道:
>> As mentioned before I don't think we should make this even more
>> crufty in the block layer.  See the series I just sent to move it int
>> dm.
>
> It seems we had some misunderstanding, the problem I tried to fix here
> should not just related to dm, but all the caller of
> bd_link_disk_holder().

As far as I can tell the problem was just that patch 1 in my series blows
away the bd_holder_dir pointer in part0 on del_gendisk.  Each holder
actually holds a reference to the kobject, so the memory for it is
still valid, it's just that the pointer got cleared.  I'll send a v2
in a bit.

Re: [PATCH -nect RFC v2 0/2] block: fix uaf in bd_link_disk_holder()

[Yu Kuai]

Hi, Christoph

在 2022/10/30 23:30, Christoph Hellwig 写道:
> On Fri, Oct 21, 2022 at 11:15:34AM +0800, Yu Kuai wrote:
>> Hi,
>>
>> 在 2022/10/21 0:47, Christoph Hellwig 写道:
>>> As mentioned before I don't think we should make this even more
>>> crufty in the block layer.  See the series I just sent to move it int
>>> dm.
>>
>> It seems we had some misunderstanding, the problem I tried to fix here
>> should not just related to dm, but all the caller of
>> bd_link_disk_holder().
> 
> As far as I can tell the problem was just that patch 1 in my series blows
> away the bd_holder_dir pointer in part0 on del_gendisk.  Each holder
> actually holds a reference to the kobject, so the memory for it is
> still valid, it's just that the pointer got cleared.  I'll send a v2
> in a bit.

This is not the real case. In bd_link_disk_hoder(), bd_hodler_dir is
accessed first by add_symlink(), and then reference is grabed later.
The reference should be grabed before bd_holder_dir is accessed, like
what I try to do in patch 2.

Thanks,
Kuai
> 
> .
>