Re: [PATCH v2 2/2] livepatch: Fix UAF of unregistered patch kobjects

On Wed 2026-08-19 20:37:17, Yafang Shao wrote:
> On Wed, Aug 19, 2026 at 7:18 PM Petr Mladek <[email protected]> wrote:
> >
> > On Sun 2026-08-16 17:04:42, Yafang Shao wrote:
> > > When klp_enable_patch() fails after klp_init_patch_early() has run,
> > > the error path calls klp_free_patch_start() and klp_free_patch_finish().
> > > The former drops the references of all object and function kobjects via
> > > klp_free_objects(), the latter drops the patch kobject reference and
> > > waits for the patch kobject release only:
> > >
> > >     klp_free_patch_finish():
> > >         kobject_put(&patch->kobj);
> > >         wait_for_completion(&patch->finish);
> > >
> > > With CONFIG_DEBUG_KOBJECT_RELEASE enabled, kobject_put() does not
> > > release the kobject synchronously but schedules a delayed release with
> > > a random delay of up to 4 seconds (see kobject_release() in
> > > lib/kobject.c).
> >
> > Yes.
> >
> > > Because klp_free_patch_finish() only waits for the
> > > patch kobject release, it may return while object and function kobject
> > > releases are still pending. The caller can then unload the livepatch
> > > module, which frees the klp_object and klp_func structures. The delayed
> > > kobject release callbacks later access this freed memory in
> > > kobject_cleanup(), resulting in a use-after-free.
> > >
> > > This issue can occur in two scenarios:
> > >
> > > 1. The patch kobject was never added to sysfs (e.g., klp_init_patch()
> > >    failed at kobject_add()). All child kobjects were only initialized
> > >    via kobject_init() but never added to sysfs. They do not hold
> > >    references to the patch kobject, so the patch kobject can be
> > >    released independently, unblocking patch->finish before the child
> > >    releases complete.
> > >
> > > 2. The patch kobject was added to sysfs, but a subsequent operation
> > >    such as klp_add_nops() or klp_init_object() failed. Some child
> > >    kobjects were initialized but not yet added to sysfs. These
> > >    un-added children do not hold references to the patch kobject
> > >    either, so the same race can occur.
> >
> > In short, this says that the races might happen when some kobjects
> > were not added into sysfs. Am I right, please?
> 
> right
> 
> >
> > I agree. My undestading:
> >
> > The klp_kobj_release_*() callbacks are called by kobject_cleanup()
> > which calls kobject_put(parent) as the last step. It should make sure
> > that:
> >
> >   + klp_kobj_release_patch() is scheduled/called only when
> >     klp_kobj_release_object() has been called for all patch->objs.
> >
> >   + klp_kobj_release_object() is scheduled/called only when
> >     klp_kobj_release_func() has been called for all obj->funcs.
> >
> > But it works only when "kobj->parent" is set and
> > "parent->kref" has been incremented for each child before.
> >
> > This is true only when kobject_add() is called for all all used
> > kobjects. But it is not guaranteed when any klp_init_*() failed.
> 
> correct
> 
> >
> > > Fix this by tracking all static kobject releases with a per-patch
> > > atomic counter (kobj_pending). klp_free_patch_start() counts the
> > > patch kobject plus all static object and function kobjects.
> > > klp_free_patch_finish() waits until kobj_pending reaches zero,
> > > ensuring all kobject releases have completed before the module is
> > > unloaded.
> >
> > I think that we do not need an extra couter. We might use
> > the existing kobj->kref. We just need to explicitely
> > increment/decrement it.
> 
> good idea
> 
> >
> > I mean something like:
> >
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index 28d15ba58a26..023f666ddcc4 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -652,6 +652,8 @@ static void klp_kobj_release_object(struct kobject *kobj)
> >
> >         if (obj->dynamic)
> >                 klp_free_object_dynamic(obj);
> > +       else
> > +               kobject_put(&obj->patch.kobj);
> >  }
> 
> It appears that klp_init_object_early() also initializes non-dynamic
> objects, right?

I was a bit confused by the sentence. The dynamic objects
do not exist when klp_init_object_early() is called in
klp_init_patch_early().

But I see that it is called also in klp_alloc_object_dynamic().

> Therefore, we should call kobject_put() unconditionally.

Great catch. Yes, we should call it unconditionally.

>  static void klp_kobj_release_object(struct kobject *kobj)
>  {
>         struct klp_object *obj;
> +       struct klp_patch *patch;
> 
>         obj = container_of(kobj, struct klp_object, kobj);
> +       patch = obj->patch;
> 
>         if (obj->dynamic)
>                 klp_free_object_dynamic(obj);
> +
> +       kobject_put(&patch->kobj);
>  }

Looks good. Same with klp_kobj_release_func().

Best Regards,
Petr