On Sat, Oct 31, 2026 at 11:48:09AM +0000, [email protected] wrote: > 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.
The stale transport headers seems to be all pre-existing issues, based on the fact that Sashiko reported it on this other patch as well, triggerable from bpf_skb_net_hdr_pop() there: https://lore.kernel.org/netdev/[email protected]/ As Eric says, the broken skb_transport_header should be fixed at producer side, so there is nothing really actionable for this patch, I think. > > + > > 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. Hmm, the problem seems valid, but AFAIU, the problem that __skb_gso_segment() does not unshare the skb every time it should should be fixed in __skb_gso_segment() itself, nothing actionable for this change. If anything, cloned GSO skbs can already come from bridge flooding and will reach __skb_gso_segment() from __dev_queue_xmit(), so the problem is pre-existing. > [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(). Correction: 'offset' is int, not unsigned int (although this doesn't change the outcome, since skb_headlen() returns unsigned int, the comparison is made on unsigned values regardless). So skb_checksum_start_offset() returns the offset between skb->csum_start and skb->data, and Sashiko says that if bpf_xdp_adjust_head() pulls skb->data beyond skb->csum_start for a non-GSO packet, this offset would become negative, it would trigger the WARN_ONCE() and the skb would be dropped. I think the expected alternative outcome would be to do something like bpf_skb_adjust_room() -> bpf_skb_net_shrink() -> bpf_skb_net_hdr_pop() -> bpf_skb_generic_pop() -> skb_postpull_rcsum() -> __skb_postpull_rcsum() does, and convert CHECKSUM_PARTIAL to CHECKSUM_NONE. In any case this is something that needs to be addressed in bpf_xdp_adjust_head() and bpf_xdp_adjust_tail(), because I think the exact same thing can already happen even without XDP_TX / XDP_REDIRECT, i.e. with XDP_PASS plus some sort of forwarding to the same kind of egress device (bridging or IP forwarding). In short, I don't think this patch makes any of the above problems worse than they already are.
