[PATCH 0/3] Deny the patched modules to be removed

[PATCH 0/3] Deny the patched modules to be removed

[Miroslav Benes]

Quoting the second patch's changelog:

    Josh reported a bug:
    
      When the object to be patched is a module, and that module is
      rmmod'ed and reloaded, it fails to load with:
    
      module: x86/modules: Skipping invalid relocation target, existing value is nonzero for type 2, loc 00000000ba0302e9, val ffffffffa03e293c
      livepatch: failed to initialize patch 'livepatch_nfsd' for module 'nfsd' (-8)
      livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing to load module 'nfsd'
    
      The livepatch module has a relocation which references a symbol
      in the _previous_ loading of nfsd. When apply_relocate_add()
      tries to replace the old relocation with a new one, it sees that
      the previous one is nonzero and it errors out.
    
      On ppc64le, we have a similar issue:
    
      module_64: livepatch_nfsd: Expected nop after call, got e8410018 at e_show+0x60/0x548 [livepatch_nfsd]
      livepatch: failed to initialize patch 'livepatch_nfsd' for module 'nfsd' (-8)
      livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing to load module 'nfsd'
    
    He also proposed three different solutions. We could remove the error
    check in apply_relocate_add() introduced by commit eda9cec4c9a1
    ("x86/module: Detect and skip invalid relocations"). However the check
    is useful for detecting corrupted modules.
    
    We could also reverse the relocation patching (clear all relocation
    targets on x86_64, or return back nops on powerpc) in
    klp_unpatch_object(). The solution is not universal and is too much
    arch-specific.
    
    We decided to deny the patched modules to be removed. If it proves to be
    a major drawback for users, we can still implement a different approach.

There were many subtle details to check, so it may be broken somewhere.
Hopefully it is not, but please review carefully.

Miroslav Benes (3):
  livepatch: Nullify obj->mod in klp_module_coming()'s error path
  livepatch: Deny the patched modules to be removed
  module: Remove superfluous call to klp_module_going()

 kernel/livepatch/core.c | 25 +++++++++++++++++++++++--
 kernel/module.c         |  1 -
 2 files changed, 23 insertions(+), 3 deletions(-)

-- 
2.17.0

[PATCH 1/3] livepatch: Nullify obj->mod in klp_module_coming()'s error path

[Miroslav Benes]

klp_module_coming() is called for every module appearing in the system.
It sets obj->mod to a patched module for klp_object obj. Unfortunately
it leaves it set even if an error happens later in the function and the
patched module is not allowed to be loaded.

klp_is_object_loaded() uses obj->mod variable and could currently give a
wrong return value. The bug is probably harmless as of now, but we're
gonna rely on klp_is_object_loaded() and correct obj->mod much more and
the bug would be visible then.

Signed-off-by: Miroslav Benes <mbenes@suse.cz>
---
 kernel/livepatch/core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 3a4656fb7047..36eb5cf38766 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -1015,6 +1015,7 @@ int klp_module_coming(struct module *mod)
 	pr_warn("patch '%s' failed for module '%s', refusing to load module '%s'\n",
 		patch->mod->name, obj->mod->name, obj->mod->name);
 	mod->klp_alive = false;
+	obj->mod = NULL;
 	klp_cleanup_module_patches_limited(mod, patch);
 	mutex_unlock(&klp_mutex);
 
-- 
2.17.0

[PATCH 2/3] livepatch: Deny the patched modules to be removed

[Miroslav Benes]

Josh reported a bug:

  When the object to be patched is a module, and that module is
  rmmod'ed and reloaded, it fails to load with:

  module: x86/modules: Skipping invalid relocation target, existing value is nonzero for type 2, loc 00000000ba0302e9, val ffffffffa03e293c
  livepatch: failed to initialize patch 'livepatch_nfsd' for module 'nfsd' (-8)
  livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing to load module 'nfsd'

  The livepatch module has a relocation which references a symbol
  in the _previous_ loading of nfsd. When apply_relocate_add()
  tries to replace the old relocation with a new one, it sees that
  the previous one is nonzero and it errors out.

  On ppc64le, we have a similar issue:

  module_64: livepatch_nfsd: Expected nop after call, got e8410018 at e_show+0x60/0x548 [livepatch_nfsd]
  livepatch: failed to initialize patch 'livepatch_nfsd' for module 'nfsd' (-8)
  livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing to load module 'nfsd'

He also proposed three different solutions. We could remove the error
check in apply_relocate_add() introduced by commit eda9cec4c9a1
("x86/module: Detect and skip invalid relocations"). However the check
is useful for detecting corrupted modules.

We could also reverse the relocation patching (clear all relocation
targets on x86_64, or return back nops on powerpc) in
klp_unpatch_object(). The solution is not universal and is too much
arch-specific.

We decided to deny the patched modules to be removed. If it proves to be
a major drawback for users, we can still implement a different approach.

The reference of a patched module has to be taken regardless of a
patch's state. Thus it is not taken and dropped in enable/disable paths,
but in register/unregister paths.

The new behaviour should not collide with mod->klp_alive variable.

Reported-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Miroslav Benes <mbenes@suse.cz>
---
 kernel/livepatch/core.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 36eb5cf38766..2fa9eaee7bb5 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -645,6 +645,10 @@ static void klp_free_object_loaded(struct klp_object *obj)
 {
 	struct klp_func *func;
 
+	/*
+	 * Drop the reference from klp_init_object_loaded()
+	 */
+	module_put(obj->mod);
 	obj->mod = NULL;
 
 	klp_for_each_func(obj, func)
@@ -663,6 +667,9 @@ static void klp_free_objects_limited(struct klp_patch *patch,
 	for (obj = patch->objs; obj->funcs && obj != limit; obj++) {
 		klp_free_funcs_limited(obj, NULL);
 		kobject_put(&obj->kobj);
+
+		if (klp_is_module(obj) && klp_is_object_loaded(obj))
+			module_put(obj->mod);
 	}
 }
 
@@ -739,6 +746,14 @@ static int klp_init_object_loaded(struct klp_patch *patch,
 		}
 	}
 
+	/*
+	 * Do not allow patched modules to be removed.
+	 *
+	 * All callers of klp_init_object_loaded() set obj->mod.
+	 */
+	if (klp_is_module(obj) && !try_module_get(obj->mod))
+		return -ENODEV;
+
 	return 0;
 }
 
@@ -984,7 +999,7 @@ int klp_module_coming(struct module *mod)
 			if (ret) {
 				pr_warn("pre-patch callback failed for object '%s'\n",
 					obj->name);
-				goto err;
+				goto err_after_init;
 			}
 
 			ret = klp_patch_object(obj);
@@ -993,7 +1008,7 @@ int klp_module_coming(struct module *mod)
 					patch->mod->name, obj->mod->name, ret);
 
 				klp_post_unpatch_callback(obj);
-				goto err;
+				goto err_after_init;
 			}
 
 			if (patch != klp_transition_patch)
@@ -1007,6 +1022,11 @@ int klp_module_coming(struct module *mod)
 
 	return 0;
 
+err_after_init:
+	/*
+	 * Drop the reference from klp_init_object_loaded().
+	 */
+	module_put(obj->mod);
 err:
 	/*
 	 * If a patch is unsuccessfully applied, return
-- 
2.17.0

[PATCH 3/3] module: Remove superfluous call to klp_module_going()

[Miroslav Benes]

Now that patched modules cannot suddenly disappear we could
theoretically remove klp_module_going() altogether. Unfortunately we
cannot do that in practice. Loading of a patched module may fail and we
need to execute right the actions implemented in klp_module_going().

Remove the call from delete_module syscall though, because that one is
really superfluous.

Signed-off-by: Miroslav Benes <mbenes@suse.cz>
---
 kernel/module.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/kernel/module.c b/kernel/module.c
index a6e43a5806a1..af29a0d3708f 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1019,7 +1019,6 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
 		mod->exit();
 	blocking_notifier_call_chain(&module_notify_list,
 				     MODULE_STATE_GOING, mod);
-	klp_module_going(mod);
 	ftrace_release_mod(mod);
 
 	async_synchronize_full();
-- 
2.17.0

Re: [PATCH 1/3] livepatch: Nullify obj->mod in klp_module_coming()'s error path

[Petr Mladek]

On Thu 2018-06-07 11:29:47, Miroslav Benes wrote:
> klp_module_coming() is called for every module appearing in the system.
> It sets obj->mod to a patched module for klp_object obj. Unfortunately
> it leaves it set even if an error happens later in the function and the
> patched module is not allowed to be loaded.
> 
> klp_is_object_loaded() uses obj->mod variable and could currently give a
> wrong return value. The bug is probably harmless as of now, but we're
> gonna rely on klp_is_object_loaded() and correct obj->mod much more and
> the bug would be visible then.
> 
> Signed-off-by: Miroslav Benes <mbenes@suse.cz>

Great catch!

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [PATCH 3/3] module: Remove superfluous call to klp_module_going()

[Jessica Yu]

+++ Miroslav Benes [07/06/18 11:29 +0200]:
>Now that patched modules cannot suddenly disappear we could
>theoretically remove klp_module_going() altogether. Unfortunately we
>cannot do that in practice. Loading of a patched module may fail and we
>need to execute right the actions implemented in klp_module_going().
>
>Remove the call from delete_module syscall though, because that one is
>really superfluous.
>
>Signed-off-by: Miroslav Benes <mbenes@suse.cz>

Acked-by: Jessica Yu <jeyu@kernel.org>

>---
> kernel/module.c | 1 -
> 1 file changed, 1 deletion(-)
>
>diff --git a/kernel/module.c b/kernel/module.c
>index a6e43a5806a1..af29a0d3708f 100644
>--- a/kernel/module.c
>+++ b/kernel/module.c
>@@ -1019,7 +1019,6 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
> 		mod->exit();
> 	blocking_notifier_call_chain(&module_notify_list,
> 				     MODULE_STATE_GOING, mod);
>-	klp_module_going(mod);
> 	ftrace_release_mod(mod);
>
> 	async_synchronize_full();
>-- 
>2.17.0
>

Re: [PATCH 2/3] livepatch: Deny the patched modules to be removed

[Petr Mladek]

On Thu 2018-06-07 11:29:48, Miroslav Benes wrote:
> We decided to deny the patched modules to be removed. If it proves to be
> a major drawback for users, we can still implement a different approach.
> 
> The reference of a patched module has to be taken regardless of a
> patch's state. Thus it is not taken and dropped in enable/disable paths,
> but in register/unregister paths.

> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -739,6 +746,14 @@ static int klp_init_object_loaded(struct klp_patch *patch,
>  		}
>  	}
>  
> +	/*
> +	 * Do not allow patched modules to be removed.
> +	 *
> +	 * All callers of klp_init_object_loaded() set obj->mod.
> +	 */
> +	if (klp_is_module(obj) && !try_module_get(obj->mod))
> +		return -ENODEV;
> +
>  	return 0;
>  }

This looks strange. We try to take the reference after we do all
actions needed for a loaded module. There is nothing that would
prevent obj->mod to get into the going state.

Note that it was safe (except for the relocated symbols) because
the module was not able to really go until it called
klp_module_going(). But you removed this last break
by the 3rd patch.


I suggest another approach:

I would rename klp_find_object_module() to klp_get_object_module()
and get the reference there. Note that it should be fine to call
it later in klp_init_object() (before klp_init_object_loaded()).
This would solve klp_init_patch().

We will also need to get the reference in klp_module_coming().
It can be called anywhere there. If it fails, we will block
loading the module.


Note that the mod->klp_alive logic will still be necessary.
It solves various races when both the patch and the patched
module are loaded in parallel.

Sigh, this also means that we still could call klp_init_object_loaded()
and apply reloacations even when the patched module fails to loaded
later from other reasons. This means that this approach does not
solve the original problem completely.

I wonder how complicated would be the arch-specific part that
would clean up relocations when the module is unloaded.

Best Regards,
Petr

Re: [PATCH 2/3] livepatch: Deny the patched modules to be removed

[Miroslav Benes]

On Mon, 11 Jun 2018, Petr Mladek wrote:

> On Thu 2018-06-07 11:29:48, Miroslav Benes wrote:
> > We decided to deny the patched modules to be removed. If it proves to be
> > a major drawback for users, we can still implement a different approach.
> > 
> > The reference of a patched module has to be taken regardless of a
> > patch's state. Thus it is not taken and dropped in enable/disable paths,
> > but in register/unregister paths.
> 
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -739,6 +746,14 @@ static int klp_init_object_loaded(struct klp_patch *patch,
> >  		}
> >  	}
> >  
> > +	/*
> > +	 * Do not allow patched modules to be removed.
> > +	 *
> > +	 * All callers of klp_init_object_loaded() set obj->mod.
> > +	 */
> > +	if (klp_is_module(obj) && !try_module_get(obj->mod))
> > +		return -ENODEV;
> > +
> >  	return 0;
> >  }
> 
> This looks strange. We try to take the reference after we do all
> actions needed for a loaded module. There is nothing that would
> prevent obj->mod to get into the going state.

Yes, there was an intention to keep the number of necessary modifications 
of the error paths as low as possible.
 
> Note that it was safe (except for the relocated symbols) because
> the module was not able to really go until it called
> klp_module_going(). But you removed this last break
> by the 3rd patch.

Yes, try_module_get() would return an error here.
 
> I suggest another approach:
> 
> I would rename klp_find_object_module() to klp_get_object_module()
> and get the reference there. Note that it should be fine to call
> it later in klp_init_object() (before klp_init_object_loaded()).
> This would solve klp_init_patch().
> 
> We will also need to get the reference in klp_module_coming().
> It can be called anywhere there. If it fails, we will block
> loading the module.

I can as well move the above hunk up in klp_init_object_loaded(), no? Not 
that I'm seeing a problem to have it in klp_find_object_module().
 
> Note that the mod->klp_alive logic will still be necessary.
> It solves various races when both the patch and the patched
> module are loaded in parallel.
>
> Sigh, this also means that we still could call klp_init_object_loaded()
> and apply reloacations even when the patched module fails to loaded
> later from other reasons. This means that this approach does not
> solve the original problem completely.

That would be in klp_module_coming(). Hm, you're right.
 
> I wonder how complicated would be the arch-specific part that
> would clean up relocations when the module is unloaded.

I don't think it would be complicated. We just want to avoid it, because 
it is arch-specific. It is more difficult to maintain such code.

Regards,
Miroslav