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

[Maciej Fijalkowski <maciej.fijalkowski@intel.com>]

On Wed, Jul 15, 2026 at 02:08:44PM +0000, sashiko-bot@kernel.org wrote:
> 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?

This is clamped on later patch:

+	nb_pkts = min(nb_pkts, pool->tx_descs_nentries);
+	if (!nb_pkts)
+		goto out;
+
+	umem_shared = !list_is_singular(&pool->xsk_tx_list);
+
+	if (umem_shared && !(pool->umem->flags & XDP_UMEM_SG_FLAG)) {
+		rcu_read_unlock();
+		return xsk_tx_peek_release_fallback(pool, nb_pkts);
+	}

> 
> 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