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+7fb8a372e1f6add93...@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
