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

[sashiko-bot@kernel.org]

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 <kerneljasonxing@gmail.com>

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.

> 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?

>  		xskq_cons_release(xs->tx);
>  	} else {
>  		/* Let application retry */

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