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

On Sun, Apr 19, 2026 at 12:58 PM <[email protected]> wrote:
>
> 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 <[email protected]>
>
> 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/[email protected]/
>
> > 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?

Ack, the same discussion again.

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

Right. See the following inline reply.

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

Right. It can happen!

Brilliant, sashiko found another two pre-existing bugs here! I'm so
shocked at sashiko that handles the extremely complicated path so
well. It's really useful to capture the bug beforehand.

I will include the corresponding two separate patches to fix the same
problematic commit:
commit 0ebc27a4c67d44e5ce88d21cdad8201862b78837
Author: Fernando Fernandez Mancera <[email protected]>
Date:   Mon Nov 24 18:14:09 2025 +0100

    xsk: avoid data corruption on cq descriptor number

    Since commit 30f241fcf52a ("xsk: Fix immature cq descriptor
    production"), the descriptor number is stored in skb control block and
    xsk_cq_submit_addr_locked() relies on it to put the umem addrs onto
    pool's completion queue.

    skb control block shouldn't be used for this purpose as after transmit
    xsk doesn't have control over it and other subsystems could use it. This
    leads to the following kernel panic due to a NULL pointer dereference.

Thanks,
Jason

>
> >
> >       if (err == -EOVERFLOW) {
> >               if (xs->skb) {
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4