Re: [PATCH net 1/1] xsk: validate consumer ring producer index

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] xsk: unbounded re-peek loop in __xp_alloc() despite __xskq_cons_peek() check
--

commit b1eb51f9c983fb773e7eaf99ed508d1895d9b804
Author: Zhiling Zou <zhilinz@nebusec.ai>

xsk: validate consumer ring producer index

This commit adds a bounds check in __xskq_cons_peek() to clamp q->cached_prod
to q->cached_cons when the producer-consumer distance exceeds q->nentries,
aiming to bound the consumer loop in __xp_alloc() to the ring size.

Link: https://bugtracker.nebusec.ai/f/13760

> diff --git a/net/xdp/xsk_queue.h b/net/xdp/xsk_queue.h
> index 1bc42c8902f4b..315b656534749 100644
> --- a/net/xdp/xsk_queue.h
> +++ b/net/xdp/xsk_queue.h
> @@ -344,6 +344,9 @@ static inline void __xskq_cons_peek(struct xsk_queue *q)
>  {
>  	/* Refresh the local pointer */
>  	q->cached_prod = smp_load_acquire(&q->ring->producer);  /* C, matches B */
> +	/* A malformed producer must not make us consume beyond the ring. */
> +	if (q->cached_prod - q->cached_cons > q->nentries)
> +		q->cached_prod = q->cached_cons;
>  }
>  
>  static inline void xskq_cons_get_entries(struct xsk_queue *q)

[Severity: High]
Does this check in __xskq_cons_peek() actually bound the loop in __xp_alloc()
to at most q->nentries descriptors?

In net/xdp/xsk_buff_pool.c:__xp_alloc(), invalid fill descriptors are consumed
in an unbounded loop that calls xskq_cons_peek_addr_unchecked() on every
iteration:

net/xdp/xsk_buff_pool.c:__xp_alloc() {
	for (;;) {
		if (!xskq_cons_peek_addr_unchecked(pool->fq, &addr)) {
			pool->fq->queue_empty_descs++;
			return NULL;
		}

		ok = pool->unaligned ? xp_check_unaligned(pool, &addr) :
		     xp_check_aligned(pool, &addr);
		if (!ok) {
			pool->fq->invalid_descs++;
			xskq_cons_release(pool->fq);
			continue;
		}
		break;
	}
}

Whenever pool->fq->cached_cons catches up to pool->fq->cached_prod after
consuming a batch of invalid descriptors, the next iteration of the loop does
not terminate. Instead, xskq_cons_peek_addr_unchecked() in
net/xdp/xsk_queue.h calls xskq_cons_get_entries():

net/xdp/xsk_queue.h:xskq_cons_peek_addr_unchecked() {
	if (q->cached_prod == q->cached_cons)
		xskq_cons_get_entries(q);
	return xskq_cons_read_addr_unchecked(q, addr);
}

xskq_cons_get_entries() then calls __xskq_cons_release() to publish the newly
advanced q->cached_cons to the user-mapped q->ring->consumer, followed by
__xskq_cons_peek() to reload q->cached_prod from q->ring->producer and
validate it against the updated q->cached_cons.

Because __xskq_cons_peek() re-evaluates q->cached_prod - q->cached_cons >
q->nentries relative to the newly advanced q->cached_cons on each reload, two
cases can still exceed q->nentries in a single __xp_alloc() call:

1. On a one-entry ring (q->nentries == 1) containing an invalid descriptor
address, a concurrent userspace thread updating q->ring->producer in the
mmap'd fill ring as q->ring->consumer advances (for example keeping
q->ring->producer equal to q->ring->consumer + 1) satisfies
q->cached_prod - q->cached_cons <= q->nentries on every re-peek in
xskq_cons_get_entries(), keeping the kernel in the __xp_alloc() loop
indefinitely in receive context.

2. Even without concurrent updates during __xp_alloc(), if k > 0 entries are
already cached on entry to __xp_alloc() (q->cached_prod - q->cached_cons == k)
and userspace has written q->ring->producer = q->ring->consumer + k +
q->nentries, __xp_alloc() first consumes the k cached invalid descriptors
until q->cached_cons reaches q->cached_prod. Then xskq_cons_get_entries()
advances q->ring->consumer by k and reloads q->ring->producer, where the
remaining distance is now q->nentries <= q->nentries, causing __xp_alloc() to
consume an additional q->nentries descriptors.

Should __xp_alloc() also bound the number of iterations or avoid re-fetching
new batches via xskq_cons_get_entries() once the initial batch of descriptors
has been exhausted?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792654486.git.zhilinz@nebusec.ai?part=1