Re: [PATCH 1/4] ftrace: Fix function pid filter on instances

[Namhyung Kim]

Hi Masami,

On Mon, Apr 17, 2017 at 8:52 PM, Masami Hiramatsu <mhiramat@xxxxxxxxxx> wrote:
> On Mon, 17 Apr 2017 11:44:27 +0900
> Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
>
>> When function tracer has a pid filter, it adds a probe to sched_switch
>> to track if current task can be ignored.  The probe checks the
>> ftrace_ignore_pid from current tr to filter tasks.  But it misses to
>> delete the probe when removing an instance so that it can cause a crash
>> due to the invalid tr pointer (use-after-free).
>>
>> This is easily reproducible with the following:
>>
>>   # cd /sys/kernel/debug/tracing
>>   # mkdir instances/buggy
>>   # echo $$ > instances/buggy/set_ftrace_pid
>>   # rmdir instances/buggy
>>
>>   ============================================================================
>>   BUG: KASAN: use-after-free in ftrace_filter_pid_sched_switch_probe+0x3d/0x90
>>   Read of size 8 by task kworker/0:1/17
>>   CPU: 0 PID: 17 Comm: kworker/0:1 Tainted: G    B           4.11.0-rc3  #198
>>   Call Trace:
>>    dump_stack+0x68/0x9f
>>    kasan_object_err+0x21/0x70
>>    kasan_report.part.1+0x22b/0x500
>>    ? ftrace_filter_pid_sched_switch_probe+0x3d/0x90
>>    kasan_report+0x25/0x30
>>    __asan_load8+0x5e/0x70
>>    ftrace_filter_pid_sched_switch_probe+0x3d/0x90
>>    ? fpid_start+0x130/0x130
>>    __schedule+0x571/0xce0
>>    ...
>>
>> To fix it, use ftrace_pid_reset() to unregister the probe.  As
>                    ^^^^^^^^^^^^^^^^ ftrace_clear_pids()?

Oops, forgot to update the changelog, sorry.

>
>> instance_rmdir() already updated ftrace codes, it can just free the
>> filter safely.
>
> And following explanation may not need.
>
> The code looks good to me.
>
> Reviewed-by: Masami Hiramatsu <mhiramat@xxxxxxxxxx>

Thanks!
Namhyung

>>
>> Fixes: 0c8916c34203 ("tracing: Add rmdir to remove multibuffer instances")
>> Signed-off-by: Namhyung Kim <namhyung@xxxxxxxxxx>
>> ---
>>  kernel/trace/ftrace.c | 9 +++++++++
>>  kernel/trace/trace.c  | 1 +
>>  kernel/trace/trace.h  | 2 ++
>>  3 files changed, 12 insertions(+)
>>
>> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
>> index 34f63e78d661..7b27066c5ed0 100644
>> --- a/kernel/trace/ftrace.c
>> +++ b/kernel/trace/ftrace.c
>> @@ -5610,6 +5610,15 @@ static void clear_ftrace_pids(struct trace_array *tr)
>>       trace_free_pid_list(pid_list);
>>  }
>>
>> +void ftrace_clear_pids(struct trace_array *tr)
>> +{
>> +     mutex_lock(&ftrace_lock);
>> +
>> +     clear_ftrace_pids(tr);
>> +
>> +     mutex_unlock(&ftrace_lock);
>> +}
>> +
>>  static void ftrace_pid_reset(struct trace_array *tr)
>>  {
>>       mutex_lock(&ftrace_lock);
>> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
>> index b5d4b80f2d45..0bf623c182da 100644
>> --- a/kernel/trace/trace.c
>> +++ b/kernel/trace/trace.c
>> @@ -7485,6 +7485,7 @@ static int instance_rmdir(const char *name)
>>
>>       tracing_set_nop(tr);
>>       event_trace_del_tracer(tr);
>> +     ftrace_clear_pids(tr);
>>       ftrace_destroy_function_files(tr);
>>       tracefs_remove_recursive(tr->dir);
>>       free_trace_buffers(tr);
>> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
>> index 571acee52a32..ee27163b7549 100644
>> --- a/kernel/trace/trace.h
>> +++ b/kernel/trace/trace.h
>> @@ -897,6 +897,7 @@ void ftrace_init_tracefs(struct trace_array *tr, struct dentry *d_tracer);
>>  void ftrace_init_tracefs_toplevel(struct trace_array *tr,
>>                                 struct dentry *d_tracer);
>>  int init_function_trace(void);
>> +void ftrace_clear_pids(struct trace_array *tr);
>>  #else
>>  static inline int ftrace_trace_task(struct trace_array *tr)
>>  {
>> @@ -916,6 +917,7 @@ static inline void ftrace_reset_array_ops(struct trace_array *tr) { }
>>  static inline void ftrace_init_tracefs(struct trace_array *tr, struct dentry *d) { }
>>  static inline void ftrace_init_tracefs_toplevel(struct trace_array *tr, struct dentry *d) { }
>>  static inline int init_function_trace(void) { return 0; }
>> +static inline void ftrace_clear_pids(struct trace_array *tr) { }
>>  /* ftace_func_t type is not defined, use macro instead of static inline */
>>  #define ftrace_init_array_ops(tr, func) do { } while (0)
>>  #endif /* CONFIG_FUNCTION_TRACER */
>> --
>> 2.12.2
>>
>
>
> --
> Masami Hiramatsu <mhiramat@xxxxxxxxxx>



-- 
Thanks,
Namhyung