[PATCH] perf: allow non-privileged uprobe for user processes

[PATCH] perf: allow non-privileged uprobe for user processes

[Song Liu]

Currently, non-privileged user could only use uprobe with

    kernel.perf_event_paranoid = -1

However, setting perf_event_paranoid to -1 leaks other users' processes to
non-privileged uprobes.

To introduce proper permission control of uprobes, we are building the
following system:
  A daemon with CAP_SYS_ADMIN is in charge to create uprobes via tracefs;
  Users asks the daemon to create uprobes;
  Then user can attach uprobe only to processes owned by the user.

This patch allows non-privileged user to attach uprobe to processes owned
by the user.

The following example shows how to use uprobe with non-privileged user.
This is based on Brendan's blog post [1]

1. Create uprobe with root:
  sudo perf probe -x 'readline%return +0($retval):string'

2. Then non-root user can use the uprobe as:
  perf record -vvv -e probe_bash:readline__return -p <pid> sleep 20
  perf script

[1] http://www.brendangregg.com/blog/2015-06-28/linux-ftrace-uprobe.html

Signed-off-by: Song Liu <songliubraving@fb.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
---
 kernel/events/core.c        | 5 +++--
 kernel/trace/trace_uprobe.c | 2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index abbd4b3b96c2..0508774d82e4 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -8532,9 +8532,10 @@ static int perf_tp_event_match(struct perf_event *event,
 	if (event->hw.state & PERF_HES_STOPPED)
 		return 0;
 	/*
-	 * All tracepoints are from kernel-space.
+	 * All tracepoints except uprobes are from kernel-space.
 	 */
-	if (event->attr.exclude_kernel)
+	if (event->attr.exclude_kernel &&
+	    ((event->tp_event->flags & TRACE_EVENT_FL_UPROBE) == 0))
 		return 0;
 
 	if (!perf_tp_filter_match(event, data))
diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index be78d99ee6bc..bfd3040b4cfb 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -1304,7 +1304,7 @@ static inline void init_trace_event_call(struct trace_uprobe *tu,
 	call->event.funcs = &uprobe_funcs;
 	call->class->define_fields = uprobe_event_define_fields;
 
-	call->flags = TRACE_EVENT_FL_UPROBE;
+	call->flags = TRACE_EVENT_FL_UPROBE | TRACE_EVENT_FL_CAP_ANY;
 	call->class->reg = trace_uprobe_register;
 	call->data = tu;
 }
-- 
2.17.1

Re: [PATCH] perf: allow non-privileged uprobe for user processes

[Peter Zijlstra]

On Tue, May 07, 2019 at 12:43:15AM -0700, Song Liu wrote:
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index abbd4b3b96c2..0508774d82e4 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -8532,9 +8532,10 @@ static int perf_tp_event_match(struct perf_event *event,
>  	if (event->hw.state & PERF_HES_STOPPED)
>  		return 0;
>  	/*
> -	 * All tracepoints are from kernel-space.
> +	 * All tracepoints except uprobes are from kernel-space.
>  	 */
> -	if (event->attr.exclude_kernel)
> +	if (event->attr.exclude_kernel &&
> +	    ((event->tp_event->flags & TRACE_EVENT_FL_UPROBE) == 0))

That doesn't make sense; should you not be checking user_mode(regs)
instead?

Re: [PATCH] perf: allow non-privileged uprobe for user processes

[Song Liu]

> On May 7, 2019, at 4:40 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> 
> On Tue, May 07, 2019 at 12:43:15AM -0700, Song Liu wrote:
>> diff --git a/kernel/events/core.c b/kernel/events/core.c
>> index abbd4b3b96c2..0508774d82e4 100644
>> --- a/kernel/events/core.c
>> +++ b/kernel/events/core.c
>> @@ -8532,9 +8532,10 @@ static int perf_tp_event_match(struct perf_event *event,
>> 	if (event->hw.state & PERF_HES_STOPPED)
>> 		return 0;
>> 	/*
>> -	 * All tracepoints are from kernel-space.
>> +	 * All tracepoints except uprobes are from kernel-space.
>> 	 */
>> -	if (event->attr.exclude_kernel)
>> +	if (event->attr.exclude_kernel &&
>> +	    ((event->tp_event->flags & TRACE_EVENT_FL_UPROBE) == 0))
> 
> That doesn't make sense; should you not be checking user_mode(regs)
> instead?

Yes! user_mode(regs) is better! V2 coming soon.

Thanks,
Song