Re: [PATCH v10 1/4] ring-buffer: Allow splice reads on static buffers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Double fetch vulnerability in ring_buffer_read_page() when parsing events from remote/static buffers leads to out-of-bounds memcpy.
--

commit c069a5fa2bee1114a0a60482e4b506024892ddbd
Author: Vincent Donnefort <vdonnefort@google.com>

ring-buffer: Allow splice reads on static buffers

ring_buffer_read_page() rejects splice (full=1) reads on static buffers
because the !read check assumes unread pages must be swapped. This commit
changes the logic to allow memcpy for static buffers, returning an error
only when the writer is still on the reader page.

> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index b0963ac6fd167..84fd4cdd486f8 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
> @@ -7193,15 +7193,8 @@ int ring_buffer_read_page(struct trace_buffer *buffer,
>  		unsigned int event_size;
>  		unsigned int flags = 0;
>  
> -		/*
> -		 * If a full page is expected, this can still be returned
> -		 * if there's been a previous partial read and the
> -		 * rest of the page can be read and the commit page is off
> -		 * the reader page.
> -		 */
> -		if (full &&
> -		    (!read || (len < (size - read)) ||
> -		     cpu_buffer->reader_page == cpu_buffer->commit_page))
> +		/* If a full page is requested, it cannot be the commit page */
> +		if (full && cpu_buffer->reader_page == cpu_buffer->commit_page)
>  			return -1;
>  
>  		if (len > (size - read))

[Severity: Critical]
This is a pre-existing issue, but is there a double fetch vulnerability when
parsing events from remote or static buffers?

In ring_buffer_read_page(), when reading from remote buffers, the event
pointer directly references shared memory. The event size is first fetched
and validated against the remaining length:

    event_size = rb_event_ts_length(event);
    if (len < event_size)
        return -1;

Inside the do-while loop, it is refetched and used directly in memcpy():

    do {
        event_size = rb_event_length(event);
        memcpy(dpage->data + pos, rpage->data + rpos, event_size);
        ...
        event_size = rb_event_ts_length(event);
    } while (len >= event_size);

If a malicious remote writer (such as an untrusted guest VM) modifies the
length fields in shared memory between the initial check and the memcpy(),
can event_size become arbitrarily large?

Could this bypass the bounds check and cause an out-of-bounds read from
rpage->data and a massive out-of-bounds write to dpage->data?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904164450.1345852-1-vdonnefort@google.com?part=1