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 <[email protected]> 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/[email protected]?part=1
