Re: [PATCH v5 10/10] ring-buffer: Make nr_pages unsigned int

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