Re: [PATCH net 3/4] xsk: handle NULL dereference of the skb without frags issue

[sashiko-bot@kernel.org]

Sashiko AI review found 2 potential issue(s):
- [Critical] Calling `kfree_skb(skb)` without clearing the SKB destructor causes Completion Queue (CQ) ring corruption and false completions.
- [High] Use-after-free of `xs->skb` in the continuation path when an error like `-EAGAIN` occurs.
--

commit 1c728000f884ce0a536d9c1fd763eb0c560ada51
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 path of
xsk_build_skb_zerocopy() by checking if xs->skb is valid before
dereferencing it during -EOVERFLOW handling. It also adds an alternative
branch to handle bookkeeping for the first-descriptor case.

> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index 3d6c6caa7ff82..2f00f188533c1 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c
> @@ -978,9 +978,14 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,
>  		kfree_skb(skb);

Can this lead to a use-after-free on xs->skb? 

In the non-zerocopy path, if the first descriptor is linear, nr_frags remains
0 and the caller __xsk_generic_xmit() stores the SKB in xs->skb.

During the next iteration for a continuation descriptor, if an error like
-EAGAIN occurs (e.g., from alloc_page()), the code jumps to the free_err
label. The cleanup logic if (skb && !skb_shinfo(skb)->nr_frags) would
evaluate to true, freeing the entire SKB.

The function then returns -EAGAIN, leaving the freed SKB pointer in xs->skb.
On the next application retry, the code could dereference the freed xs->skb.

Note that this regression is fixed later in the patch series by commit 'xsk:
fix use-after-free of xs->skb in xsk_build_skb() free_err path'.

Also, could calling kfree_skb(skb) without clearing the SKB destructor cause
Completion Queue (CQ) ring corruption and false completions?

In xsk_build_skb_zerocopy(), xsk_skb_init_misc() sets
skb->destructor = xsk_destruct_skb. When an error occurs on the first
descriptor (e.g., xsk_skb_metadata() returning -EINVAL), kfree_skb(skb) is
called.

Because the destructor is not overridden with sock_wfree (like
xsk_consume_skb() does), kfree_skb(skb) triggers xsk_destruct_skb(), which
submits the descriptor to the CQ and increments q->ring->producer.

>  	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++;
> +		}
>  		xskq_cons_release(xs->tx);
>  	} else {
>  		/* Let application retry */
>  		xsk_cq_cancel_locked(xs->pool, 1);
>  	}

The error handling paths then call xsk_cq_cancel_locked(), which decrements
the reservation counter q->cached_prod.

Does decrementing the reservation while the actual producer pointer has
advanced break queue accounting?

The kernel calculates available space using cached_prod; this artificial
underflow could cause the queue to over-report free space, allowing the
producer to silently overwrite unread user-space CQ entries.

Furthermore, in the -EINVAL path, does the invalid descriptor generate a
false completion to user space without being released from the TX ring? When
the application reuses the completed buffer, could the kernel retransmit the
same descriptor on retry, causing a use-after-free and data corruption?

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