Re: [PATCH v4 5/9] tracing: Fix subbuf resize races in trace_pipe_raw readers

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

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.

quoted

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

[ ... ]

quoted

@@ -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?

[ ... ]

quoted

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

[ ... ]

quoted

@@ -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?