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

[Petr Mladek <pmladek@suse.com>]

On Wed 2026-08-12 22:02:32, Harry Hsu wrote:
> 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

Interesting. Great catch!

> 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.

Right.

> 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.

This might fix klp_check_stack_func() for A. But not for B. B won't
be the last entry so that klp_check_stack_func() would use
the list_next_entry() and will check for A on stack instead of
the original function.

Another _big problem_ is in klp_ftrace_handler(). It would use A
in PATCHED state and B in UNPATCHED. But it is not clear whether
A or B should be used in the PATCHED state. And it should use
the original code in UNPATCHED state.

IMHO, we must catch this situation when preparing livepatches
and when enabling the livepatch. A single livepatch must never
create two entries on any ops->func_stack.

IMHO, we should catch the duplicate (aliased) entries in
klp_init_object_loaded() and return -EINVAL when they are found.

I do not see any other solution. We could not decide which
struct klp_func should be used for the redirection when
more of them point to the same original function.

Best Regards,
Petr