Re: [PATCH net 2/2] net: validate xmit skbs in generic_xdp_tx()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] net: out-of-bounds read in qdisc_pkt_len_segs_init() via generic_xdp_tx()
- [High] net: shared skb->head corruption on cloned broadcast skbs in generic_xdp_tx()
- [Medium] net: WARN_ONCE in skb_checksum_help() from generic_xdp_tx() after XDP resize
--

commit 47b74c796cb5f05688236335aeb274a972cf3b13
Author: Vladimir Oltean <vladimir.oltean@nxp.com>

net: validate xmit skbs in generic_xdp_tx()

This commit validates skbs in generic_xdp_tx() using
qdisc_pkt_len_segs_init() and validate_xmit_skb_list() before passing them to
netdev_start_xmit(). This ensures SKB_GSO_DODGY packets have gso_segs
recomputed and unsupported offloads handled in software.

> diff --git a/net/core/dev.c b/net/core/dev.c
> index 88860185f5832..e98d85a539916 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -5855,23 +5855,44 @@ void generic_xdp_tx(struct sk_buff *skb, const struct bpf_prog *xdp_prog,
>  		    u32 act)
>  {
>  	struct net_device *dev = skb->dev;
> +	enum skb_drop_reason reason;
>  	struct netdev_queue *txq;
> -	bool free_skb = true;
> +	bool again = false;
>  	int cpu, rc;
>  
> +	reason = qdisc_pkt_len_segs_init(skb);
> +	if (unlikely(reason)) {
> +		trace_xdp_exception(dev, xdp_prog, act);
> +		dev_core_stats_tx_dropped_inc(dev);
> +		kfree_skb_reason(skb, reason);
> +		return;
> +	}

[Severity: High]
Can calling qdisc_pkt_len_segs_init() here cause an out-of-bounds read when a
generic XDP program adjusts the packet head on a GSO packet?

In bpf_prog_run_generic_xdp(), when bpf_xdp_adjust_head() shifts xdp->data by
a non-zero offset, skb->data, skb->mac_header, and skb->network_header are
updated, but skb->transport_header (and skb->inner_transport_header) is left
at its pre-adjustment offset:

net/core/dev.c:bpf_prog_run_generic_xdp() {
	...
	off = xdp->data - orig_data;
	if (off) {
		if (off > 0)
			__skb_pull(skb, off);
		else if (off < 0)
			__skb_push(skb, -off);

		skb->mac_header += off;
		skb_reset_network_header(skb);
	}
	...
}

When the XDP program returns XDP_TX or XDP_REDIRECT, generic_xdp_tx() calls
qdisc_pkt_len_segs_init(skb), which assigns the signed int return value of
skb_transport_offset(skb) (or skb_inner_transport_offset(skb)) to unsigned
int hdr_len:

net/core/dev.c:qdisc_pkt_len_segs_init() {
	...
	if (!skb->encapsulation) {
		if (unlikely(!skb_transport_header_was_set(skb)))
			return SKB_NOT_DROPPED_YET;
		hdr_len = skb_transport_offset(skb);
	} else {
		hdr_len = skb_inner_transport_offset(skb);
	}
	/* + transport layer */
	if (likely(shinfo->gso_type & (SKB_GSO_TCPV4 | SKB_GSO_TCPV6))) {
		const struct tcphdr *th;

		if (!pskb_may_pull(skb, hdr_len + sizeof(struct tcphdr)))
			return SKB_DROP_REASON_SKB_BAD_GSO;

		th = (const struct tcphdr *)(skb->data + hdr_len);
		tlen = __tcp_hdrlen(th);
	...
}

If bpf_xdp_adjust_head() pulled skb->data 1 to 20 bytes past the original
skb_transport_header(skb) on a TCP GSO packet (or 1 to 8 bytes on a
SKB_GSO_UDP_L4 packet), skb_transport_offset(skb) is in [-20, -1] and hdr_len
becomes 0xffffffecU..0xffffffffU. In pskb_may_pull(skb, hdr_len +
sizeof(struct tcphdr)), the 32-bit unsigned addition wraps to 0..19 and
succeeds if skb_headlen(skb) is at least that small wrapped length. Then
skb->data + hdr_len zero-extends hdr_len to 64 bits on 64-bit platforms and
__tcp_hdrlen(th) reads approximately 4 GiB past skb->data.

Even when skb_transport_offset(skb) remains non-negative after
bpf_xdp_adjust_head(), qdisc_pkt_len_segs_init() computes hdr_len and
shinfo->gso_segs from the stale transport_header offset, whereas
validate_xmit_skb_list() -> __skb_gso_segment() -> inet_gso_segment() /
ipv6_gso_segment() later resets skb->transport_header and tcp_gso_segment()
recomputes skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(skb->len, mss) without
re-checking dev->gso_max_segs.

> +
>  	txq = netdev_core_pick_tx(dev, skb, NULL);
> +	skb = validate_xmit_skb_list(skb, dev, &again);
> +	if (!skb) {
> +		trace_xdp_exception(dev, xdp_prog, act);
> +		return;
> +	}

[Severity: High]
Could passing skb to validate_xmit_skb_list() corrupt shared skb->head memory
when a generic XDP program redirects with BPF_F_BROADCAST through a DEVMAP or
DEVMAP_HASH?

During broadcast redirects, dev_map_redirect_multi() calls
dev_map_redirect_clone() for each destination except the last, which clones
the packet with skb_clone(skb, GFP_ATOMIC). In dev_map_generic_redirect(),
skb_copy() is only called when dst->xdp_prog is attached:

kernel/bpf/devmap.c:dev_map_generic_redirect() {
	...
	if (dst->xdp_prog && skb_cloned(skb)) {
		struct sk_buff *nskb;

		nskb = skb_copy(skb, GFP_ATOMIC);
	...
}

When dst->xdp_prog is NULL, the cloned skb reaches generic_xdp_tx() ->
validate_xmit_skb_list() -> validate_xmit_skb() -> __skb_gso_segment(). In
__skb_gso_segment(), skb_cow_head(skb, 0) is only called when
skb_needs_check(skb, true) is true, so it is skipped whenever ip_summed is
CHECKSUM_PARTIAL or CHECKSUM_UNNECESSARY:

net/core/gso.c:__skb_gso_segment() {
	...
	if (unlikely(skb_needs_check(skb, tx_path))) {
		int err;

		/* We're going to init ->check field in TCP or UDP header */
		err = skb_cow_head(skb, 0);
		if (err < 0)
			return ERR_PTR(err);
	}
	...
}

If the packet is an IPv6 UFO skb with ip_summed == CHECKSUM_PARTIAL,
udp6_ufo_fragment() skips gso_pskb_expand_head() because generic XDP ensures
at least XDP_PACKET_HEADROOM (256 bytes) of headroom, which is larger than
tnl_hlen + sizeof(struct frag_hdr). It then modifies uh->check and *prevhdr
and shifts the MAC and IPv6 headers 8 bytes left in place via memmove() inside
the shared skb->head buffer:

net/ipv6/udp_offload.c:udp6_ufo_fragment() {
		if (skb->mac_header < (tnl_hlen + frag_hdr_sz)) {
			if (gso_pskb_expand_head(skb, tnl_hlen + frag_hdr_sz))
				goto out;
		}
		...
		packet_start = (u8 *) skb->head + SKB_GSO_CB(skb)->mac_offset;
		memmove(packet_start-frag_hdr_sz, packet_start, unfrag_len);
}

Similarly, udp4_ufo_fragment(), tcp4_gso_segment(), and tcp6_gso_segment()
overwrite the L4 checksum field in place when ip_summed ==
CHECKSUM_UNNECESSARY. When dev_map_redirect_multi() continues to the remaining
broadcast destinations using the original skb, its mac_header and
network_header still reference the pre-shift offsets in the mutated skb->head
buffer.

[Severity: Medium]
Can calling validate_xmit_skb_list() here also trigger WARN_ONCE() in
skb_checksum_help() when a generic XDP program shrinks or pulls a
CHECKSUM_PARTIAL packet?

In bpf_prog_run_generic_xdp(), adjusting the packet via bpf_xdp_adjust_head()
or bpf_xdp_adjust_tail() updates skb->data, skb->tail, and skb->len without
updating or resetting skb->ip_summed, csum_start, or csum_offset. If a
non-GSO packet with ip_summed == CHECKSUM_PARTIAL has its head pulled past
csum_start or its tail shrunk below csum_start + csum_offset +
sizeof(__sum16), and generic_xdp_tx() transmits it on a device without
checksum offload (such as XDP_TX on a tun device without TUN_F_CSUM),
validate_xmit_skb() -> skb_csum_hwoffload_help() -> skb_checksum_help() hits:

net/core/dev.c:skb_checksum_help() {
	...
	offset = skb_checksum_start_offset(skb);
	ret = -EINVAL;
	if (unlikely(offset >= skb_headlen(skb))) {
		DO_ONCE_LITE(skb_dump, KERN_ERR, skb, false);
		WARN_ONCE(true, "offset (%d) >= skb_headlen() (%u)\n",
			  offset, skb_headlen(skb));
		goto out;
	}
	csum = skb_checksum(skb, offset, skb->len - offset, 0);

	offset += skb->csum_offset;
	if (unlikely(offset + sizeof(__sum16) > skb_headlen(skb))) {
		DO_ONCE_LITE(skb_dump, KERN_ERR, skb, false);
		WARN_ONCE(true, "offset+2 (%zu) > skb_headlen() (%u)\n",
			  offset + sizeof(__sum16), skb_headlen(skb));
		goto out;
	}
	...
}

Since offset is unsigned int, a negative skb_checksum_start_offset(skb) wraps
to a large unsigned value, and either out-of-bounds offset triggers skb_dump()
and WARN_ONCE().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030114753.185281-1-vladimir.oltean@nxp.com?part=2