Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Critical] Integer overflow in ring_buffer_resize() allows an attacker to bypass allocation checks and trigger a massive out-of-bounds ring buffer shrink, leading to a Use-After-Free. - [Medium] 32-bit shift overflow in __rb_map_vma() truncates the mapped page count for huge ring buffers, causing mmap to fail. --
--- Patch [10]: [PATCH v5 10/10] ring-buffer: Make nr_pages unsigned int --- commit 25e7b6bfdf3741f228ceb9dc41c8bbf6e253f525 Author: Vincent Donnefort <[email protected]> ring-buffer: Make nr_pages unsigned int nr_pages is an int or unsigned int almost everywhere already. Also, all the meta-data ring_buffer_desc, ring_buffer_cpu_meta and trace_buffer_meta allowing to share information about the ring buffer are already capping this value to 32-bits. > diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c > index 79459c99ce9a0..d9dcd6d4e7cc4 100644 > --- a/kernel/trace/ring_buffer.c > +++ b/kernel/trace/ring_buffer.c > @@ -561,7 +561,7 @@ struct ring_buffer_per_cpu { > struct ring_buffer_remote *remote; > > /* ring buffer pages to update, > 0 to add, < 0 to remove */ > - long nr_pages_to_update; > + int nr_pages_to_update; > struct list_head new_pages; /* new pages to add */ > struct work_struct update_pages_work; > struct completion update_done; [ ... ] > @@ -3264,7 +3263,7 @@ int ring_buffer_resize(struct trace_buffer *buffer, > unsigned long size, > int cpu_id) > { > struct ring_buffer_per_cpu *cpu_buffer; > - unsigned long nr_pages; > + unsigned int nr_pages; [Severity: Critical] If a huge value is written to buffer_size_kb, could the unsigned difference between nr_pages and cpu_buffer->nr_pages overflow when assigned to the now 32-bit signed cpu_buffer->nr_pages_to_update? Later in this function, we do: cpu_buffer->nr_pages_to_update = nr_pages - cpu_buffer->nr_pages; if (cpu_buffer->nr_pages_to_update <= 0) continue; If the difference overflows into a negative integer, wouldn't it skip this allocation limits check but still schedule the worker thread (since it is non-zero)? The worker thread calls rb_update_pages(): success = rb_remove_pages(cpu_buffer, -cpu_buffer->nr_pages_to_update); This would pass a large positive count to rb_remove_pages(). As it wildly wraps around the small circular list: for (nr_removed = 0; nr_removed < nr_pages; nr_removed++) { to_remove = rb_list_head(to_remove)->next; head_bit |= (unsigned long)to_remove & RB_PAGE_HEAD; } Could this unlink and double-free actively used pages, leaving ring buffer structures pointing to freed memory and causing a use-after-free? > int cpu, err; > > /* [ ... ] > @@ -7708,8 +7708,8 @@ static int __rb_inc_dec_mapped(struct > ring_buffer_per_cpu *cpu_buffer, > static int __rb_map_vma(struct ring_buffer_per_cpu *cpu_buffer, > struct vm_area_struct *vma) > { > - unsigned long nr_subbufs, nr_pages, nr_vma_pages, pgoff = vma->vm_pgoff; > - unsigned int subbuf_pages, subbuf_order; > + unsigned long nr_pages, nr_vma_pages, pgoff = vma->vm_pgoff; > + unsigned int nr_subbufs, subbuf_pages, subbuf_order; [Severity: Medium] Since nr_subbufs is now an unsigned int, could the bitwise shift operation later in this function silently truncate for exceptionally large ring buffers? __rb_map_vma() later computes: nr_pages = ((nr_subbufs + 1) << subbuf_order) - 1; Because the operands are now 32-bit, the shift is computed entirely in 32-bit math before assignment to the unsigned long nr_pages. If a buffer is large enough to overflow 32-bits, will this cause bounds checks to fail and mmap to return -EINVAL? > struct page **pages __free(kfree) = NULL; > int p = 0, s = 0; > int err; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=10
