| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 3 Mar 2015 08:55:17 -0600
From: Josh Poimboeuf <jpoimboe@...hat.com>
To: Petr Mladek <pmladek@...e.cz>
Cc: Seth Jennings <sjenning@...hat.com>, Jiri Kosina <jkosina@...e.cz>,
Rusty Russell <rusty@...tcorp.com.au>,
Miroslav Benes <mbenes@...e.cz>,
Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>,
mingo@...nel.org, mathieu.desnoyers@...icios.com, oleg@...hat.com,
paulmck@...ux.vnet.ibm.com, linux-kernel@...r.kernel.org,
andi@...stfloor.org, rostedt@...dmis.org, tglx@...utronix.de
Subject: Re: [RFC PATCH] livepatch/module: Do not patch modules that are not
ready
Hi Petr,
Good find. Some comments below.
On Tue, Mar 03, 2015 at 12:38:29PM +0100, Petr Mladek wrote:
> There is a notifier that handles live patches for coming and going modules.
> It takes klp_mutex lock to avoid races with coming and going patches.
>
> Unfortunately, there are some possible races in the current implementation.
> The problem is that we do not keep the klp_mutex lock all the time when
> the module is being added or removed.
>
> First, the module is visible even before ftrace is ready. If we enable a patch
> in this time frame, adding ftrace ops will fail and the patch will get rejected
> just because bad timing.
>
> Second, if we are "lucky" and enable the patch for the coming module when the
> ftrace is ready but before the module notifier has been called.
Based on the notifier priorities, it looks like the ftrace notifier gets
called last, so I think this particular case can't happen.
> The notifier
> will try to enable the patch as well. It will detect that it is already patched,
> return error, and the module will get rejected just because bad timing.
> The more serious problem is that it will not call the notifier for
> going module, so that the mess will stay there and we wont be able to load
> the module later.
>
> Third, similar problems are there for going module. If a patch is enabled after
> the notifier finishes but before the module is removed from the list of modules,
> the new patch will be applied to the module. The module might disappear at
> anytime when the patch enabling is in progress, so there might be an access out
> of memory. Or the whole patch might be applied and some mess will be left,
> so it will not be possible to load/patch the module again.
>
> This patch solves the problem by adding two flags into struct module. They are
> switched when the notifier is called. Note that we try to solve a race with a
> coming patch, therefore we do not know which modules will get patched and we
> need to monitor all modules. This is why I added this to the struct module.
>
> The flags are set and checked under the klp_mutex lock. The related operation
> is finished under the same lock. Therefore they are properly serialized now.
>
> Note that the patch solves only the situation when a new patch is registered or
> enabled.
Did we have a reason for calling klp_find_object_module() in both
register and enable? I'd think we'd only need it for the register path,
since the notifier would catch any future loads/unloads. Or am I
missing something?
> There are no such problems when the patch is being removed. it does
> not matter who disable the patch first, whether the normal disable_patch() or
> the module notifier. There is nothing to do once the patch is disabled.
>
> Signed-off-by: Petr Mladek <pmladek@...e.cz>
> ---
> include/linux/module.h | 5 +++++
> kernel/livepatch/core.c | 20 +++++++++++++++++++-
> kernel/module.c | 6 +++++-
> 3 files changed, 29 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/module.h b/include/linux/module.h
> index b653d7c0a05a..7e50d87da510 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -344,6 +344,11 @@ struct module {
> unsigned long *ftrace_callsites;
> #endif
>
> +#ifdef CONFIG_LIVEPATCH
> + bool klp_patched;
> + bool klp_unpatched;
> +#endif
> +
> #ifdef CONFIG_MODULE_UNLOAD
> /* What modules depend on me? */
> struct list_head source_list;
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index a664e485365f..dee4bbcb60e6 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -89,6 +89,8 @@ static bool klp_is_object_loaded(struct klp_object *obj)
> /* sets obj->mod if object is not vmlinux and module is found */
> static void klp_find_object_module(struct klp_object *obj)
> {
> + struct module *mod;
> +
> if (!klp_is_module(obj))
> return;
>
> @@ -98,7 +100,14 @@ static void klp_find_object_module(struct klp_object *obj)
> * the klp_mutex, which is also taken by the module notifier. This
> * prevents any module from unloading until we release the klp_mutex.
> */
> - obj->mod = find_module(obj->name);
> + mod = find_module(obj->name);
> + /* Do not mess work of the module notifier */
> + if ((mod->state == MODULE_STATE_COMING && !mod->klp_patched) ||
> + (mod->state == MODULE_STATE_GOING && mod->klp_unpatched))
> + obj->mod = NULL;
> + else
> + obj->mod = mod;
> +
> mutex_unlock(&module_mutex);
> }
>
Why do we need two flags for this? The notifer already
sets/clears obj->mod, so can we rely on the value obj->mod to determine
if the notifier already ran?
For example:
@@ -89,7 +89,7 @@ static bool klp_is_object_loaded(struct klp_object *obj)
/* sets obj->mod if object is not vmlinux and module is found */
static void klp_find_object_module(struct klp_object *obj)
{
- if (!klp_is_module(obj))
+ if (!klp_is_module(obj) || obj->mod)
return;
mutex_lock(&module_mutex);
@@ -98,7 +98,9 @@ static void klp_find_object_module(struct klp_object *obj)
* the klp_mutex, which is also taken by the module notifier. This
* prevents any module from unloading until we release the klp_mutex.
*/
- obj->mod = find_module(obj->name);
+ mod = find_module(obj->name);
+ if (mod->state == MODULE_STATE_LIVE)
+ obj->mod = mod;
mutex_unlock(&module_mutex);
}
> @@ -927,6 +936,15 @@ static int klp_module_notify(struct notifier_block *nb, unsigned long action,
>
> mutex_lock(&klp_mutex);
>
> + /*
> + * Each module has to know that the notifier has been called.
> + * We never know what module will get patched by a new patch.
> + */
> + if (action == MODULE_STATE_COMING)
> + mod->klp_patched = true;
> + else
> + mod->klp_unpatched = true;
> +
> list_for_each_entry(patch, &klp_patches, list) {
> for (obj = patch->objs; obj->funcs; obj++) {
> if (!klp_is_module(obj) || strcmp(obj->name, mod->name))
> diff --git a/kernel/module.c b/kernel/module.c
> index d856e96a3cce..8357f15b7ed0 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -852,7 +852,6 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
>
> /* Store the name of the last unloaded module for diagnostic purposes */
> strlcpy(last_unloaded_module, mod->name, sizeof(last_unloaded_module));
> -
> free_module(mod);
> return 0;
> out:
Gratuitous whitespace change.
> @@ -3271,6 +3270,11 @@ static int load_module(struct load_info *info, const char __user *uargs,
> }
> #endif
>
> +#ifdef CONFIG_LIVEPATCH
> + mod->klp_patched = false;
> + mod->klp_unpatched = false;
> +#endif
> +
> /* To avoid stressing percpu allocator, do this once we're unique. */
> err = percpu_modalloc(mod, info);
> if (err)
> --
> 1.8.5.6
>
--
Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists