[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

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

[Petr Mladek]

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

[PATCH 0/2] livepatch: Clear relocations when klp_init_object_loaded() fails

[Petr Mladek]

Hi,

this patchset fixes an older problem found by Sashiko AI,
see https://lore.kernel.org/r/20260823062313.1321B1F000E9@smtp.kernel.org

The first patch just shuffles the existing code.
The second patch is the real fix.

The patchset is made on top of the patch where Sashiko found
this older problem, see
https://lore.kernel.org/all/20260823060734.58443-1-x90613@gmail.com/

It might make sense to handle them together in a single patchset
because they touch the same code and conflict.

It seems that we will need v3 for the original patch.
Harry, could you please send it together with these additional fixes? [*]

[*] I am not sure if you did this before. It is easy. You create
    the patchset as usual and send it. git will do the trick
    and keep me as the author of the two additional patches.

Alternatively, I could send this separately and solve
the conflicts when merging. They are simple after all.
I just must not forget to replace the new "return err"
with "goto err" ;-)


Petr Mladek (2):
  livepatch: Move code for updating livepatch object relocations
  livepatch: Clean up klp_init_object_loaded() when fails

 kernel/livepatch/core.c | 102 ++++++++++++++++++++++------------------
 1 file changed, 56 insertions(+), 46 deletions(-)

-- 
2.55.0

[PATCH 1/2] livepatch: Move code for updating livepatch object relocations

[Petr Mladek]

klp_free_object_loaded() is supposed to clear changes made by
klp_init_object_loaded(). It should call klp_clear_object_relocs()
which is currently defined later.

Move the code for updating object relocations up.

This is just a preparation step. No functional changes.

Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 kernel/livepatch/core.c | 72 ++++++++++++++++++++---------------------
 1 file changed, 36 insertions(+), 36 deletions(-)

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index a3b377a3f47c..cdb25949f73b 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -342,6 +342,42 @@ int klp_apply_section_relocs(struct module *pmod, Elf_Shdr *sechdrs,
 					secndx, objname, true);
 }
 
+static int klp_write_object_relocs(struct klp_patch *patch,
+				   struct klp_object *obj,
+				   bool apply)
+{
+	int i, ret;
+	struct klp_modinfo *info = patch->mod->klp_info;
+
+	for (i = 1; i < info->hdr.e_shnum; i++) {
+		Elf_Shdr *sec = info->sechdrs + i;
+
+		if (!(sec->sh_flags & SHF_RELA_LIVEPATCH))
+			continue;
+
+		ret = klp_write_section_relocs(patch->mod, info->sechdrs,
+					       info->secstrings,
+					       patch->mod->core_kallsyms.strtab,
+					       info->symndx, i, obj->name, apply);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int klp_apply_object_relocs(struct klp_patch *patch,
+				   struct klp_object *obj)
+{
+	return klp_write_object_relocs(patch, obj, true);
+}
+
+static void klp_clear_object_relocs(struct klp_patch *patch,
+				    struct klp_object *obj)
+{
+	klp_write_object_relocs(patch, obj, false);
+}
+
 /*
  * Sysfs Interface
  *
@@ -823,42 +859,6 @@ static int klp_init_func(struct klp_object *obj, struct klp_func *func)
 			   func->old_sympos ? func->old_sympos : 1);
 }
 
-static int klp_write_object_relocs(struct klp_patch *patch,
-				   struct klp_object *obj,
-				   bool apply)
-{
-	int i, ret;
-	struct klp_modinfo *info = patch->mod->klp_info;
-
-	for (i = 1; i < info->hdr.e_shnum; i++) {
-		Elf_Shdr *sec = info->sechdrs + i;
-
-		if (!(sec->sh_flags & SHF_RELA_LIVEPATCH))
-			continue;
-
-		ret = klp_write_section_relocs(patch->mod, info->sechdrs,
-					       info->secstrings,
-					       patch->mod->core_kallsyms.strtab,
-					       info->symndx, i, obj->name, apply);
-		if (ret)
-			return ret;
-	}
-
-	return 0;
-}
-
-static int klp_apply_object_relocs(struct klp_patch *patch,
-				   struct klp_object *obj)
-{
-	return klp_write_object_relocs(patch, obj, true);
-}
-
-static void klp_clear_object_relocs(struct klp_patch *patch,
-				    struct klp_object *obj)
-{
-	klp_write_object_relocs(patch, obj, false);
-}
-
 /* parts of the initialization that is done only when the object is loaded */
 static int klp_init_object_loaded(struct klp_patch *patch,
 				  struct klp_object *obj)
-- 
2.55.0

Re: [PATCH 1/2] livepatch: Move code for updating livepatch object relocations

[Song Liu]

On Fri, Aug 28, 2026 at 5:53 AM Petr Mladek <pmladek@suse.com> wrote:
>
> klp_free_object_loaded() is supposed to clear changes made by
> klp_init_object_loaded(). It should call klp_clear_object_relocs()
> which is currently defined later.
>
> Move the code for updating object relocations up.
>
> This is just a preparation step. No functional changes.
>
> Signed-off-by: Petr Mladek <pmladek@suse.com>

Do we really need to move them? I think adding a function
declaration is cleaner for git log/blame, etc.

Thanks,
Song

Re: [PATCH 1/2] livepatch: Move code for updating livepatch object relocations

[Josh Poimboeuf]

On Fri, Aug 28, 2026 at 10:02:09AM -0700, Song Liu wrote:
> On Fri, Aug 28, 2026 at 5:53 AM Petr Mladek <pmladek@suse.com> wrote:
> >
> > klp_free_object_loaded() is supposed to clear changes made by
> > klp_init_object_loaded(). It should call klp_clear_object_relocs()
> > which is currently defined later.
> >
> > Move the code for updating object relocations up.
> >
> > This is just a preparation step. No functional changes.
> >
> > Signed-off-by: Petr Mladek <pmladek@suse.com>
> 
> Do we really need to move them? I think adding a function
> declaration is cleaner for git log/blame, etc.

Personally I prefer a cleaner end result (put the function where it more
naturally belongs), though I wouldn't object to combining them into a
single patch.

-- 
Josh

Re: [PATCH 1/2] livepatch: Move code for updating livepatch object relocations

[Song Liu]

On Fri, Aug 28, 2026 at 10:41 AM Josh Poimboeuf <jpoimboe@kernel.org> wrote:
>
> On Fri, Aug 28, 2026 at 10:02:09AM -0700, Song Liu wrote:
> > On Fri, Aug 28, 2026 at 5:53 AM Petr Mladek <pmladek@suse.com> wrote:
> > >
> > > klp_free_object_loaded() is supposed to clear changes made by
> > > klp_init_object_loaded(). It should call klp_clear_object_relocs()
> > > which is currently defined later.
> > >
> > > Move the code for updating object relocations up.
> > >
> > > This is just a preparation step. No functional changes.
> > >
> > > Signed-off-by: Petr Mladek <pmladek@suse.com>
> >
> > Do we really need to move them? I think adding a function
> > declaration is cleaner for git log/blame, etc.
>
> Personally I prefer a cleaner end result (put the function where it more
> naturally belongs), though I wouldn't object to combining them into a
> single patch.

Noted. It is indeed a trade-off.

Thanks,
Song

[PATCH 2/2] livepatch: Clean up klp_init_object_loaded() when fails

[Petr Mladek]

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

However, the cleanup loop skips the failing patch. Each function called
in klp_init_object_loaded() is supposed to clean its own changes. This
works except for the changes done by klp_init_object_loaded().

The current code is a bit messy. The changes done by
klp_init_object_loaded() should get cleared by klp_free_object_loaded().
But this function also clears obj->mod which is set by
klp_module_coming(). And relocations are cleared separately.

Fix the situations by updating klp_free_object_loaded(). It should
revert all and only changes made by klp_init_object_loaded().
This requires some shuffling:

 + Clear obj->mod explicitly in klp_cleanup_module_patches_limited()
   and do not rely on klp_free_object_loaded().

 + Clear relocations in klp_free_object_loaded(). Remove the explicit
   call from klp_cleanup_module_patches_limited(). This requires
   adding the @patch parameter.

Finally, call klp_free_object_loaded() in the error path in
klp_init_object_loaded().

Reported-by: sashiko-bot@kernel.org
Closes: https://lore.kernel.org/r/20260823062313.1321B1F000E9@smtp.kernel.org
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 kernel/livepatch/core.c | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index cdb25949f73b..1e59a3cc0895 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -725,18 +725,20 @@ static void __klp_free_funcs(struct klp_object *obj, bool nops_only)
 }
 
 /* Clean up when a patched object is unloaded */
-static void klp_free_object_loaded(struct klp_object *obj)
+static void klp_free_object_loaded(struct klp_patch *patch,
+				   struct klp_object *obj)
 {
 	struct klp_func *func;
 
-	obj->mod = NULL;
-
 	klp_for_each_func(obj, func) {
 		func->old_func = NULL;
 
 		if (func->nop)
 			func->new_func = NULL;
 	}
+
+	if (klp_is_module(obj))
+		klp_clear_object_relocs(patch, obj);
 }
 
 static void __klp_free_objects(struct klp_patch *patch, bool nops_only)
@@ -875,7 +877,7 @@ static int klp_init_object_loaded(struct klp_patch *patch,
 		 */
 		ret = klp_apply_object_relocs(patch, obj);
 		if (ret)
-			return ret;
+			goto err;
 	}
 
 	klp_for_each_func(obj, func) {
@@ -883,7 +885,7 @@ static int klp_init_object_loaded(struct klp_patch *patch,
 					     func->old_sympos,
 					     (unsigned long *)&func->old_func);
 		if (ret)
-			return ret;
+			goto err;
 
 		/*
 		 * Aliased symbols share one address, so they would resolve to
@@ -896,7 +898,8 @@ static int klp_init_object_loaded(struct klp_patch *patch,
 			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 = -EINVAL;
+				goto err;
 			}
 		}
 
@@ -905,7 +908,8 @@ static int klp_init_object_loaded(struct klp_patch *patch,
 		if (!ret) {
 			pr_err("kallsyms size lookup failed for '%s'\n",
 			       func->old_name);
-			return -ENOENT;
+			ret = -ENOENT;
+			goto err;
 		}
 
 		if (func->nop)
@@ -916,11 +920,17 @@ static int klp_init_object_loaded(struct klp_patch *patch,
 		if (!ret) {
 			pr_err("kallsyms size lookup failed for '%s' replacement\n",
 			       func->old_name);
-			return -ENOENT;
+			ret = -ENOENT;
+			goto err;
 		}
 	}
 
 	return 0;
+
+err:
+	klp_free_object_loaded(patch, obj);
+
+	return ret;
 }
 
 static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
@@ -1274,8 +1284,8 @@ static void klp_cleanup_module_patches_limited(struct module *mod,
 			klp_unpatch_object(obj);
 
 			klp_post_unpatch_callback(obj);
-			klp_clear_object_relocs(patch, obj);
-			klp_free_object_loaded(obj);
+			klp_free_object_loaded(patch, obj);
+			obj->mod = NULL;
 			break;
 		}
 	}
-- 
2.55.0

Re: [PATCH 2/2] livepatch: Clean up klp_init_object_loaded() when fails

[Song Liu]

On Fri, Aug 28, 2026 at 5:53 AM Petr Mladek <pmladek@suse.com> wrote:
>
> When a module is loaded, klp_module_coming() iterates over patches and
> calls klp_init_object_loaded(). If initialization fails, it delegates
> cleanup to klp_cleanup_module_patches_limited().
>
> However, the cleanup loop skips the failing patch. Each function called
> in klp_init_object_loaded() is supposed to clean its own changes. This
> works except for the changes done by klp_init_object_loaded().
>
> The current code is a bit messy. The changes done by
> klp_init_object_loaded() should get cleared by klp_free_object_loaded().
> But this function also clears obj->mod which is set by
> klp_module_coming(). And relocations are cleared separately.
>
> Fix the situations by updating klp_free_object_loaded(). It should
> revert all and only changes made by klp_init_object_loaded().
> This requires some shuffling:
>
>  + Clear obj->mod explicitly in klp_cleanup_module_patches_limited()
>    and do not rely on klp_free_object_loaded().
>
>  + Clear relocations in klp_free_object_loaded(). Remove the explicit
>    call from klp_cleanup_module_patches_limited(). This requires
>    adding the @patch parameter.
>
> Finally, call klp_free_object_loaded() in the error path in
> klp_init_object_loaded().
>
> Reported-by: sashiko-bot@kernel.org
> Closes: https://lore.kernel.org/r/20260823062313.1321B1F000E9@smtp.kernel.org
> Signed-off-by: Petr Mladek <pmladek@suse.com>

Acked-by: Song Liu <song@kernel.org>

With one nitpick

> ---
>  kernel/livepatch/core.c | 30 ++++++++++++++++++++----------
>  1 file changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index cdb25949f73b..1e59a3cc0895 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -725,18 +725,20 @@ static void __klp_free_funcs(struct klp_object *obj, bool nops_only)
>  }
>
>  /* Clean up when a patched object is unloaded */
> -static void klp_free_object_loaded(struct klp_object *obj)
> +static void klp_free_object_loaded(struct klp_patch *patch,
> +                                  struct klp_object *obj)

nit: Do we still need to fit every line in 80 characters? checkpatch.pl
only enforce 100 characters these days.

Thanks
Song

>  {
>         struct klp_func *func;

[...]

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

[Petr Mladek]

On Sun 2026-08-23 14:07:34, Harry Hsu wrote:
> 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(-)

The patch makes sense, looks good and passes selftests:

Reviewed-by: Petr Mladek <pmladek@suse.com>
Tested-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

PS: I am going to wait one week. Then I will push it if nobody
    complained in the meantime.

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

[Josh Poimboeuf]

On Sun, Aug 23, 2026 at 02:07:34PM +0800, Harry Hsu wrote:
> 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.

Acked-by: Josh Poimboeuf <jpoimboe@kernel.org>

-- 
Josh

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

[Miroslav Benes]

> 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>
>
> 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.
> +		 */

The comment is imprecise because you reject the live patch only in
klp_enable_patch()->klp_init_object() path. However,
klp_init_object_loaded() is also called in klp_module_coming() under
load_module() for just loaded modules. In this case, the loaded module
is rejected and not loaded if there is an error in the live patch
application.

The change still makes sense but your patch is not a live patch
validation. The comment should be fixed and perhaps the changelog as
well.

With that

Acked-by: Miroslav Benes <mbenes@suse.cz>

-- 
Miroslav

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

[Song Liu]

On Sat, Aug 22, 2026 at 11:07 PM Harry Hsu <x90613@gmail.com> wrote:
>
> 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>

Acked-by: Song Liu <song@kernel.org>

Can we add a selftest for this case?