linux-mtd.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3] kset: fix memory leak when kset_register() returns error
@ 2022-10-25  7:15 Yang Yingliang
  2022-10-25 16:51 ` Luben Tuikov
  2022-10-25 16:57 ` Greg KH
  0 siblings, 2 replies; 3+ messages in thread
From: Yang Yingliang @ 2022-10-25  7:15 UTC (permalink / raw)
  To: linux-kernel, qemu-devel, linux-f2fs-devel, linux-erofs,
	ocfs2-devel, linux-mtd, amd-gfx
  Cc: gregkh, rafael, somlo, mst, jaegeuk, chao, hsiangkao,
	huangjianan, mark, jlbec, joseph.qi, akpm, alexander.deucher,
	luben.tuikov, richard, liushixin2, yangyingliang

Inject fault while loading module, kset_register() may fail.
If it fails, the kset.kobj.name allocated by kobject_set_name()
which must be called before a call to kset_register() may be
leaked, since refcount of kobj was set in kset_init().

To mitigate this, we free the name in kset_register() when an
error is encountered, i.e. when kset_register() returns an error.

A kset may be embedded in a larger structure which may be dynamically
allocated in callers, it needs to be freed in ktype.release() or error
path in callers, in this case, we can not call kset_put() in kset_register(),
or it will cause double free, so just call kfree_const() to free the
name and set it to NULL to avoid accessing bad pointer in callers.

With this fix, the callers don't need care about freeing the name
and may call kset_put() if kset_register() fails.

Suggested-by: Luben Tuikov <luben.tuikov@amd.com>
Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
v2 -> v3:
  Update commit message and comment of kset_register().

v1 -> v2:
  Free name inside of kset_register() instead of calling kset_put()
  in drivers.
---
 lib/kobject.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/lib/kobject.c b/lib/kobject.c
index a0b2dbfcfa23..3cd19b9ca5ab 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -834,6 +834,9 @@ EXPORT_SYMBOL_GPL(kobj_sysfs_ops);
 /**
  * kset_register() - Initialize and add a kset.
  * @k: kset.
+ *
+ * NOTE: On error, the kset.kobj.name allocated by() kobj_set_name()
+ * is freed, it can not be used any more.
  */
 int kset_register(struct kset *k)
 {
@@ -844,8 +847,12 @@ int kset_register(struct kset *k)
 
 	kset_init(k);
 	err = kobject_add_internal(&k->kobj);
-	if (err)
+	if (err) {
+		kfree_const(k->kobj.name);
+		/* Set it to NULL to avoid accessing bad pointer in callers. */
+		k->kobj.name = NULL;
 		return err;
+	}
 	kobject_uevent(&k->kobj, KOBJ_ADD);
 	return 0;
 }
-- 
2.25.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH v3] kset: fix memory leak when kset_register() returns error
  2022-10-25  7:15 [PATCH v3] kset: fix memory leak when kset_register() returns error Yang Yingliang
@ 2022-10-25 16:51 ` Luben Tuikov
  2022-10-25 16:57 ` Greg KH
  1 sibling, 0 replies; 3+ messages in thread
From: Luben Tuikov @ 2022-10-25 16:51 UTC (permalink / raw)
  To: Yang Yingliang, linux-kernel, qemu-devel, linux-f2fs-devel,
	linux-erofs, ocfs2-devel, linux-mtd, amd-gfx
  Cc: gregkh, rafael, somlo, mst, jaegeuk, chao, hsiangkao,
	huangjianan, mark, jlbec, joseph.qi, akpm, alexander.deucher,
	richard, liushixin2

On 2022-10-25 03:15, Yang Yingliang wrote:
> Inject fault while loading module, kset_register() may fail.
> If it fails, the kset.kobj.name allocated by kobject_set_name()
> which must be called before a call to kset_register() may be
> leaked, since refcount of kobj was set in kset_init().

Technically, this is saying "If it fails, the kset.kobj.name may be leaked."
We want then to clarify that this is "allocated by kobj_set_name() which
must be called before a call to kset_register", so that needs to
be surrounded by commas (like a literary segue):

"If kset_register() fails, the kset.kobj.name allocated by kobject_set_name(),
 which must be called before a call to kset_register(), may be
 leaked."

I don't feel that the reason for the leak is "refcount of kobj was set in kset_init()".
It's a true statement, but not the reason for the leak--the reason for the leak is that
no one frees it on the error path.
 
> To mitigate this, we free the name in kset_register() when an
> error is encountered, i.e. when kset_register() returns an error.
> 
> A kset may be embedded in a larger structure which may be dynamically
> allocated in callers, it needs to be freed in ktype.release() or error

"_by_ callers", since it's something they _do_: allocate.

> path in callers, in this case, we can not call kset_put() in kset_register(),
> or it will cause double free, so just call kfree_const() to free the
> name and set it to NULL to avoid accessing bad pointer in callers.

That's good.

> With this fix, the callers don't need care about freeing the name
> and may call kset_put() if kset_register() fails.

"don't need to care about freeing the name" --> "don't need to free the name"

> Suggested-by: Luben Tuikov <luben.tuikov@amd.com>
> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
> ---
> v2 -> v3:
>   Update commit message and comment of kset_register().
> 
> v1 -> v2:
>   Free name inside of kset_register() instead of calling kset_put()
>   in drivers.
> ---
>  lib/kobject.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/kobject.c b/lib/kobject.c
> index a0b2dbfcfa23..3cd19b9ca5ab 100644
> --- a/lib/kobject.c
> +++ b/lib/kobject.c
> @@ -834,6 +834,9 @@ EXPORT_SYMBOL_GPL(kobj_sysfs_ops);
>  /**
>   * kset_register() - Initialize and add a kset.
>   * @k: kset.
> + *
> + * NOTE: On error, the kset.kobj.name allocated by() kobj_set_name()
> + * is freed, it can not be used any more.

There's no need for "NOTE:"--it's just natural explanation of
what's happening, and what's expected in the doc comment to a function.
Drop it.

"is freed" is correct--that's a good change.

With these fixed, this patch is
Reviewed-by: <luben.tuikov@amd.com>

Regards,
Luben

>   */
>  int kset_register(struct kset *k)
>  {
> @@ -844,8 +847,12 @@ int kset_register(struct kset *k)
>  
>  	kset_init(k);
>  	err = kobject_add_internal(&k->kobj);
> -	if (err)
> +	if (err) {
> +		kfree_const(k->kobj.name);
> +		/* Set it to NULL to avoid accessing bad pointer in callers. */
> +		k->kobj.name = NULL;
>  		return err;
> +	}
>  	kobject_uevent(&k->kobj, KOBJ_ADD);
>  	return 0;
>  }


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v3] kset: fix memory leak when kset_register() returns error
  2022-10-25  7:15 [PATCH v3] kset: fix memory leak when kset_register() returns error Yang Yingliang
  2022-10-25 16:51 ` Luben Tuikov
@ 2022-10-25 16:57 ` Greg KH
  1 sibling, 0 replies; 3+ messages in thread
From: Greg KH @ 2022-10-25 16:57 UTC (permalink / raw)
  To: Yang Yingliang
  Cc: linux-kernel, qemu-devel, linux-f2fs-devel, linux-erofs,
	ocfs2-devel, linux-mtd, amd-gfx, rafael, somlo, mst, jaegeuk,
	chao, hsiangkao, huangjianan, mark, jlbec, joseph.qi, akpm,
	alexander.deucher, luben.tuikov, richard, liushixin2

On Tue, Oct 25, 2022 at 03:15:49PM +0800, Yang Yingliang wrote:
> Inject fault while loading module, kset_register() may fail.
> If it fails, the kset.kobj.name allocated by kobject_set_name()
> which must be called before a call to kset_register() may be
> leaked, since refcount of kobj was set in kset_init().
> 
> To mitigate this, we free the name in kset_register() when an
> error is encountered, i.e. when kset_register() returns an error.
> 
> A kset may be embedded in a larger structure which may be dynamically
> allocated in callers, it needs to be freed in ktype.release() or error
> path in callers, in this case, we can not call kset_put() in kset_register(),
> or it will cause double free, so just call kfree_const() to free the
> name and set it to NULL to avoid accessing bad pointer in callers.
> 
> With this fix, the callers don't need care about freeing the name
> and may call kset_put() if kset_register() fails.
> 
> Suggested-by: Luben Tuikov <luben.tuikov@amd.com>
> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
> ---
> v2 -> v3:
>   Update commit message and comment of kset_register().
> 
> v1 -> v2:
>   Free name inside of kset_register() instead of calling kset_put()
>   in drivers.

Thank you for all of this, it's a much nicer and cleaner fix than
forcing all callers to try to handle it instead.

greg k-h

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2022-10-25 16:58 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-25  7:15 [PATCH v3] kset: fix memory leak when kset_register() returns error Yang Yingliang
2022-10-25 16:51 ` Luben Tuikov
2022-10-25 16:57 ` Greg KH

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).