Re: [PATCH v6 2/2] ring-buffer: Improve nr_pages type

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