Re: [PATCH net v5] 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 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The `struct xsk_tx_metadata` UAPI structure has an inconsistent memory layout between 32-bit and 64-bit architectures, breaking compat mode due to implicit padding.
- [High] Silent 16-bit integer overflow when calculating `skb->csum_start` leads to a negative checksum offset and out-of-bounds hardware DMA faults.
--

commit 8339ac65115fadb8d99c0e54d06d57fab30b5e84
Author: Jason Xing <kerneljasonxing@gmail.com>
Subject: xsk: cache csum_start/csum_offset to fix TOCTOU in xsk_skb_metadata()

This commit addresses a TOCTOU vulnerability where userspace could modify
csum_start and csum_offset between bounds validation and assignment. It fixes
this by using READ_ONCE() to cache the values into local variables, ensuring
that the same validated values are used for SKB assignment.

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

[Severity: High]
This isn't a bug introduced by this patch, but can the UAPI layout for
xsk_tx_metadata cause issues for 32-bit userspace?

include/uapi/linux/if_xdp.h:
struct xsk_tx_metadata {
	__u64 flags;

	union {
		struct {
			/* XDP_TXMD_FLAGS_CHECKSUM */

			/* Offset from desc->addr where checksumming should start. */
			__u16 csum_start;
			/* Offset from csum_start where checksum should be stored. */
			__u16 csum_offset;

			/* XDP_TXMD_FLAGS_LAUNCH_TIME */
			/* Launch time in nanosecond against the PTP HW Clock */
			__u64 launch_time;
		} request;

Because launch_time is an 8-byte type following two 2-byte types, on 64-bit
architectures there will be 4 bytes of implicit padding inserted after
csum_offset. On 32-bit architectures like x86, __u64 only requires 4-byte
alignment, so there is no padding. 

Does this mean a 32-bit compat userspace will write launch_time to an offset
4 bytes earlier than where a 64-bit kernel will read it?

> +		csum_offset = READ_ONCE(meta->request.csum_offset);
> +
> +		if (unlikely(csum_start + csum_offset +
>  			     sizeof(__sum16) > desc->len))
>  			return -EINVAL;
>  
> -		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;

[Severity: High]
This is a pre-existing issue, but is it possible for skb->csum_start to silently
overflow here?

If desc->len is large (e.g. in unaligned_chunks mode or on platforms with
>= 64KB PAGE_SIZE) and csum_start is near 65535, could hr + csum_start exceed
the 16-bit skb->csum_start field?

For example, if hr is 256 and csum_start is 65500, hr + csum_start is 65756
which truncates to 220. Later, skb_checksum_start_offset(skb) calculates
skb->csum_start - skb_headroom(skb) (e.g. 220 - 256 = -36). 

When hardware checksum offload is used, NIC drivers might use this negative
offset to compute header lengths and shift it into the TX descriptor,
potentially causing hardware DMA faults or PCIe completion timeouts.

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