linux-kernel - Re: [PATCH v2 1/2] kprobes: propagate error from arm_kprobe

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]

Date:   Sat, 4 Nov 2017 05:35:20 +0900
From:   Masami Hiramatsu <mhiramat@...nel.org>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     Josh Poimboeuf <jpoimboe@...hat.com>, Jessica Yu <jeyu@...nel.org>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Ananth N Mavinakayanahalli <ananth@...ux.vnet.ibm.com>,
        Anil S Keshavamurthy <anil.s.keshavamurthy@...el.com>,
        "David S . Miller" <davem@...emloft.net>,
        Ingo Molnar <mingo@...nel.org>, Petr Mladek <pmladek@...e.com>,
        Joe Lawrence <joe.lawrence@...hat.com>,
        Jiri Kosina <jikos@...nel.org>,
        Miroslav Benes <mbenes@...e.cz>, live-patching@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/2] kprobes: propagate error from
 arm_kprobe_ftrace()

On Fri, 3 Nov 2017 13:33:12 -0400
Steven Rostedt <rostedt@...dmis.org> wrote:

> On Fri, 3 Nov 2017 09:53:37 -0500
> Josh Poimboeuf <jpoimboe@...hat.com> wrote:
> 
> > > > -static void arm_kprobe_ftrace(struct kprobe *p)
> > > > +static int arm_kprobe_ftrace(struct kprobe *p)
> > > >  {
> > > > -	int ret;
> > > > +	int ret = 0;
> > > >  
> > > >  	ret = ftrace_set_filter_ip(&kprobe_ftrace_ops,
> > > >  				   (unsigned long)p->addr, 0, 0);
> > > > -	WARN(ret < 0, "Failed to arm kprobe-ftrace at %p (%d)\n", p->addr, ret);
> > > > -	kprobe_ftrace_enabled++;
> > > > -	if (kprobe_ftrace_enabled == 1) {
> > > > +	if (WARN(ret < 0, "Failed to arm kprobe-ftrace at %p (%d)\n", p->addr, ret))
> > > > +		return ret;
> > > > +
> > > > +	if (kprobe_ftrace_enabled == 0) {
> > > >  		ret = register_ftrace_function(&kprobe_ftrace_ops);
> > > > -		WARN(ret < 0, "Failed to init kprobe-ftrace (%d)\n", ret);
> > > > +		if (WARN(ret < 0, "Failed to init kprobe-ftrace (%d)\n", ret))
> > > > +			goto err_ftrace;
> > > >  	}
> > > > +
> > > > +	kprobe_ftrace_enabled++;
> > > > +	return ret;
> > > > +
> > > > +err_ftrace:
> > > > +	ftrace_set_filter_ip(&kprobe_ftrace_ops, (unsigned long)p->addr, 1, 0);  
> > > 
> > > Hmm, this could have a very nasty side effect. If you remove a function
> > > from the ops, and it was the last function, an empty ops means to trace
> > > *all* functions.  
> > 
> > But this error path only runs when register_ftrace_function() fails, in
> > which case the ops aren't live anyway, right?
> 
> I was thinking that if there was more than one function that is going
> to be registered, that only this one would be black listed. But yeah,
> if there was only one function in the hash, then it probably wouldn't
> matter if it was cleared, because it failed. But I'm paranoid about
> things like this, and prefer to be more robust than to depend on the
> design to enforce correctness than to have each individual function
> being contained and do what is expected of it.

So, what coding will be better? I can only think of this;

ret = ftrace_set_filter_ip(&kprobe_ftrace_ops,
			(unsigned long)p->addr, 0, !kprobe_ftrace_enabled);

And do not remove ip from filter in error case of register_ftrace_function.


BTW, ftrace_set_filter_ip(..., 1, 0); is not easy to read (and @reset
is meaningless in removing)

ftrace_new_filter_ip(ops, addr);
ftrace_append_filter_ip(ops, addr);
ftrace_remove_filter_ip(ops, addr);

wrappers will be more useful.

Thank you,

> 
> -- Steve


-- 
Masami Hiramatsu <mhiramat@...nel.org>