Re: [PATCH v6 1/2] tracing: Fix subbuf resize races with trace_pipe_raw readers

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The local variable `spare_size` is left uninitialized via `goto read;` in `tracing_buffers_read()`, resulting in a kernel memory leak to userspace.
--

--- Patch [1]: [PATCH v6 1/2] tracing: Fix subbuf resize races with trace_pipe_raw readers ---

commit bdc760fe7b1aa6eb8c8a8b8c38e9fd02d25051b6
Author: Vincent Donnefort [off-list ref]

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. This commit
modifies ring_buffer_alloc_read_page() to handle resizing and introduces
ring_buffer_read_page_size().

quoted

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 395238b2b715c..355278083b3fb 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c

[ ... ]

quoted

@@ -7091,36 +7091,21 @@ 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);
-
-	/* 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;
-		}
-	}
+again:
+	/* Do we have previous read data to read? */
+	if (info->spare && (info->read < ring_buffer_read_page_size(info->spare)))
+		goto read;
 
-	if (!info->spare) {
-		info->spare = ring_buffer_alloc_read_page(iter->array_buffer->buffer,
-							  iter->cpu_file);
-		if (IS_ERR(info->spare)) {
-			ret = PTR_ERR(info->spare);
-			info->spare = NULL;
-		} else {
-			info->spare_cpu = iter->cpu_file;
-			info->spare_size = page_size;
-		}
-	}
-	if (!info->spare)
+	/* Make sure the read page order is aligned with the current buffer subbuf order */
+	ret = ring_buffer_alloc_read_page(iter->array_buffer->buffer, iter->cpu_file,
+					  &info->spare);
+	if (ret < 0)
 		return ret;
 
-	/* Do we have previous read data to read? */
-	if (info->read < page_size)
-		goto read;
+	spare_size = ring_buffer_read_page_size(info->spare);
+	info->read = spare_size;
+	info->spare_cpu = iter->cpu_file;
 
- again:
 	trace_access_lock(iter->cpu_file);
 	ret = ring_buffer_read_page(iter->array_buffer->buffer,
 				    info->spare,

[ ... ]

quoted

@@ -7146,8 +7133,9 @@ ssize_t tracing_buffers_read(struct file *filp, char __user *ubuf,
 	}
 
 	info->read = 0;
+
  read:
-	size = page_size - info->read;
+	size = spare_size - info->read;

[Severity: High]
Can this regression leak uninitialized stack memory to userspace?

If a userspace process does a partial read (consuming fewer bytes than a full
subbuffer page), the next read syscall will take the goto read; branch at the
top of tracing_buffers_read().

When that branch is taken, spare_size is completely uninitialized stack memory.

Could size end up being a large garbage value (or underflow if spare_size <
info->read), causing copy_to_user() below to read beyond the bounds of
info->spare and leak arbitrary kernel memory?