[PATCH] livepatch: Prevent to enable uninitialized patch

[PATCH] livepatch: Prevent to enable uninitialized patch

[Minfei Huang]

From: Minfei Huang <minfei.huang@hotmail.com>

The previous patches can be applied, while the corresponding module is
loaded. Now the code cannot handle correct behavior to deal with the
case that the patch fail to be initialized when the module is being
loaded.

In general, the patch will do relocation (if necessary) and
obtain/verify function address before we start to enable patch. But we
can still trigger to enable the patch (disable the patch firstly, then
enable it), although the patch fail to be initialized in the function
klp_module_notify_coming.

To fix it, we can make obj->mod to NULL, if the object fails to be
initialized.

Signed-off-by: Minfei Huang <minfei.huang@hotmail.com>
---
 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] livepatch: Prevent to enable uninitialized patch

[Miroslav Benes]

On Mon, 11 May 2015, Minfei Huang wrote:

> From: Minfei Huang <minfei.huang@hotmail.com>
> 
> The previous patches can be applied, while the corresponding module is
> loaded. Now the code cannot handle correct behavior to deal with the
> case that the patch fail to be initialized when the module is being
> loaded.
> 
> In general, the patch will do relocation (if necessary) and
> obtain/verify function address before we start to enable patch. But we
> can still trigger to enable the patch (disable the patch firstly, then
> enable it), although the patch fail to be initialized in the function
> klp_module_notify_coming.
> 
> To fix it, we can make obj->mod to NULL, if the object fails to be
> initialized.
> 
> Signed-off-by: Minfei Huang <minfei.huang@hotmail.com>

Hi,

just to be sure, is the following what makes you worried?

The module comes and our notifier is called. We verify that it needs to be 
patched and we call klp_module_notify_coming where the object (for this 
module) is enabled. But that could fail somewhere and we print warning to 
the log (pr_warn). Now, you can disable and enable patch, during which the 
object for this very module is enabled again. And it could fail again.

Is this correct? Do you want to prevent printing of the warning again and 
again to the log? 

It could happen that the first enablement could fail because of something 
which would not be true for the second try. In such case the module would 
not be patched with your fix (it would be skipped in __klp_enable_patch 
loop).

It is possible that I do not understand the changelog and the patch 
correctly, so please shed some light on this if necessary...

Thanks,
Miroslav

> ---
>  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
> 
> --
> To unsubscribe from this list: send the line "unsubscribe live-patching" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

--
Miroslav Benes
SUSE Labs

Re: [PATCH] livepatch: Prevent to enable uninitialized patch

[Minfei Huang]

On 05/11/15 at 02:02P, Miroslav Benes wrote:
> On Mon, 11 May 2015, Minfei Huang wrote:
> 
> > From: Minfei Huang <minfei.huang@hotmail.com>
> > 
> > The previous patches can be applied, while the corresponding module is
> > loaded. Now the code cannot handle correct behavior to deal with the
> > case that the patch fail to be initialized when the module is being
> > loaded.
> > 
> > In general, the patch will do relocation (if necessary) and
> > obtain/verify function address before we start to enable patch. But we
> > can still trigger to enable the patch (disable the patch firstly, then
> > enable it), although the patch fail to be initialized in the function
> > klp_module_notify_coming.
> > 
> > To fix it, we can make obj->mod to NULL, if the object fails to be
> > initialized.
> > 

Hi, Miroslav.

This patch is used to prevent the patch to be enabled. I will use the
code to explain what I want to show you.

1) Patched a patch to fix the issue for module A.
2) livepatch will try to enable the patch, while the corresponding
module is loaded ( call klp_module_notify_coming )
3) Firstly, livepatch will do the instruction "obj->mod = mod", whatever
the result of klp_module_notify_coming is.
4) livepatch may fail to call the klp_init_object_loaded or
klp_enable_object
5) klp_module_notify_coming returns

6) For the userspace, we can enable the patch again ( disable the patch
firstly, then enable the patch from the sysfs )
7) In order to enable the patch, livepatch will call __klp_enable_patch
8) we can pass the limitation (klp_is_object_loaded), because the value
of obj->mod is not NULL ( the obj->mod obtains the value from the step 3 )
9) the patch may be applied, although the patch is not initialized, if
the value of func->old_addr is not NULL

>From the above description, we can see the uninitialized patch ( the
patch should be initialized by the klp_init_object_loaded in general )
can be applied to the kernel.

Thanks
Minfei

> > Signed-off-by: Minfei Huang <minfei.huang@hotmail.com>
> 
> Hi,
> 
> just to be sure, is the following what makes you worried?
> 
> The module comes and our notifier is called. We verify that it needs to be 
> patched and we call klp_module_notify_coming where the object (for this 
> module) is enabled. But that could fail somewhere and we print warning to 
> the log (pr_warn). Now, you can disable and enable patch, during which the 
> object for this very module is enabled again. And it could fail again.
> 
> Is this correct? Do you want to prevent printing of the warning again and 
> again to the log? 
> 
> It could happen that the first enablement could fail because of something 
> which would not be true for the second try. In such case the module would 
> not be patched with your fix (it would be skipped in __klp_enable_patch 
> loop).
> 
> It is possible that I do not understand the changelog and the patch 
> correctly, so please shed some light on this if necessary...
> 
> Thanks,
> Miroslav
> 
> > ---
> >  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
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe live-patching" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> 
> --
> Miroslav Benes
> SUSE Labs

Re: [PATCH] livepatch: Prevent to enable uninitialized patch

[Jiri Kosina]

On Mon, 11 May 2015, Minfei Huang wrote:

> 1) Patched a patch to fix the issue for module A.
> 2) livepatch will try to enable the patch, while the corresponding
> module is loaded ( call klp_module_notify_coming )
> 3) Firstly, livepatch will do the instruction "obj->mod = mod", whatever
> the result of klp_module_notify_coming is.
> 4) livepatch may fail to call the klp_init_object_loaded or
> klp_enable_object
> 5) klp_module_notify_coming returns
> 
> 6) For the userspace, we can enable the patch again ( disable the patch
> firstly, then enable the patch from the sysfs )
> 7) In order to enable the patch, livepatch will call __klp_enable_patch
> 8) we can pass the limitation (klp_is_object_loaded), because the value
> of obj->mod is not NULL ( the obj->mod obtains the value from the step 3 )
> 9) the patch may be applied, although the patch is not initialized, if
> the value of func->old_addr is not NULL
> 
> From the above description, we can see the uninitialized patch ( the
> patch should be initialized by the klp_init_object_loaded in general )
> can be applied to the kernel.

This indeed looks like a valid breakage scenario.

Could you please resend v2 of this patch with much more detailed 
description in the changelog? (i.e. some reformulated variation on the 
text above). Your original submission didn't describe the problem your 
patch is fixing at all.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] livepatch: Prevent to enable uninitialized patch

[Minfei Huang]

On 05/12/15 at 12:49P, Jiri Kosina wrote:
> On Mon, 11 May 2015, Minfei Huang wrote:
> 
> > 1) Patched a patch to fix the issue for module A.
> > 2) livepatch will try to enable the patch, while the corresponding
> > module is loaded ( call klp_module_notify_coming )
> > 3) Firstly, livepatch will do the instruction "obj->mod = mod", whatever
> > the result of klp_module_notify_coming is.
> > 4) livepatch may fail to call the klp_init_object_loaded or
> > klp_enable_object
> > 5) klp_module_notify_coming returns
> > 
> > 6) For the userspace, we can enable the patch again ( disable the patch
> > firstly, then enable the patch from the sysfs )
> > 7) In order to enable the patch, livepatch will call __klp_enable_patch
> > 8) we can pass the limitation (klp_is_object_loaded), because the value
> > of obj->mod is not NULL ( the obj->mod obtains the value from the step 3 )
> > 9) the patch may be applied, although the patch is not initialized, if
> > the value of func->old_addr is not NULL
> > 
> > From the above description, we can see the uninitialized patch ( the
> > patch should be initialized by the klp_init_object_loaded in general )
> > can be applied to the kernel.
> 
> This indeed looks like a valid breakage scenario.
> 
> Could you please resend v2 of this patch with much more detailed 
> description in the changelog? (i.e. some reformulated variation on the 
> text above). Your original submission didn't describe the problem your 
> patch is fixing at all.
> 
> Thanks,

Thanks for your review. I will repost a new patch.

Thanks
Minfei

> 
> -- 
> Jiri Kosina
> SUSE Labs

Re: [PATCH] livepatch: Prevent to enable uninitialized patch

[Miroslav Benes]

On Mon, 11 May 2015, Minfei Huang wrote:

> On 05/11/15 at 02:02P, Miroslav Benes wrote:
> > On Mon, 11 May 2015, Minfei Huang wrote:
> > 
> > > From: Minfei Huang <minfei.huang@hotmail.com>
> > > 
> > > The previous patches can be applied, while the corresponding module is
> > > loaded. Now the code cannot handle correct behavior to deal with the
> > > case that the patch fail to be initialized when the module is being
> > > loaded.
> > > 
> > > In general, the patch will do relocation (if necessary) and
> > > obtain/verify function address before we start to enable patch. But we
> > > can still trigger to enable the patch (disable the patch firstly, then
> > > enable it), although the patch fail to be initialized in the function
> > > klp_module_notify_coming.
> > > 
> > > To fix it, we can make obj->mod to NULL, if the object fails to be
> > > initialized.
> > > 
> 
> Hi, Miroslav.
> 
> This patch is used to prevent the patch to be enabled. I will use the
> code to explain what I want to show you.
> 
> 1) Patched a patch to fix the issue for module A.
> 2) livepatch will try to enable the patch, while the corresponding
> module is loaded ( call klp_module_notify_coming )
> 3) Firstly, livepatch will do the instruction "obj->mod = mod", whatever
> the result of klp_module_notify_coming is.
> 4) livepatch may fail to call the klp_init_object_loaded or
> klp_enable_object
> 5) klp_module_notify_coming returns
> 
> 6) For the userspace, we can enable the patch again ( disable the patch
> firstly, then enable the patch from the sysfs )
> 7) In order to enable the patch, livepatch will call __klp_enable_patch
> 8) we can pass the limitation (klp_is_object_loaded), because the value
> of obj->mod is not NULL ( the obj->mod obtains the value from the step 3 )
> 9) the patch may be applied, although the patch is not initialized, if
> the value of func->old_addr is not NULL
> 
> >From the above description, we can see the uninitialized patch ( the
> patch should be initialized by the klp_init_object_loaded in general )
> can be applied to the kernel.

Hi,

thanks for an explanation. This is really valid.

Concerning 9), func->old_addr should not be used for the modules (I know 
we had some discussion about that). So it could happen that func->old_addr 
is not initialized as you describe and the user gets warning from 
klp_enable_func. This should be fixed as you proposed.

Please resend as Jiri requested

Thanks
Miroslav

> 
> Thanks
> Minfei
> 
> > > Signed-off-by: Minfei Huang <minfei.huang@hotmail.com>
> > 
> > Hi,
> > 
> > just to be sure, is the following what makes you worried?
> > 
> > The module comes and our notifier is called. We verify that it needs to be 
> > patched and we call klp_module_notify_coming where the object (for this 
> > module) is enabled. But that could fail somewhere and we print warning to 
> > the log (pr_warn). Now, you can disable and enable patch, during which the 
> > object for this very module is enabled again. And it could fail again.
> > 
> > Is this correct? Do you want to prevent printing of the warning again and 
> > again to the log? 
> > 
> > It could happen that the first enablement could fail because of something 
> > which would not be true for the second try. In such case the module would 
> > not be patched with your fix (it would be skipped in __klp_enable_patch 
> > loop).
> > 
> > It is possible that I do not understand the changelog and the patch 
> > correctly, so please shed some light on this if necessary...
> > 
> > Thanks,
> > Miroslav
> > 
> > > ---
> > >  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
> > > 
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe live-patching" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > 
> > 
> > --
> > Miroslav Benes
> > SUSE Labs
>