Re: [PATCH RFC 1/2] kobject: add return value for kobject_put()

Re: [PATCH RFC 1/2] kobject: add return value for kobject_put()

[Greg KH]

On Tue, Oct 18, 2022 at 09:14:31PM +0800, Yu Kuai wrote:
> The return value will be used in later patch to fix uaf for slave_dir
> and bd_holder_dir in block layer.

Then the user will be incorrect, this is not ok, you should never care
if you are the last "put" on an object at all.  Hint, what happens right
after you call this and get the result?

sorry, but NAK.

greg k-h

Re: [PATCH RFC 1/2] kobject: add return value for kobject_put()

[Yu Kuai]

在 2022/10/18 21:00, Greg KH 写道:
> On Tue, Oct 18, 2022 at 09:14:31PM +0800, Yu Kuai wrote:
>> The return value will be used in later patch to fix uaf for slave_dir
>> and bd_holder_dir in block layer.
> 
> Then the user will be incorrect, this is not ok, you should never care
> if you are the last "put" on an object at all.  Hint, what happens right
> after you call this and get the result?
> 

I tried to reset the pointer to NULL in patch 2 to prevent uaf. And the
whole kobject_put() and pointer reset is protected by a mutex, the mutex
will be used on the reader side before kobject_get as well. So, in fact,
I'm protecting them by the mutex...

I can bypass it by using another reference anyway. But let's see if
anyone has suggestions on the other patch.

> sorry, but NAK.

I know the best way is too refactor the lifecycle of the problematic
bd_holder_dir/slave_dir, however, I gave that up because this seems
quite complicated and influence is very huge...

Thanks,
Kuai
> 
> greg k-h
> .
> 

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

[Yu Kuai]

Yu Kuai (2):
  kobject: add return value for kobject_put()
  block: protect slave_dir/bd_holder_dir by open_mutex

 block/genhd.c           |  8 ++++++--
 block/holder.c          | 13 ++++++++++++-
 block/partitions/core.c |  5 ++++-
 include/linux/kobject.h |  2 +-
 lib/kobject.c           |  7 +++++--
 5 files changed, 28 insertions(+), 7 deletions(-)

-- 
2.31.1

[PATCH RFC 1/2] kobject: add return value for kobject_put()

[Yu Kuai]

The return value will be used in later patch to fix uaf for slave_dir
and bd_holder_dir in block layer.

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 include/linux/kobject.h | 2 +-
 lib/kobject.c           | 7 +++++--
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/include/linux/kobject.h b/include/linux/kobject.h
index 57fb972fea05..f12de6274c51 100644
--- a/include/linux/kobject.h
+++ b/include/linux/kobject.h
@@ -110,7 +110,7 @@ extern int __must_check kobject_move(struct kobject *, struct kobject *);
 extern struct kobject *kobject_get(struct kobject *kobj);
 extern struct kobject * __must_check kobject_get_unless_zero(
 						struct kobject *kobj);
-extern void kobject_put(struct kobject *kobj);
+extern bool kobject_put(struct kobject *kobj);
 
 extern const void *kobject_namespace(struct kobject *kobj);
 extern void kobject_get_ownership(struct kobject *kobj,
diff --git a/lib/kobject.c b/lib/kobject.c
index a0b2dbfcfa23..f86c55ae7376 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -711,15 +711,18 @@ static void kobject_release(struct kref *kref)
  *
  * Decrement the refcount, and if 0, call kobject_cleanup().
  */
-void kobject_put(struct kobject *kobj)
+bool kobject_put(struct kobject *kobj)
 {
 	if (kobj) {
 		if (!kobj->state_initialized)
 			WARN(1, KERN_WARNING
 				"kobject: '%s' (%p): is not initialized, yet kobject_put() is being called.\n",
 			     kobject_name(kobj), kobj);
-		kref_put(&kobj->kref, kobject_release);
+		if (kref_put(&kobj->kref, kobject_release))
+			return true;
 	}
+
+	return false;
 }
 EXPORT_SYMBOL(kobject_put);
 
-- 
2.31.1

[PATCH RFC 2/2] block: protect slave_dir/bd_holder_dir by open_mutex

[Yu Kuai]

Lifecycle of slave_dir/bd_holder_dir is problematic currently:

t1:			t2:

// get bdev of lower disk
blkdev_get_by_dev
			// remove lower disk
			del_gendisk
			 // initial reference is released, and
			 // slave_dir/bd_holder_dir can be freed
			 kobject_put
// uaf is triggered
bd_link_disk_holder

Fix the problem by protecting them by open_mutex.

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 block/genhd.c           |  8 ++++++--
 block/holder.c          | 13 ++++++++++++-
 block/partitions/core.c |  5 ++++-
 3 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/block/genhd.c b/block/genhd.c
index 17b33c62423d..d9ad889d011a 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -622,8 +622,12 @@ void del_gendisk(struct gendisk *disk)
 
 	blk_unregister_queue(disk);
 
-	kobject_put(disk->part0->bd_holder_dir);
-	kobject_put(disk->slave_dir);
+	mutex_lock(&disk->open_mutex);
+	if (kobject_put(disk->part0->bd_holder_dir))
+		disk->part0->bd_holder_dir = NULL;
+	if (kobject_put(disk->slave_dir))
+		disk->slave_dir = NULL;
+	mutex_unlock(&disk->open_mutex);
 
 	part_stat_set_all(disk->part0, 0);
 	disk->part0->bd_stamp = 0;
diff --git a/block/holder.c b/block/holder.c
index 5283bc804cc1..fdfbe82e31e3 100644
--- a/block/holder.c
+++ b/block/holder.c
@@ -75,6 +75,13 @@ int bd_link_disk_holder(struct block_device *bdev, struct gendisk *disk)
 	struct bd_holder_disk *holder;
 	int ret = 0;
 
+	mutex_lock(&bdev->bd_disk->open_mutex);
+	/* Failed if bd_holder_dir is freed by del_gendisk() */
+	if (!bdev->bd_holder_dir) {
+		mutex_unlock(&bdev->bd_disk->open_mutex);
+		return -ENODEV;
+	}
+
 	mutex_lock(&disk->open_mutex);
 
 	WARN_ON_ONCE(!bdev->bd_holder);
@@ -111,6 +118,7 @@ int bd_link_disk_holder(struct block_device *bdev, struct gendisk *disk)
 
 out_unlock:
 	mutex_unlock(&disk->open_mutex);
+	mutex_unlock(&bdev->bd_disk->open_mutex);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(bd_link_disk_holder);
@@ -136,16 +144,19 @@ void bd_unlink_disk_holder(struct block_device *bdev, struct gendisk *disk)
 {
 	struct bd_holder_disk *holder;
 
+	mutex_lock(&bdev->bd_disk->open_mutex);
 	mutex_lock(&disk->open_mutex);
 	holder = bd_find_holder_disk(bdev, disk);
 	if (!WARN_ON_ONCE(holder == NULL) && !--holder->refcnt) {
 		if (disk->slave_dir)
 			__unlink_disk_holder(bdev, disk);
-		kobject_put(bdev->bd_holder_dir);
+		if (kobject_put(bdev->bd_holder_dir))
+			bdev->bd_holder_dir = NULL;
 		list_del_init(&holder->list);
 		kfree(holder);
 	}
 	mutex_unlock(&disk->open_mutex);
+	mutex_unlock(&bdev->bd_disk->open_mutex);
 }
 EXPORT_SYMBOL_GPL(bd_unlink_disk_holder);
 
diff --git a/block/partitions/core.c b/block/partitions/core.c
index b8112f52d388..eef7b8615419 100644
--- a/block/partitions/core.c
+++ b/block/partitions/core.c
@@ -279,7 +279,10 @@ 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);
+	mutex_lock(&part->bd_disk->open_mutex);
+	if (kobject_put(part->bd_holder_dir))
+		part->bd_holder_dir = NULL;
+	mutex_unlock(&part->bd_disk->open_mutex);
 	device_del(&part->bd_device);
 
 	/*
-- 
2.31.1

Re: [PATCH RFC 1/2] kobject: add return value for kobject_put()

[Greg KH]

On Tue, Oct 18, 2022 at 09:12:08PM +0800, Yu Kuai wrote:
> 
> 
> 在 2022/10/18 21:00, Greg KH 写道:
> > On Tue, Oct 18, 2022 at 09:14:31PM +0800, Yu Kuai wrote:
> > > The return value will be used in later patch to fix uaf for slave_dir
> > > and bd_holder_dir in block layer.
> > 
> > Then the user will be incorrect, this is not ok, you should never care
> > if you are the last "put" on an object at all.  Hint, what happens right
> > after you call this and get the result?
> > 
> 
> I tried to reset the pointer to NULL in patch 2 to prevent uaf.

That is not ok, sorry.

> And the
> whole kobject_put() and pointer reset is protected by a mutex, the mutex
> will be used on the reader side before kobject_get as well. So, in fact,
> I'm protecting them by the mutex...

Still not ok.  You never know who else has a reference on a kobject,
that's the point of reference counted objects.

> I can bypass it by using another reference anyway. But let's see if
> anyone has suggestions on the other patch.
> 
> > sorry, but NAK.
> 
> I know the best way is too refactor the lifecycle of the problematic
> bd_holder_dir/slave_dir, however, I gave that up because this seems
> quite complicated and influence is very huge...

Please fix it up properly, core changes like this should not be needed.

thanks,

greg k-h