Re: [PATCH v2] selftests/livepatch: fix resource leak in test_klp_syscall init error path

On Thu 2026-06-04 08:42:13, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Relying exclusively on kobject_put() to clean up sysfs attributes backed by module memory introduces a use-after-free window during module unload if CONFIG_DEBUG_KOBJECT_RELEASE is enabled.
> --
> 
> commit 89541aa9ecd8e356eaf4884fa2b111bc605a7933
> Author: Rui Qi <[email protected]>
> 
> selftests/livepatch: fix resource leak in test_klp_syscall init error path
> 
> This commit attempts to fix a resource leak in livepatch_init() by calling
> kobject_put() on the error path if klp_enable_patch() fails.
> 
> > --- a/tools/testing/selftests/livepatch/test_modules/test_klp_syscall.c
> > +++ b/tools/testing/selftests/livepatch/test_modules/test_klp_syscall.c
> [ ... ]
> > @@ -98,7 +98,11 @@ static int livepatch_init(void)
> >  	 */
> >  	npids = npids_pending;
> >  
> > -	return klp_enable_patch(&patch);
> > +	ret = klp_enable_patch(&patch);
> > +	if (ret)
> > +		kobject_put(klp_kobj);
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but it replicates a pre-existing
> issue into the error path.
> 
> Does relying exclusively on kobject_put() here create a use-after-free window
> during module unload?

Sigh, I always get lost when looking at kobject-related API.

> If CONFIG_DEBUG_KOBJECT_RELEASE is enabled, kobject_put() drops the refcount
> but schedules kobject_cleanup() (and thus sysfs_remove_dir()) to run
> asynchronously via a workqueue after a 1 to 4 second delay. Because klp_attr
> and its show callback are allocated in the module's memory, returning an error
> here causes the module loader to free that memory immediately.
>
> During this delay, the sysfs file remains fully accessible. If a user reads
> this file, would sysfs_kf_seq_show() dereference the attribute and callback
> that now point to unmapped module memory?

This sounds correct.

> Should kobject_del(klp_kobj) be called prior to kobject_put() to
> synchronously remove the sysfs entries?

I am not sure if this would be a complete solution. I do not see
anywhere that kobject_del() would be synchronous. I think that
the file still might stay opened.

> > +
> > +	return ret;
> >  }
> >  
> >  static void livepatch_exit(void)
> >  {
> >  	kobject_put(klp_kobj);
> >  }
> 
> [Severity: High]
> This is a pre-existing issue, but the exact same vulnerability exists here in
> the unmodified module exit path where kobject_put() is the only cleanup
> mechanism before the module unloads.
> 
> Could we also add kobject_del(klp_kobj) before kobject_put() in
> livepatch_exit() to ensure sysfs entries are synchronously removed before
> module memory is freed?

I would ignore this. The same code (just kobject_put()) is used in
samples/kobject/kobject-example.c which is supposed to show how
the API should be used.

It is just a test module. The interface is used only by the selftest.

I believe that this is a problem of the kobject API and should
be fixed there.

Best Regards,
Petr