Re: [PATCH bpf 1/2] bpf: Load ringbuf consumer_pos under the spinlock

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] bpf: ABI compat break for 32-bit userspace on 64-bit ringbuffer
--

commit 2d8c0038759a4aa9aef622680e46c8ac2c7b7e99
Author: Igor Malovitsa <igor.malovitsa@gmail.com>

bpf: Load ringbuf consumer_pos under the spinlock

__bpf_ringbuf_reserve() loads consumer_pos before taking the spinlock.
Nothing bounds how stale that value is when the lock is finally taken.
This patch moves the load of consumer_pos inside the spinlock to ensure
it is exact.

> diff --git a/kernel/bpf/ringbuf.c b/kernel/bpf/ringbuf.c
> index 3f1013d805446..6b86c4db51088 100644
> --- a/kernel/bpf/ringbuf.c
> +++ b/kernel/bpf/ringbuf.c
> @@ -473,11 +473,12 @@ static void *__bpf_ringbuf_reserve(struct bpf_ringbuf *rb, u64 size)
>  	if (len > ringbuf_total_data_sz(rb))
>  		return NULL;
>  
> -	cons_pos = smp_load_acquire(&rb->consumer_pos);
> -
>  	if (raw_res_spin_lock_irqsave(&rb->spinlock, flags))
>  		return NULL;
>  
> +	/* Under the lock, so at most one ring behind producer_pos. */
> +	cons_pos = smp_load_acquire(&rb->consumer_pos);

[Severity: High]
Does this code handle 32-bit userspace running on a 64-bit kernel correctly?

Since consumer_pos and producer_pos are defined as unsigned long in
struct bpf_ringbuf, they are 64-bit on a 64-bit kernel but treated as
32-bit by 32-bit userspace applications.

On little-endian architectures, 32-bit userspace updates only the lower
32 bits. When the userspace counter wraps after 4GB, it writes 0.

Since the kernel's 64-bit producer_pos will exceed 4GB, new_prod_pos -
cons_pos evaluates to a value greater than 4GB.

Because this value is larger than the ringbuffer mask, does this cause the
kernel to incorrectly believe the ringbuffer is permanently full?

Can an unprivileged 32-bit process trigger this state after 4GB of traffic,
causing a permanent DoS on the map?

> +
>  	pend_pos = rb->pending_pos;
>  	prod_pos = rb->producer_pos;
>  	new_prod_pos = prod_pos + len;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016003318.529860-1-igor.malovitsa@gmail.com?part=1