Re: [PATCH 6.12 117/337] tracing/mmiotrace: Add NULL check for mmio_trace_array in logging functions

Hi Greg/Sasha,

On 07/08/26 8:05 pm, Greg Kroah-Hartman wrote:
> 6.12-stable review patch.  If anyone has any objections, please let me know.
> 
> ------------------
> 
> From: Masami Hiramatsu (Google) <[email protected]>
> 
> [ Upstream commit 12b80cdbc54cf615b4717a4e8180063408091ea2 ]
> 
> mmio_trace_rw() and mmio_trace_mapping() retrieve mmio_trace_array into
> tr and pass it to __trace_mmiotrace_rw() and __trace_mmiotrace_map().
> If these functions are invoked while mmio_trace_array is NULL (e.g. before
> initialization or after disabled), accessing tr->array_buffer.buffer will
> result in a NULL pointer dereference crash.
> 
> Fix this by adding an explicit NULL check for tr at the beginning of
> __trace_mmiotrace_rw() and __trace_mmiotrace_map().
> 
> Link: https://patch.msgid.link/178524300062.56416.8362487250709962380.stgit@devnote2
> Fixes: f984b51e0779 ("ftrace: add mmiotrace plugin")
> Assisted-by: Antigravity:gemini-3.6-flash
> Signed-off-by: Masami Hiramatsu (Google) <[email protected]>
> Signed-off-by: Steven Rostedt <[email protected]>
> Signed-off-by: Sasha Levin <[email protected]>
> ---
>   kernel/trace/trace_mmiotrace.c | 12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/trace/trace_mmiotrace.c b/kernel/trace/trace_mmiotrace.c
> index 6b964471265e1..251ed7051e409 100644
> --- a/kernel/trace/trace_mmiotrace.c
> +++ b/kernel/trace/trace_mmiotrace.c
> @@ -295,11 +295,15 @@ static void __trace_mmiotrace_rw(struct trace_array *tr,
>   				struct trace_array_cpu *data,

^^^ lets keep this in mind

>   				struct mmiotrace_rw *rw)
>   {
> -	struct trace_buffer *buffer = tr->array_buffer.buffer;
> +	struct trace_buffer *buffer;
>   	struct ring_buffer_event *event;
>   	struct trace_mmiotrace_rw *entry;
>   	unsigned int trace_ctx;
>   
> +	if (!tr)
> +		return;
> +
> +	buffer = tr->array_buffer.buffer;

I ran an AI-assisted backport review and then checked the 6.12.y code. I
think this backport is missing a small prerequisite.

Upstream's wrappers pass tr directly to __trace_mmiotrace_rw() and
__trace_mmiotrace_map().Each function checks tr before dereferencing it:

if (!tr)
	return;
buffer = tr->array_buffer.buffer;


The upstream fix follows commit: 6936298393d8 ("tracing/mmiotrace: 
Remove reference to unused per CPU data pointer"), which removes the 
unused data argument.

6.12.y does not have that commit, so both functions still have that 
argument and their wrappers compute it first:

struct trace_array *tr = mmio_trace_array;
struct trace_array_cpu *data =
	per_cpu_ptr(tr->array_buffer.data, smp_processor_id());
__trace_mmiotrace_rw(tr, data, rw);


Consequently, if mmio_trace_array is NULL, the dereference in
per_cpu_ptr(tr->array_buffer.data, ...) occurs before either function's 
new NULL check.

I think 6.12.y needs commit: 6936298393d8 ("tracing/mmiotrace: Remove
reference to unused per CPU data pointer") before this fix.

Thoughts?


thanks,
Harshit
>   	trace_ctx = tracing_gen_ctx_flags(0);
>   	event = trace_buffer_lock_reserve(buffer, TRACE_MMIO_RW,
>   					  sizeof(*entry), trace_ctx);
> @@ -324,11 +328,15 @@ static void __trace_mmiotrace_map(struct trace_array *tr,
>   				struct trace_array_cpu *data,
>   				struct mmiotrace_map *map)
>   {
> -	struct trace_buffer *buffer = tr->array_buffer.buffer;
> +	struct trace_buffer *buffer;
>   	struct ring_buffer_event *event;
>   	struct trace_mmiotrace_map *entry;
>   	unsigned int trace_ctx;
>   
> +	if (!tr)
> +		return;
> +
> +	buffer = tr->array_buffer.buffer;
>   	trace_ctx = tracing_gen_ctx_flags(0);
>   	event = trace_buffer_lock_reserve(buffer, TRACE_MMIO_MAP,
>   					  sizeof(*entry), trace_ctx);