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
