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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Miroslav Benes <[email protected]>
---
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
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 <[email protected]>
Great catch!
Reviewed-by: Petr Mladek <[email protected]>
Best Regards,
Petr
+++ 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 <[email protected]>
Acked-by: Jessica Yu <[email protected]>
>---
> 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
>
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
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