Re: [PATCH v6 2/2] ring-buffer: Improve nr_pages type
From: Vincent Donnefort <hidden>
Date: 2026-08-17 10:14:52
Also in:
sashiko-reviews
On Fri, Aug 14, 2026 at 03:59:44PM +0000, sashiko-bot@kernel.org wrote:
quoted hunk ↗ jump to hunk
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 [off-list ref] 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. [ ... ]quoted
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[ ... ]quoted
@@ -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/
[ ... ]quoted
@@ -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.
[ ... ]quoted
@@ -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/20260814154823.755406-1-vdonnefort@google.com?part=2