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

> 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
[ ... ]
> @@ -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,
[ ... ]
> @@ -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?

>  	if (size > count)
>  		size = count;
>  	trace_data = ring_buffer_read_page_data(info->spare);
>  	ret = copy_to_user(ubuf, trace_data + info->read, size);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1