[PATCH v2] livepatch: Reject livepatches with aliased old_func

[PATCH v2] livepatch: Reject livepatches with aliased old_func

[Harry Hsu]

Several symbols can share one address:

  ffffffff8ed7fef0 t __do_sys_fork
  ffffffff8ed7fef0 T __ia32_sys_fork
  ffffffff8ed7fef0 T __x64_sys_fork

klp_find_ops() looks the ops up by func->old_func, i.e. by address, so
two klp_funcs of the same livepatch naming two of these symbols resolve
to the same klp_ops and are both pushed onto one ops->func_stack.

This breaks the assumption that a single livepatch contributes at most
one entry to any func_stack.  klp_ftrace_handler() picks the entry at
the top of the stack, but when both entries belong to the same livepatch
there is nothing that says which of them should be used in the PATCHED
state, and the UNPATCHED state has to end up at the original function
either way.  klp_check_stack_func() cannot tell them apart either: it
asks whether the preceding entry is the original function or another
livepatch's replacement, and an aliased sibling is neither.

Patching two aliases of one function from a single livepatch was never
meaningful, so reject it while the object is being initialized rather
than leave the redirection undefined.  Compare the resolved old_func of
each klp_func against the ones already resolved for the same klp_object
and return -EINVAL on a match, naming both symbols so that the offending
pair can be found in the livepatch source.

Fixes: 3c33f5b99d68 ("livepatch: support for repatching a function")
Suggested-by: Petr Mladek <pmladek@suse.com>
Signed-off-by: Harry Hsu <x90613@gmail.com>
---
v2:
  - Drop the klp_check_stack_func() change.  As Petr pointed out, using
    list_is_last() only made the last entry behave, still checked the
    aliased sibling's range for the other entries, and did nothing about
    klp_ftrace_handler() being unable to pick between them.  Reject the
    livepatch in klp_init_object_loaded() instead, as suggested.
  - Rewrite the changelog around rejecting the configuration rather than
    around the out-of-bounds read that v1 described.

Link: https://lore.kernel.org/all/20260812140232.48079-1-x90613@gmail.com/

 kernel/livepatch/core.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 28d15ba58a26..c35cf08c27c8 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;
+			}
+		}
+
 		ret = kallsyms_lookup_size_offset((unsigned long)func->old_func,
 						  &func->old_size, NULL);
 		if (!ret) {
-- 
2.43.0

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

[sashiko-bot]

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?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260823060734.58443-1-x90613@gmail.com?part=1