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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The kernel fails to validate the untrusted `meta->nr_subbufs` value from persistent memory against the expected `nr_pages + 1`, leading to arbitrary heap out-of-bounds writes and potential arbitrary kernel memory corruption.
--

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

ring-buffer: Prevent truncation of nr_pages / nr_subbufs

Although ring_buffer_per_cpu::nr_pages is defined as unsigned long, it
is capped to 32-bits in a few places, limiting the operations possible
on a very large buffer. Use unsigned long where appropriate and
prevent truncation of values using nr_pages (or nr_subbufs).

While at it, subbuf_size must be at least unsigned int.

Note that persistent, remote and user-mapped ring buffers are capping
the number of pages to 31 bits already, making "int" safe in many
places.

> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index 6089fcc67e2b6..1fcf55e2ca81f 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
> @@ -1745,7 +1744,7 @@ static void *rb_range_meta(struct trace_buffer *buffer, int nr_pages, int cpu)
>  /* Return the start of subbufs given the meta pointer */
>  static void *rb_subbufs_from_meta(struct ring_buffer_cpu_meta *meta)
>  {
> -	int subbuf_size = meta->subbuf_size;
> +	unsigned int subbuf_size = meta->subbuf_size;
>  	unsigned long ptr;
>  
>  	ptr = (unsigned long)meta;

[Severity: High]
This isn't a bug introduced by this patch, but does the next line in this
function perform pointer arithmetic using an unvalidated meta->nr_subbufs?

	ptr = rb_range_align_subbuf(ptr, subbuf_size, meta->nr_subbufs);

Could this yield a corrupted or attacker-controlled subbuf base pointer if
meta->nr_subbufs is read from persistent memory without being checked against
expected limits?

[ ... ]

> @@ -1854,13 +1853,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 isn't a bug introduced by this patch, but is there a risk of an
out-of-bounds heap write further down in this function?

The subbuf_mask is allocated with nr_pages + 1 bits in rb_range_meta_init(),
but later in rb_cpu_meta_valid() operations use the unvalidated
meta->nr_subbufs value read from persistent memory:

	bitmap_clear(subbuf_mask, 0, meta->nr_subbufs);

	/*
	 * Ensure the meta::buffers array has correct data. The data in each subbufs
	 * are checked later in rb_meta_validate_events().
	 */
	for (i = 0; i < meta->nr_subbufs; i++) {
		if (meta->buffers[i] < 0 ||
		    meta->buffers[i] >= meta->nr_subbufs) {
			pr_info("Ring buffer boot meta [%d] array out of range\n", cpu);
			return false;
		}

		if (test_bit(meta->buffers[i], subbuf_mask)) {
			pr_info("Ring buffer boot meta [%d] array has duplicates\n", cpu);
			return false;
		}

		set_bit(meta->buffers[i], subbuf_mask);
	}

If meta->nr_subbufs is larger than nr_pages + 1, could this write out of
bounds and corrupt kernel memory?

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