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

On Sun, May 3, 2026 at 11:09 PM <[email protected]> wrote:
>
> 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 <[email protected]>
>
> 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/[email protected]/
>
> > 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()".

Those three points you brought up are all handled in the series.
Again, the order of this series is designed deliberately to avoid
new/severe problems.

Thanks,
Jason

>
> >
> >       if (err == -EOVERFLOW) {
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4