Re: [PATCH net v2 5/8] xsk: avoid skb leak in XDP_TX_METADATA case

[sashiko-bot@kernel.org]

Sashiko AI review found 2 potential issue(s):
- [Critical] Time-of-Check to Time-of-Use (TOCTOU) vulnerability in `xsk_skb_metadata` allows unprivileged users to bypass bounds checks and cause out-of-bounds memory accesses.
- [High] Software checksumming calculation fails completely because `skb_checksum_help()` is invoked on an incomplete and non-linear SKB.
--

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

xsk: avoid skb leak in XDP_TX_METADATA case

This commit explicitly adds a call to kfree_skb() (via
xsk_drop_untrans_skb) when xsk_skb_metadata() returns an error, preventing
an skb leak if metadata processing fails.

Link: https://lore.kernel.org/all/20260415085204.3F87AC19424@smtp.kernel.org/

> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index 55b448abb8178..fd5432fb807e1 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c
> @@ -829,8 +829,10 @@ static struct sk_buff *xsk_build_skb_zerocopy(struct xdp_sock *xs,
>  		xsk_skb_init_misc(skb, xs, desc->addr);
>  		if (desc->options & XDP_TX_METADATA) {
>  			err = xsk_skb_metadata(skb, buffer, desc, pool, hr);

While looking at this metadata handling, I noticed a couple of pre-existing
regressions.

Will skb_checksum_help() always fail when called here?

xsk_skb_metadata() is called right after allocating the SKB but before any
payload fragments are appended via skb_fill_page_desc() further down in
xsk_build_skb_zerocopy(). This means skb->len and skb_headlen(skb) are 0.

If software checksumming is enabled, xsk_skb_metadata() calls
skb_checksum_help(skb). Since skb_headlen(skb) is 0, won't the check
offset >= skb_headlen(skb) in skb_checksum_help() always evaluate to true,
triggering a warning and causing the packet to be dropped?

Also, can a user-space program modify the checksum metadata concurrently?

In xsk_skb_metadata(), the metadata is read directly from the UMEM buffer,
which is shared memory mapped into user-space:

net/xdp/xsk.c:xsk_skb_metadata() {
    ...
	if (meta->flags & XDP_TXMD_FLAGS_CHECKSUM) {
		if (unlikely(meta->request.csum_start +
			     meta->request.csum_offset +
			     sizeof(__sum16) > desc->len))
			return -EINVAL;

		skb->csum_start = hr + meta->request.csum_start;
		skb->csum_offset = meta->request.csum_offset;
    ...
}

The bounds check does not use READ_ONCE(). Could a user-space application
modify meta->request.csum_start and meta->request.csum_offset between the
check and the assignment? If so, it might bypass the bounds check and specify
arbitrarily large offsets, causing out-of-bounds writes when the hardware
inserts the checksum.

> -			if (unlikely(err))
> +			if (unlikely(err)) {
> +				xsk_drop_untrans_skb(skb);
>  				return ERR_PTR(err);
> +			}
>  		}
>  	} else {
>  		struct xsk_addrs *xsk_addr;

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