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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Critical] The newly added release signaling logic introduces a direct Use-After-Free when processing dynamically allocated objects and NOP functions in the kobject release paths.

Pre-existing issues:
- [High] The patch fails to fully resolve the original delayed kobject release Use-After-Free if partial initialization fails after the patch kobject has been added, leaving child kobjects un-added.
--

--- Patch [2]: [PATCH 2/2] livepatch: Fix UAF of unregistered patch kobjects ---
commit ac38837d810b471eed544191021bf254f8b9e585
Author: Yafang Shao <[email protected]>

livepatch: Fix UAF of unregistered patch kobjects

When klp_enable_patch() fails after klp_init_patch_early() has run,
delayed kobject releases can cause use-after-free bugs when the module
is unloaded. This patch tracks pending kobject releases and waits for
them before completion.

Link: https://lore.kernel.org/all/[email protected]/

> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 28d15ba58a26d..e07b026feeb99 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c

[ ... ]

> @@ -652,6 +653,14 @@ static void klp_kobj_release_object(struct kobject *kobj)
>  
>  	if (obj->dynamic)
>  		klp_free_object_dynamic(obj);
> +
> +	/*
> +	 * The object kobject was initialized but never added to sysfs.
> +	 * Signal the release to the owning patch.
> +	 */
> +	if (!obj->patch->kobj_added &&
> +	    atomic_dec_and_test(&obj->patch->kobj_pending))
> +		complete(&obj->patch->finish);
>  }

[Severity: Critical]
Does this dereference the freed obj pointer?

If obj->dynamic is true, klp_free_object_dynamic(obj) is called, which
frees the object structure. Immediately after this intentional free, the
newly added logic dereferences the freed pointer by evaluating
!obj->patch->kobj_added.

> @@ -668,6 +677,13 @@ static void klp_kobj_release_func(struct kobject *kobj)
>  
>  	if (func->nop)
>  		klp_free_func_nop(func);
> +	/*
> +	 * The function kobject was initialized but never added to sysfs.
> +	 * Signal the release to the owning patch.
> +	 */
> +	if (!func->obj->patch->kobj_added &&
> +	    atomic_dec_and_test(&func->obj->patch->kobj_pending))
> +		complete(&func->obj->patch->finish);
>  }

[Severity: Critical]
Similarly here, does this access func after it has been freed?

If func->nop is true, klp_free_func_nop(func) frees the function,
but the new check immediately dereferences func->obj->patch->kobj_added.

[ ... ]

> @@ -737,9 +753,29 @@ static void klp_free_objects_dynamic(struct klp_patch *patch)
>   */
>  static void klp_free_patch_start(struct klp_patch *patch)
>  {
> +	struct klp_object *obj;
> +	struct klp_func *func;
> +
>  	if (!list_empty(&patch->list))
>  		list_del(&patch->list);
>  
> +	if (!patch->kobj_added) {

[Severity: High]
This is a pre-existing issue, but does this check leave child kobjects
unprotected if partial initialization fails after the patch kobject has
been added?

If klp_init_patch() successfully adds the patch kobject, it sets
patch->kobj_added = true. If a subsequent operation like klp_add_nops()
or klp_init_object() fails, klp_free_patch_start() is called on the
error path.

Because patch->kobj_added is true, this check is bypassed and
patch->kobj_pending tracking is not initialized. However, child kobjects
that were initialized early but not yet added to sysfs do not hold a
reference to patch->kobj. Thus, patch->kobj will be released
independently, unblocking patch->finish, and allowing the livepatch
module to be unloaded while un-added child kobjects may still trigger
their delayed releases and access freed module memory.

> +		/*
> +		 * The kobjects were only initialized and never added to
> +		 * sysfs. Count them so that klp_free_patch_finish() can
> +		 * wait for all their releases before the patch module is
> +		 * unloaded. Without this, a delayed kobject release
> +		 * (CONFIG_DEBUG_KOBJECT_RELEASE) could access the freed
> +		 * module memory.
> +		 */
> +		atomic_set(&patch->kobj_pending, 1); /* the patch kobject */
> +		klp_for_each_object(patch, obj) {
> +			atomic_inc(&patch->kobj_pending);
> +			klp_for_each_func(obj, func)
> +				atomic_inc(&patch->kobj_pending);
> +		}
> +	}
> +
>  	klp_free_objects(patch);
>  }

[ ... ]

> @@ -987,6 +1027,7 @@ static int klp_init_patch(struct klp_patch *patch)
>  	ret = kobject_add(&patch->kobj, klp_root_kobj, "%s", patch->mod->name);
>  	if (ret)
>  		return ret;
> +	patch->kobj_added = true;
>  
>  	if (patch->replace) {
>  		ret = klp_add_nops(patch);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2