Re: [PATCH 2/2] ring-buffer: Stop remote reader update when page swap fails

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Missing bounds check on shared memory fetch allows out-of-bounds read and Double Fetch (TOCTOU) arbitrary memory write.
- [High] Logic Error: Function fails to abort when remote reader swap succeeds but the reader ID is left unchanged, corrupting the ring buffer.
--

--- Patch [2]: [PATCH 2/2] ring-buffer: Stop remote reader update when page swap fails ---
commit 90c9d4edbac629b3b97121e741bbf3aba71f7d7d
Author: Ivan Immanuel Shaji <ivanimmanuel1234@gmail.com>

ring-buffer: Stop remote reader update when page swap fails

This commit modifies __rb_get_reader_page_from_remote to abort and return
NULL when the remote swap_reader_page callback fails. This prevents the
function from splicing the same page as both the previous and new reader,
avoiding linked-list corruption.

> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
> @@ -5805,8 +5805,9 @@ __rb_get_reader_page_from_remote(struct ring_buffer_per_cpu *cpu_buffer)
>  
>  	prev_reader = cpu_buffer->subbuf_ids[cpu_buffer->meta_page->reader.id];

[Severity: Critical]
This isn't a bug introduced by this patch, but does the initial fetch of
meta_page->reader.id need a bounds check and READ_ONCE protection?

If a remote entity (such as a guest VM) concurrently writes an
out-of-bounds index to meta_page->reader.id in shared memory, it is used
immediately as an index into the subbuf_ids array. 

If the remote then updates the index to a valid value before the second
fetch lower down, the later bounds check will pass, but prev_reader will
still hold a garbage pointer from the first out-of-bounds read. This
pointer is later used in linked-list manipulations, which could lead to
arbitrary memory corruption.

> -	WARN_ON_ONCE(cpu_buffer->remote->swap_reader_page(cpu_buffer->cpu,
> -							  cpu_buffer->remote->priv));
> +	if (WARN_ON_ONCE(cpu_buffer->remote->swap_reader_page(cpu_buffer->cpu,
> +							      cpu_buffer->remote->priv)))
> +		return NULL;
>  	/* nr_pages doesn't include the reader page */
>  	if (WARN_ON_ONCE(cpu_buffer->meta_page->reader.id > cpu_buffer->nr_pages))
>  		return NULL;
>
>  	new_reader = cpu_buffer->subbuf_ids[cpu_buffer->meta_page->reader.id];
>
>  	WARN_ON_ONCE(prev_reader == new_reader);

[Severity: High]
This is a pre-existing issue, but should the code abort here if prev_reader
and new_reader are equal?

If the remote reader swap succeeds but the remote erroneously leaves
meta_page->reader.id unchanged, this condition evaluates to true. 
The function correctly identifies the invalid state with a warning but 
continues execution rather than returning NULL.

By continuing, the code blindly manipulates the list pointers of the same 
page (for example, overwriting new_reader's list.prev to point to itself), 
triggering the same kind of linked list corruption this patch was intended 
to fix.

>  
>  	prev_head = new_reader;  /* New reader was also the previous head */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825-kernel-patch-1-v1-0-e9349aa0b165@gmail.com?part=2