Re: [PATCH v4 3/9] ring-buffer: Fix subbuf resize race with ring buffer readers

[sashiko-bot@kernel.org]

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

New issues:
- [High] Use-After-Free dereference of `reader_page` in `ring_buffer_read_start()` when bypassing buffer mutex.

Pre-existing issues:
- [High] 32-bit integer overflow in ring buffer capacity calculation truncates large trace buffers.
- [High] Out-of-bounds kernel memory write and VMAP flush in `ring_buffer_map_get_reader()` due to global capacity TOCTOU race.
--

--- Patch [3]: [PATCH v4 3/9] ring-buffer: Fix subbuf resize race with ring buffer readers ---
commit 9618f28d9a6939c645e7579fa798c42557eb075c
Author: Vincent Donnefort <vdonnefort@google.com>

ring-buffer: Fix subbuf resize race with ring buffer readers

This commit changes the trace_buffer to use bpage order instead of
trace_buffer::subbuf_size in ring_buffer_read_start() and
ring_buffer_read_page(). It also removes trace_buffer::subbuf_size and
introduces capacity accessors.

> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index ec4f5a0c93e85..97449423d3a68 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
[ ... ]
> @@ -6380,36 +6417,44 @@ EXPORT_SYMBOL_GPL(ring_buffer_consume);
>  struct ring_buffer_iter *
>  ring_buffer_read_start(struct trace_buffer *buffer, int cpu, gfp_t flags)
>  {
> +	struct ring_buffer_iter *iter __free(kfree) = kzalloc_obj(*iter, flags);
>  	struct ring_buffer_per_cpu *cpu_buffer;
> -	struct ring_buffer_iter *iter;
> -
> -	if (!cpumask_test_cpu(cpu, buffer->cpumask))
> -		return NULL;
>  
> -	iter = kzalloc_obj(*iter, flags);
>  	if (!iter)
>  		return NULL;
>  
> -	/* Holds the entire event: data and meta data */
> -	iter->event_size = buffer->subbuf_size;
> -	iter->event = kmalloc(iter->event_size, flags);
> -	if (!iter->event) {
> -		kfree(iter);
> +	if (!cpumask_test_cpu(cpu, buffer->cpumask))
>  		return NULL;
> -	}
>  
>  	cpu_buffer = buffer->buffers[cpu];
>  
> -	iter->cpu_buffer = cpu_buffer;
> +	/*
> +	 * Only KDB is using GFP_ATOMIC, for the others, lock the buffer to
> +	 * prevent concurrent resizing.
> +	 */
> +	if (gfpflags_allow_blocking(flags))
> +		mutex_lock(&buffer->mutex);
>  
>  	atomic_inc(&cpu_buffer->resize_disabled);
>  
> +	if (gfpflags_allow_blocking(flags))
> +		mutex_unlock(&buffer->mutex);
> +
> +	/* Holds the entire event: data and meta data. */
> +	iter->event_size = rb_page_capacity(READ_ONCE(cpu_buffer->reader_page));

[Severity: High]
Can this result in a use-after-free dereference of the reader page?

When ring_buffer_read_start() is called with GFP_ATOMIC, it skips acquiring
buffer->mutex and only increments cpu_buffer->resize_disabled.

If a concurrent ring_buffer_subbuf_order_set() has already passed its
resize_disabled check, it will proceed to allocate new pages and free the old
reader page.

A thread executing ring_buffer_read_start() could load the old reader page
pointer just before it is freed by the other CPU, dereferencing freed memory
when accessing the order field.

> +	iter->event = kmalloc(iter->event_size, flags);
> +	if (!iter->event) {
> +		atomic_dec(&cpu_buffer->resize_disabled);
> +		return NULL;
> +	}
[ ... ]
> @@ -7367,8 +7413,8 @@ int ring_buffer_subbuf_order_set(struct trace_buffer *buffer, int order)
>  		}
>  
>  		/* Update the number of pages to match the new size */
> -		nr_pages = old_size * buffer->buffers[cpu]->nr_pages;
> -		nr_pages = DIV_ROUND_UP(nr_pages, buffer->subbuf_size);
> +		nr_pages = old_capacity * buffer->buffers[cpu]->nr_pages;

[Severity: High]
This is a pre-existing issue, but can this 32-bit integer capacity calculation
truncate large trace buffers?

For buffers larger than 4GB, the multiplication of old_capacity and
buffer->buffers[cpu]->nr_pages can exceed 32 bits. Because nr_pages is a
32-bit signed integer, the result is truncated, and the buffer is shrunk to a
tiny fraction of its intended size during a resize operation.

> +		nr_pages = DIV_ROUND_UP(nr_pages, rb_subbuf_capacity(buffer));
>  
>  		/* we need a minimum of two pages */
>  		if (nr_pages < 2)
[ ... ]
> @@ -7896,7 +7941,7 @@ int ring_buffer_map_get_reader(struct trace_buffer *buffer, int cpu)
>  			 * missed events, then record it there.
>  			 */
>  			commit = rb_page_size(reader);
> -			if (buffer->subbuf_size - commit >= sizeof(missed_events)) {
> +			if (rb_subbuf_capacity(buffer) - commit >= sizeof(missed_events)) {

[Severity: High]
This is a pre-existing issue, but does this create a time-of-check to
time-of-use race leading to an out-of-bounds kernel memory write and
VMAP flush?

During a sub-buffer order resize, ring_buffer_subbuf_order_set() modifies
buffer->subbuf_order globally before verifying if resize_disabled is set. If
the buffer is currently mapped, the resize aborts and restores the old order.

In that race window, a concurrent ring_buffer_map_get_reader() call will see
the new, larger capacity via rb_subbuf_capacity(buffer) here. It could then
write missed_events past the end of the old, smaller reader page, and
subsequently flush memory beyond the mapped VMAP range.

>  				memcpy(&dpage->data[commit], &missed_events,
>  				       sizeof(missed_events));
>  				local_add(RB_MISSED_STORED, &dpage->commit);

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