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

On Tue, Apr 21, 2026 at 5:40 PM <[email protected]> wrote:
>
> Sashiko AI review found 1 potential issue(s):
> - [Critical] Dangling `xs->skb` pointer on `-EOVERFLOW` error path leading to Use-After-Free.
> --
>
> commit 4ce0cecf5e8e18bd369c02242f70a42cc8fbdf03
> Author: Jason Xing <[email protected]>
>
> xsk: fix use-after-free of xs->skb in xsk_build_skb() free_err path
>
> This commit aims to fix a use-after-free in the xsk_build_skb() error
> handling path by ensuring we only free newly allocated skbs when a
> continuation descriptor fails. It prevents freeing an in-progress
> multi-buffer skb by adding a check for !xs->skb before kfree_skb(skb).
>
> 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,
> [ ... ]
> >  free_err:
> > -     if (skb && !skb_shinfo(skb)->nr_frags)
> > +     if (skb && !xs->skb && !skb_shinfo(skb)->nr_frags)
> >               kfree_skb(skb);
> >
> >       if (err == -EOVERFLOW) {
>
> Does this patch leave a use-after-free on the -EOVERFLOW error path?
>
> When xsk_build_skb() handles a continuation descriptor and encounters an
> -EOVERFLOW error, it jumps to free_err and enters this block. Looking at the
> subsequent code in xsk_build_skb():
>
>         if (err == -EOVERFLOW) {
>                 if (xs->skb) {
>                         /* Drop the packet */
>                         xsk_inc_num_desc(xs->skb);
>                         xsk_drop_skb(xs->skb);
>                 } else {
>
> The xsk_drop_skb(xs->skb) call consumes and frees the socket buffer, but
> xs->skb is never set to NULL.

This conclusion doesn't hold since this path (xsk_consume_skb(skb) ->
xs->skb = NULL) handles this.

>
> Because xsk_build_skb() returns ERR_PTR(-EOVERFLOW), the caller
> __xsk_generic_xmit() intercepts this error and continues its transmission
> loop without aborting.
>
> On the next iteration, __xsk_generic_xmit() would pass the dangling xs->skb
> pointer back into xsk_build_skb(), which will dereference the freed memory
> when checking skb_shinfo(skb)->nr_frags.
>
> Should xs->skb be cleared to NULL after calling xsk_drop_skb() here?
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3