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 <[email protected]> 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/[email protected]?part=2
