linux-kernel - Re: [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20180317102211.a648f26ae0d5a53d95c63022@kernel.org>
Date:   Sat, 17 Mar 2018 10:22:11 +0900
From:   Masami Hiramatsu <mhiramat@...nel.org>
To:     Masami Hiramatsu <mhiramat@...nel.org>
Cc:     Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
        rostedt <rostedt@...dmis.org>,
        Francis Deslauriers <francis.deslauriers@...icios.com>,
        Peter Zijlstra <peterz@...radead.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe
 blacklist

On Sat, 17 Mar 2018 09:13:34 +0900
Masami Hiramatsu <mhiramat@...nel.org> wrote:

> On Fri, 16 Mar 2018 13:53:01 -0400 (EDT)
> Mathieu Desnoyers <mathieu.desnoyers@...icios.com> wrote:
> 
> > ----- On Mar 16, 2018, at 12:48 PM, rostedt rostedt@...dmis.org wrote:
> > 
> > > On Fri, 16 Mar 2018 12:41:34 -0400
> > > Steven Rostedt <rostedt@...dmis.org> wrote:
> > > 
> > >> Yes, kprobes are dangerous. I'm not saying it shouldn't be fixed, I'm
> > >> saying that I don't have time to fix it now, but would be happy to
> > >> accept patches if someone else does so.
> > > 
> > > And looking at what I replied before for the original patch. It would
> > > probably be a good idea to blacklist directories. Like we do with
> > > function tracing. We probably should black list both kernel/tracing and
> > > kernel/events from being probed.
> > > 
> > > Did this come up at plumbers? You were there too, I don't remember
> > > discussing it there.
> > 
> > I don't remember this coming up last Plumbers nor KS neither, given
> > that we were focused on other topics.
> > 
> > Would the general approach you envision be based on emitting all code
> > generated by compilation of all objects under kernel/tracing and
> > kernel/events into a specific "nokprobes" text section of the kernel ?
> > Perhaps we could create a specific linker scripts for those directories,
> > or do you have in mind a neater way to do this ?
> 
> .kprobes.text section still exists. As I pointed in previous mail, I don't
> think we have to put all those code into that section. But if you want,
> it is acceptable to have a kconfig which push most of those ftrace related
> code into .kprobes.text section.

Or, we can check it by ftrace_location_range() as below patch.

Note that as a side-effect, we can not trace functions in trace_kprobe.c
anymore, and this means it is hard to me to make a testcase of kprobe events.
It was the easiest (and maintainable) way to make test cases... sigh.

-----
tracing: kprobes: Prohibit probing on notrace function

From: Masami Hiramatsu <mhiramat@...nel.org>

Prohibit kprobe-events probing on notrace function.
Since probing on the notrace function can cause recursive
event call. In most case those are just skipped, but
in some case it falls into infinit recursive call.

Signed-off-by: Masami Hiramatsu <mhiramat@...nel.org>
---
 kernel/trace/trace_kprobe.c |   23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index 5ce9b8cf7be3..7404b012e51a 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -487,6 +487,23 @@ disable_trace_kprobe(struct trace_kprobe *tk, struct trace_event_file *file)
 	return ret;
 }
 
+#ifdef CONFIG_KPROBES_ON_FTRACE
+static bool within_notrace_func(struct trace_kprobe *tk)
+{
+	unsigned long offset, size, addr;
+
+	addr = kallsyms_lookup_name(trace_kprobe_symbol(tk));
+	addr += trace_kprobe_offset(tk);
+
+	if (!kallsyms_lookup_size_offset(addr, &size, &offset))
+		return true;	/* Out of range. */
+
+	return !ftrace_location_range(addr - offset, addr - offset + size);
+}
+#else
+#define within_notrace_func(tk)	(false)
+#endif
+
 /* Internal register function - just handle k*probes and flags */
 static int __register_trace_kprobe(struct trace_kprobe *tk)
 {
@@ -495,6 +512,12 @@ static int __register_trace_kprobe(struct trace_kprobe *tk)
 	if (trace_probe_is_registered(&tk->tp))
 		return -EINVAL;
 
+	if (within_notrace_func(tk)) {
+		pr_warn("Could not probe notrace function %s\n",
+			trace_kprobe_symbol(tk));
+		return -EINVAL;
+	}
+
 	for (i = 0; i < tk->tp.nr_args; i++)
 		traceprobe_update_arg(&tk->tp.args[i]);
 

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