Re: [PATCH net v2 2/8] xsk: handle NULL dereference of the skb without frags issue

On Tue, Apr 21, 2026 at 5:40 PM <[email protected]> wrote:
>
> Sashiko AI review found 1 potential issue(s):
> - [High] Memory leak of newly allocated skb in `xsk_build_skb_zerocopy()` on error paths for the first descriptor.
> --
>
> commit c0083470d60454acdbd801635be8f00a9b79a9eb
> Author: Jason Xing <[email protected]>
>
> xsk: handle NULL dereference of the skb without frags issue
>
> This commit addresses a NULL pointer dereference in the error handling
> path of xsk_build_skb by explicitly checking if xs->skb is NULL. For the
> first-descriptor case, it cancels the CQ slot and tracks the invalid
> descriptor instead of dropping the skb.
>
> > In this series, the skb is already freed by kfree_skb() inside
> > xsk_build_skb_zerocopy for the first-descriptor case, so we only need
> > to do the bookkeeping: cancel the one reserved CQ slot and account for
> > the single invalid descriptor.
>
> Is this description accurate regarding the skb being freed?
>
> Looking at xsk_build_skb(), if xsk_build_skb_zerocopy() encounters an
> error and returns an error pointer, the local skb variable is set to
> NULL before jumping to the free_err label:
>
>     skb = xsk_build_skb_zerocopy(xs, desc);
>     if (IS_ERR(skb)) {
>         err = PTR_ERR(skb);
>         skb = NULL;
>         goto free_err;
>     }
>
> Since skb is NULL, the cleanup code at free_err is bypassed:
>
> free_err:
>     if (skb && !skb_shinfo(skb)->nr_frags)
>         kfree_skb(skb);
>
> Does this code leak the newly allocated skb and its sk_wmem_alloc charge
> when xsk_build_skb_zerocopy() fails on the first descriptor?
>
> I see this is addressed later in the series by commit aef1b3344bd95
> ("xsk: avoid skb leak in XDP_TX_METADATA case") and commit 93315725e1616
> ("xsk: free the skb when hitting the upper bound MAX_SKB_FRAGS"), but
> the commit message here appears to incorrectly assume the free is already
> happening.

They are already fixed in the series. And what I mentioned is "In the
series,...".

>
> > diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> > index 887abed254668..54d4895fc11eb 100644
> > --- a/net/xdp/xsk.c
> > +++ b/net/xdp/xsk.c
>
> [ ... ]
>
> > @@ -973,9 +973,14 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,
> >               kfree_skb(skb);
> >
> >       if (err == -EOVERFLOW) {
> > -             /* Drop the packet */
> > -             xsk_inc_num_desc(xs->skb);
> > -             xsk_drop_skb(xs->skb);
> > +             if (xs->skb) {
> > +                     /* Drop the packet */
> > +                     xsk_inc_num_desc(xs->skb);
> > +                     xsk_drop_skb(xs->skb);
> > +             } else {
> > +                     xsk_cq_cancel_locked(xs->pool, 1);
> > +                     xs->tx->invalid_descs++;
> > +             }
>
> Because the skb wasn't freed as the commit message assumed, can a
> malicious application exploit this missing kfree_skb by repeatedly
> supplying abnormally large descriptors to exhaust socket buffer memory?

It can happen but it doesn't belong to what I should here.

>
> >               xskq_cons_release(xs->tx);
> >       } else {
> >               /* Let application retry */
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2