Message-Id: <20180317090854.aef4a3dc6552eeeb00e50498@kernel.org>
Date:   Sat, 17 Mar 2018 09:08:54 +0900
From:   Masami Hiramatsu <mhiramat@...nel.org>
To:     Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
Cc:     rostedt <rostedt@...dmis.org>,
        Francis Deslauriers <francis.deslauriers@...icios.com>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        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 Fri, 16 Mar 2018 12:28:59 -0400 (EDT)
Mathieu Desnoyers <mathieu.desnoyers@...icios.com> wrote:

> ----- On Mar 16, 2018, at 11:25 AM, rostedt rostedt@...dmis.org wrote:
> 
> > On Fri, 16 Mar 2018 11:18:25 -0400
> > Francis Deslauriers <francis.deslauriers@...icios.com> wrote:
> > 
> >> Hi Steven,
> >> 
> >> I completely forgot about this issue until recently when I encountered it again.
> >> Instrumenting the ftrace_ops_assist_func symbol and some other symbol
> >> seems to be causing problems.
> >> 
> >> Placing kretprobes like in the following configuration crashes my
> >> kernel (4.16.0-rc5) on a Qemu/KVM virtual machine:
> >> 
> >> config 1:
> >> echo "r:event_1 __fdget" >> kprobe_events
> >> echo "r:event_2 ftrace_ops_assist_func" >> kprobe_events
> >> 
> >> config 2:
> >> echo "r:event_1 __fdget_pos" >> kprobe_events
> >> echo "r:event_2 ftrace_ops_assist_func" >> kprobe_events
> >> 
> >> config 3:
> >> echo 'r:event_1 arch_dup_task_struct' >> kprobe_events
> >> echo 'r:event_2 ftrace_ops_assist_func' >> kprobe_events
> >> 
> >> config 4:
> >> echo 'r:event_1 sys_open' >> kprobe_events
> >> echo 'r:event_2 ftrace_ops_assist_func' >> kprobe_events
> >> 
> >> Here is my kernel config [1]:
> >> 
> >> In a previous email [2], you mentioned that you would like to add the
> >> ftrace-related symbols to a section to un-blacklist them all at once
> >> on demand but wanted to discuss it at Linux Plumbers. Do you still
> >> think that it's the right approach?
> > 
> > We probably didn't discuss it (as there was a lot to discuss, and this
> > was probably overshadowed by that). But yes, you should not probe
> > ftrace called functions. That is guaranteed to crash and that crash is
> > not a bug, but a feature.
> 
> Are you really arguing that crashing the kernel from an ABI visible from
> userspace (even if it's only root user) is not a bug ? You are joking right ?
> Is there an EXPERIMENTAL config option that people need to select in order to
> make sure those ftrace interfaces don't end up on production systems ?
> 
> > 
> > The ftrace and ring buffer files should be blacklisted from being
> > probed. Perhaps the entire directory.
> 
> All code reachable from a kprobe handler should be blacklisted from
> kprobes, yes.

No, actually that is incorrect, since user can make a module to probe
those functions from outside of ftrace via kprobes.

The problematic functions are the functions called between probe-hit
and set current_kprobe (iow, kprobe_ftrace_handler()). After setting
current_kprobe, all probe hits are just skipped.

The issue that Francis faced is actual bug. It must be blacklisted,
since it is called before calling kprobe_ftrace_handler().

target_func
  -> fentry-code
     -> ftrace-code
          -> kprobe_ftrace_handler
                (safe area)
          <- ret
      <- ret (ftrace-code)
  <- ret (fentry-code)

In above model, functions in fentry-code and ftrace-code must be
blacklisted.

Thanks,

> > Anyway, I don't see this as much of an urgent matter, as it's one of
> > those "Patient: Doc, it hurts when I do this. Doc: Don't do that"
> > cases. And there's a lot of urgent issues that currently need to be
> > dealt with.
> 
> OK, short-term we'll remove everything related to ftrace functions
> from our CI fuzzer coverage. Arguably, the fact that a root user can
> crash the kernel through tracefs files is not that great security-wise
> though.
> 
> Considering that our current focus is to test the kprobe instrumentation
> layer (and not ftrace per se), we will move our fuzzer to the LTTng ABI
> instead, which should take care of removing crashes introduced by ftrace
> from our fuzzing results.
> 
> Thanks,
> 
> Mathieu
> 
> 
> > 
> > -- Steve
> > 
> > 
> >> 
> >> I can easily test any patch regarding this issue.
> >> 
> >> [1] http://paste.ubuntu.com/p/BJWvgMnW8z/
> >> [2] https://lkml.org/lkml/2017/7/14/568
> >> 
> >> Thank you,
> >> 
> >> 2017-07-14 14:29 GMT-04:00 Steven Rostedt <rostedt@...dmis.org>:
> >> > On Fri, 14 Jul 2017 10:58:35 -0400
> >> > Francis Deslauriers <francis.deslauriers@...icios.com> wrote:
> >> >  
> >> >> This function is called when a kprobe is hit. Thus it should be
> >> >> blacklisted to prevent kprobe to be triggered by kprobes.
> >> >>
> >> >> Signed-off-by: Francis Deslauriers <francis.deslauriers@...icios.com>
> >> >> ---
> >> >>  kernel/trace/ftrace.c | 2 ++
> >> >>  1 file changed, 2 insertions(+)
> >> >>
> >> >> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> >> >> index b308be3..c473d9b 100644
> >> >> --- a/kernel/trace/ftrace.c
> >> >> +++ b/kernel/trace/ftrace.c
> >> >> @@ -36,6 +36,7 @@
> >> >>
> >> >>  #include <trace/events/sched.h>
> >> >>
> >> >> +#include <asm/kprobes.h>
> >> >>  #include <asm/sections.h>
> >> >>  #include <asm/setup.h>
> >> >>
> >> >> @@ -5739,6 +5740,7 @@ static void ftrace_ops_assist_func(unsigned long ip,
> >> >> unsigned long parent_ip,
> >> >>       preempt_enable_notrace();
> >> >>       trace_clear_recursion(bit);
> >> >>  }
> >> >> +NOKPROBE_SYMBOL(ftrace_ops_assist_func);
> >> >
> >> > Continuing from what I said in the other email, this is fixing a
> >> > symptom and not the problem. The real fix will be much more involved. I
> >> > have a good idea on how to accomplish it too.
> >> >
> >> > -- Steve
> >> >
> >> >  
> >> >>
> >> >>  /**
> >> >>   * ftrace_ops_get_func - get the function a trampoline should call
> >> >  
> >> 
> >> 
> 
> -- 
> Mathieu Desnoyers
> EfficiOS Inc.
> http://www.efficios.com


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

Powered by blists - more mailing lists