[PATCH] kernel: fix error handling in lookup_or_create_module_kobject()

[PATCH] kernel: fix error handling in lookup_or_create_module_kobject()

[Dmitry Antipov]

In 'lookup_or_create_module_kobject()', an internal kobject is created
using 'module_ktype'. So plain 'kobject_put()' causes an attempt to use
an uninitialied completion pointer in 'module_kobject_release()' and
'mod_kobject_put()' should be used instead. But if the driver (e.g. USB
gadget as in this particular case reported by syzkaller) is configured
as compiled-in, THIS_MODULE is NULL and there is no relevant module
object to call the latter against. So introduce an internal wrapper
'__module_kobject_put()' which is expected 'struct module_kobject' and
so fix error handling in 'lookup_or_create_module_kobject()'.

Reported-by: syzbot+7fb8a372e1f6add936dd@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=7fb8a372e1f6add936dd
Fixes: 1c7777feb0e2 ("kernel: refactor lookup_or_create_module_kobject()")
Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
 include/linux/module.h |  3 +++
 kernel/module/sysfs.c  | 15 ++++++++++-----
 kernel/params.c        |  3 ++-
 3 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index b3329110d668..2b4ab389c1bc 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -891,6 +891,9 @@ static inline void module_for_each_mod(int(*func)(struct module *mod, void *data
 #ifdef CONFIG_SYSFS
 extern struct kset *module_kset;
 extern const struct kobj_type module_ktype;
+void __module_kobject_put(struct module_kobject *mkobj);
+#else /* not CONFIG_SYSFS */
+static inline void __module_kobject_put(struct module_kobject *mkobj) { }
 #endif /* CONFIG_SYSFS */
 
 #define symbol_request(x) try_then_request_module(symbol_get(x), "symbol:" #x)
diff --git a/kernel/module/sysfs.c b/kernel/module/sysfs.c
index b401ff4b02d2..7519920f4f55 100644
--- a/kernel/module/sysfs.c
+++ b/kernel/module/sysfs.c
@@ -323,11 +323,7 @@ static int module_add_modinfo_attrs(struct module *mod)
 
 static void mod_kobject_put(struct module *mod)
 {
-	DECLARE_COMPLETION_ONSTACK(c);
-
-	mod->mkobj.kobj_completion = &c;
-	kobject_put(&mod->mkobj.kobj);
-	wait_for_completion(&c);
+	__module_kobject_put(&mod->mkobj);
 }
 
 static int mod_sysfs_init(struct module *mod)
@@ -362,6 +358,15 @@ static int mod_sysfs_init(struct module *mod)
 	return err;
 }
 
+void __module_kobject_put(struct module_kobject *mkobj)
+{
+	DECLARE_COMPLETION_ONSTACK(c);
+
+	mkobj->kobj_completion = &c;
+	kobject_put(&mkobj->kobj);
+	wait_for_completion(&c);
+}
+
 int mod_sysfs_setup(struct module *mod,
 		    const struct load_info *info,
 			   struct kernel_param *kparam,
diff --git a/kernel/params.c b/kernel/params.c
index e668fc90b83e..2cf1f17840e0 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -780,7 +780,8 @@ struct module_kobject __modinit * lookup_or_create_module_kobject(const char *na
 	if (IS_ENABLED(CONFIG_MODULES) && !err)
 		err = sysfs_create_file(&mk->kobj, &module_uevent.attr);
 	if (err) {
-		kobject_put(&mk->kobj);
+		/* Not 'mod_kobject_put()' because THIS_MODULE may be NULL. */
+		__module_kobject_put(mk);
 		pr_crit("Adding module '%s' to sysfs failed (%d), the system may be unstable.\n",
 			name, err);
 		return NULL;
-- 
2.49.0

Re: [PATCH] kernel: fix error handling in lookup_or_create_module_kobject()

[Petr Pavlu]

On 5/6/25 13:17, Dmitry Antipov wrote:
> In 'lookup_or_create_module_kobject()', an internal kobject is created
> using 'module_ktype'. So plain 'kobject_put()' causes an attempt to use
> an uninitialied completion pointer in 'module_kobject_release()' and
> 'mod_kobject_put()' should be used instead. But if the driver (e.g. USB
> gadget as in this particular case reported by syzkaller) is configured
> as compiled-in, THIS_MODULE is NULL and there is no relevant module
> object to call the latter against. So introduce an internal wrapper
> '__module_kobject_put()' which is expected 'struct module_kobject' and
> so fix error handling in 'lookup_or_create_module_kobject()'.
> 
> Reported-by: syzbot+7fb8a372e1f6add936dd@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=7fb8a372e1f6add936dd
> Fixes: 1c7777feb0e2 ("kernel: refactor lookup_or_create_module_kobject()")

I think this specific commit is harmless, rather the underlying problem
was introduced already in 942e443127e9 ("module: Fix mod->mkobj.kobj
potentially freed too early"). Commit f95bbfe18512 ("drivers: base:
handle module_kobject creation") now allowed the problematic code to be
reached by more paths and enabled syzkaller to find it.

> [...]
> diff --git a/include/linux/module.h b/include/linux/module.h
> index b3329110d668..2b4ab389c1bc 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -891,6 +891,9 @@ static inline void module_for_each_mod(int(*func)(struct module *mod, void *data
>  #ifdef CONFIG_SYSFS
>  extern struct kset *module_kset;
>  extern const struct kobj_type module_ktype;
> +void __module_kobject_put(struct module_kobject *mkobj);
> +#else /* not CONFIG_SYSFS */
> +static inline void __module_kobject_put(struct module_kobject *mkobj) { }
>  #endif /* CONFIG_SYSFS */
>  
>  #define symbol_request(x) try_then_request_module(symbol_get(x), "symbol:" #x)
> diff --git a/kernel/module/sysfs.c b/kernel/module/sysfs.c
> index b401ff4b02d2..7519920f4f55 100644
> --- a/kernel/module/sysfs.c
> +++ b/kernel/module/sysfs.c
> @@ -323,11 +323,7 @@ static int module_add_modinfo_attrs(struct module *mod)
>  
>  static void mod_kobject_put(struct module *mod)
>  {
> -	DECLARE_COMPLETION_ONSTACK(c);
> -
> -	mod->mkobj.kobj_completion = &c;
> -	kobject_put(&mod->mkobj.kobj);
> -	wait_for_completion(&c);
> +	__module_kobject_put(&mod->mkobj);
>  }
>  
>  static int mod_sysfs_init(struct module *mod)
> @@ -362,6 +358,15 @@ static int mod_sysfs_init(struct module *mod)
>  	return err;
>  }
>  
> +void __module_kobject_put(struct module_kobject *mkobj)
> +{
> +	DECLARE_COMPLETION_ONSTACK(c);
> +
> +	mkobj->kobj_completion = &c;
> +	kobject_put(&mkobj->kobj);
> +	wait_for_completion(&c);
> +}
> +
>  int mod_sysfs_setup(struct module *mod,
>  		    const struct load_info *info,
>  			   struct kernel_param *kparam,
> diff --git a/kernel/params.c b/kernel/params.c
> index e668fc90b83e..2cf1f17840e0 100644
> --- a/kernel/params.c
> +++ b/kernel/params.c
> @@ -780,7 +780,8 @@ struct module_kobject __modinit * lookup_or_create_module_kobject(const char *na
>  	if (IS_ENABLED(CONFIG_MODULES) && !err)
>  		err = sysfs_create_file(&mk->kobj, &module_uevent.attr);
>  	if (err) {
> -		kobject_put(&mk->kobj);
> +		/* Not 'mod_kobject_put()' because THIS_MODULE may be NULL. */
> +		__module_kobject_put(mk);
>  		pr_crit("Adding module '%s' to sysfs failed (%d), the system may be unstable.\n",
>  			name, err);
>  		return NULL;

This looks as a valid fix, but I wonder if it wouldn't be simpler to
have module_kobj_release() check if mk->kobj_completion is NULL.

-- 
Thanks,
Petr

Re: [PATCH] kernel: fix error handling in lookup_or_create_module_kobject()

[Dmitry Antipov]

On 5/6/25 5:49 PM, Petr Pavlu wrote:

> This looks as a valid fix, but I wonder if it wouldn't be simpler to
> have module_kobj_release() check if mk->kobj_completion is NULL.

Indeed. Just submitted to syzbot and will send v2 if passed.

Dmitry

[PATCH] module: ensure that kobject_put() is safe for module type kobjects

[Dmitry Antipov]

In 'lookup_or_create_module_kobject()', an internal kobject is created
using 'module_ktype'. So call to 'kobject_put()' on error handling
path causes an attempt to use an uninitialized completion pointer in
'module_kobject_release()'. In this scenario, we just want to release
kobject without an extra synchronization required for a regular module
unloading process, so adding an extra check whether 'complete()' is
actually required makes 'kobject_put()' safe.

Reported-by: syzbot+7fb8a372e1f6add936dd@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=7fb8a372e1f6add936dd
Fixes: 942e443127e9 ("module: Fix mod->mkobj.kobj potentially freed too early")
Suggested-by: Petr Pavlu <petr.pavlu@suse.com>
Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
 kernel/params.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/kernel/params.c b/kernel/params.c
index e668fc90b83e..b92d64161b75 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -943,7 +943,9 @@ struct kset *module_kset;
 static void module_kobj_release(struct kobject *kobj)
 {
 	struct module_kobject *mk = to_module_kobject(kobj);
-	complete(mk->kobj_completion);
+
+	if (mk->kobj_completion)
+		complete(mk->kobj_completion);
 }
 
 const struct kobj_type module_ktype = {
-- 
2.49.0

Re: [PATCH] module: ensure that kobject_put() is safe for module type kobjects

[Petr Pavlu]

On 5/7/25 08:50, Dmitry Antipov wrote:
> In 'lookup_or_create_module_kobject()', an internal kobject is created
> using 'module_ktype'. So call to 'kobject_put()' on error handling
> path causes an attempt to use an uninitialized completion pointer in
> 'module_kobject_release()'. In this scenario, we just want to release
> kobject without an extra synchronization required for a regular module
> unloading process, so adding an extra check whether 'complete()' is
> actually required makes 'kobject_put()' safe.
> 
> Reported-by: syzbot+7fb8a372e1f6add936dd@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=7fb8a372e1f6add936dd
> Fixes: 942e443127e9 ("module: Fix mod->mkobj.kobj potentially freed too early")
> Suggested-by: Petr Pavlu <petr.pavlu@suse.com>
> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>

Added on modules-fixes.

I plan to send the fix to Linus on Friday. While the underlying problem
was already there, commit f95bbfe18512 ("drivers: base: handle
module_kobject creation"), merged in v6.15-rc5, made it more exposed and
I think it's better to have it fixed in this cycle. The patch itself is
also safe.

-- 
Thanks,
Petr