Re: [PATCH v4 5/9] tracing: Fix subbuf resize races in trace_pipe_raw readers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] A logic error in the retry loop of `tracing_buffers_read()` bypasses `ring_buffer_read_page()` on retry, leaking uninitialized or stale kernel memory to userspace.

Pre-existing issues:
- [Critical] A race condition during subbuffer resize causes `cpu_buffer->free_page` to retain an old allocation size while `buffer->subbuf_order` reflects the new order, leading to a kernel buffer overflow.
--

--- Patch [5]: [PATCH v4 5/9] tracing: Fix subbuf resize races in trace_pipe_raw readers ---
commit c5c1a16fabb91c8f580283f14e5e76cc1ccb99b0
Author: Vincent Donnefort <vdonnefort@google.com>

tracing: Fix subbuf resize races in trace_pipe_raw readers

Concurrent subbuffer resizes may crash trace_pipe_raw readers or leak
uninitialized memory to userspace due to stale size values.

> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index 94552a433228d..f62d6853ee5c0 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c

[ ... ]

> @@ -6988,22 +6988,34 @@ EXPORT_SYMBOL_GPL(ring_buffer_swap_cpu);
>   * Returns:
>   *  The page allocated, or ERR_PTR
>   */
> -struct buffer_data_read_page *
> -ring_buffer_alloc_read_page(struct trace_buffer *buffer, int cpu)
> +struct buffer_data_read_page *ring_buffer_alloc_read_page(struct trace_buffer *buffer, int cpu,
> +							  struct buffer_data_read_page *prev)
>  {
> +	struct buffer_data_read_page *bpage = prev;
>  	struct ring_buffer_per_cpu *cpu_buffer;
> -	struct buffer_data_read_page *bpage = NULL;
>  	unsigned long flags;
> +	unsigned int order;
>  
>  	if (!cpumask_test_cpu(cpu, buffer->cpumask))
>  		return ERR_PTR(-ENODEV);
>  
> -	bpage = kzalloc_obj(*bpage);
> -	if (!bpage)
> -		return ERR_PTR(-ENOMEM);
> -
> -	bpage->order = buffer->subbuf_order;
> +	order = buffer->subbuf_order;
>  	cpu_buffer = buffer->buffers[cpu];
> +
> +	if (!bpage) {
> +		bpage = kzalloc_obj(*bpage);
> +		if (!bpage)
> +			return ERR_PTR(-ENOMEM);
> +	} else {
> +		if (bpage->order == order)
> +			return bpage;
> +
> +		free_pages((unsigned long)bpage->data, bpage->order);
> +		bpage->data = NULL;
> +	}
> +
> +	bpage->order = order;
> +
>  	local_irq_save(flags);
>  	arch_spin_lock(&cpu_buffer->lock);
>  
>  	if (cpu_buffer->free_page) {
>  		bpage->data = cpu_buffer->free_page;
>  		cpu_buffer->free_page = NULL;
>  	}

[Severity: Critical]
This is a pre-existing issue, but can this lead to an out-of-bounds write
during a subbuffer resize?

In ring_buffer_alloc_read_page(), the newly read buffer->subbuf_order is
assigned to a page stolen from cpu_buffer->free_page without checking if
their size orders match.

If a concurrent call to ring_buffer_subbuf_order_set() updates
buffer->subbuf_order locklessly, and this function is called before
cpu_buffer->free_page is cleared, it will steal the old free_page but assign
it the new size order.

Will this malformed page be swapped into the ring buffer, allowing writers
to write past its actual bounds?

[ ... ]

> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 395238b2b715c..0409d20a168bb 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c

[ ... ]

> @@ -7080,8 +7080,8 @@ ssize_t tracing_buffers_read(struct file *filp, char __user *ubuf,
>  {
>  	struct ftrace_buffer_info *info = filp->private_data;
>  	struct trace_iterator *iter = &info->iter;
> -	void *trace_data;
> -	int page_size;
> +	void *trace_data, *prev_spare;
> +	unsigned int spare_size;
>  	ssize_t ret = 0;
>  	ssize_t size;
>  
> @@ -7091,36 +7091,31 @@ ssize_t tracing_buffers_read(struct file *filp, char __user *ubuf,
>  	if (iter->snapshot && tracer_uses_snapshot(iter->tr->current_trace))
>  		return -EBUSY;
>  
> -	page_size = ring_buffer_subbuf_size_get(iter->array_buffer->buffer);
> +again:
> +	prev_spare = info->spare;
> +	if (prev_spare) {
> +		spare_size = ring_buffer_read_page_size(info->spare);
>  
> -	/* Make sure the spare matches the current sub buffer size */
> -	if (info->spare) {
> -		if (page_size != info->spare_size) {
> -			ring_buffer_free_read_page(iter->array_buffer->buffer,
> -						   info->spare_cpu, info->spare);
> -			info->spare = NULL;
> -		}
> +		/* Do we have previous read data to read? */
> +		if (info->read < spare_size)
> +			goto read;
>  	}

[Severity: High]
Can the retry path bypass the actual read function and copy stale data
to userspace?

When reading from an empty trace_pipe_raw file, ring_buffer_read_page()
returns < 0. The code then blocks in wait_on_pipe(). Once woken up, it
executes a goto again.

At the again label, info->read is 0 and info->spare is already allocated,
so the condition info->read < spare_size will evaluate to true.

Does this cause execution to jump directly to the read label, bypassing
ring_buffer_read_page(), and copy the uninitialized or stale info->spare
page to userspace?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260812153311.2328812-1-vdonnefort@google.com?part=5