linux-kernel - Re: ftrace/kprobes: Warning when insmod two modules

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <87bnvunhs9.fsf@rustcorp.com.au>
Date:	Tue, 22 Apr 2014 13:21:18 +0930
From:	Rusty Russell <rusty@...tcorp.com.au>
To:	Steven Rostedt <rostedt@...dmis.org>,
	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>
Cc:	Takao Indoh <indou.takao@...fujitsu.com>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Ingo Molnar <mingo@...hat.com>,
	Ananth N Mavinakayanahalli <ananth@...ibm.com>,
	Anil S Keshavamurthy <anil.s.keshavamurthy@...el.com>,
	"David S. Miller" <davem@...emloft.net>,
	linux-kernel@...r.kernel.org
Subject: Re: ftrace/kprobes: Warning when insmod two modules

Steven Rostedt <rostedt@...dmis.org> writes:
> On Mon, 24 Mar 2014 20:26:05 +0900
> Masami Hiramatsu <masami.hiramatsu.pt@...achi.com> wrote:
>
>
>> Thank you for reporting with this pretty backtrace :)
>> Steven, I think this is not the kprobe bug but ftrace (and perhaps, module).
>
> Looks to be more of a module issue than a ftrace issue.
>
>> 
>> If the ftrace can set loading module text read only before the module subsystem
>> expected, I think it should be protected by the module subsystem itself
>> (e.g. set_all_modules_text_ro(rw) skips the modules which is MODULE_STATE_COMING)
>> 
>
> Does this patch fix it?
>
> In-review-off-by: Steven Rostedt <rostedt@...dmis.org>

Sorry, was on paternity leave.

I'm always nervous about adding more states, since every place which
examines the state has to be audited.

We set the mod->state to MOD_STATE_COMING in complete_formation;
why don't we set NX there instead?  It also makes more sense to
set NX before we hit parse_args() which can execute code in the module.

In fact, we should probably call the notifier there too, so people
can breakpoint/tracepoint/etc parameter calls.

Of course, this means that we set NX before the notifier; does anything
break?

Subject: module: set nx before marking module MODULE_STATE_COMING.

This prevents a WARN_ON() where ftrace calls set_all_modules_text_ro()
which races with the module setting its own set_section_ro_nx().

It also means we're NX protected before we call parse_args(), which
can execute module code.

This means that the notifiers will be called on a module which
is already NX, so that may cause problems.

Signed-off-by: Rusty Russell <rusty@...tcorp.com.au>

diff --git a/kernel/module.c b/kernel/module.c
index 11869408f79b..83a437e5d429 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3023,21 +3023,6 @@ static int do_init_module(struct module *mod)
 	 */
 	current->flags &= ~PF_USED_ASYNC;
 
-	blocking_notifier_call_chain(&module_notify_list,
-			MODULE_STATE_COMING, mod);
-
-	/* Set RO and NX regions for core */
-	set_section_ro_nx(mod->module_core,
-				mod->core_text_size,
-				mod->core_ro_size,
-				mod->core_size);
-
-	/* Set RO and NX regions for init */
-	set_section_ro_nx(mod->module_init,
-				mod->init_text_size,
-				mod->init_ro_size,
-				mod->init_size);
-
 	do_mod_ctors(mod);
 	/* Start the module */
 	if (mod->init != NULL)
@@ -3168,9 +3153,26 @@ static int complete_formation(struct module *mod, struct load_info *info)
 	/* This relies on module_mutex for list integrity. */
 	module_bug_finalize(info->hdr, info->sechdrs, mod);
 
+	/* Set RO and NX regions for core */
+	set_section_ro_nx(mod->module_core,
+				mod->core_text_size,
+				mod->core_ro_size,
+				mod->core_size);
+
+	/* Set RO and NX regions for init */
+	set_section_ro_nx(mod->module_init,
+				mod->init_text_size,
+				mod->init_ro_size,
+				mod->init_size);
+
 	/* Mark state as coming so strong_try_module_get() ignores us,
 	 * but kallsyms etc. can see us. */
 	mod->state = MODULE_STATE_COMING;
+	mutex_unlock(&module_mutex);
+
+	blocking_notifier_call_chain(&module_notify_list,
+				     MODULE_STATE_COMING, mod);
+	return 0;
 
 out:
 	mutex_unlock(&module_mutex);
--
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/