[PATCH] livepatch: Fix stack check for aliased old_func

[PATCH] livepatch: Fix stack check for aliased old_func

[Harry Hsu]

klp_check_stack_func() decides which address range to look for on a
task's stack by asking whether the func preceding @func on
ops->func_stack is the original kernel function or another livepatch's
replacement.  It uses list_is_singular(&ops->func_stack), which only
tells "one func on this stack" from "more than one".  That assumes every
klp_func of a patch gets its own func_stack.

Aliases break the assumption.  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 patch naming two of these symbols resolve to
the same klp_ops and are both pushed onto one func_stack.

The stack is then head -> B -> A.  A is the last node and does
correspond to the original function, but list_is_singular() is false, so
the "previously patched function" branch runs: list_next_entry() applies
container_of() to &ops->func_stack, treating the list head as a struct
klp_func, and reads func_addr/func_size from past the object.  Besides
the out-of-bounds read, the bogus range can keep matching stack entries,
so tasks that are safe to switch get -EAGAIN forever and the transition
never completes.

Test whether @func itself is the last entry instead.  The answer is
derived from @func's position rather than from the list length, so it
holds however many klp_funcs share a func_stack and never steps onto the
list head.  A single-entry stack is still trivially last, so existing
behaviour is unchanged.

Fixes: d83a7cb375ee ("livepatch: change to a per-task consistency model")
Signed-off-by: Harry Hsu <x90613@gmail.com>
---
 kernel/livepatch/transition.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index 2351a19ac2a9..8d6e3a58101f 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -223,7 +223,7 @@ static int klp_check_stack_func(struct klp_func *func, unsigned long *entries,
 		 */
 		ops = klp_find_ops(func->old_func);
 
-		if (list_is_singular(&ops->func_stack)) {
+		if (list_is_last(&func->stack_node, &ops->func_stack)) {
 			/* original function */
 			func_addr = (unsigned long)func->old_func;
 			func_size = func->old_size;
-- 
2.43.0

Re: [PATCH] livepatch: Fix stack check for aliased old_func

[sashiko-bot]

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

Pre-existing issues:
- [High] klp_ftrace_handler() incorrectly executes newly patched code for unpatched tasks when multiple aliased functions exist in the same transition patch.
- [High] Premature freeing of dynamically allocated NOP functions without RCU synchronization leads to a Use-After-Free in klp_ftrace_handler().
- [High] Concurrent module loading or unloading during a livepatch transition causes list corruption and Use-After-Free due to lockless klp_ops traversal.
--

--- Patch [1]: [PATCH] livepatch: Fix stack check for aliased old_func ---
commit 3438e93da695fdf6d92c99235da90b29e8a2a08f
Author: Harry Hsu <x90613@gmail.com>

livepatch: Fix stack check for aliased old_func

This patch fixes an out-of-bounds read and an infinite -EAGAIN loop in
klp_check_stack_func() when aliased functions share a single func_stack.
It addresses the issue by testing the function's position in the stack
rather than relying on list_is_singular().

> diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
> index 2351a19ac2a9e..8d6e3a58101fc 100644
> --- a/kernel/livepatch/transition.c
> +++ b/kernel/livepatch/transition.c
> @@ -223,7 +223,7 @@ static int klp_check_stack_func(struct klp_func *func, unsigned long *entries,
>  		 */
>  		ops = klp_find_ops(func->old_func);

[Severity: High]
This is a pre-existing issue and was not introduced by the patch under review,
but does klp_find_ops() traverse the global klp_ops list safely here?

Since klp_check_stack_func() can be called during a context switch, it executes
without klp_mutex protection. If a patched module is concurrently unloaded,
klp_module_going() acquires klp_mutex and calls klp_unpatch_func(), which
removes the ops from the list and frees it:

kernel/livepatch/patch.c:klp_unpatch_func() {
        ...
        list_del_rcu(&func->stack_node);
        list_del(&ops->node);
        kfree(ops);
        ...
}

Could this concurrent module unloading race with the lockless traversal in
klp_find_ops() and lead to a use-after-free or list corruption?

>  
> -		if (list_is_singular(&ops->func_stack)) {
> +		if (list_is_last(&func->stack_node, &ops->func_stack)) {

[Severity: High]
This is a pre-existing issue and was not introduced by the patch under review,
but does the same list_is_singular() assumption cause a use-after-free in
klp_unpatch_func()?

When a replacement livepatch overrides only one of several aliased functions
from an older patch, a NOP function is dynamically allocated for the
un-overridden alias. When the transition finishes, klp_discard_nops() triggers
unpatching and immediate freeing of the NOP functions:

kernel/livepatch/core.c:klp_discard_nops() {
        klp_unpatch_objects_dynamic(klp_transition_patch);
        klp_free_objects_dynamic(klp_transition_patch);
}

Since the stack also contains the overridden real function, the check for
list_is_singular() in klp_unpatch_func() evaluates to false, which bypasses
unregister_ftrace_function() and its associated ftrace_sync:

kernel/livepatch/patch.c:klp_unpatch_func() {
        ...
        if (list_is_singular(&ops->func_stack)) {
                /* ... */
        } else {
                list_del_rcu(&func->stack_node);
        }
        ...
}

The NOP is then immediately freed by kfree(). If klp_ftrace_handler() executes
concurrently on another CPU, might it access the freed NOP object?

>  			/* original function */
>  			func_addr = (unsigned long)func->old_func;
>  			func_size = func->old_size;


While reviewing the alias handling, I noticed one more pre-existing issue:

[Severity: High]
This is a pre-existing issue and was not introduced by the patch under review,
but does klp_ftrace_handler() correctly handle unpatched tasks when multiple
aliased functions exist in the same transition patch?

When an unpatched task invokes the intercepted function, klp_ftrace_handler()
observes that the patch is in transition and advances exactly once:

kernel/livepatch/patch.c:klp_ftrace_handler() {
        ...
        if (patch_state == KLP_TRANSITION_UNPATCHED) {
                /*
                 * Use the previously patched version of the function.
                 * If no previous patches exist, continue with the
                 * original function.
                 */
                func = list_entry_rcu(func->stack_node.next,
                                      struct klp_func, stack_node);
        ...
}

If multiple aliases are placed on the same ops->func_stack by the same patch,
won't the next entry just be another alias that is also in transition? Can this
cause the handler to mistakenly execute newly patched code for unpatched tasks?

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