Re: [PATCH net v5 4/8] xsk: fix use-after-free of xs->skb in xsk_build_skb() free_err path

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Memory leak of SKB in xsk_build_skb_zerocopy() when TX metadata processing fails.
- [High] Memory leak of xsk_addr array when an SKB fragment allocation fails and the packet is subsequently dropped.
- [High] Double completion and Completion Queue (CQ) state desynchronization when xsk_skb_metadata() fails.
--

commit 7c8b265d19cf8eaa2671c3f79683f55810dfc68a
Author: Jason Xing <kerneljasonxing@gmail.com>

xsk: fix use-after-free of xs->skb in xsk_build_skb() free_err path

When xsk_build_skb() processes multi-buffer packets in copy mode, the
first descriptor stores data into the skb linear area without adding
any frags, so nr_frags stays at 0. The caller then sets xs->skb = skb
to accumulate subsequent descriptors.

Fix by using a !xs->skb check to handle first frag situation, ensuring
we only free skbs that were freshly allocated in this call.

Closes: https://lore.kernel.org/all/20260415082654.21026-4-kerneljasonxing@gmail.com/

> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index 06ee260f3afc..55378c3855d5 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c
> @@ -972,7 +972,7 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,
>  	return skb;
>  
>  free_err:
> -	if (skb && !skb_shinfo(skb)->nr_frags)
> +	if (skb && !xs->skb)

Does this code leak the SKB if xsk_build_skb_zerocopy() allocations fail
during TX metadata processing?

If xsk_build_skb_zerocopy() allocates a new SKB using sock_alloc_send_skb()
but metadata parsing fails (e.g., xsk_skb_metadata() returns an error), it
returns ERR_PTR(err). The caller xsk_build_skb() explicitly overwrites its
local pointer with skb = NULL before jumping to free_err:

        skb = xsk_build_skb_zerocopy(xs, desc);
        if (IS_ERR(skb)) {
                err = PTR_ERR(skb);
                skb = NULL;
                goto free_err;
        }

Since skb is NULL, this check is bypassed, and the newly allocated SKB and
its associated socket write memory quota are leaked.

I understand this is fixed later in the patch series by commit "xsk: avoid
skb leak in XDP_TX_METADATA case".

Does this code leak the xsk_addr array when an SKB fragment allocation
fails and the packet is subsequently dropped?

When appending a fragment to an in-progress packet (xs->skb != NULL), the
code identifies an inline address tag and replaces it by allocating an
xsk_addr array via kmem_cache_zalloc(), initializing xsk_addr->num_descs
to 1.

If alloc_page() subsequently fails, the code returns -EAGAIN to retry later,
leaving xs->skb intact.

If the packet is dropped before a successful retry (e.g., socket close
triggering xsk_release() and xsk_drop_skb()), xsk_consume_skb() attempts to
clean up the array. However, xsk_consume_skb() uses the check
if (unlikely(num_descs > 1)) to determine if it should call
kmem_cache_free().

Since num_descs is exactly 1, the check evaluates to false, and the xsk_addrs
allocation is leaked.

I see this is fixed later in the patch series by commit "xsk: fix xsk_addrs
slab leak on multi-buffer error path".

>  		kfree_skb(skb);

Can this kfree_skb() call lead to a double completion and Completion Queue
(CQ) state desynchronization if xsk_skb_metadata() fails?

In xsk_build_skb() copy mode, xsk_skb_init_misc() sets
skb->destructor = xsk_destruct_skb. Immediately after, if xsk_skb_metadata()
fails, execution jumps to free_err and triggers this kfree_skb(skb).

This executes the SKB destructor, which unconditionally submits the failed
descriptor to the Completion Queue by advancing the user-visible
ring->producer pointer.

Immediately following the free, the free_err handler calls
xsk_cq_cancel_locked(), which decrements the internally cached cached_prod
reservation counter. This desynchronizes the internal reservation counter from
the user-visible ring pointer and falsely pushes a completion event to user
space for a descriptor that failed transmission.

I note this is fixed later in the patch series by commit "xsk: prevent CQ
desync when freeing half-built skbs in xsk_build_skb()".

>  
>  	if (err == -EOVERFLOW) {

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