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

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

[Michael S. Tsirkin]

If a TSO packet is processed in the stack before a transport
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

commit 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

commit 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, report the packet
as not fitting.  What happens to it then is up to the caller: the IP
output path segments in software, a forwarder answers with an ICMP
error, policers apply their exceed action.

No unprivileged path is known to produce one - IP input resets the
transport header to the parsed position, local delivery pulls data
exactly to it, and tunnel decapsulation clears it altogether - but a tc
BPF program can leave one behind through bpf_skb_adjust_room().  The
comparison is free, so let's not rely on enumerating producers.

Found by code review.

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>
---
 include/linux/skbuff.h | 30 +++++++++++++++++
 net/core/gso.c         | 75 ++++++++++++++++++++++++++++++++++++------
 2 files changed, 95 insertions(+), 10 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index a3ff380d43f0..6f33c014e568 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -3041,6 +3041,13 @@ 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)
+{
+	typeof(skb->inner_transport_header) th = skb->inner_transport_header;
+
+	return th > 0 && th != (typeof(th))~0U;
+}
+
 static inline unsigned char *skb_inner_mac_header(const struct sk_buff *skb)
 {
 	return skb->head + skb->inner_mac_header;
@@ -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);
+}
+
 static inline u32 skb_network_header_len(const struct sk_buff *skb)
 {
 	DEBUG_NET_WARN_ON_ONCE(!skb_transport_header_was_set(skb));
diff --git a/net/core/gso.c b/net/core/gso.c
index e96ef6350064..204ab9d08c18 100644
--- a/net/core/gso.c
+++ b/net/core/gso.c
@@ -131,6 +131,22 @@ struct sk_buff *__skb_gso_segment(struct sk_buff *skb,
 }
 EXPORT_SYMBOL(__skb_gso_segment);
 
+/*
+ * TSO processing (but not UDP/SCTP GSO, which uses fixed header
+ * sizes) reads the TCP header as specified by
+ * skb->transport_header/inner_transport_header.  Make sure it is
+ * there to read.
+ */
+static bool skb_tso_transport_header_ok(const struct sk_buff *skb)
+{
+	const struct skb_shared_info *shinfo = skb_shinfo(skb);
+
+	if (unlikely(!(shinfo->gso_type & (SKB_GSO_TCPV4 | SKB_GSO_TCPV6))))
+		return true;
+
+	return skb_transport_header_linear(skb, sizeof(struct tcphdr));
+}
+
 /**
  * skb_gso_transport_seglen - Return length of individual segments of a gso packet
  *
@@ -140,12 +156,18 @@ EXPORT_SYMBOL(__skb_gso_segment);
  * individual segments, including Layer4 headers (TCP/UDP).
  *
  * The MAC/L2 or network (IP, IPv6) headers are not accounted for.
+ *
+ * Return: the length, or a negative errno if the TCP header is not
+ * there to read.
  */
-static unsigned int skb_gso_transport_seglen(const struct sk_buff *skb)
+static int skb_gso_transport_seglen(const struct sk_buff *skb)
 {
 	const struct skb_shared_info *shinfo = skb_shinfo(skb);
 	unsigned int thlen = 0;
 
+	if (unlikely(!skb_tso_transport_header_ok(skb)))
+		return -EINVAL;
+
 	if (skb->encapsulation) {
 		thlen = skb_inner_transport_header(skb) -
 			skb_transport_header(skb);
@@ -175,13 +197,21 @@ static unsigned int skb_gso_transport_seglen(const struct sk_buff *skb)
  * individual segments, including Layer3 (IP, IPv6) and L4 headers (TCP/UDP).
  *
  * The MAC/L2 header is not accounted for.
+ *
+ * Return: the length, or a negative errno if the TCP header is not
+ * there to read.
  */
-static unsigned int skb_gso_network_seglen(const struct sk_buff *skb)
+static int skb_gso_network_seglen(const struct sk_buff *skb)
 {
-	unsigned int hdr_len = skb_transport_header(skb) -
-			       skb_network_header(skb);
+	int tlen = skb_gso_transport_seglen(skb);
+	unsigned int hdr_len;
 
-	return hdr_len + skb_gso_transport_seglen(skb);
+	if (tlen < 0)
+		return tlen;
+
+	hdr_len = skb_transport_header(skb) - skb_network_header(skb);
+
+	return hdr_len + tlen;
 }
 
 /**
@@ -192,12 +222,21 @@ static unsigned int skb_gso_network_seglen(const struct sk_buff *skb)
  * skb_gso_mac_seglen is used to determine the real size of the
  * individual segments, including MAC/L2, Layer3 (IP, IPv6) and L4
  * headers (TCP/UDP).
+ *
+ * Return: the length, or a negative errno if the TCP header is not
+ * there to read.
  */
-static unsigned int skb_gso_mac_seglen(const struct sk_buff *skb)
+static int skb_gso_mac_seglen(const struct sk_buff *skb)
 {
-	unsigned int hdr_len = skb_transport_header(skb) - skb_mac_header(skb);
+	int tlen = skb_gso_transport_seglen(skb);
+	unsigned int hdr_len;
 
-	return hdr_len + skb_gso_transport_seglen(skb);
+	if (tlen < 0)
+		return tlen;
+
+	hdr_len = skb_transport_header(skb) - skb_mac_header(skb);
+
+	return hdr_len + tlen;
 }
 
 /**
@@ -250,10 +289,18 @@ static inline bool skb_gso_size_check(const struct sk_buff *skb,
  * skb_gso_validate_network_len validates if a given skb will fit a
  * wanted MTU once split. It considers L3 headers, L4 headers, and the
  * payload.
+ *
+ * Return: Whether the skb will fit.  Also false if the TCP header
+ * is not there to read.
  */
 bool skb_gso_validate_network_len(const struct sk_buff *skb, unsigned int mtu)
 {
-	return skb_gso_size_check(skb, skb_gso_network_seglen(skb), mtu);
+	int seg_len = skb_gso_network_seglen(skb);
+
+	if (seg_len < 0)
+		return false;
+
+	return skb_gso_size_check(skb, seg_len, mtu);
 }
 EXPORT_SYMBOL_GPL(skb_gso_validate_network_len);
 
@@ -265,10 +312,18 @@ EXPORT_SYMBOL_GPL(skb_gso_validate_network_len);
  *
  * skb_gso_validate_mac_len validates if a given skb will fit a wanted
  * length once split, including L2, L3 and L4 headers and the payload.
+ *
+ * Return: Whether the skb will fit.  Also false if the TCP header
+ * is not there to read.
  */
 bool skb_gso_validate_mac_len(const struct sk_buff *skb, unsigned int len)
 {
-	return skb_gso_size_check(skb, skb_gso_mac_seglen(skb), len);
+	int seg_len = skb_gso_mac_seglen(skb);
+
+	if (seg_len < 0)
+		return false;
+
+	return skb_gso_size_check(skb, seg_len, len);
 }
 EXPORT_SYMBOL_GPL(skb_gso_validate_mac_len);
 
-- 
MST

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

[Eric Dumazet]

Le mer. 21 oct. 2026 à 17:13, Michael S. Tsirkin <mst@redhat.com> a écrit :
>
> If a TSO packet is processed in the stack before a transport
> 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
>
> commit 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
>
> commit 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

I do not think we want this.

The root cause was fixed at the source by 44378d02c5aa. This patch adds
code to skb_gso_validate_network_len(), which ip_finish_output_gso() and
ip6_finish_output_gso() call for every GSO packet, including locally
generated TCP, to guard against a state the stack no longer produces.

The Fixes: tag is misleading: by your own changelog, nothing reachable is
fixed, yet this would be routed to stable.

As for bpf_skb_adjust_room(): if it can leave an inconsistent transport
header, please send a fix for bpf_skb_adjust_room(). Note that what it
would leave behind is a stale offset inside skb->head, which this check
does not catch anyway. Drivers using skb_tcp_all_headers() would still
read it.

Turning a broken invariant into "does not fit" also has odd side effects
(ip_forward() sending ICMP FRAG_NEEDED with a bogus MTU, policers
applying their exceed action) and hides the producer instead of
exposing it. If you want coverage, a DEBUG_NET_WARN_ON_ONCE() as in
skb_network_header_len() is the right tool: free in production, and
syzbot will tell us who produced such a packet.

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

[Michael S. Tsirkin]

On Wed, Oct 21, 2026 at 05:49:35PM +0200, Eric Dumazet wrote:
> Le mer. 21 oct. 2026 à 17:13, Michael S. Tsirkin <mst@redhat.com> a écrit :
> >
> > If a TSO packet is processed in the stack before a transport
> > 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
> >
> > commit 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
> >
> > commit 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
> 
> I do not think we want this.
> 
> The root cause was fixed at the source by 44378d02c5aa. This patch adds
> code to skb_gso_validate_network_len(), which ip_finish_output_gso() and
> ip6_finish_output_gso() call for every GSO packet, including locally
> generated TCP, to guard against a state the stack no longer produces.

I just worry we missed some entry points. Shrug. As you wish.

-- 
MST

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

[netdev-bot+sashiko]

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));

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/52b8bfe4ee848aff8ace47752352de486171d45f.1792595507.git.mst%40redhat.com

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@kernel.org 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@redhat.com>

---

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

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

[Eric Dumazet]

Le dim. 25 oct. 2026 à 17:51, Michael S. Tsirkin <mst@redhat.com> a écrit :
>
> On Sun, Oct 25, 2026 at 04:16:44AM +0000, netdev-bot+sashiko@kernel.org 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?

The report looks valid, but this patches _one_ consumer of a broken
transport_header, not its _producer_.

bpf_skb_net_hdr_pop() adjusts mac_header and network_header, but leaves
transport_header alone unless it was equal to network_header. If the
popped region covers the transport header (BPF_ADJ_ROOM_NET shrink,
or a DECAP_* shrink in BPF_ADJ_ROOM_MAC mode), we keep a stale offset.
It can be negative, or positive and pointing to the wrong bytes.

qdisc_pkt_len_segs_init() is only one reader. TSO drivers
(skb_tcp_all_headers(), tcp_hdr() in ndo_features_check()/ndo_start_xmit())
and the GSO validators would read the same stale offset.
A 'hdr_len < 0' test does not catch the positive case either.

bpf_skb_net_grow() already sets the transport header properly,
the shrink side should do the same. Something like (untested):

diff --git a/net/core/filter.c b/net/core/filter.c
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ static int bpf_skb_net_hdr_pop(struct sk_buff *skb, u32 off, u32 len)
 {
        bool trans_same = skb->transport_header == skb->network_header;
+       bool trans_popped = false;
        int ret;

+       if (!trans_same && skb_transport_header_was_set(skb)) {
+               int thoff = skb_transport_offset(skb);
+
+               trans_popped = thoff >= (int)off && thoff < (int)(off + len);
+       }
+
        /* Same here, __skb_push()/__skb_pull() pair not needed. */
        ret = bpf_skb_generic_pop(skb, off, len);
        if (likely(!ret)) {
                skb->mac_header += len;
                skb->network_header += len;
                if (trans_same)
                        skb->transport_header = skb->network_header;
+               else if (trans_popped)
+                       skb_set_transport_header(skb, off);
        }

        return ret;
 }

For the decap case, BPF folks might prefer to restore the transport header
from skb->inner_transport_header when skb->encapsulation is cleared.

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

[Michael S. Tsirkin]

On Sun, Oct 25, 2026 at 08:40:57PM +0100, Eric Dumazet wrote:
> Le dim. 25 oct. 2026 à 17:51, Michael S. Tsirkin <mst@redhat.com> a écrit :
> >
> > On Sun, Oct 25, 2026 at 04:16:44AM +0000, netdev-bot+sashiko@kernel.org 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?
> 
> The report looks valid, but this patches _one_ consumer of a broken
> transport_header, not its _producer_.
> 
> bpf_skb_net_hdr_pop() adjusts mac_header and network_header, but leaves
> transport_header alone unless it was equal to network_header. If the
> popped region covers the transport header (BPF_ADJ_ROOM_NET shrink,
> or a DECAP_* shrink in BPF_ADJ_ROOM_MAC mode), we keep a stale offset.
> It can be negative, or positive and pointing to the wrong bytes.
> 
> qdisc_pkt_len_segs_init() is only one reader. TSO drivers
> (skb_tcp_all_headers(), tcp_hdr() in ndo_features_check()/ndo_start_xmit())
> and the GSO validators would read the same stale offset.
> A 'hdr_len < 0' test does not catch the positive case either.
> 
> bpf_skb_net_grow() already sets the transport header properly,
> the shrink side should do the same. Something like (untested):
> 
> diff --git a/net/core/filter.c b/net/core/filter.c
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ static int bpf_skb_net_hdr_pop(struct sk_buff *skb, u32 off, u32 len)
>  {
>         bool trans_same = skb->transport_header == skb->network_header;
> +       bool trans_popped = false;
>         int ret;
> 
> +       if (!trans_same && skb_transport_header_was_set(skb)) {
> +               int thoff = skb_transport_offset(skb);
> +
> +               trans_popped = thoff >= (int)off && thoff < (int)(off + len);
> +       }
> +
>         /* Same here, __skb_push()/__skb_pull() pair not needed. */
>         ret = bpf_skb_generic_pop(skb, off, len);
>         if (likely(!ret)) {
>                 skb->mac_header += len;
>                 skb->network_header += len;
>                 if (trans_same)
>                         skb->transport_header = skb->network_header;
> +               else if (trans_popped)
> +                       skb_set_transport_header(skb, off);
>         }
> 
>         return ret;
>  }
> 
> For the decap case, BPF folks might prefer to restore the transport header
> from skb->inner_transport_header when skb->encapsulation is cleared.


OK, do you want to propose a patch, or need me to look into it?
And are you open to patching also all known consumers,
in case we miss a producer, or only the producers?

-- 
MST