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

[Minfei Huang <mnfhuang@gmail.com>]

On 05/12/15 at 01:41P, Miroslav Benes wrote:
> 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.

The function may be registered successfully by ftrace, although the
function address is incorrect.

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

Thanks for your effort about the patch log. It is more clearly to the
patch log as you suggested.

I will modify the patch log, then post the next version.

> 
> See few more remarks below.
> 
> > 
> > Signed-off-by: Minfei Huang <mnfhuang@gmail.com>
> > ---
> > 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.

I will modify the error message.

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

Will modify.

Thanks
Minfei