Re: [PATCH v7 3/3] ring-buffer: Prevent truncation of nr_pages / nr_subbufs

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Integer underflow in alloc_buffer() for mapped ring buffers leads to memory corruption on 32-bit systems.
- [High] Out-of-bounds memory corruption in rb_cpu_meta_valid during ring buffer boot meta validation due to unvalidated meta->nr_subbufs.
--

--- Patch [3]: [PATCH v7 3/3] ring-buffer: Prevent truncation of nr_pages / nr_subbufs ---
commit ca74d5f745c91c8b4a7ee42fae91ab814132919c
Author: Vincent Donnefort <[email protected]>

ring-buffer: Prevent truncation of nr_pages / nr_subbufs

This commit prevents truncation operations on large ring buffers by changing
nr_pages and nr_subbufs definitions to unsigned long. It also upgrades
subbuf_size to unsigned int.

> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c

[ ... ]

> @@ -1849,13 +1848,13 @@ 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 long subbuf_size = PAGE_SIZE;
>  	unsigned long buffers_start;
>  	unsigned long buffers_end;
> -	int i;
> +	unsigned long i;
>  
>  	if (!subbuf_mask)
>  		return false;

[Severity: High]
This is a pre-existing issue, but can this lead to an out-of-bounds memory
overwrite on the kernel heap?

In rb_range_meta_init(), subbuf_mask is allocated using the current boot's
nr_pages + 1:

    subbuf_mask = bitmap_alloc(nr_pages + 1, GFP_KERNEL);

However, later in rb_cpu_meta_valid(), the legacy persistent metadata is used 
to clear and set bits in this mask:

    bitmap_clear(subbuf_mask, 0, meta->nr_subbufs);
    ...
    for (i = 0; i < meta->nr_subbufs; i++) {
        ...
        set_bit(meta->buffers[i], subbuf_mask);
    }

If the system boots with more CPUs than the previous cycle, the per-CPU
nr_pages constraint decreases. Consequently, the legacy meta->nr_subbufs 
could be larger than the newly allocated subbuf_mask, causing bitmap_clear()
and set_bit() to overflow the allocation.

[ ... ]

> @@ -2782,8 +2782,8 @@ static struct trace_buffer *alloc_buffer(unsigned long size, unsigned flags,
>  					 struct ring_buffer_remote *remote)
>  {
>  	struct trace_buffer *buffer __free(kfree) = NULL;
> -	long nr_pages;
> -	int subbuf_size;
> +	unsigned int subbuf_size;
> +	unsigned long nr_pages;
>  	int bsize;
>  	int cpu;
>  	int ret;

[Severity: High]
This is a pre-existing issue, but does this function suffer from an integer
underflow that could bypass length boundary validations?

Further down in alloc_buffer(), when calculating the per-CPU size for mapped
buffers:

    size = end - buffers_start;
    size = size / nr_cpu_ids;
    nr_pages = (size - sizeof(struct ring_buffer_cpu_meta)) /
               (subbuf_size + sizeof(int));

If the user-provided mapped range is smaller than the meta structure size,
size - sizeof(struct ring_buffer_cpu_meta) will underflow. On 32-bit systems,
this yields a massive nr_pages value that successfully passes the
rb_static_max_pages() threshold check. 

The subsequently calculated pointers will wrap around the 32-bit address
space, defeating the bounds check against the end boundary, and allowing
mapped metadata structures to be written into arbitrary out-of-bounds memory.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3