Re: [PATCH net v5 8/8] xsk: fix u64 descriptor address truncation on 32-bit architectures

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] In the copy-mode TX path (`xsk_build_skb`), if a multi-buffer fragment exceeds the `MAX_SKB_FRAGS` limit, the buffer's address is leaked to userspace because `0` is erroneously submitted to the completion queue.
--

commit 4a776aefc60aa4d3123e7e63fd8f927f3748d008
Author: Jason Xing <kerneljasonxing@gmail.com>

xsk: fix u64 descriptor address truncation on 32-bit architectures

This commit fixes a 32-bit truncation issue when storing 64-bit descriptor
addresses in the SKB destructor argument by allocating an xsk_addrs struct.
It consolidates the allocation logic into xsk_addrs_alloc() and uses it in
both zerocopy and copy modes.

Link: https://lore.kernel.org/all/20260419045824.D9E5EC2BCAF@smtp.kernel.org/

> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index 079abd4bcb69..5e5786cd9af5 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c

[ ... ]

> @@ -929,19 +959,10 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,
>  			struct page *page;
>  			u8 *vaddr;
>  
> -			if (xsk_skb_destructor_is_addr(skb)) {
> -				xsk_addr = kmem_cache_zalloc(xsk_tx_generic_cache,
> -							     GFP_KERNEL);
> -				if (!xsk_addr) {
> -					err = -ENOMEM;
> -					goto free_err;
> -				}
> -
> -				xsk_addr->num_descs = 1;
> -				xsk_addr->addrs[0] = xsk_skb_destructor_get_addr(skb);
> -				skb_shinfo(skb)->destructor_arg = (void *)xsk_addr;
> -			} else {
> -				xsk_addr = (struct xsk_addrs *)skb_shinfo(skb)->destructor_arg;
> +			xsk_addr = xsk_addrs_alloc(skb);
> +			if (!xsk_addr) {
> +				err = -ENOMEM;
> +				goto free_err;
>  			}
>  
>  			if (unlikely(nr_frags == (MAX_SKB_FRAGS - 1) && xp_mb_desc(desc))) {

This isn't a regression introduced by this patch, but while looking at the
modified code, does this error handling path leak the buffer's address to
userspace?

If this overflow check evaluates to true, the code sets err = -EOVERFLOW and
jumps to the free_err label before assigning desc->addr to
xsk_addr->addrs[xsk_addr->num_descs].

In the free_err block, it handles -EOVERFLOW by calling
xsk_inc_num_desc(xs->skb) and xsk_drop_skb(xs->skb). This increments
num_descs to account for the dropped descriptor and eventually calls
xsk_destruct_skb() which calls xsk_cq_submit_addr_locked().

Since xsk_addr->addrs was never initialized with desc->addr for this
descriptor, does this mean it submits 0 (from kmem_cache_zalloc) to the
completion queue instead of the actual buffer address, causing userspace
to permanently lose track of the buffer?

By comparison, xsk_build_skb_zerocopy() assigns desc->addr to the array
before checking the fragment limit:

net/xdp/xsk.c:xsk_build_skb_zerocopy() {
    ...
		xsk_addr = xsk_addrs_alloc(skb);
		if (!xsk_addr)
			return ERR_PTR(-ENOMEM);

		/* in case of -EOVERFLOW that could happen below,
		 * xsk_consume_skb() will release this node as whole skb
		 * would be dropped, which implies freeing all list elements
		 */
		xsk_addr->addrs[xsk_addr->num_descs] = desc->addr;
	}

	len = desc->len;
    ...
}

Should xsk_build_skb() also record the descriptor address before checking
the -EOVERFLOW condition?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260502200722.53960-1-kerneljasonxing@gmail.com?part=8