diff mbox series

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

Message ID 20221018131432.434167-2-yukuai3@huawei.com
State New
Headers
Series block: fix uaf in bd_link_disk_holder() |

Commit Message

Yu Kuai Oct. 18, 2022, 1:14 p.m. UTC 
  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(-)

Comments

Greg KH Oct. 18, 2022, 1 p.m. UTC | #1 
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
Yu Kuai Oct. 18, 2022, 1:12 p.m. UTC | #2 
在 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
> .
>
Greg KH Oct. 18, 2022, 6:18 p.m. UTC | #3 
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
diff mbox series

Patch

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