Re: [PATCH v5 04/10] ring-buffer: Fix subbuf resize race with ring buffer readers

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

New issues:
- [High] `ring_buffer_read_start()` introduces a Use-After-Free vulnerability for callers using `GFP_ATOMIC`.

Pre-existing issues:
- [High] Race condition and Out-of-Bounds write in `ring_buffer_map_get_reader()` due to evaluating the global `buffer->subbuf_order`.
--

--- Patch [4]: [PATCH v5 04/10] ring-buffer: Fix subbuf resize race with ring buffer readers ---
commit 71a276d1d136c0fb8371689f58251aa6fc99f54c
Author: Vincent Donnefort <[email protected]>

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

This commit replaces the lockless read of trace_buffer::subbuf_size with
bpage::order in ring_buffer_read_start() and ring_buffer_read_page() to
prevent races during resizing. It also removes subbuf_size entirely and
introduces new capacity accessors.

> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index b6fa258aafe2c..ec520c72124e5 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)
>  {

[ ... ]

> +	/*
> +	 * 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 cause a use-after-free when called with GFP_ATOMIC?

If this is called with GFP_ATOMIC, it skips taking buffer->mutex and
increments resize_disabled without synchronization. If a concurrent
ring_buffer_subbuf_order_set() evaluated resize_disabled to 0 just before
this increment, it can proceed to replace and free the reader_page.

The lockless dereference of bpage->order inside rb_page_capacity() on the
freed reader_page would then access freed memory. Prior to this patch, it
read the scalar buffer->subbuf_size which was safe from this use-after-free.

[ ... ]

> @@ -7340,18 +7383,21 @@ int ring_buffer_subbuf_order_set(struct trace_buffer *buffer, int order)

[ ... ]

>  	/* prevent another thread from changing buffer sizes */
>  	guard(mutex)(&buffer->mutex);
> +
> +	old_order = buffer->subbuf_order;
> +	if (old_order == order)
> +		return 0;
> +
> +	old_capacity = rb_subbuf_capacity(buffer);
> +
>  	atomic_inc(&buffer->record_disabled);
>  
>  	/* Make sure all commits have finished */
>  	synchronize_rcu();
>  
>  	buffer->subbuf_order = order;

[ ... ]

> @@ -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 can this lead to an out-of-bounds write?

Since ring_buffer_map_get_reader() does not hold buffer->mutex, it can run
concurrently with ring_buffer_subbuf_order_set(). As shown in the hunk above,
ring_buffer_subbuf_order_set() updates the global buffer->subbuf_order before
checking if resize_disabled is set on the CPU buffers.

If ring_buffer_map_get_reader() reads this temporarily inflated global
capacity while processing an older, smaller reader_page, the capacity check
could incorrectly pass. This would allow the following memcpy() to write
missed_events out of bounds.

Should this use rb_page_capacity(reader) instead of the global
rb_subbuf_capacity(buffer), similar to the fix applied to
ring_buffer_read_page()?

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

[ ... ]

> @@ -7928,7 +7973,7 @@ int ring_buffer_map_get_reader(struct trace_buffer *buffer, int cpu)
>  out:
>  	/* Some archs do not have data cache coherency between kernel and user-space */
>  	flush_kernel_vmap_range(cpu_buffer->reader_page->page,
> -				buffer->subbuf_size + BUF_PAGE_HDR_SIZE);
> +				rb_subbuf_size(buffer));

[Severity: High]
This isn't a bug introduced by this patch, but could this flush more memory
than is actually mapped?

Similar to the check above, if this reads a temporarily inflated global
capacity while flushing an older, smaller reader_page, the flush range
might exceed the actual allocated page bounds.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4