Re: [PATCH] tracing: Fix NULL pointer dereference in func_set_flag()

[Steven Rostedt <rostedt@goodmis.org>]

From: https://sashiko.dev/#/patchset/20260624061715.1445655-1-xiangzao%40linux.alibaba.com

> commit 2140381622b995438751cdb59eb4bae6923f6a60
> Author: Yuanhe Shu <xiangzao@linux.alibaba.com>
> tracing: Fix NULL pointer dereference in func_set_flag()
> This commit fixes a NULL pointer dereference in func_set_flag() by
> moving the tracer type check before dereferencing tr->current_trace_flags.
> > diff --git a/kernel/trace/trace_functions.c b/kernel/trace/trace_functions.c
> > index f283391a4dc80..cd37f20137581 100644
> > --- a/kernel/trace/trace_functions.c
> > +++ b/kernel/trace/trace_functions.c
> > @@ -458,12 +458,12 @@ func_set_flag(struct trace_array *tr, u32 old_flags, u32 bit, int set)
> >  	ftrace_func_t func;
> >  	u32 new_flags;
> >  
> > -	/* Do nothing if already set. */
> > -	if (!!set == !!(tr->current_trace_flags->val & bit))
> > +	/* We can change this flag only when current tracer is function. */
> > +	if (tr->current_trace != &function_trace)
> >  		return 0;
> >  
> > -	/* We can change this flag only when not running. */
> > -	if (tr->current_trace != &function_trace)
> > +	/* Do nothing if already set. */
> > +	if (!!set == !!(tr->current_trace_flags->val & bit))
> >  		return 0;
>
> This isn't a bug introduced by this patch, but while reviewing this fix for
> tr->current_trace_flags, I noticed a similar NULL pointer dereference
> can occur in print_graph_function() in kernel/trace/trace_functions_graph.c.
> If a user enables the function_graph tracer, populating the ring buffer
> with graph events, and then switches to a tracer that lacks flags and
> does not automatically clear the ring buffer (such as hwlat, osnoise, or
> timerlat), tr->current_trace_flags becomes NULL.
> When the user subsequently reads the trace buffer, the fallback event handler
> calls print_graph_function(), which unconditionally dereferences
> tr->current_trace_flags:
>
> kernel/trace/trace_functions_graph.c:print_graph_function() {
>     struct trace_array *tr = iter->tr;
>     return print_graph_function_flags(iter, tr->current_trace_flags->val);
> }
>
> Does this unconditional dereference of tr->current_trace_flags->val also
> need a similar safety check to prevent a kernel crash?

No, because even though hwlat, osnoise and timerlat may not clear the main
ring buffer, it reads it differently. Thus the print_graph_function will
not be called.

Unless you can figure out a way to get to that function with one of those
other tracers?

-- Steve