Re: [PATCH net v3 1/5] xsk: cache csum_start/csum_offset to fix TOCTOU in xsk_skb_metadata()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] The `skb->csum_start` assignment in `xsk_skb_metadata` can silently wrap around 16 bits on architectures with large page sizes (e.g., 64KB), leading to negative checksum offsets and hardware descriptor corruption.
- [High] Software checksum fallback (`skb_checksum_help`) in `xsk_build_skb_zerocopy()` operates on an empty linear SKB, unconditionally failing and dropping packets.
- [High] Returning `-EINVAL` from `xsk_skb_metadata` permanently wedges the AF_XDP TX queue, causing a local Denial of Service.
--

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

xsk: cache csum_start/csum_offset to fix TOCTOU in xsk_skb_metadata()

The TX metadata area resides in the UMEM buffer which is memory-mapped
and concurrently writable by userspace. In xsk_skb_metadata(),
csum_start and csum_offset are read from shared memory for bounds
validation, then read again for skb assignment. This commit fixes a TOCTOU
race by reading these values into local variables once.

Closes: https://lore.kernel.org/all/20260503200927.73EA1C2BCB4@smtp.kernel.org/
Fixes: 48eb03dd2630 ("xsk: Add TX timestamp and TX checksum offload support")

> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index 5e5786cd9af55..f8c8a8c9dfba5 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c
> @@ -811,13 +812,15 @@ static int xsk_skb_metadata(struct sk_buff *skb, void *buffer,
>  		return -EINVAL;
>  
>  	if (meta->flags & XDP_TXMD_FLAGS_CHECKSUM) {
> -		if (unlikely(meta->request.csum_start +
> -			     meta->request.csum_offset +
> +		csum_start = READ_ONCE(meta->request.csum_start);
> +		csum_offset = READ_ONCE(meta->request.csum_offset);
> +
> +		if (unlikely(csum_start + csum_offset +
>  			     sizeof(__sum16) > desc->len))
>  			return -EINVAL;

Does returning -EINVAL here wedge the AF_XDP TX queue?

If xsk_skb_metadata() rejects invalid metadata and returns -EINVAL,
xsk_build_skb() appears to propagate this error without advancing the
consumer pointer via xskq_cons_release(xs->tx).

When __xsk_generic_xmit() receives the -EINVAL error, it bypasses
the post-loop cleanup and the invalid descriptor is never consumed.

Could this put the socket into an infinite loop on subsequent sendmsg() calls?

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

Could this addition overflow skb->csum_start on architectures with large page
sizes?

Since desc->len can be up to PAGE_SIZE (e.g., 65536 bytes on ARM64 or
PowerPC), csum_start could be as large as 65534. Adding the driver headroom
(hr) could cause the sum to exceed 65535, truncating when assigned to the
16-bit skb->csum_start field.

When the driver later computes skb_checksum_start_offset(skb), could this
result in a negative integer and cause out-of-bounds offsets to be programmed
into the hardware?

>  		skb->ip_summed = CHECKSUM_PARTIAL;
>  
>  		if (unlikely(pool->tx_sw_csum)) {
>  			int err;
>  
>  			err = skb_checksum_help(skb);
>  			if (err)
>  				return err;
>  		}

Does skb_checksum_help() unconditionally fail here for zero-copy AF_XDP
sockets?

xsk_build_skb_zerocopy() creates a non-linear SKB where data is stored in
fragments, meaning skb_headlen(skb) is 0.

Inside skb_checksum_help(), it checks if the offset is greater than or equal
to skb_headlen(skb). Since skb_headlen() is 0, this check always evaluates
to true, triggering a WARN_ONCE and returning -EINVAL.

Could this cause all checksummed packets to be dropped when hardware offload
is unsupported and pool->tx_sw_csum is true?

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