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

[Petr Mladek <pmladek@suse.com>]

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?

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.

> 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.

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);
 }
 
 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);
 }
 
 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.

Best Regards,
Petr