| 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
| ||
|
Message-ID: <87a8zpy5wb.fsf@rustcorp.com.au>
Date: Sat, 07 Mar 2015 11:34:36 +1030
From: Rusty Russell <rusty@...tcorp.com.au>
To: Petr Mladek <pmladek@...e.cz>, Seth Jennings <sjenning@...hat.com>,
Josh Poimboeuf <jpoimboe@...hat.com>,
Jiri Kosina <jkosina@...e.cz>
Cc: 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, live-patching@...r.kernel.org,
linux-kernel@...r.kernel.org, andi@...stfloor.org,
rostedt@...dmis.org, tglx@...utronix.de,
Petr Mladek <pmladek@...e.cz>
Subject: Re: [PATCH v2 2/2] livepatch/module: Correctly handle going modules
Petr Mladek <pmladek@...e.cz> writes:
> Existing live patches are removed from going modules using a notify handler.
> There are two problems with the current implementation.
>
> First, new patch could still see the module in the GOING state even after
> the notifier has been called. It will try to initialize the related
> object structures but the module could disappear at any time. There will
> stay mess in the structures. It might even cause an invalid memory access.
>
> Second, if we start supporting patches with semantic changes between function
> calls, we would need to apply any new patch even for going modules. Note that
> the code from the module could be called even in the GOING state until
> mod->exit() finishes. See below for example.
I don't think you should handle going modules at all. Rarely happens,
and it should happen fast.
If you can hold the module_lock, the easiest thing to do is have us wake
module_wq when a module is freed, then you can just:
retry:
err = wait_event_interruptible(module_wq,
!modules_unloading());
if (err)
goto out;
/* Now re-check under lock. */
mutex_lock(&module_lock);
if (unlikely(modules_unloading()) {
mutex_unlock(&module_lock);
goto retry;
}
Cheers,
Rusty.
>
> This patch solves the problem by adding boolean flag into struct module.
> It is switched when the going module handler is called. It marks the point
> when it is safe and we actually have to ignore the going module.
>
> Alternative solutions:
>
> We could add another lock to make the switch to GOING state and mod->exit()
> call an atomic operation. But this a nogo. It might cause a dead lock when
> some mod->exit() depends on mod->exit() from another module.
>
> We could wait until the GOING module is moved to the UNFORMED state.
> But this might take ages when mod->exit() has to wait for something.
>
> We could refuse to load the patch when a module is going but this is
> pretty ugly.
>
> Example of the race:
>
> CPU0 CPU1
>
> delete_module() #SYSCALL
>
> try_stop_module()
> mod->state = MODULE_STATE_GOING;
>
> mutex_unlock(&module_mutex);
>
> klp_register_patch()
> klp_enable_patch()
>
> #save place to switch universe
>
> b() # from module that is going
> a() # from core (patched)
>
> mod->exit();
>
> Note that the function b() can be called until we call mod->exit().
>
> If we do not apply patch against b() because it is in MODULE_STATE_GOING.
> It will call patched a() with modified semantic and things might get wrong.
>
> Signed-off-by: Petr Mladek <pmladek@...e.cz>
> ---
> include/linux/module.h | 4 ++++
> kernel/livepatch/core.c | 30 ++++++++++++++++++++++++++----
> 2 files changed, 30 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/module.h b/include/linux/module.h
> index b653d7c0a05a..c12f93497b74 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -344,6 +344,10 @@ struct module {
> unsigned long *ftrace_callsites;
> #endif
>
> +#ifdef CONFIG_LIVEPATCH
> + bool klp_gone;
> +#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 198f7733604b..0b38357fad0f 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -89,16 +89,32 @@ 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;
>
> mutex_lock(&module_mutex);
> +
> + /*
> + * We do not want to block removal of patched modules and therefore
> + * we do not take a reference here. Instead, the patches are removed
> + * by the going module handler instead.
> + */
> + mod = find_module(obj->name);
> +
> /*
> - * We don't need to take a reference on the module here because we have
> - * the klp_mutex, which is also taken by the module notifier. This
> - * prevents any module from unloading until we release the klp_mutex.
> + * We must not init the object when the module is going and the notifier
> + * has already been called. But the patch might still be needed before.
> + * Note that module functions can be called even in the GOING state
> + * until mod->exit() finishes. This is especially important for patches
> + * that modify semantic of the functions.
> */
> - obj->mod = find_module(obj->name);
> + if (mod && mod->state == MODULE_STATE_GOING && mod->klp_gone)
> + mod = NULL;
> +
> + obj->mod = mod;
> +
> mutex_unlock(&module_mutex);
> }
>
> @@ -929,7 +945,10 @@ int klp_module_init(struct module *mod)
> int ret = 0;
>
> mutex_lock(&klp_mutex);
> +
> + mod->klp_gone = false;
> ret = klp_module_coming(mod);
> +
> mutex_unlock(&klp_mutex);
>
> return ret;
> @@ -985,7 +1004,10 @@ static int klp_module_notify_going(struct notifier_block *nb,
> return 0;
>
> mutex_lock(&klp_mutex);
> +
> klp_module_going(mod);
> + mod->klp_gone = true;
> +
> mutex_lock(&klp_mutex);
>
> return 0;
> --
> 1.8.5.6
--
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