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

[sashiko-bot@kernel.org]

Sashiko AI review found 3 potential issue(s):
- [High] Calling `kfree_skb(skb)` in `free_err` after `xsk_skb_init_misc()` triggers premature CQ submission and corrupts Queue invariants.
- [High] Memory leak of `xsk_addr` when `alloc_page()` fails during multi-buffer packet processing.
- [High] On 32-bit architectures, `xsk_skb_destructor_set_addr()` truncates the 64-bit UMEM offset, corrupting the Completion Queue address.
--

commit e60e7fb34b1c59698d11da732db02bb3302ab41a
Author: Jason Xing <kerneljasonxing@gmail.com>

xsk: fix use-after-free of xs->skb in xsk_build_skb() free_err path

When xsk_build_skb() processes multi-buffer packets in copy mode, the
first descriptor stores data into the skb linear area without adding any
frags. If a continuation descriptor fails, the error path frees the skb
while xs->skb still points to it. This patch adds a check to ensure we
only free skbs that were freshly allocated in this call.

Link: https://lore.kernel.org/all/20260415082654.21026-4-kerneljasonxing@gmail.com/

> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index 2f00f188533c1..470f6d25ddb93 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c
> @@ -974,7 +974,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 && !skb_shinfo(skb)->nr_frags)
>  		kfree_skb(skb);

This isn't a regression introduced by this patch, but could calling
kfree_skb() here trigger a premature completion queue submission?

If xsk_skb_metadata() fails earlier in the function, the skb destructor
is already set to xsk_destruct_skb by xsk_skb_init_misc().

Freeing the skb here synchronously invokes xsk_destruct_skb(), which
submits the completion to user space and increments the queue producer.
The error path then executes xsk_cq_cancel_locked(), which decrements the
cached producer.

Will this permanently desync the completion queue invariants and report a
successful completion for a failed transmission? Would it be safer to
clear the skb destructor before freeing it?

Also, is there a potential memory leak of the xsk_addr struct when
alloc_page() fails during multi-buffer packet processing?

When processing a continuation descriptor, an xsk_addr struct is
dynamically allocated and stored in skb_shinfo(skb)->destructor_arg
with num_descs set to 1. If the subsequent alloc_page() fails, the code
jumps here to free_err and returns -EAGAIN, leaving the partial packet
in xs->skb for a retry.

If the socket is closed instead of retrying, xsk_drop_skb() is called on
xs->skb, which calls xsk_consume_skb(). Inside xsk_consume_skb(), the
cleanup logic uses this condition:

    if (unlikely(num_descs > 1))

Since num_descs is exactly 1, the condition fails. Does this permanently
leak the xsk_addr memory?

Finally, does xsk_skb_destructor_set_addr() truncate the 64-bit umem
offset on 32-bit architectures?

The inline storage attempts to cast the address to uintptr_t:

    skb_shinfo(skb)->destructor_arg = (void *)((uintptr_t)addr | 0x1UL);

On 32-bit architectures, uintptr_t is 32 bits, which drops the upper 32
bits of the u64 addr. In af_xdp unaligned mode, the chunk offset is
stored in bits 48-63.

Could this truncation lose the offset entirely, causing
xsk_skb_destructor_get_addr() to retrieve a truncated address and
submit it to the user-space completion queue?

>  
>  	if (err == -EOVERFLOW) {
>  		if (xs->skb) {

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