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

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

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.

[ ... ]

[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;
+	}

[ ... ]

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

[ ... ]

[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