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

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

[Minfei Huang]

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.

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);
+	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;
 	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

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

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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

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

[Minfei Huang]

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