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

["Michael S. Tsirkin" <mst@redhat.com>]

Thanks for the quick review! I'll wait a bit for more comments
and post v2.

On Sun, Oct 18, 2026 at 10:16:23AM -0400, Willem de Bruijn wrote:
> Michael S. Tsirkin wrote:
> > If a TCP GSO packet is processed in the stack before a transport
> 
> TSO

right.

> > header was ever established, the TCP data offset is read through the
> > unset header offset, far outside the packet.
> > 
> > This was demonstrated by syzkaller, back when the input path did not
> > dissect such packets: a virtio net header supplying a malformed
> > network header was enough.  Qdisc length estimation has carried a
> > guard against exactly this since a0dce8752193 ("net: Skip GSO
> > length estimation if transport header is not set"); the GSO length
> > validators never got one.
> > 
> > We used to, but we no longer allow such packets in: since 44378d02c5aa
> > ("net: always dissect GSO packets in __virtio_net_hdr_to_skb()") the
> > virtio net input path dissects all GSO packets.
> > 
> > But as a defence in depth measure, let's also check before we read: if
> > the TCP header is unset or outside the linear data, drop it.
> > 
> > Fixes: f43798c27684 ("tun: Allow GSO using virtio_net_hdr")
> > Cc: Willem de Bruijn <willemb@google.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> > 
> > Can be net or net-next, but given we have Eric's patch in net already,
> > net-next seems more appropriate, and it's safer.
> > 
> >  include/linux/skbuff.h |  5 +++++
> >  net/core/gso.c         | 36 ++++++++++++++++++++++++++++++++++++
> >  2 files changed, 41 insertions(+)
> > 
> > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> > index a3ff380d43f0..aa5bc608de06 100644
> > --- a/include/linux/skbuff.h
> > +++ b/include/linux/skbuff.h
> > @@ -3041,6 +3041,11 @@ static inline bool skb_inner_network_header_was_set(const struct sk_buff *skb)
> >  	return skb->inner_network_header > 0;
> >  }
> >  
> > +static inline bool skb_inner_transport_header_was_set(const struct sk_buff *skb)
> > +{
> > +	return skb->inner_transport_header > 0;
> 
> skb_transport_header_was_set has
> 
>         return skb->transport_header != (typeof(skb->transport_header))~0U;
> 
> is that initialization different for inner_transport_header?
> If so, slightly confusing. But also understandable that zero is the
> default initialization, I guess.


Indeed.


> > +}
> > +
> >  static inline unsigned char *skb_inner_mac_header(const struct sk_buff *skb)
> >  {
> >  	return skb->head + skb->inner_mac_header;
> > diff --git a/net/core/gso.c b/net/core/gso.c
> > index e96ef6350064..d98acfd68595 100644
> > --- a/net/core/gso.c
> > +++ b/net/core/gso.c
> > @@ -131,6 +131,36 @@ struct sk_buff *__skb_gso_segment(struct sk_buff *skb,
> >  }
> >  EXPORT_SYMBOL(__skb_gso_segment);
> >  
> > +/*
> > + * GSO processing (but not UDP/SCTP GSO, which uses fixed header
> 
> TSO

right.

> > + * sizes) reads the TCP header as specified by
> > + * skb->transport_header/inner_transport_header.  Make sure it is
> > + * there to read.
> > + */
> > +static bool skb_gso_transport_header_ok(const struct sk_buff *skb)
> > +{
> > +	const struct skb_shared_info *shinfo = skb_shinfo(skb);
> > +	unsigned int thoff;
> > +
> > +	if (likely(!(shinfo->gso_type & (SKB_GSO_TCPV4 | SKB_GSO_TCPV6))))
> > +		return true;
> 
> The likely case for GSO is TSO.

indeed.

> > +
> > +	if (skb->encapsulation) {
> > +		if (!skb_inner_transport_header_was_set(skb))
> > +			return false;
> > +
> > +		thoff = skb_inner_transport_offset(skb);
> > +	} else {
> > +		if (!skb_transport_header_was_set(skb))
> > +			return false;
> > +
> > +		thoff = skb_transport_offset(skb);
> > +	}
> 
> This is not GSO specific. Could be a helper in skbuff.h?

will do.

> > +
> > +	/* A negative offset converts to a huge value and fails. */
> > +	return thoff + sizeof(struct tcphdr) <= skb_headlen(skb);
> > +}
> > +
> >  /**
> >   * skb_gso_transport_seglen - Return length of individual segments of a gso packet
> >   *
> > @@ -253,6 +283,9 @@ static inline bool skb_gso_size_check(const struct sk_buff *skb,
> >   */
> >  bool skb_gso_validate_network_len(const struct sk_buff *skb, unsigned int mtu)
> >  {
> > +	if (unlikely(!skb_gso_transport_header_ok(skb)))
> > +		return false;
> > +
> 
> seems weird to test a transport header feature in a network
> header test?
> 
> Should this instead where it is needed?
> >  	return skb_gso_size_check(skb, skb_gso_network_seglen(skb), mtu);
> >  }
> >  EXPORT_SYMBOL_GPL(skb_gso_validate_network_len);
> > @@ -268,6 +301,9 @@ EXPORT_SYMBOL_GPL(skb_gso_validate_network_len);
> >   */
> >  bool skb_gso_validate_mac_len(const struct sk_buff *skb, unsigned int len)
> >  {
> > +	if (unlikely(!skb_gso_transport_header_ok(skb)))
> > +		return false;
> > +
> 
> even more so in a link layer header test
> 

skb_gso_transport_seglen maybe? I'll have to make it return int
and make callers check that then.


> >  	return skb_gso_size_check(skb, skb_gso_mac_seglen(skb), len);
> >  }
> >  EXPORT_SYMBOL_GPL(skb_gso_validate_mac_len);
> > -- 
> > MST
> > 
>