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
