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

[Petr Mladek <pmladek@suse.com>]

On Sun 2026-08-23 06:23:12, sashiko-bot@kernel.org wrote:
> 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 <x90613@gmail.com>
> 
> 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/20260812140232.48079-1-x90613@gmail.com/
> 
> > 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?

This is a good catch. Functions called in klp_module_coming() are
supposed to revert their own changes on failure. Or
klp_module_coming() is supposed to call the counter-part
(pre/post scripts).

It seems to work, except for the changes done by
klp_init_object_loaded().

I think that it was not important in the original design.
The only important revert is the clearing of relocations
which was added later.

I am working on a patch which would fix it. I want to do it
the right way and call also klp_free_object_loaded() and
clearing obj->mod separately...

It is getting late here. I hope that I'll send it tomorrow...

Best Regards,
Petr