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

[sashiko-bot@kernel.org]

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

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/20260415085204.3F87AC19424@smtp.kernel.org/

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

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).

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