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

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?
Therefore, we should call kobject_put() 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);
 }

>
>  static const struct kobj_type klp_ktype_object = {
> @@ -668,6 +670,8 @@ static void klp_kobj_release_func(struct kobject *kobj)
>
>         if (func->nop)
>                 klp_free_func_nop(func);
> +       else
> +               kobject_put(&func->obj.kobj);
>  }

Similarly, klp_init_func_early() initializes the nop function, so
kobject_put() must be called unconditionally.

 static void klp_kobj_release_func(struct kobject *kobj)
 {
        struct klp_func *func;
+       struct klp_object *obj;

        func = container_of(kobj, struct klp_func, kobj);
+       obj = func->obj;

        if (func->nop)
                klp_free_func_nop(func);
+
+       kobject_put(&obj->kobj);
 }

>
>  static const struct kobj_type klp_ktype_func = {
> @@ -946,6 +950,7 @@ static void klp_init_func_early(struct klp_object *obj,
>                                 struct klp_func *func)
>  {
>         kobject_init(&func->kobj, &klp_ktype_func);
> +       kobject_get(&obj->kobj);
>         list_add_tail(&func->node, &obj->func_list);
>  }
>
> @@ -954,6 +959,7 @@ static void klp_init_object_early(struct klp_patch *patch,
>  {
>         INIT_LIST_HEAD(&obj->func_list);
>         kobject_init(&obj->kobj, &klp_ktype_object);
> +       kobject_get(&patch->kobj);
>         list_add_tail(&obj->node, &patch->obj_list);
>  }
>
> We really would need to add the back references (obj->patch,
> func->obj) because we could not rely on kobj->parent. It is
> set only when kobject_add() was called...
>
> That said, I doubt that livepatching is the only subsystem using
> kobjects in static structures. It might make sense to handle
> this on the kobject API level. I mean to add a kobject() API
> which would just set kobj->parent and increment kobj->kref
> and can't fail. But it seems to be against the existing philosophy
> of the kobject API. So, we might need the workaround after all.

-- 
Regards
Yafang