On Fri, Aug 14, 2026 at 03:59:44PM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s)
> to consider:
>
> New issues:
> - [High] When `nr_pages` is exactly `U32_MAX - 1`, the expression
> `meta->nr_subbufs + 1` overflows to 0, silently skipping persistent ring
> buffer validation and wiping data.
>
> Pre-existing issues:
> - [Critical] Loop counters iterating over `meta->nr_subbufs` are typed as
> `signed int`, which wrap to negative values for extremely large buffers,
> leading to out-of-bounds memory accesses.
> - [Critical] The `meta->nr_subbufs` field is read from potentially corrupted
> persistent memory and used without validation against `nr_pages`, causing a
> heap out-of-bounds write in `bitmap_clear()`.
> - [High] Multiplication of `subbuf_size` and `idx` overflows 32 bits for ring
> buffers > 4GB, causing severe memory aliasing.
> --
>
> --- Patch [2]: [PATCH v6 2/2] ring-buffer: Improve nr_pages type ---
> commit 030ec6a7ceec6e71123077bd1fc64e0cf5c27cc8
> Author: Vincent Donnefort <[email protected]>
>
> ring-buffer: Improve nr_pages type
>
> If ring_buffer_per_cpu::nr_pages is defined as unsigned long, it is
> capped to 32-bits in a few places, limiting the operations possible on a
> very large buffer.
>
> [ ... ]
>
> > diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> > index ec13779922ffb..ec127e2ad0527 100644
> > --- a/kernel/trace/ring_buffer.c
> > +++ b/kernel/trace/ring_buffer.c
>
> [ ... ]
>
> > @@ -1746,8 +1745,8 @@ static void *rb_subbufs_from_meta(struct
> > ring_buffer_cpu_meta *meta)
> > static void *rb_range_buffer(struct ring_buffer_per_cpu *cpu_buffer, int
> > idx)
> > {
> > struct ring_buffer_cpu_meta *meta;
> > + unsigned int subbuf_size;
> > unsigned long ptr;
> > - int subbuf_size;
> >
> > meta = rb_range_meta(cpu_buffer->buffer, 0, cpu_buffer->cpu);
> > if (!meta)
>
> [Severity: High]
> This is a pre-existing issue, but does the multiplication of subbuf_size and
> idx overflow 32 bits for ring buffers larger than 4GB?
>
> In rb_range_buffer(), we have:
>
> subbuf_size = meta->subbuf_size;
> idx = meta->buffers[idx];
> ptr = (unsigned long)rb_subbufs_from_meta(meta);
> ptr += subbuf_size * idx;
>
> Since subbuf_size is an unsigned int and idx is an int, the multiplication
> evaluates as a 32-bit unsigned integer. For ring buffers over 4GB, this can
> overflow and wrap around before being zero-extended and added to the 64-bit
> ptr, causing memory aliasing by overwriting earlier subbuffers.
I can add this to this patch/
>
> [ ... ]
>
> > @@ -1840,10 +1839,10 @@ static bool rb_meta_init(struct trace_buffer
> > *buffer, int scratch_size)
> > * must be the same.
> > */
> > static bool rb_cpu_meta_valid(struct ring_buffer_cpu_meta *meta, int cpu,
> > - struct trace_buffer *buffer, int nr_pages,
> > + struct trace_buffer *buffer, unsigned long
> > nr_pages,
> > unsigned long *subbuf_mask)
> > {
> > - int subbuf_size = PAGE_SIZE;
> > + unsigned int subbuf_size = PAGE_SIZE;
> > unsigned long buffers_start;
> > unsigned long buffers_end;
> > int i;
>
> [Severity: Critical]
> This is a pre-existing issue, but will loop counters iterating over
> meta->nr_subbufs wrap to negative values for extremely large buffers?
>
> In rb_cpu_meta_valid():
>
> for (i = 0; i < meta->nr_subbufs; i++) {
> if (meta->buffers[i] < 0 || ...
>
> And in rb_range_meta_init():
>
> for (i = 0; i < meta->nr_subbufs; i++) {
> meta->buffers[i] = i;
>
> The loop counter i is a signed int. Since nr_pages can now go up to
> U32_MAX - 1, if the buffer size pushes meta->nr_subbufs past INT_MAX, i
> will wrap to negative. i < meta->nr_subbufs will promote i to unsigned,
> evaluating to true, and the negative index will be sign-extended on 64-bit
> architectures, causing out-of-bounds reads and writes.
Yeah sure, I'll make it unsigned int... doesn't sound too critical though!
>
> [Severity: Critical]
> This is a pre-existing issue, but could meta->nr_subbufs cause a heap
> out-of-bounds write if it doesn't match nr_pages?
>
> In rb_cpu_meta_valid():
>
> bitmap_clear(subbuf_mask, 0, meta->nr_subbufs);
>
> for (i = 0; i < meta->nr_subbufs; i++) {
> ...
> set_bit(meta->buffers[i], subbuf_mask);
>
> meta->nr_subbufs is read directly from persistent memory and used without
> being validated against the dynamically allocated subbuf_mask size, which is
> based on nr_pages + 1. If meta->nr_subbufs is maliciously or accidentally set
> larger than nr_pages + 1, bitmap_clear() and set_bit() will write zeroes and
> ones past the end of the bitmap allocation.
Sure, but that'll be another patch.
>
> [ ... ]
>
> > @@ -2837,6 +2837,10 @@ static struct trace_buffer *alloc_buffer(unsigned
> > long size, unsigned flags,
> > */
> > nr_pages = (size - sizeof(struct ring_buffer_cpu_meta)) /
> > (subbuf_size + sizeof(int));
> > +
> > + /* limited by ring_buffer_cpu_meta::nr_subbufs */
> > + if (nr_pages > U32_MAX - 1)
> > + goto fail_free_buffers;
> > /* Need at least two pages plus the reader page */
> > if (nr_pages < 3)
> > goto fail_free_buffers;
>
> [Severity: High]
> Does meta->nr_subbufs + 1 overflow when nr_pages is exactly U32_MAX - 1?
>
> If nr_pages is exactly U32_MAX - 1, meta->nr_subbufs becomes U32_MAX.
> In rb_meta_validate_events(), there are loops with this condition:
>
> for (i = 0; i < meta->nr_subbufs + 1; i++, rb_dec_page(&head_page)) {
>
> U32_MAX + 1 overflows to 0 in 32-bit unsigned arithmetic. This makes the loop
> condition i < 0, causing the loops to terminate immediately without rewinding
> and validating the persistent ring buffer from a previous boot.
Sure, this can be unsigned it as well...
And there are plenty of other functions that need fixing then.
rb_meta_subbuf_idx, rb_cpu_meta_valid, __rb_get_reader_page, rb_iter_peek...
I need a bit more time to audit what must be modified.
--
Vincent
>
> --
> Sashiko AI review ยท
> https://sashiko.dev/#/patchset/[email protected]?part=2
