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

On Sat, Apr 25, 2026 at 12:17 PM <[email protected]> wrote:
>
> Sashiko AI review found 2 potential issue(s):
> - [High] Incorrect completion address reporting to user-space and leaked descriptor on `-EOVERFLOW` in multi-buffer packets.
> - [High] Packet stream corruption when dropping oversized multi-buffer packets.
> --
>
> commit 38322c7d2a85e9ef029404cbf4ae750b54520d27
> Author: Jason Xing <[email protected]>
> xsk: fix use-after-free of xs->skb in xsk_build_skb() free_err path
>
> This patch fixes a use-after-free in the xsk_build_skb() error path
> by ensuring that in-progress multi-buffer socket buffers are not
> freed if they are still referenced by the xdp_sock.
>
> Link: https://lore.kernel.org/all/[email protected]/
>
> > diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> > --- a/net/xdp/xsk.c
> > +++ b/net/xdp/xsk.c
> > @@ -969,7 +969,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)
> >               kfree_skb(skb);
> >
> >       if (err == -EOVERFLOW) {
>
> I noticed a couple of pre-existing regressions in this error path.

Good job. I'll cook a separate patchset to fix this new bug as a
follow-up instead of polluting the current series :)

>
> Do the error handling paths for -EOVERFLOW leak the original descriptor
> address and submit an invalid address to user-space?
>
> When a continuation descriptor exceeds the fragment limit, the code
> jumps to the free_err label before the current descriptor address is
> stored in the xsk_addr array.
>
> However, the error path still increments the descriptor count:
>
>         if (xs->skb) {
>             /* Drop the packet */
>             xsk_inc_num_desc(xs->skb);
>             xsk_drop_skb(xs->skb);
>         }
>
> When xsk_destruct_skb() loops up to the new descriptor count, could it
> read past the initialized entries, find a zero address from the earlier
> kmem_cache_zalloc(), and submit address 0 to the completion queue instead
> of the actual dropped address?

Exactly! It doesn't have any change to run:
        xsk_addr->addrs[xsk_addr->num_descs] = desc->addr;

At the stage of destruction, the right address has never been put into
the completion queue, so the application will not be able to reclaim
them.

>
> Does dropping the socket buffer here cause packet stream corruption?
>
> When a multi-buffer packet is dropped due to an -EOVERFLOW error,
> the partial packet is discarded and the socket buffer pointer is cleared.
>
> Because there is no state tracking to discard the remaining descriptors
> of the dropped packet, will the transmission loop process the next
> descriptor (which is just a continuation of the dropped packet) as the
> beginning of a completely new packet?

It's another critical issue but unlikely to occur. It indeed happens
when the num of frags exceeds 17 as default. The remaining body of the
packet will be treated as the start frag of a new packet since the old
skb carrying 17 frags is freed.

Thanks,
Jason

>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3