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 <[email protected]> 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/[email protected]?part=2
