Re: [PATCH v4 9/9] ring-buffer: Make nr_pages unsigned int

On Wed, Aug 12, 2026 at 03:47:11PM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Critical] Integer truncation of `nr_pages_to_update` allows a massive buffer resize to bypass memory allocation checks, directly triggering an out-of-bounds page removal loop causing a critical kernel Use-After-Free (UAF).
> - [High] Multiplication overflow in `subbuf_size * nr_pages` evaluates as an `unsigned int` when allocating huge persistent ring buffers, destroying trace data isolation.
> --
> 
> --- Patch [9]: [PATCH v4 9/9] ring-buffer: Make nr_pages unsigned int ---
> commit cb1365ebdbbed7daa3a685ae1d6be7a4167052dd
> Author: Vincent Donnefort <[email protected]>
> Date:   Wed Aug 12 15:33:10 2026 +0000
> 
>     ring-buffer: Make nr_pages unsigned int
>     
>     nr_pages is an int or unsigned int almost everywhere already. Also, all
>     the meta-data ring_buffer_desc, ring_buffer_cpu_meta and
>     trace_buffer_meta allowing to share information about the ring buffer
>     are already capping this value to 32-bits.
>     
>     Make ring_buffer_per_cpu::nr_pages unsigned and align all the users to
>     it. As a side effect, this makes ring_buffer_per_cpu slightly smaller.
> 
> > diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> > index 990a904cefe85..2fccb950e593c 100644
> > --- a/kernel/trace/ring_buffer.c
> > +++ b/kernel/trace/ring_buffer.c
> [ ... ]
> > @@ -561,7 +561,7 @@ struct ring_buffer_per_cpu {
> >  	struct ring_buffer_remote	*remote;
> >  
> >  	/* ring buffer pages to update, > 0 to add, < 0 to remove */
> > -	long				nr_pages_to_update;
> > +	int				nr_pages_to_update;
> >  	struct list_head		new_pages; /* new pages to add */
> >  	struct work_struct		update_pages_work;
> >  	struct completion		update_done;
> 
> [Severity: Critical]
> Could this type change lead to an out-of-bounds page removal regression?
> 
> In ring_buffer_resize(), size is an unsigned long and can be quite large,
> resulting in an unsigned int nr_pages near the maximum 32-bit value. When
> calculating nr_pages_to_update:
> 
> ring_buffer_resize() {
>     ...
>     cpu_buffer->nr_pages_to_update = nr_pages - cpu_buffer->nr_pages;
>     if (cpu_buffer->nr_pages_to_update <= 0)
>         continue;
>     ...
> }
> 
> If nr_pages is very large (e.g. 4294967286), subtracting the current nr_pages
> (e.g. 10) results in an unsigned value that wraps into a small negative number
> when stored in the 32-bit int nr_pages_to_update (e.g. -20).
> 
> This would bypass the allocation checks and trigger rb_remove_pages() to
> remove more pages than exist in the buffer, causing it to loop past the end
> of the list and free pages that are still in use.

That would mean 15 TiB for a single CPU right?

> 
> [ ... ]
> > @@ -2773,7 +2773,7 @@ static struct trace_buffer *alloc_buffer(unsigned long size, unsigned flags,
> >  					 struct ring_buffer_remote *remote)
> >  {
> >  	struct trace_buffer *buffer __free(kfree) = NULL;
> > -	long nr_pages;
> > +	unsigned int nr_pages;
> >  	int subbuf_size;
> >  	int bsize;
> >  	int cpu;
> 
> [Severity: High]
> Will this type change cause a multiplication overflow when allocating large
> persistent ring buffers?
> 
> Later in alloc_buffer(), the pointer is advanced for the next CPU's buffer:
> 
> alloc_buffer() {
>     ...
>     ptr += subbuf_size * nr_pages;
>     ...
> }
> 
> Because subbuf_size is an int and nr_pages is now an unsigned int, the
> multiplication is evaluated as a 32-bit unsigned integer. 
> 
> If the buffer is larger than 4GB per CPU, this will overflow before being
> added to ptr, placing the next CPU's buffer inside the previous CPU's buffer.
> 
> Previously, when nr_pages was a long, this math was safely performed as 64-bit
> on 64-bit systems.

This one is worth improving though!

-- 
Vincent

> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=9