Re: [PATCH v7 2/3] ring-buffer: Cap static ring buffer nr_pages

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

Pre-existing issues:
- [High] Integer underflow silently bypasses the newly introduced rb_static_max_pages() bound check on 32-bit platforms.
- [High] Integer overflow in pointer arithmetic when ring buffer is large causes out-of-bounds memory mapping.
- [Medium] Mixing goto-based error handling with scope-based cleanup (__free and guard) violates subsystem guidelines.
--

--- Patch [2]: [PATCH v7 2/3] ring-buffer: Cap static ring buffer nr_pages ---
commit 5ef3e045e7449785cd305ffb3c9bd4573685ad34
Author: Vincent Donnefort <[email protected]>

ring-buffer: Cap static ring buffer nr_pages
    
This commit introduces a maximum page limit for static ring buffers to
ensure the page count fits within the buffer_page::id field. It enforces
this limit during buffer allocation and user-mapping, while increasing
the id field to 31 bits.

> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index ec13779922ffb..5e26eeff588f2 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
[ ... ]
> @@ -652,6 +652,15 @@ static bool rb_is_static(struct ring_buffer_per_cpu *cpu_buffer)
>  	return cpu_buffer->user_mapped || cpu_buffer->remote || cpu_buffer->ring_meta;
>  }
>  
> +static unsigned long rb_static_max_pages(void)
> +{
> +	/*
> +	 * Static ring buffers are using bpage::id and must account for the
> +	 * reader page.
> +	 */
> +	return (1UL << 31) - 1;
> +}

[Severity: High]
This is a pre-existing issue, but does a maximum page limit this large cause
integer overflow in pointer arithmetic when mapping memory?

While the limit restricts the number of pages to (1UL << 31) - 1, configuring
a very large memory-mapped trace buffer (e.g., >= 2GB per CPU, or ~524,288
pages) can cause multiplication overflows elsewhere:

kernel/trace/ring_buffer.c:rb_range_meta() {
    ...
    ptr += subbuf_size * nr_subbufs;
    ...
}

And similarly in rb_range_buffer():

kernel/trace/ring_buffer.c:rb_range_buffer() {
    ...
    ptr += subbuf_size * idx;
    ...
}

Since the operands are treated as 32-bit signed integers, configuring a
buffer with 524,288 pages causes the multiplication to exceed INT_MAX and
overflow into a negative value. This negative offset is added to the starting
pointer, resulting in the mapped buffer incorrectly pointing to memory before
the valid allocation range.

> +
>  struct ring_buffer_iter {
>  	struct ring_buffer_per_cpu	*cpu_buffer;
>  	unsigned long			head;
[ ... ]
> @@ -2837,6 +2846,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));

[Severity: High]
This is a pre-existing issue, but does an integer underflow here silently
bypass the newly introduced bound check on 32-bit platforms?

If the memory-mapped ring buffer size is extremely small, the calculation of
the available size for per-CPU data underflows, causing the size variable to
become a massive positive value.

On 32-bit architectures, the nr_pages variable evaluates to approximately 1
million pages. Since this is less than the rb_static_max_pages() limit, the
check passes. The subsequent loop calculating subbuffer pointers then
overflows 32-bit arithmetic, bypassing bounds checks and allowing
out-of-bounds memory corruption.

> +
> +		if (nr_pages > rb_static_max_pages())
> +			goto fail_free_buffers;

[Severity: Medium]
This is a pre-existing issue, but does adding new goto error paths here
perpetuate a mixture of goto-based error handling and scope-based cleanup?

The alloc_buffer() function uses scope-based cleanup:

    struct trace_buffer *buffer __free(kfree) = NULL;

Mixing __free() declarations and manual goto cleanup labels in the same
function violates subsystem guidelines and increases the risk of memory leaks
or confusing ownership semantics.

> +
>  		/* Need at least two pages plus the reader page */
>  		if (nr_pages < 3)
>  			goto fail_free_buffers;

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