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
