Re: [PATCH v10 3/4] ring-buffer: Cap static ring buffer nr_pages

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Integer underflow in size calculation bypasses the max pages limit on 32-bit systems, leading to massive out-of-bounds memory initialization.
- [High] Unchecked return value from `ring_buffer_desc()` leading to a NULL pointer dereference in remote buffer allocation.
--

commit be7971253ae6dad60dcb1cc2d29fe4019ffb6e16
Author: Vincent Donnefort <vdonnefort@google.com>

ring-buffer: Cap static ring buffer nr_pages

This commit introduces a limit on the number of pages for static ring
buffers to ensure the page count fits within the bpage::id field. It
adds bounds checks during ring buffer creation and mapping to prevent
exceeding this maximum limit.

> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index 2f61cb510b0ac..6b914abd80e3c 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c

[ ... ]

> @@ -2833,6 +2842,8 @@ static struct trace_buffer *alloc_buffer(unsigned long size, unsigned flags,
>  		size = end - buffers_start;
>  		size = size / nr_cpu_ids;
>  
> +		if (size < sizeof(struct ring_buffer_cpu_meta))
> +			goto fail_free_buffers;

[Severity: High]
This is a pre-existing issue, but could this size calculation result in
an integer underflow if a user configures a small range size where end
is less than buffers_start?

On 32-bit systems, the unsigned subtraction would wrap around to a huge
positive integer. Because the resulting size is massive, the new size
less than sizeof(struct ring_buffer_cpu_meta) check would pass.

Since nr_pages would become approximately 262,144 in this wrapped state,
it would also bypass the newly added nr_pages > rb_static_max_pages()
limit. Could this loop then accumulate pointers wrapping around the 32-bit
address space, bypassing the bounds check and leading to massive
out-of-bounds memory initialization?

>  		/*
>  		 * The number of sub-buffers (nr_pages) is determined by the
>  		 * total size allocated minus the meta data size.
> @@ -2842,6 +2853,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));
> +
> +		if (nr_pages > rb_static_max_pages())
> +			goto fail_free_buffers;
> +
>  		/* Need at least two pages plus the reader page */
>  		if (nr_pages < 3)
>  			goto fail_free_buffers;

[ ... ]

> @@ -2874,6 +2889,10 @@ static struct trace_buffer *alloc_buffer(unsigned long size, unsigned flags,
>  		/* The writer is remote. This ring-buffer is read-only */
>  		atomic_inc(&buffer->record_disabled);
>  		nr_pages = desc->nr_page_va - 1;

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for
ring_buffer_desc() to return NULL here if the provided remote descriptor
lacks an entry for the currently executing CPU?

If the descriptor does not have an entry for the executing CPU,
unconditionally dereferencing desc->nr_page_va would cause a NULL pointer
dereference.

Other functions in this file, such as __rb_allocate_pages(), appear to
check if desc is NULL before proceeding. Could this lead to a kernel panic
if an untrusted hypervisor or client provides an incomplete remote trace
descriptor?

> +
> +		if (nr_pages > rb_static_max_pages())
> +			goto fail_free_buffers;
> +
>  		if (nr_pages < 2)
>  			goto fail_free_buffers;
>  	} else {

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904164450.1345852-1-vdonnefort@google.com?part=3