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

[Petr Pavlu <petr.pavlu@suse.com>]

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