Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [High] Infinite busy-wait loop in tracing reads (tracing_buffers_read and tracing_buffers_splice_read) causing lockups during concurrent ring buffer resizes. Pre-existing issues: - [Critical] Concurrent io_uring reads trigger a double free on info->spare->data due to missing VFS locks. - [High] Use-After-Free vulnerability in VFS pipe buffer destruction path due to lifecycle mismatch with the trace array. -- --- Patch [1]: [PATCH v7 1/3] tracing: Fix subbuf resize races with trace_pipe_raw readers --- commit 3b5685c224f6977e19f6580474cf69ff34b547a9 Author: Vincent Donnefort <[email protected]> tracing: Fix subbuf resize races with trace_pipe_raw readers Concurrent subbuffer resizes may crash trace_pipe_raw readers or leak uninitialized memory to userspace due to stale size values. Modify ring_buffer_alloc_read_page() to handle resizing and add ring_buffer_read_page_size() to avoid racy subbuf size gets. > diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c > index 5fc009edc1ec9..ec13779922ffb 100644 > --- a/kernel/trace/ring_buffer.c > +++ b/kernel/trace/ring_buffer.c [ ... ] > @@ -6990,56 +6990,78 @@ EXPORT_SYMBOL_GPL(ring_buffer_swap_cpu); > +int ring_buffer_alloc_read_page(struct trace_buffer *buffer, int cpu, > + struct buffer_data_read_page **rpage) > { [ ... ] > + if (*rpage) { > + if ((*rpage)->order == order) > + return 0; > > - bpage->order = buffer->subbuf_order; > + /* We can reuse rpage, but we discard the payload */ > + free_pages((unsigned long)(*rpage)->data, (*rpage)->order); > + (*rpage)->data = NULL; [Severity: Critical] This is a pre-existing issue, but concurrent io_uring reads can call vfs_read() without holding f_pos_lock, leading to concurrent execution of tracing_buffers_read(). Neither tracing_buffers_read() nor this function protects the shared info->spare pointer with a lock. If these threads race during a subbuffer resize, can both observe the order mismatch simultaneously and execute free_pages() on the same data address, causing a double free? > + } else { > + *rpage = kzalloc_obj(**rpage); > + if (!*rpage) > + return -ENOMEM; > + } > + > + (*rpage)->order = order; > cpu_buffer = buffer->buffers[cpu]; [ ... ] > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c > index 395238b2b715c..737922b236d46 100644 > --- a/kernel/trace/trace.c > +++ b/kernel/trace/trace.c [ ... ] > @@ -7128,7 +7116,9 @@ ssize_t tracing_buffers_read(struct file *filp, char > __user *ubuf, > iter->cpu_file, 0); > trace_access_unlock(iter->cpu_file); > > - if (ret < 0) { > + if (ret == -EAGAIN) { > + goto again; > + } else if (ret < 0) { [Severity: High] If ring_buffer_read_page() returns -EAGAIN because the subbuffer order was updated but cpu_buffer->reader_page->order hasn't been updated yet, this branches back to the again label. Since the loop does not yield the CPU or call cond_resched(), could this cause a hard lockup if the reader and the resize thread are on the same CPU? > if (trace_empty(iter) && !iter->closed) { > if (update_last_data_if_empty(iter->tr)) > return 0; [ ... ] > @@ -7268,23 +7259,12 @@ ssize_t tracing_buffers_splice_read(struct file > *file, loff_t *ppos, > }; > struct buffer_ref *ref; > bool woken = false; > - int page_size; > int entries, i; > ssize_t ret = 0; [ ... ] > for (i = 0; i < spd.nr_pages_max && len && entries; i++) { > + unsigned int page_size; > struct page *page; > int r; > > @@ -7304,25 +7285,38 @@ ssize_t tracing_buffers_splice_read(struct file > *file, loff_t *ppos, > > refcount_set(&ref->refcount, 1); > ref->buffer = iter->array_buffer->buffer; [Severity: High] This is a pre-existing issue, but splice_to_pipe() links kernel pages to a pipe buffer and stashes ref->buffer as private data. The VFS operation never acquires a reference on the trace_array. If the user closes the trace file and deletes the trace instance, the trace_buffer is freed. When the pipe buffer is subsequently read or destroyed, won't buffer_ref_release() dereference the dangling ref->buffer pointer when calling ring_buffer_free_read_page(), causing a use-after-free? > - ref->page = ring_buffer_alloc_read_page(ref->buffer, > iter->cpu_file); > - if (IS_ERR(ref->page)) { > - ret = PTR_ERR(ref->page); > - ref->page = NULL; > + > +new_read_page: > + ret = ring_buffer_alloc_read_page(ref->buffer, iter->cpu_file, > &ref->rpage); > + if (ret) { > kfree(ref); > break; > } > ref->cpu = iter->cpu_file; > > - r = ring_buffer_read_page(ref->buffer, ref->page, > - len, iter->cpu_file, 1); > + page_size = ring_buffer_read_page_size(ref->rpage); > + > + r = -EINVAL; > + if (IS_ALIGNED(*ppos, page_size) && len >= page_size) { > + r = ring_buffer_read_page(ref->buffer, ref->rpage, len, > iter->cpu_file, 1); > + if (r == -EAGAIN) > + goto new_read_page; [Severity: High] Similar to the read path above, this catches -EAGAIN and loops back to new_read_page. Because goto new_read_page executes continuously without releasing trace_access_lock() or yielding the CPU, can this permanently deadlock the system during a resize by preventing the resize thread from acquiring the necessary locks? > + } else if (!i) { > + /* > + * If the first iteration fails this is an invalid > userspace input. > + * Otherwise, this is because the subbuf order has been > modified. Do not > + * report an error and finish the read. > + */ > + ret = -EINVAL; > + } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
