Re: [PATCH v1 1/2] tracing/remotes: Account for ring buffer page header in size calculation

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Integer overflow in DIV_ROUND_UP macro when buffer_size is near ULONG_MAX, resulting in silently undersized trace buffer allocation and bypassing the nr_pages overflow check.
--

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

tracing/remotes: Account for ring buffer page header in size calculation

This commit adjusts the trace buffer size calculation to account for the ring
buffer page header size. It also introduces bounds checking for the number of
pages to prevent out-of-bounds calculations.

> diff --git a/include/linux/ring_buffer.h b/include/linux/ring_buffer.h
> index afc7daa6ee7dc..7a1a92f876503 100644
> --- a/include/linux/ring_buffer.h
> +++ b/include/linux/ring_buffer.h
[ ... ]
> @@ -281,9 +283,14 @@ static inline struct ring_buffer_desc *__first_ring_buffer_desc(struct trace_buf
>  
>  static inline size_t trace_buffer_desc_size(size_t buffer_size, unsigned int nr_cpus)
>  {
> -	unsigned int nr_pages = max(DIV_ROUND_UP(buffer_size, PAGE_SIZE), 2UL) + 1;
> +	unsigned long nr_pages =
> +		max(DIV_ROUND_UP(buffer_size, PAGE_SIZE - BUF_PAGE_HDR_SIZE), 2UL) + 1;

[Severity: High]
This isn't a bug introduced by this patch, but is there an integer overflow
when evaluating DIV_ROUND_UP() with a very large buffer_size?

If a privileged user writes a near-maximum value (like ULONG_MAX - 1023) to
the tracefs buffer_size_kb file, it passes the earlier check_shl_overflow()
because the top 10 bits are zero.

When calculating the pages here:

    DIV_ROUND_UP(buffer_size, PAGE_SIZE - BUF_PAGE_HDR_SIZE)

the macro expands to adding (PAGE_SIZE - BUF_PAGE_HDR_SIZE - 1) to
buffer_size. This addition overflows the 64-bit integer, wrapping around
to a small number.

This causes nr_pages to be miscalculated as a tiny value (like 3), completely
bypassing the newly added "> UINT_MAX" safety check.

Since the backing pages might be sized based on a PAGE_ALIGN() wrap, while
nr_page_va is set to 3, could this discrepancy cause out-of-bounds hypervisor
memory accesses on systems with >= 64 CPUs?

>  	struct ring_buffer_desc *rbdesc;
>  
> +	/* Capped by ring_buffer_desc::nr_page_va */
> +	if (nr_pages > UINT_MAX)
> +		return SIZE_MAX;
> +
>  	return size_add(offsetof(struct trace_buffer_desc, __data),
>  			size_mul(nr_cpus, struct_size(rbdesc, page_va, nr_pages)));
>  }
> diff --git a/kernel/trace/trace_remote.c b/kernel/trace/trace_remote.c
> index 75fa1ffc4c96d..2e0fdbb730b7b 100644
> --- a/kernel/trace/trace_remote.c
> +++ b/kernel/trace/trace_remote.c
[ ... ]
> @@ -991,6 +994,7 @@ int trace_remote_alloc_buffer(struct trace_buffer_desc *desc, size_t desc_size,
>  	desc->struct_len = min_desc_size;
>  
>  	rb_desc = __first_ring_buffer_desc(desc);
> +	nr_pages = max(DIV_ROUND_UP(buffer_size, PAGE_SIZE - BUF_PAGE_HDR_SIZE), 2UL) + 1;

[Severity: High]
This is a pre-existing issue, but are we exposing the same wrapping
vulnerability here?

Since the same macro is used to compute the actual page allocation loop
bounds, would it duplicate the same integer overflow when calculating the
loop limit for allocating the buffer?

>  
>  	for_each_cpu(cpu, cpumask) {
>  		unsigned int id;

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