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

[Miroslav Benes <mbenes@suse.cz>]

> 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