Re: [PATCH v3 net 3/6] xsk: provide sufficient space in pool->tx_descs

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Heap buffer overflow in AF_XDP TX batch processing due to undersized `pool->tx_descs` in shared UMEM configurations.
--

--- Patch [3]: [PATCH v3 net 3/6] xsk: provide sufficient space in pool->tx_descs ---
commit 6f635cabf694299ed3f7be4ce0e5f328e6bd90e0
Author: Maciej Fijalkowski <maciej.fijalkowski@intel.com>

xsk: provide sufficient space in pool->tx_descs

The temporary Tx descriptor array in an XSK buffer pool is currently
sized from the Tx ring of the socket that creates the pool.

[ ... ]

> Size the temporary descriptor array to the larger of the first Tx ring
> and the device's xdp_zc_max_segs capability. This keeps the array large
> enough to inspect one maximum-sized valid packet. Larger shared Tx rings
> do not require further resizing, as they can be processed over multiple
> batches.

[Severity: High]
This is a pre-existing issue, but is the statement that larger shared Tx
rings do not require further resizing accurate?

If a secondary socket shares the UMEM and has a much larger TX ring than
the first socket, pool->tx_descs is not resized, locking its size to
max(first_socket_nentries, dev->xdp_zc_max_segs).

However, the batch reading functions don't seem to bound their reads to
the allocated size of the array. They appear to read up to the driver's
budget (e.g., 64) and the current socket's available elements. 

Could this cause a heap buffer overflow when writing into descs?

net/xdp/xsk.c:xsk_tx_peek_release_fallback() {
    ...
	while (nb_pkts < max_entries && xsk_tx_peek_desc(pool, &descs[nb_pkts]))
		nb_pkts++;
    ...
}

net/xdp/xsk_queue.h:xskq_cons_read_desc_batch() {
    ...
	while (cached_cons != q->cached_prod && nb_entries < max) {
		...
		descs[nb_entries] = ring->desc[idx];
		cached_cons++;
    ...
}

If a user binds a minimal first socket (e.g. 2 entries) and a large shared
second socket, the softirq batch process could write past the end of the
array. Since ring->desc is memory-mapped to userspace, could an attacker
control the overwritten heap memory?

> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index 43791647cf182..385a3f4a1b320 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c
> @@ -1557,7 +1558,8 @@ static int xsk_bind(struct socket *sock, struct sockaddr_unsized *addr, int addr
>  			 * utilizes
>  			 */
>  			if (xs->tx && !xs->pool->tx_descs) {
> -				err = xp_alloc_tx_descs(xs->pool, xs);
> +				err = xp_alloc_tx_descs(xs->pool, xs,
> +							dev->xdp_zc_max_segs);

This isn't a bug introduced by this patch, but since tx_descs is not
reallocated if it already exists, the array size is permanently frozen
to the first socket's size.

Should the array be resized to accommodate the largest shared ring, or
should the batch readers be updated to cap their reads to the array's
actual size?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714140722.111645-1-maciej.fijalkowski@intel.com?part=3