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

[PATCH 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().

Found by code inspection.

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

Notes (changelog):
    v2->v3:
        address Willem's review: fix commit reference style in the log,
        say the packet is reported as not fitting instead of dropped,
        reject the ~0U sentinel in skb_inner_transport_header_was_set(),
        rename skb_gso_transport_header_ok() to skb_tso_transport_header_ok(),
        check the outer transport header as well when encapsulated,
        fix declaration order
        move the check to skbuff.h as skb_transport_header_linear()
    v1->v2:
        address Willem's comments, in particular, teach
        skb_gso_network_seglen() to return a negative error, and
        propagate through callers
        keep the offset signed and check for sign

 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 v3] net: gso: validate transport header before computing segment length

[Alexei Starovoitov]

On Wed, Oct 21, 2026 at 08:31 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> 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().

As Eric said it should be fixed on bpf side.
It's a bug in bpf_skb_net_hdr_pop().

Something like this (compile tested only):

diff --git a/net/core/filter.c b/net/core/filter.c
index b7ac145..ec47f26 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -3417,7 +3417,6 @@ static int bpf_skb_net_hdr_push(struct sk_buff *skb, u32 off, u32 len)

 static int bpf_skb_net_hdr_pop(struct sk_buff *skb, u32 off, u32 len)
 {
-	bool trans_same = skb->transport_header == skb->network_header;
 	int ret;

 	/* Same here, __skb_push()/__skb_pull() pair not needed. */
@@ -3425,8 +3424,12 @@ static int bpf_skb_net_hdr_pop(struct sk_buff *skb, u32 off, u32 len)
 	if (likely(!ret)) {
 		skb->mac_header += len;
 		skb->network_header += len;
-		if (trans_same)
+		/* It was the same as network header or it was popped. */
+		if (skb->transport_header < skb->network_header)
 			skb->transport_header = skb->network_header;
+		if (skb->encapsulation &&
+		    skb->inner_transport_header < skb->network_header)
+			skb->inner_transport_header = skb->network_header;
 	}

 	return ret;

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 2 · 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 adds a transport-header check only to the GSO length
  validators in net/core/gso.c.
- [High] net: gso: xfrm crypto offload tunnel TSO packets dropped in
  xfrm_output()

--

net: gso: validate transport header before computing segment length

This patch adds a check to the GSO segment length helpers in
net/core/gso.c. For TCP GSO packets, the (inner) transport header must
now be set, and the TCP header must sit in the linear data, before
tcp_hdrlen() is read. If the check fails, skb_gso_validate_network_len()
and skb_gso_validate_mac_len() report the packet as not fitting, and a
new helper, skb_transport_header_linear(), is added to skbuff.h for the
check.

> 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]
Does this break TSO over IPsec tunnel mode with crypto offload?

In xfrm_output(), when xfrm_dev_offload_ok() succeeds for an
XFRM_DEV_OFFLOAD_CRYPTO state, encapsulation is set on the skb before
any inner header offsets have been recorded:

net/xfrm/xfrm_output.c:xfrm_output() {
    ...
		xfrm_get_inner_ipproto(skb, x);
		skb->encapsulation = 1;

		if (skb_is_gso(skb)) {
			...
			skb_shinfo(skb)->gso_type |= SKB_GSO_ESP;
			goto out;
		}
    ...
}

The xfrm_output_gso() branch elided above is only taken when
skb->inner_protocol is already set. A plain TCP skb, whether locally
generated or forwarded, still has inner_protocol and
inner_transport_header at 0 here, so it goes to xfrm_output2(). The
inner offsets are only filled in by the tunnel encap helpers, and that
happens after the size check has run:

xfrm_output2()
  xfrm_output_resume()
    xfrm_output_one()
      xfrm_outer_mode_output()
        xfrm4_prepare_output()
          xfrm_inner_extract_output()
            xfrm4_tunnel_check_size()
              skb_gso_validate_network_len()
          xfrm4_tunnel_encap_add()
            skb_set_inner_transport_header()

The IPv6 path through xfrm6_prepare_output() has the same ordering.

So skb_gso_validate_network_len() is called with encapsulation set
and inner_transport_header == 0. With this patch, the following
happens:

- skb_inner_transport_header_was_set() returns false.
- skb_gso_transport_seglen() returns -EINVAL.
- The packet is reported as not fitting.

xfrm4_tunnel_check_size() (when DF is set, as it normally is for TCP)
and xfrm6_tunnel_check_size() then report the MTU to the local socket
or send an ICMP too big error, and return -EMSGSIZE. xfrm_output_one()
then frees the skb. As a result, every TSO packet over such an SA is
dropped.

The commit message expects the caller to fall back to software
segmentation, but here a false return is treated as a hard MTU
failure. The MTU reported back is the one the flow is already using,
so it is not clear what would make the sender stop building these
packets.

Before this patch, the same call read inner_tcp_hdrlen() from
skb->head and computed thlen against a zero inner offset. The result
was meaningless, but in practice it let these packets through.

Should xfrm_output() record the inner network and transport offsets
at the point where it sets encapsulation for the crypto offload case?
Or is there another way you would prefer to handle this, before the
new check starts requiring a valid inner transport header whenever
encapsulation is set?

[Severity: High]

The commit message says:

  Qdisc length estimation has carried a guard against exactly this since

  commit a0dce8752193 ("net: Skip GSO length estimation if transport
  header is not set");

Does that hold for the bpf_skb_adjust_room() case named later in the
commit message?

The guard in qdisc_pkt_len_segs_init() in net/core/dev.c only rejects an
unset transport header on a non-encapsulated skb:

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

It does not reject a transport offset that is set but negative.
bpf_skb_net_hdr_pop() in net/core/filter.c can leave one behind. After
bpf_skb_generic_pop() moves skb->data forward, only mac_header and
network_header are moved too:

net/core/filter.c:bpf_skb_net_hdr_pop() {
    ...
	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;
	}
    ...
}

Two cases leave transport_header in front of the new skb->data:

- MAC-mode decap of a locally encapsulated VXLAN packet with
  BPF_F_ADJ_ROOM_DECAP_L4_UDP. transport_header still points at the
  outer UDP header, and encapsulation is cleared.
- A NET-mode shrink with len > off.

If the tc program then calls bpf_redirect() to egress, the skb reaches
the estimator before any of the newly guarded validators run:

__bpf_redirect_common()
  __bpf_tx_skb()
    dev_queue_xmit()
      __dev_queue_xmit()
        qdisc_pkt_len_segs_init()

In qdisc_pkt_len_segs_init(), an offset of -16 does the following:

- It is stored in the unsigned hdr_len as 0xFFFFFFF0.
- hdr_len + sizeof(struct tcphdr) is truncated to 4 when passed to
  pskb_may_pull(), so the check passes.
- th ends up about 4GiB past skb->data on 64-bit, and __tcp_hdrlen(th)
  then reads th->doff from it.

Could this oops, or read unrelated memory and compute a garbage length?

The encapsulated branch also calls skb_inner_transport_offset() without
any of the rules this patch adds: inner header not 0 or ~0U, outer
header set, offset >= 0, and header inside the linear data. As a
result, the qdisc estimator and the GSO validators now apply different
rules to the same TSO skb.

Should qdisc_pkt_len_segs_init() also use skb_transport_header_linear(),
or should the commit message describe this as a gap that remains?

[ ... ]

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