Re: [PATCH net-next 2/8] net: reserve extra TX headroom for local egress

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [High] xsk: u16 overflow of skb->csum_start in xsk_skb_metadata() [1]
--

commit 5b52f2c11788442c9d7cb29588e09f1fbbe3b128
Author: David Keogh <dkeogh@amazon.com>

net: reserve extra TX headroom for local egress

This commit updates local packet allocation paths in IPv4, IPv6,
AF_PACKET, and AF_XDP to include the device's configured extra TX
headroom via LL_ALLOC_SPACE(), LL_ALLOC_SPACE_EX(), and
netdev_tx_alloc_headroom().

> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index 781ba46eb1525..3ee8ad3fd1556 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c
> @@ -1045,7 +1045,8 @@ static struct sk_buff *xsk_build_skb_zerocopy(struct xdp_sock *xs,
>  	buffer = xsk_buff_raw_get_data(pool, addr);
>  
>  	if (!skb) {
> -		hr = max(NET_SKB_PAD, L1_CACHE_ALIGN(xs->dev->needed_headroom));
> +		hr = max(NET_SKB_PAD,
> +			 L1_CACHE_ALIGN(netdev_tx_alloc_headroom(xs->dev)));
>  
>  		skb = sock_alloc_send_skb(&xs->sk, hr, 1, &err);
>  		if (unlikely(!skb))
> @@ -1128,7 +1129,8 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,
>  		len = desc->len;
>  
>  		if (!skb) {
> -			hr = max(NET_SKB_PAD, L1_CACHE_ALIGN(dev->needed_headroom));
> +			hr = max(NET_SKB_PAD,
> +				 L1_CACHE_ALIGN(netdev_tx_alloc_headroom(dev)));
>  			tr = dev->needed_tailroom;
>  			skb = sock_alloc_send_skb(&xs->sk, hr + len + tr, 1, &err);
>  			if (unlikely(!skb))

[Severity: High]
This problem wasn't introduced by this patch, but can hr + csum_start
overflow the u16 skb->csum_start field in xsk_skb_metadata() [1]?

net/xdp/xsk.c:xsk_skb_metadata() {
	...
	if (flags & XDP_TXMD_FLAGS_CHECKSUM) {
		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;

		skb->csum_start = hr + csum_start;
		skb->csum_offset = csum_offset;
		skb->ip_summed = CHECKSUM_PARTIAL;
	}
	...
}

When a UMEM is registered with chunk_size equal to PAGE_SIZE on a 64KB
page kernel (or with a large headroom reservation), desc->len can be up to
65536 and hr + csum_start can exceed U16_MAX while still passing the
desc->len check.  Truncating the sum into skb->csum_start wraps it below
skb_headroom(skb), making skb_checksum_start_offset() negative and
triggering skb_dump() and WARN_ONCE() in skb_checksum_help() or passing a
malformed skb to the driver in validate_xmit_skb().

[1] https://sashiko.dev/#/bug/linux-38d27ef1-cee4-4d03-9410-7bc1d8e0c2f6

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1794020868.git.tianyi@cloudflare.com?part=2