Re: [PATCH net v5 6/8] xsk: avoid skb leak in XDP_TX_METADATA case

On Sun, May 3, 2026 at 11:09 PM <[email protected]> wrote:
>
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Critical] Time-of-Check to Time-of-Use (TOCTOU) vulnerability on shared UMEM metadata allows bypassing checksum bounds validation.
> - [High] Leaked `xsk_addrs` structure in the non-zerocopy `xsk_build_skb()` error path when page allocation fails.
> --
>
> commit f1079e9399771f267ff5b6767db05016e5ee4840
> Author: Jason Xing <[email protected]>
>
> xsk: avoid skb leak in XDP_TX_METADATA case
>
> This commit fixes an skb leak in the XDP_TX_METADATA case by explicitly adding
> kfree_skb() before returning back to its caller. The issue happens when
> xsk_skb_metadata() returns an error, leading the caller to clear the skb
> without freeing it.
>
> Closes: https://lore.kernel.org/all/[email protected]/
>
> > diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> > index af3c5752bb63..770ba4695a9d 100644
> > --- a/net/xdp/xsk.c
> > +++ b/net/xdp/xsk.c
> > @@ -821,8 +821,10 @@ static struct sk_buff *xsk_build_skb_zerocopy(struct xdp_sock *xs,
> >               skb_reserve(skb, hr);
> >               if (desc->options & XDP_TX_METADATA) {
> >                       err = xsk_skb_metadata(skb, buffer, desc, pool, hr);
>
> This wasn't introduced by this patch, but is it possible for a shared UMEM
> metadata TOCTOU race to bypass the checksum bounds validation?
>
> In xsk_skb_metadata(), meta points to the metadata area within the umem buffer
> (buffer - pool->tx_metadata_len), which is memory-mapped and concurrently
> writable by userspace. The function performs a bounds check:
>
>     if (unlikely(meta->request.csum_start +
>                  meta->request.csum_offset +
>                  sizeof(__sum16) > desc->len))
>         return -EINVAL;
>
> If this passes, it re-reads the values directly from the shared memory:
>
>     skb->csum_start = hr + meta->request.csum_start;
>     skb->csum_offset = meta->request.csum_offset;
>
> Could a malicious user application race to overwrite these values with
> arbitrarily large 16-bit integers between the validation check and the
> assignment, leading to out-of-bounds DMA accesses? Might we need to copy
> the metadata to a local variable (e.g., using READ_ONCE()) before validation?
>
> > -                     if (unlikely(err))
> > +                     if (unlikely(err)) {
> > +                             kfree_skb(skb);
> >                               return ERR_PTR(err);
> > +                     }
> >               }
> >       } else {
> >               struct xsk_addrs *xsk_addr;
>
> This wasn't introduced by this patch, but does the non-zerocopy
> xsk_build_skb() leak the xsk_addrs structure on its error path?

This issue will be fixed by a standalone patch then :)

>
> When appending a fragment to an existing SKB (!skb is false) in
> xsk_build_skb(), the code converts the destructor_arg from a direct address
> to an allocated xsk_addrs structure and initializes xsk_addr->num_descs = 1.
>
> If the subsequent alloc_page() fails, it jumps to free_err and returns
> -EAGAIN, leaving the partially built SKB in xs->skb without incrementing
> num_descs.
>
> When the socket is closed or the SKB is subsequently dropped via
> xsk_drop_skb(), xsk_consume_skb() is called. Because destructor_arg is now a
> valid pointer, it fetches num_descs as 1. The cleanup logic relies on:
>
>     if (unlikely(num_descs > 1))
>
> to free the xsk_addrs structure. Since num_descs is exactly 1, does it skip
> the free, permanently leaking the xsk_tx_generic_cache object?
>
> I saw this issue is addressed later in the patch series (xsk: fix xsk_addrs
> slab leak on multi-buffer error path).

Yep, it is handled by the patch you said.

Thanks,
Jason

>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6