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: <87k3afdvn3.fsf@rustcorp.com.au>
Date:	Thu, 24 Apr 2014 17:08:56 +0930
From:	Rusty Russell <rusty@...tcorp.com.au>
To:	Steven Rostedt <rostedt@...dmis.org>
Cc:	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>,
	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 Tue, 22 Apr 2014 13:21:18 +0930
> Rusty Russell <rusty@...tcorp.com.au> wrote:
>
>
>> Sorry, was on paternity leave.
>
> Congratulations! Although having two teenage daughters myself, I'm more
> inclined to say "my condolences".

Thanks... I think!

>> I'm always nervous about adding more states, since every place which
>> examines the state has to be audited.
>
> I didn't really add a state but instead broke an existing state into
> two. More importantly, this second part of the state doesn't get
> exported to other parts of the kernel. That is, there is no notifier
> for it.
>
> This means the only place you need to audit is in module.c itself. Any
> other change requires auditing all module notifiers.

Yes, we only need to check everywhere which looks at mod->state.

>> 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.
>
> Well, NX is a different issue here all together. Sure if you want to,
> but that wont affect this.

RO and NX get set together in the module code, but you're right, it's
the RO which affects you.

>> +	/* 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);
>
> Here we break ftrace. ftrace expects that it can still replaces the
> calls to mcount with nops here. If the text is RO, then it will crash.

I think we should apply a patch like the above for other reasons, so...

What if we call the notifier before setting ro_nx, and then set the
state after the notifier?

OTOH, if it's just ftrace (do tracepoints have an issue?) I'd rather
hardcode a ftrace_init_module() call in exactly the right place.
Notifiers which are sensitive to their exact call location tend give me
the creeps...

Cheers,
Rusty.
--
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/