Re: [PATCH net v3] net: gso: validate transport header before computing segment length

[Michael S. Tsirkin]

On Sun, Oct 25, 2026 at 04:16:44AM +0000, netdev-bot+sashiko@xxxxxxxxxx wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 · Medium: 0 · Low: 0
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [High] The patch is incomplete for the producer its own commit message
>   names.
> 
> --
> 
> net: gso: validate transport header before computing segment length
> 
> This patch makes the GSO segment length helpers in net/core/gso.c check
> the TCP header before they read it. If the transport header is unset, or
> the TCP header is not in the linear data, skb_gso_validate_network_len()
> and skb_gso_validate_mac_len() now report that the skb does not fit. It
> also adds two helpers, skb_inner_transport_header_was_set() and
> skb_transport_header_linear(), to include/linux/skbuff.h.
> 
> > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> > index d6894f463bf43..11c8b295bd260 100644
> > --- a/include/linux/skbuff.h
> > +++ b/include/linux/skbuff.h
> 
> [ ... ]
> 
> > @@ -3250,6 +3257,29 @@ static inline int skb_transport_offset(const struct sk_buff *skb)
> >  	return skb_transport_header(skb) - skb->data;
> >  }
> >  
> > +/* Is the (inner) transport header, as specified by
> > + * skb->transport_header/inner_transport_header, in the linear data?
> > + */
> > +static inline bool skb_transport_header_linear(const struct sk_buff *skb,
> > +					       unsigned int hlen)
> > +{
> > +	int thoff;
> > +
> > +	if (!skb_transport_header_was_set(skb))
> > +		return false;
> > +
> > +	if (skb->encapsulation) {
> > +		if (!skb_inner_transport_header_was_set(skb))
> > +			return false;
> > +
> > +		thoff = skb_inner_transport_offset(skb);
> > +	} else {
> > +		thoff = skb_transport_offset(skb);
> > +	}
> > +
> > +	return thoff >= 0 && thoff + hlen <= skb_headlen(skb);
> 
> [Severity: High]
> 
> The commit message names bpf_skb_adjust_room() as a way to leave a bad
> transport header behind. It also says:
> 
>     Qdisc length estimation has carried a guard against exactly this since
> 
> Is that guard enough for the bpf_skb_adjust_room() case? In
> qdisc_pkt_len_segs_init(), the only check is for an unset transport
> header. Nothing there matches the thoff >= 0 check added here:
> 
> 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);
>     ...
> }
> 
> bpf_skb_net_hdr_pop() moves mac_header and network_header forward. It
> leaves transport_header where it was, unless it was equal to
> network_header:
> 
> net/core/filter.c:bpf_skb_net_hdr_pop() {
>     ...
> 		skb->mac_header += len;
> 		skb->network_header += len;
> 		if (trans_same)
> 			skb->transport_header = skb->network_header;
>     ...
> }
> 
> When the DECAP_L4 or DECAP_IPXIP flags remove the last tunnel GSO bit,
> bpf_skb_net_shrink() also clears skb->encapsulation.
> 
> Take a locally VXLAN-encapsulated TCP TSO skb whose transport_header
> points at the outer UDP header. If a tc program calls
> bpf_skb_adjust_room(skb, -50, BPF_ADJ_ROOM_MAC,
> BPF_F_ADJ_ROOM_DECAP_L4_UDP | ...), the transport offset ends up at -16
> and encapsulation is 0.
> 
> A simpler BPF_ADJ_ROOM_NET shrink of 40 bytes on a plain Ethernet/IPv4
> TSO skb gives an offset of -6.
> 
> If the program then redirects to egress with bpf_redirect(), this path
> runs before any of the validators hardened in this patch:
> 
> __bpf_tx_skb()
>   dev_queue_xmit()
>     __dev_queue_xmit()
>       qdisc_pkt_len_segs_init()
> 
> In that function hdr_len is an unsigned int, so -16 becomes 0xfffffff0.
> hdr_len + sizeof(struct tcphdr) is then truncated to 4 when passed as the
> unsigned int len argument of pskb_may_pull(), so the pull succeeds.
> 
> Wouldn't __tcp_hdrlen(th) then read about 4GB past skb->data on 64-bit?
> 
> Should qdisc_pkt_len_segs_init() get the same check, for example by
> using skb_transport_header_linear()? If not, could the commit message
> be changed so it does not say the qdisc path already guards against
> this?
> 
> > +}
> > +
> >  static inline u32 skb_network_header_len(const struct sk_buff *skb)
> >  {
> >  	DEBUG_NET_WARN_ON_ONCE(!skb_transport_header_was_set(skb));
> 
> [ ... ]
> 


Indeed. Not nice.

Eric, you seem to have strong opinions about how to fix these
bugs, so, before I spend time testing: a fix like the below, acceptable?

Signed-off-by: Michael S. Tsirkin <mst@xxxxxxxxxx>

---

diff --git a/net/core/dev.c b/net/core/dev.c
index e76762e29360..bef7f0a00140 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4174,7 +4174,8 @@ EXPORT_SYMBOL_GPL(validate_xmit_skb_list);
 static enum skb_drop_reason qdisc_pkt_len_segs_init(struct sk_buff *skb)
 {
 	struct skb_shared_info *shinfo = skb_shinfo(skb);
-	unsigned int hdr_len, tlen;
+	unsigned int tlen;
+	int hdr_len;
 	u16 gso_segs;
 
 	qdisc_skb_cb(skb)->pkt_len = skb->len;
@@ -4197,6 +4198,8 @@ static enum skb_drop_reason qdisc_pkt_len_segs_init(struct sk_buff *skb)
 	} else {
 		hdr_len = skb_inner_transport_offset(skb);
 	}
+	if (unlikely(hdr_len < 0))
+		return SKB_NOT_DROPPED_YET;
 	/* + transport layer */
 	if (likely(shinfo->gso_type & (SKB_GSO_TCPV4 | SKB_GSO_TCPV6))) {
 		const struct tcphdr *th;




-- 
MST