Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [High] A logic error in the retry loop of `tracing_buffers_read()` bypasses `ring_buffer_read_page()` on retry, leaking uninitialized or stale kernel memory to userspace. Pre-existing issues: - [Critical] A race condition during subbuffer resize causes `cpu_buffer->free_page` to retain an old allocation size while `buffer->subbuf_order` reflects the new order, leading to a kernel buffer overflow. -- --- Patch [5]: [PATCH v4 5/9] tracing: Fix subbuf resize races in trace_pipe_raw readers --- commit c5c1a16fabb91c8f580283f14e5e76cc1ccb99b0 Author: Vincent Donnefort <[email protected]> tracing: Fix subbuf resize races in trace_pipe_raw readers Concurrent subbuffer resizes may crash trace_pipe_raw readers or leak uninitialized memory to userspace due to stale size values. > diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c > index 94552a433228d..f62d6853ee5c0 100644 > --- a/kernel/trace/ring_buffer.c > +++ b/kernel/trace/ring_buffer.c [ ... ] > @@ -6988,22 +6988,34 @@ EXPORT_SYMBOL_GPL(ring_buffer_swap_cpu); > * Returns: > * The page allocated, or ERR_PTR > */ > -struct buffer_data_read_page * > -ring_buffer_alloc_read_page(struct trace_buffer *buffer, int cpu) > +struct buffer_data_read_page *ring_buffer_alloc_read_page(struct > trace_buffer *buffer, int cpu, > + struct > buffer_data_read_page *prev) > { > + struct buffer_data_read_page *bpage = prev; > struct ring_buffer_per_cpu *cpu_buffer; > - struct buffer_data_read_page *bpage = NULL; > unsigned long flags; > + unsigned int order; > > if (!cpumask_test_cpu(cpu, buffer->cpumask)) > return ERR_PTR(-ENODEV); > > - bpage = kzalloc_obj(*bpage); > - if (!bpage) > - return ERR_PTR(-ENOMEM); > - > - bpage->order = buffer->subbuf_order; > + order = buffer->subbuf_order; > cpu_buffer = buffer->buffers[cpu]; > + > + if (!bpage) { > + bpage = kzalloc_obj(*bpage); > + if (!bpage) > + return ERR_PTR(-ENOMEM); > + } else { > + if (bpage->order == order) > + return bpage; > + > + free_pages((unsigned long)bpage->data, bpage->order); > + bpage->data = NULL; > + } > + > + bpage->order = order; > + > local_irq_save(flags); > arch_spin_lock(&cpu_buffer->lock); > > if (cpu_buffer->free_page) { > bpage->data = cpu_buffer->free_page; > cpu_buffer->free_page = NULL; > } [Severity: Critical] This is a pre-existing issue, but can this lead to an out-of-bounds write during a subbuffer resize? In ring_buffer_alloc_read_page(), the newly read buffer->subbuf_order is assigned to a page stolen from cpu_buffer->free_page without checking if their size orders match. If a concurrent call to ring_buffer_subbuf_order_set() updates buffer->subbuf_order locklessly, and this function is called before cpu_buffer->free_page is cleared, it will steal the old free_page but assign it the new size order. Will this malformed page be swapped into the ring buffer, allowing writers to write past its actual bounds? [ ... ] > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c > index 395238b2b715c..0409d20a168bb 100644 > --- a/kernel/trace/trace.c > +++ b/kernel/trace/trace.c [ ... ] > @@ -7080,8 +7080,8 @@ ssize_t tracing_buffers_read(struct file *filp, char > __user *ubuf, > { > struct ftrace_buffer_info *info = filp->private_data; > struct trace_iterator *iter = &info->iter; > - void *trace_data; > - int page_size; > + void *trace_data, *prev_spare; > + unsigned int spare_size; > ssize_t ret = 0; > ssize_t size; > > @@ -7091,36 +7091,31 @@ ssize_t tracing_buffers_read(struct file *filp, char > __user *ubuf, > if (iter->snapshot && tracer_uses_snapshot(iter->tr->current_trace)) > return -EBUSY; > > - page_size = ring_buffer_subbuf_size_get(iter->array_buffer->buffer); > +again: > + prev_spare = info->spare; > + if (prev_spare) { > + spare_size = ring_buffer_read_page_size(info->spare); > > - /* Make sure the spare matches the current sub buffer size */ > - if (info->spare) { > - if (page_size != info->spare_size) { > - ring_buffer_free_read_page(iter->array_buffer->buffer, > - info->spare_cpu, > info->spare); > - info->spare = NULL; > - } > + /* Do we have previous read data to read? */ > + if (info->read < spare_size) > + goto read; > } [Severity: High] Can the retry path bypass the actual read function and copy stale data to userspace? When reading from an empty trace_pipe_raw file, ring_buffer_read_page() returns < 0. The code then blocks in wait_on_pipe(). Once woken up, it executes a goto again. At the again label, info->read is 0 and info->spare is already allocated, so the condition info->read < spare_size will evaluate to true. Does this cause execution to jump directly to the read label, bypassing ring_buffer_read_page(), and copy the uninitialized or stale info->spare page to userspace? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
