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

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.

[ ... ]

> @@ -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.

[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.

[ ... ]

> @@ -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.

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