Re: [RFC PATCH v3 3/3] tracing: Fix syscall tracepoint use-after-free

[Mathieu Desnoyers <mathieu.desnoyers@efficios.com>]

On 2024-10-27 21:22, Andrii Nakryiko wrote:
> On Sat, Oct 26, 2024 at 8:48 AM Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:
>>
>> The grace period used internally within tracepoint.c:release_probes()
>> uses call_rcu() to batch waiting for quiescence of old probe arrays,
>> rather than using the tracepoint_synchronize_unregister() which blocks
>> while waiting for quiescence.
>>
>> With the introduction of faultable syscall tracepoints, this causes
>> use-after-free issues reproduced with syzkaller.
>>
>> Fix this by using the appropriate call_rcu() or call_rcu_tasks_trace()
>> before invoking the rcu_free_old_probes callback. This can be chosen
>> using the tracepoint_is_syscall() API.
>>
>> A similar issue exists in bpf use of call_rcu(). Fixing this is left to
>> a separate change.
>>
>> Reported-by: syzbot+b390c8062d8387b6272a@syzkaller.appspotmail.com
>> Fixes: a363d27cdbc2 ("tracing: Allow system call tracepoints to handle page faults")
>> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
>> Cc: Michael Jeanson <mjeanson@efficios.com>
>> Cc: Steven Rostedt <rostedt@goodmis.org>
>> Cc: Masami Hiramatsu <mhiramat@kernel.org>
>> Cc: Peter Zijlstra <peterz@infradead.org>
>> Cc: Alexei Starovoitov <ast@kernel.org>
>> Cc: Yonghong Song <yhs@fb.com>
>> Cc: Paul E. McKenney <paulmck@kernel.org>
>> Cc: Ingo Molnar <mingo@redhat.com>
>> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
>> Cc: Namhyung Kim <namhyung@kernel.org>
>> Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com>
>> Cc: bpf@vger.kernel.org
>> Cc: Joel Fernandes <joel@joelfernandes.org>
>> Cc: Jordan Rife <jrife@google.com>
>> ---
>> Changes since v0:
>> - Introduce tracepoint_call_rcu(),
>> - Fix bpf_link_free() use of call_rcu as well.
>>
>> Changes since v1:
>> - Use tracepoint_call_rcu() for bpf_prog_put as well.
>>
>> Changes since v2:
>> - Do not cover bpf changes in the same commit, let bpf developers
>>    implement it.
>> ---
>>   kernel/tracepoint.c | 11 +++++++----
>>   1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
>> index 5658dc92f5b5..47569fb06596 100644
>> --- a/kernel/tracepoint.c
>> +++ b/kernel/tracepoint.c
>> @@ -106,13 +106,16 @@ static void rcu_free_old_probes(struct rcu_head *head)
>>          kfree(container_of(head, struct tp_probes, rcu));
>>   }
>>
>> -static inline void release_probes(struct tracepoint_func *old)
>> +static inline void release_probes(struct tracepoint *tp, struct tracepoint_func *old)
>>   {
>>          if (old) {
>>                  struct tp_probes *tp_probes = container_of(old,
>>                          struct tp_probes, probes[0]);
>>
>> -               call_rcu(&tp_probes->rcu, rcu_free_old_probes);
>> +               if (tracepoint_is_syscall(tp))
>> +                       call_rcu_tasks_trace(&tp_probes->rcu, rcu_free_old_probes);
> 
> should this be call_rcu_tasks_trace() -> call_rcu() chain instead of
> just call_rcu_tasks_trace()? While currently call_rcu_tasks_trace()
> implies RCU GP (as evidenced by rcu_trace_implies_rcu_gp() being
> hardcoded right now to returning true), this might not always be the
> case in the future, so it's best to have a guarantee that regardless
> of sleepable or not, we'll always have have RCU GP, and for sleepable
> tracepoint *also* RCU Tasks Trace GP.

Given that faultable tracepoints only use RCU tasks trace for the
read-side and do not rely on preempt disable, I don't see why we would
need to chain both grace periods there ?

Thanks,

Mathieu

> 
>> +               else
>> +                       call_rcu(&tp_probes->rcu, rcu_free_old_probes);
>>          }
>>   }
>>
>> @@ -334,7 +337,7 @@ static int tracepoint_add_func(struct tracepoint *tp,
>>                  break;
>>          }
>>
>> -       release_probes(old);
>> +       release_probes(tp, old);
>>          return 0;
>>   }
>>
>> @@ -405,7 +408,7 @@ static int tracepoint_remove_func(struct tracepoint *tp,
>>                  WARN_ON_ONCE(1);
>>                  break;
>>          }
>> -       release_probes(old);
>> +       release_probes(tp, old);
>>          return 0;
>>   }
>>
>> --
>> 2.39.5
>>

-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com