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 [off-list ref]

    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.

[ ... ]

[ ... ]

+	/*
+	 * 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.

[ ... ]

[ ... ]

 	/* 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;

[ ... ]

[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);

[ ... ]

[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/20260813131152.3589632-1-vdonnefort@google.com?part=4