Re: [PATCH v2] livepatch: Reject livepatches with aliased old_func

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

Pre-existing issues:
- [High] Initialization failures in klp_init_object_loaded() leave livepatch module relocations uncleared, leading to state corruption and permanently preventing target module loading.
--

--- Patch [1]: [PATCH v2] livepatch: Reject livepatches with aliased old_func ---
commit 230a471112d181b2e9810c9877ffebb05a42d61c
Author: Harry Hsu <[email protected]>

livepatch: Reject livepatches with aliased old_func

This patch adds a check during livepatch initialization to reject
configurations where multiple functions resolve to the same alias address.
It prevents ambiguity when the redirection logic evaluates the func_stack
in the ftrace handler.

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

> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 28d15ba58a26d..c35cf08c27c86 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -866,7 +866,7 @@ static void klp_clear_object_relocs(struct klp_patch *patch,
>  static int klp_init_object_loaded(struct klp_patch *patch,
>  				  struct klp_object *obj)
>  {
> -	struct klp_func *func;
> +	struct klp_func *func, *prev_func;
>  	int ret;
>  
>  	if (klp_is_module(obj)) {
[ ... ]
> @@ -888,6 +888,21 @@ static int klp_init_object_loaded(struct klp_patch *patch,
>  		if (ret)
>  			return ret;
>  
> +		/*
> +		 * Aliased symbols share one address, so they would resolve to
> +		 * the same klp_ops and stack up on a single ops->func_stack,
> +		 * leaving the redirection ambiguous.  Reject the livepatch.
> +		 */
> +		klp_for_each_func(obj, prev_func) {
> +			if (prev_func == func)
> +				break;
> +			if (prev_func->old_func == func->old_func) {
> +				pr_err("'%s' and '%s' resolve to the same address, aliased symbols are not supported\n",
> +				       prev_func->old_name, func->old_name);
> +				return -EINVAL;

[Severity: High]
This is a pre-existing issue, but returning an error here leaves the
module relocations applied earlier in this function uncleared.

When a module is loaded, klp_module_coming() iterates over patches and
calls klp_init_object_loaded(). If initialization fails, it delegates
cleanup to the error path:

kernel/livepatch/core.c:klp_module_coming() {
	...
	ret = klp_init_object_loaded(patch, obj);
	if (ret) {
		...
		goto err;
	}
	...
err:
	...
	klp_cleanup_module_patches_limited(mod, patch);
	...
}

However, the cleanup loop skips the failing patch entirely:

kernel/livepatch/core.c:klp_cleanup_module_patches_limited() {
	...
	klp_for_each_patch(patch) {
		if (patch == limit)
			break;
		...
		klp_clear_object_relocs(patch, obj);
	}
}

Because it breaks when reaching the limit patch, klp_clear_object_relocs()
is never called for the relocations that were successfully applied at the
start of klp_init_object_loaded() via klp_apply_object_relocs().

Could this permanently corrupt the livepatch module's state and prevent
the target module from safely loading again, since the relocations are
never reverted?

> +			}
> +		}
> +
>  		ret = kallsyms_lookup_size_offset((unsigned long)func->old_func,
>  						  &func->old_size, NULL);
>  		if (!ret) {

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