linux-kernel - Re: livepatch/module: remove livepatch module notifier

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:	Mon, 08 Feb 2016 11:04:52 +1030
From:	Rusty Russell <rusty@...tcorp.com.au>
To:	Jessica Yu <jeyu@...hat.com>, Petr Mladek <pmladek@...e.com>
Cc:	Josh Poimboeuf <jpoimboe@...hat.com>,
	Seth Jennings <sjenning@...hat.com>,
	Jiri Kosina <jikos@...nel.org>,
	Vojtech Pavlik <vojtech@...e.com>,
	Miroslav Benes <mbenes@...e.cz>,
	Steven Rostedt <rostedt@...dmis.org>,
	Ingo Molnar <mingo@...hat.com>, live-patching@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: livepatch/module: remove livepatch module notifier

Jessica Yu <jeyu@...hat.com> writes:
> +++ Petr Mladek [04/02/16 15:39 +0100]:
>>On Mon 2016-02-01 20:17:36, Jessica Yu wrote:
> [ snipped since email is getting long ]
>>> diff --git a/kernel/module.c b/kernel/module.c
>>> index b05d466..71c77ed 100644
>>> --- a/kernel/module.c
>>> +++ b/kernel/module.c
>>> @@ -53,6 +53,7 @@
>>>  #include <asm/sections.h>
>>>  #include <linux/tracepoint.h>
>>>  #include <linux/ftrace.h>
>>> +#include <linux/livepatch.h>
>>>  #include <linux/async.h>
>>>  #include <linux/percpu.h>
>>>  #include <linux/kmemleak.h>
>>> @@ -981,6 +982,7 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
>>>  		mod->exit();
>>>  	blocking_notifier_call_chain(&module_notify_list,
>>>  				     MODULE_STATE_GOING, mod);
>>> +	klp_module_disable(mod);
>>>  	ftrace_release_mod(mod);
>>>
>>>  	async_synchronize_full();
>>> @@ -3297,6 +3299,7 @@ fail:
>>>  	module_put(mod);
>>>  	blocking_notifier_call_chain(&module_notify_list,
>>>  				     MODULE_STATE_GOING, mod);
>>> +	klp_module_disable(mod);
>>>  	ftrace_release_mod(mod);
>>>  	free_module(mod);
>>>  	wake_up_all(&module_wq);
>>> @@ -3375,6 +3378,10 @@ static int complete_formation(struct module *mod, struct load_info *info)
>>>  	mutex_unlock(&module_mutex);
>>>
>>>  	ftrace_module_enable(mod);
>>> +	err = klp_module_enable(mod);
>>> +	if (err)
>>> +		goto out;
>>
>>If you go out here, you need to revert some some operations
>>that are normally done in the bug_cleanup: goto target
>>in load_module(). In particular, you need to do:
>>
>>	/* module_bug_cleanup needs module_mutex protection */
>>	mutex_lock(&module_mutex);
>>	module_bug_cleanup(mod);
>>	mutex_unlock(&module_mutex);
>>
>>	ftrace_release_mod(mod);
>>
>>	/* we can't deallocate the module until we clear memory protection */
>>	module_disable_ro(mod);
>>	module_disable_nx(mod);
>>
>>
>>IMHO, it would make sense to somehow split the complete_formation() function
>>and avoid a code duplication in the error paths.
>
> Argh, thank you for catching that. I think we could split up complete_formation()
> into two functions in order to make the error handling work. 
>
> We could probably take out the coming notifier calls, ftrace_module_enable(),
> and klp_module_enable() out of complete_formation(), and put them in another
> function, maybe called prepare_coming_module(), that would be called right
> after complete_formation(). It might look something like this:
>
> @@ -3614,6 +3621,9 @@ static int load_module(struct load_info *info, const char __user *uargs,
>         err = complete_formation(mod, info); 
>         if (err)
>                 goto ddebug_cleanup;
> +       err = prepare_coming_module(mod); // calls ftrace_module_enable(), klp_module_enable(), then coming notifiers
> +       if (err) // means that klp_module_enable failed
> +               goto bug_cleanup;
>  
>         /* Module is ready to execute: parsing args may do that. */
>         after_dashes = parse_args(mod->name, mod->args, mod->kp, mod->num_kp,
> @@ -3621,7 +3631,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
>                                   unknown_module_param_cb);
>         if (IS_ERR(after_dashes)) {
>                 err = PTR_ERR(after_dashes);
> -               goto bug_cleanup;
> +               goto coming_cleanup;
>         } else if (after_dashes) {
>                 pr_warn("%s: parameters '%s' after `--' ignored\n",
>                        mod->name, after_dashes);
>
> Now for the error conditions. If complete_formation() fails, goto
> ddebug_cleanup. If prepare_coming_module() fails (at that point,
> module_enable_{ro,nx} and module_bug_finalize() have already finished), goto
> bug_cleanup. Everything else that fails afterwards (meaning klp_module_enable,
> ftrace_module_enable, and the coming notifiers have finished) goto
> coming_cleanup. ftrace_release_mod() gets called in the goto free_module label
> so we don't have to call it in coming_module.

Sounds good.

> Also, one last thing, I noticed that module->state isn't
> set to MODULE_STATE_GOING anywhere before the going notifier chain is called in
> the bug_cleanup label (I think it is still COMING at that point), so the
> klp_module_disable call right afterwards would have bailed out because of that.
> To be consistent, shouldn't it be set before the going notifiers are called?

Good spotting.  You could argue that there's a difference between the
notifier argument (what we're doing) and the module state (how far it
got), but we do set 'mod->state = MODULE_STATE_GOING;' in the initcall
fail case, so this should be the same.

Patch welcome :)

Thanks,
Rusty.