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 <[email protected]> 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/[email protected]?part=3
