Re: [PATCH v2] livepatch: Prevent livepatch to apply the uninitialized patch

[Miroslav Benes]

On Tue, 12 May 2015, Minfei Huang wrote:

> The previous patches can be applied, once the corresponding module is
> loaded. In general, the patch will do relocation (if necessary) and
> obtain/verify function address before we start to enable patch.
> 
> In some case, the uninitialized patch can be applied to the kernel.
> Following is the case to describe the scenario step by step.
> 
> 1) Patch a patch to the kernel before the corresponding module being
>    loaded.
> 2) Call klp_module_notify_coming to enable the patch, once the module
>    is loaded.
> 3) Do the instruction "obj->mod = mod", whatever the result of
>    klp_module_notify_coming is
> 4) Fail to call the klp_init_object_loaded or klp_enable_object
> 5) klp_module_notify_coming returns, now the module is working
> 
> 6) Enable the patch from the userspace (disable patch firstly, then
>    enable the patch via sysfs)
> 7) Call __klp_enable_patch to enable patch
> 8) Pass the limitation (klp_init_object_loaded), because the value
>    of obj->mod is not NULL (obtain the value from step 3)
> 9) Patch is applied, though it is uninitialized (do not relocate
>    and obtain old_addr)
> 
> It is fatal to kernel, once the uninitialized patch is appled. To
> fix it, obj->mod will nerver obtain the value, if livepatch fails
> to call the klp_module_notify_coming.

Hi,

I still think the changelog needs to be improved a bit.

There are three different situations in which the coming module notifier 
can fail:

1. relocations are not applied for some reason. In this case kallsyms for 
module symbol is not called at all. The patch is not applied to the 
module. If the user disable and enable patch again, there is possible bug 
in klp_enable_func. If the user specified func->old_addr for some function 
in the module (and he shouldn't do that, but nevertheless) our warning 
would not catch it, ftrace will fail in the process and the patch is 
not enabled.

2. relocations are applied successfully, but kallsyms lookup fails. In 
this case func->old_addr can be correct for all previous lookups, 0 for 
current failed one, and "unspecified" for the rest. If we undergo the same 
scenario as in 1. the behaviour differs for three cases, but the patch is 
not enabled anyway.

3. the object is initialized but klp_enable_object fails in the notifier 
due to possible ftrace error. Since it is improbable that ftrace would 
heal itself in the future, we would get those errors everytime the patch 
is enabled.

Your patch fixes all three cases, but consider to change the changelog to 
describe it all.

See few more remarks below.

> 
> Signed-off-by: Minfei Huang <mnfhuang@xxxxxxxxx>
> ---
> v1:
> - modify the commit log, describe the issue more details
> ---
>  kernel/livepatch/core.c | 23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 284e269..4bbcdda 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -883,30 +883,30 @@ int klp_register_patch(struct klp_patch *patch)
>  }
>  EXPORT_SYMBOL_GPL(klp_register_patch);
>  
> -static void klp_module_notify_coming(struct klp_patch *patch,
> +static int klp_module_notify_coming(struct klp_patch *patch,
>  				     struct klp_object *obj)
>  {
>  	struct module *pmod = patch->mod;
>  	struct module *mod = obj->mod;
> -	int ret;
> +	int ret = 0;
>  
>  	ret = klp_init_object_loaded(patch, obj);
>  	if (ret)
> -		goto err;
> +		goto out;
>  
>  	if (patch->state == KLP_DISABLED)
> -		return;
> +		goto out;
>  
>  	pr_notice("applying patch '%s' to loading module '%s'\n",
>  		  pmod->name, mod->name);
>  
>  	ret = klp_enable_object(obj);
> -	if (!ret)
> -		return;
>  
> -err:
> -	pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
> -		pmod->name, mod->name, ret);
> +out:
> +	if (ret)
> +		pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
> +				pmod->name, mod->name, ret);

I think this message should be extended. We failed to apply the patch to 
this object and we won't ever try that again. The user should know that.

Also note, that this warning is printed in two different situations. 
Either the initialization failed, or klp_enable_object failed. Maybe it 
would be nice to inform the user about that.

> +	return ret;
>  }
>
>  static void klp_module_notify_going(struct klp_patch *patch,
> @@ -930,6 +930,7 @@ disabled:
>  static int klp_module_notify(struct notifier_block *nb, unsigned long action,
>  			     void *data)
>  {
> +	int ret = 0;

Do we really need the initialization? klp_module_notify_coming returns the 
error or 0 by itself.

Thanks a lot,
Miroslav

>  	struct module *mod = data;
>  	struct klp_patch *patch;
>  	struct klp_object *obj;
> @@ -955,7 +956,9 @@ static int klp_module_notify(struct notifier_block *nb, unsigned long action,
>  
>  			if (action == MODULE_STATE_COMING) {
>  				obj->mod = mod;
> -				klp_module_notify_coming(patch, obj);
> +				ret = klp_module_notify_coming(patch, obj);
> +				if (ret)
> +					obj->mod = NULL;
>  			} else /* MODULE_STATE_GOING */
>  				klp_module_notify_going(patch, obj);
>  
> -- 
> 2.2.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/