[PATCH net v6 0/3] net: virtio: validate outer protocol of UDP tunnel GSO packets

[PATCH net v6 0/3] net: virtio: validate outer protocol of UDP tunnel GSO packets

[Eric Dumazet]

virtio_net_hdr_tnl_to_skb() does not check that a UDP tunnel GSO
packet really has outer IPv4/IPv6 and UDP headers matching the
declared tunnel type. A guest, or any tap writer, can send a frame
with another outer ethertype (NSH in the report), and the GSO stack
then builds segments with skb->data before skb->head. The host
crashes in skb_under_panic() as soon as a header is pushed.

Patch 1 fixes the producer side, virtio_net_hdr_tnl_from_skb(), for
VLAN tagged packets:
- the tunnel offsets did not account for the VLAN tag tun inserts in
  the frame, and were 4 bytes short.
- the outer family came from skb->protocol, which is the VLAN protocol
  when the tag is in the packet data, so an outer IPv6 header was
  reported as IPv4.
Without it, patch 3 would drop such packets. Kernels without this fix
still send such headers, so it should go to stable along with patch 3.

Patch 2 clears a stale skb->protocol left on recycled NAPI frags skbs,
which could make the checks of patch 3 reject valid frames.

Patch 3 validates the outer headers, using the parsing helpers from
Michael's series "net: resolve uncommon partial checksums at ingress",
as Willem suggested. The callers tell whether frames start with an
Ethernet header, as the device type of a tun device can be changed
with TUNSETLINK. The rest of Michael's series (checksum validation and
completion) is left to him.

The selftests of v3 will follow in a separate series, with the fixes
Sashiko found in them.

v6:
- Patch 1: also take the outer family from vlan_get_protocol()
  instead of skb->protocol (Sashiko). New title.
- Move the tun skb->protocol fix (now patch 2) before the outer
  protocol check (now patch 3), so that no commit rejects valid frames
  (Sashiko). Reword its changelog accordingly.
- No change to patch 3.

v5: https://lore.kernel.org/netdev/20261109163507.3102091-1-edumazet@kernel.org/
- New patch 1: account for the VLAN tag in the tunnel offsets built by
  virtio_net_hdr_tnl_from_skb() (Sashiko).
- Patch 2: add Michael's Acked-by, no code change.

v4: https://lore.kernel.org/netdev/20261106095340.2113683-1-edumazet@kernel.org/
- Squash v3 patch 2 into patch 1 (Willem). Dropped Michael's Acked-by,
  as virtio_net_l3_start() now takes a has_eth argument.
- Patch 2: clear skb->protocol in tun_napi_alloc_frags(), the only
  place that needs it (Willem).
- Drop the selftests from this series (Willem).

v3: https://lore.kernel.org/netdev/20261105115715.1466924-1-edumazet@kernel.org/
- Patch 1: fix a comment, ipv6_find_hdr() stops at AH (Sashiko).
- New patch 2: do not use the device type to find the outer L3 header,
  TUNSETLINK can change it (Sashiko).
- New patch 3: clear a stale skb->protocol on IFF_NAPI_FRAGS skbs
  (Sashiko).
- More selftests, and big-endian fixes for them (Sashiko).

v2: https://lore.kernel.org/netdev/20261104050131.629967-1-edumazet@kernel.org/
- Use Michael's helpers instead of a new one: find the L3 header from
  the device type instead of assuming Ethernet when skb->protocol is
  unset, do not trust skb->protocol when it disagrees with the frame,
  and require UDP exactly at outer_th_offset. Stop at IPv6 AH headers
  (Willem).
- Selftest fixes, IFF_TUN tests.

v1: https://lore.kernel.org/netdev/20261103203113.550604-1-edumazet@kernel.org/

Eric Dumazet (3):
  net: virtio: fix UDP tunnel GSO header of VLAN tagged packets
  tun: clear stale skb->protocol of NAPI frags skbs
  net: virtio: validate outer protocol of UDP tunnel GSO packets

 drivers/net/tun.c          |  15 ++-
 drivers/net/tun_vnet.h     |   5 +-
 drivers/net/virtio_net.c   |   2 +-
 include/linux/virtio_net.h | 181 ++++++++++++++++++++++++++++++++++++-
 4 files changed, 195 insertions(+), 8 deletions(-)


base-commit: fdca4b030c78fa2773a94fda3a6dc3ce5558a727
-- 
2.53.0

[PATCH net v6 1/3] net: virtio: fix UDP tunnel GSO header of VLAN tagged packets

[Eric Dumazet]

virtio_net_hdr_tnl_from_skb() builds a wrong header for VLAN tagged UDP
tunnel GSO packets.

1) When the skb carries a hwaccel VLAN tag, tun_put_user() inserts the
   802.1Q header in the frame it copies to user space or vhost-net, and
   passes vlan_hlen = VLAN_HLEN. virtio_net_hdr_from_skb() adds
   vlan_hlen to csum_start, but inner_nh_offset and outer_th_offset do
   not account for it, and are 4 bytes short of the real inner L3 and
   outer UDP headers in the emitted frame.

   tun advertises NETIF_F_HW_VLAN_CTAG_TX and NETIF_F_HW_VLAN_STAG_TX
   in hw_enc_features, and its vlan_features keep the UDP tunnel GSO
   features, so this happens for example with a VLAN filtering bridge
   with a tagged port, or with a vlan device on top of a tap.

2) The outer family comes from skb->protocol, which is ETH_P_8021Q or
   ETH_P_8021AD when the VLAN tag is in the packet data, so a packet
   with an outer IPv6 header is reported as
   VIRTIO_NET_HDR_GSO_UDP_TUNNEL_IPV4. This happens when
   __vlan_hwaccel_push_inside() inserted the tag because the device does
   not offload it: virtio_net never offloads VLAN insertion but keeps
   UDP tunnel GSO in its vlan_features, and tun does the same with VLAN
   TX offload disabled, or with QinQ (outer tag offloaded, inner tag in
   the data).

The receiver sets inner_network_header and transport_header from these
offsets, and checks them with the wrong outer family. A forwarded
packet that gets segmented again uses the wrong inner headers, and a
receiver checking the outer headers against the virtio header drops
the packet.

Add vlan_hlen to both offsets like csum_start, and take the outer
family from vlan_get_protocol(), which skips VLAN tags in the data.

Fixes: a2fb4bc4e2a6 ("net: implement virtio helpers to handle UDP GSO tunneling.")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/netdev/179421814766.994134.3669228181251158303@kernel.org/
Closes: https://lore.kernel.org/netdev/179433950084.994134.15100186606737698605@kernel.org/
Signed-off-by: Eric Dumazet <edumazet@kernel.org>
---
 include/linux/virtio_net.h | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
index d6466f96cdd0..83f801ff5700 100644
--- a/include/linux/virtio_net.h
+++ b/include/linux/virtio_net.h
@@ -453,7 +453,10 @@ virtio_net_hdr_tnl_from_skb(const struct sk_buff *skb,
 	if (feature_hdrlen && hdr->hdr_len)
 		__virtio_net_set_tnl_hdrlen(skb, hdr);
 
-	if (skb->protocol == htons(ETH_P_IPV6))
+	/* skb->protocol is the VLAN protocol when the tag is in the
+	 * packet data.
+	 */
+	if (vlan_get_protocol(skb) == htons(ETH_P_IPV6))
 		hdr->gso_type |= VIRTIO_NET_HDR_GSO_UDP_TUNNEL_IPV6;
 	else
 		hdr->gso_type |= VIRTIO_NET_HDR_GSO_UDP_TUNNEL_IPV4;
@@ -461,8 +464,11 @@ virtio_net_hdr_tnl_from_skb(const struct sk_buff *skb,
 	if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_TUNNEL_CSUM)
 		hdr->flags |= VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM;
 
-	inner_nh = skb->inner_network_header - skb_headroom(skb);
-	outer_th = skb->transport_header - skb_headroom(skb);
+	/* Like csum_start, the offsets must account for a VLAN tag
+	 * the caller inserts in the frame.
+	 */
+	inner_nh = skb->inner_network_header - skb_headroom(skb) + vlan_hlen;
+	outer_th = skb->transport_header - skb_headroom(skb) + vlan_hlen;
 	vhdr->inner_nh_offset = cpu_to_le16(inner_nh);
 	vhdr->outer_th_offset = cpu_to_le16(outer_th);
 	return 0;
-- 
2.53.0

[PATCH net v6 2/3] tun: clear stale skb->protocol of NAPI frags skbs

[Eric Dumazet]

With IFF_NAPI_FRAGS, tun_napi_alloc_frags() gets its skb from
napi_get_frags(). If GRO merged the previous frame with
GRO_MERGED_FREE, that skb was recycled by napi_reuse_skb(), which does
not reset skb->protocol set by napi_frags_skb() for the previous frame.

tun_get_user() calls tun_vnet_hdr_tnl_to_skb() before eth_type_trans(),
so the virtio header checks see the protocol of the previous frame:
GSO packets without VIRTIO_NET_HDR_F_NEEDS_CSUM are dissected with the
wrong protocol. A following change checks the outer protocol of UDP
tunnel GSO packets, which would then reject a valid frame when the two
ethertypes differ.

This needs GRO to hold packets across writes, and IFF_NAPI_FRAGS needs
CAP_NET_ADMIN.

Clear skb->protocol in tun_napi_alloc_frags(), like skbs allocated
by tun_alloc_skb() or build_skb(). eth_type_trans() sets it later.

Fixes: 90e33d459407 ("tun: enable napi_gro_frags() for TUN/TAP driver")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/netdev/179386753589.488856.11455181721004759454@kernel.org/
Suggested-by: Willem de Bruijn <willemb@google.com>
Reviewed-by: Willem de Bruijn <willemb@google.com>
Signed-off-by: Eric Dumazet <edumazet@kernel.org>
---
 drivers/net/tun.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 3b292bce4438..d1fcb25ab026 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1500,6 +1500,12 @@ static struct sk_buff *tun_napi_alloc_frags(struct tun_file *tfile,
 	if (!skb)
 		return ERR_PTR(-ENOMEM);
 
+	/* An skb recycled by napi_reuse_skb() keeps the protocol of a
+	 * previous frame. eth_type_trans() sets it after the virtio
+	 * header checks, which must not see a stale one.
+	 */
+	skb->protocol = 0;
+
 	linear = iov_iter_single_seg_count(it);
 	err = __skb_grow(skb, linear);
 	if (err)
-- 
2.53.0

Re: [PATCH net v6 2/3] tun: clear stale skb->protocol of NAPI frags skbs

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message understates what the bug does in the tree as
  it stands.

--

tun: clear stale skb->protocol of NAPI frags skbs

This commit clears skb->protocol in tun_napi_alloc_frags(). After a
GRO_MERGED_FREE merge, the skb recycled by napi_reuse_skb() still holds
the previous frame's ethertype. The virtio header checks in tun_get_user()
run before eth_type_trans(), so without this change they see that stale
value.

Closes: https://lore.kernel.org/netdev/179386753589.488856.11455181721004759454@kernel.org/

> tun_get_user() calls tun_vnet_hdr_tnl_to_skb() before eth_type_trans(),
> so the virtio header checks see the protocol of the previous frame:
> GSO packets without VIRTIO_NET_HDR_F_NEEDS_CSUM are dissected with the
> wrong protocol. A following change checks the outer protocol of UDP
> tunnel GSO packets, which would then reject a valid frame when the two
> ethertypes differ.

[Severity: Low]
This isn't a bug, but could the commit message say more about the impact
in the current tree? As written, the visible failure seems to depend on
the follow-up change. The stale protocol looks like it already causes
problems without that change.

In __virtio_net_hdr_to_skb(), the GSO branch without NEEDS_CSUM only
parses the real ethertype when skb->protocol is zero:

include/linux/virtio_net.h:__virtio_net_hdr_to_skb() {
    ...
	} else if (gso_type) {
		...
		if (!skb->protocol) {
			skb->protocol = dev_parse_header_protocol(skb);
			if (!skb->protocol)
				virtio_net_hdr_set_proto(skb, hdr);
		}
retry:
		if (!skb_flow_dissect_flow_keys_basic(NULL, skb, &keys,
    ...
}

When napi_frags_skb() has left a non-zero value from the previous frame,
the dissector starts at the network offset and uses the previous frame's
ethertype.

After that, the only check is virtio_net_hdr_match_proto() on the
dissected n_proto. Nothing compares it with the frame's real ethertype.

Two outcomes seem possible today:

  - A valid frame is rejected with -EINVAL. For example, a TCPV4 GSO
    frame arrives after an IPv6 frame was merged. The IPv4 header is then
    parsed as IPv6, and either ip_proto or n_proto fails to match.

  - A GSO frame whose real ethertype does not match its GSO type is
    accepted, because the stale ethertype makes the payload parse as the
    expected protocol. eth_type_trans() then sets an skb->protocol that
    disagrees with gso_type.

Could these effects be mentioned in the message? That would show stable
backporters that the fix matters on its own, not only once the follow-up
change lands.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261110213628.3476051-1-edumazet%40kernel.org

[PATCH net v6 3/3] net: virtio: validate outer protocol of UDP tunnel GSO packets

[Eric Dumazet]

virtio_net_hdr_tnl_to_skb() trusts the virtio header to describe a
UDP tunnel over IPv4 or IPv6, but never checks that the frame actually
carries such outer headers. Because VIRTIO_NET_HDR_F_NEEDS_CSUM is
mandatory for tunnel GSO, __virtio_net_hdr_to_skb() skips the flow
dissector, so virtio_net_hdr_match_proto() is never applied either.

A guest (or a tap writer) can then inject a frame with an arbitrary
outer ethertype, for example ETH_P_NSH, with SKB_GSO_UDP_TUNNEL set.
The GSO stack then runs nsh_gso_segment() on top of the UDP tunnel
segmentation, which assumes a different header layout. The result is
segments with skb->data before skb->head, and a host crash in
skb_under_panic() as soon as a later layer pushes a header:

  skbuff: skb_under_panic: ... put:4 ... dev:gretap1
  kernel BUG at net/core/skbuff.c:215!
   skb_push
   __gre_xmit
   gre_tap_xmit
   ...
   tun_get_user

Validate the outer headers with the parsing helpers from Michael's
series "net: resolve uncommon partial checksums at ingress":

- locate the L3 header: Ethernet with optional VLAN tags, or the
  network header set by the caller. skb->protocol is not trusted when
  it disagrees with the frame.
- require the outer L3 protocol to match the declared tunnel type.
- require the outer UDP header exactly at outer_th_offset, after IPv4
  options or IPv6 extension headers, and reject fragments.

Unlike the original helpers:

- the callers tell whether the frame starts with an Ethernet header,
  with a new has_eth argument of virtio_net_hdr_tnl_to_skb(), instead
  of using the device type. tun frames packets from its IFF_TAP and
  IFF_TUN flags, and TUNSETLINK can set the device type to any value.
  virtio_net and tun_xdp_one() always pass true, tun_get_user() passes
  whether the device is IFF_TAP.
- ipv6_find_hdr() stops at an AH header, as Willem suggested in the
  review of that series.

Fixes: a2fb4bc4e2a6 ("net: implement virtio helpers to handle UDP GSO tunneling.")
Reported-by: Hassan Sultan <hassansultan@google.com>
Suggested-by: Willem de Bruijn <willemb@google.com>
Link: https://lore.kernel.org/netdev/93d0e37c99711bebdf60c120af03010455c2f2e2.1793276116.git.mst@redhat.com/
Cc: Willem de Bruijn <willemb@google.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Co-developed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Willem de Bruijn <willemb@google.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Eric Dumazet <edumazet@kernel.org>
---
 drivers/net/tun.c          |   9 +-
 drivers/net/tun_vnet.h     |   5 +-
 drivers/net/virtio_net.c   |   2 +-
 include/linux/virtio_net.h | 169 +++++++++++++++++++++++++++++++++++++
 4 files changed, 180 insertions(+), 5 deletions(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index d1fcb25ab026..ced5a1596e60 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1950,7 +1950,8 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 		break;
 	}
 
-	if (tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, &hdr)) {
+	if (tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, &hdr,
+				    (tun->flags & TUN_TYPE_MASK) == IFF_TAP)) {
 		atomic_long_inc(&tun->rx_frame_errors);
 		err = -EINVAL;
 		goto free_skb;
@@ -2627,7 +2628,11 @@ static int tun_xdp_one(struct tun_struct *tun,
 	tnl_hdr = (struct virtio_net_hdr_v1_hash_tunnel *)gso;
 	skb->dev = tun->dev;
 	skb_set_network_header(skb, ETH_HLEN);
-	if (tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, tnl_hdr)) {
+	/* Frames always start with an Ethernet header here, as the
+	 * eth_type_trans() call below expects.
+	 */
+	if (tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, tnl_hdr,
+				    true)) {
 		atomic_long_inc(&tun->rx_frame_errors);
 		kfree_skb(skb);
 		ret = -EINVAL;
diff --git a/drivers/net/tun_vnet.h b/drivers/net/tun_vnet.h
index f4c652b1fa44..3364e08dd6e0 100644
--- a/drivers/net/tun_vnet.h
+++ b/drivers/net/tun_vnet.h
@@ -197,9 +197,10 @@ static inline netdev_features_t tun_vnet_hdr_guest_features(int vnet_hdr_sz)
 static inline int
 tun_vnet_hdr_tnl_to_skb(unsigned int flags, netdev_features_t features,
 			struct sk_buff *skb,
-			const struct virtio_net_hdr_v1_hash_tunnel *hdr)
+			const struct virtio_net_hdr_v1_hash_tunnel *hdr,
+			bool has_eth)
 {
-	return virtio_net_hdr_tnl_to_skb(skb, hdr,
+	return virtio_net_hdr_tnl_to_skb(skb, hdr, has_eth,
 				features & NETIF_F_GSO_UDP_TUNNEL,
 				features & NETIF_F_GSO_UDP_TUNNEL_CSUM,
 				tun_vnet_is_little_endian(flags));
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index dd2c41ef6fbe..ed20e76fbc4c 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2533,7 +2533,7 @@ static void virtnet_receive_done(struct virtnet_info *vi, struct receive_queue *
 
 	skb->dev = dev;
 	skb_set_network_header(skb, ETH_HLEN);
-	if (virtio_net_hdr_tnl_to_skb(skb, &hdr->tnl_hdr, vi->rx_tnl,
+	if (virtio_net_hdr_tnl_to_skb(skb, &hdr->tnl_hdr, true, vi->rx_tnl,
 				      vi->rx_tnl_csum,
 				      virtio_is_little_endian(vi->vdev))) {
 		net_warn_ratelimited("%s: bad gso: type: %x, size: %u, flags %x tunnel %d tnl csum %d\n",
diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
index 83f801ff5700..fc9667176f23 100644
--- a/include/linux/virtio_net.h
+++ b/include/linux/virtio_net.h
@@ -5,7 +5,10 @@
 #include <linux/if_vlan.h>
 #include <linux/ip.h>
 #include <linux/ipv6.h>
+#include <linux/netdevice.h>
 #include <linux/udp.h>
+#include <net/ip.h>
+#include <net/ipv6.h>
 #include <net/tcp.h>
 #include <uapi/linux/tcp.h>
 #include <uapi/linux/virtio_net.h>
@@ -48,6 +51,145 @@ static inline int virtio_net_hdr_set_proto(struct sk_buff *skb,
 	return 0;
 }
 
+/* TODO: nothing virtio specific here, move to ip.h */
+
+/*
+ * Pull and minimally validate the IPv4 header at @nhoff.
+ * Returns the header and stores the transport header offset in
+ * @thoff, or NULL if truncated or malformed.  The whole header,
+ * options included, is guaranteed linear on return.
+ */
+static inline struct iphdr *virtio_net_ip4_parse(struct sk_buff *skb,
+						  unsigned int nhoff,
+						  unsigned int *thoff)
+{
+	unsigned int ihl;
+	struct iphdr *iph;
+
+	if (!pskb_may_pull(skb, nhoff + sizeof(struct iphdr)))
+		return NULL;
+
+	iph = (struct iphdr *)(skb->data + nhoff);
+	if (iph->version != 4 ||
+	    iph->ihl < sizeof(struct iphdr) / sizeof(u32))
+		return NULL;
+
+	ihl = iph->ihl * sizeof(u32);
+	*thoff = nhoff + ihl;
+	/* Pull the options as well: the transport header offset
+	 * returned above must be within the linear data.  iph goes
+	 * stale across the pull.
+	 */
+	if (!pskb_may_pull(skb, nhoff + ihl))
+		return NULL;
+
+	return (struct iphdr *)(skb->data + nhoff);
+}
+
+/* Where does the L3 header start, and what protocol is it?
+ * With @has_eth, the frame starts with an Ethernet header, possibly
+ * followed by VLAN tags. Otherwise the L3 header starts at the network
+ * header offset the caller set up.
+ *
+ * The caller knows how the frame will be parsed later: the device
+ * type does not tell, as tun can change it with TUNSETLINK.
+ */
+static inline bool
+virtio_net_l3_start(struct sk_buff *skb, bool has_eth, int *nhoff,
+		    __be16 *proto)
+{
+	if (has_eth) {
+		__be16 h_proto;
+
+		if (!pskb_may_pull(skb, ETH_HLEN))
+			return false;
+		h_proto = eth_hdr(skb)->h_proto;
+		/* skb->protocol is a sender claim, and parts of the
+		 * stack parse the packet per it: when it disagrees with
+		 * the frame we do not know which view the packet will
+		 * be parsed as, so do not validate.
+		 */
+		if (skb->protocol && skb->protocol != h_proto)
+			return false;
+		*nhoff = ETH_HLEN;
+		*proto = h_proto;
+		if (eth_type_vlan(*proto)) {
+			*proto = __vlan_get_protocol(skb, *proto, nhoff);
+			if (!*proto)
+				return false;
+		}
+	} else {
+		const struct iphdr *iph;
+
+		*nhoff = skb_network_offset(skb);
+		/* An unset network header shows up as an offset
+		 * outside of the packet.
+		 */
+		if (*nhoff < 0 || (unsigned int)*nhoff >= skb->len)
+			return false;
+		/* No L2 header to cross-check with: take the protocol
+		 * from the IP version at the network header.
+		 */
+		if (!pskb_may_pull(skb, *nhoff + 1))
+			return false;
+		iph = (const struct iphdr *)(skb->data + *nhoff);
+		if (iph->version == 4)
+			*proto = htons(ETH_P_IP);
+		else if (iph->version == 6)
+			*proto = htons(ETH_P_IPV6);
+		else
+			return false;
+		if (skb->protocol && skb->protocol != *proto)
+			return false;
+	}
+	return true;
+}
+
+/* Walk the L3 header at @nhoff: where does its transport header
+ * start, and which protocol is it?  Returns false if the header
+ * is truncated, malformed or fragmented.
+ */
+static inline bool
+virtio_net_l4_start(struct sk_buff *skb, unsigned int nhoff, __be16 proto,
+		    unsigned int *thoff, int *ip_proto)
+{
+	if (proto == htons(ETH_P_IP)) {
+		const struct iphdr *iph = virtio_net_ip4_parse(skb, nhoff, thoff);
+
+		if (!iph || ip_is_fragment(iph))
+			return false;
+		*ip_proto = iph->protocol;
+		return true;
+	}
+
+	if (proto == htons(ETH_P_IPV6)) {
+		const struct ipv6hdr *iph;
+		int flags = IP6_FH_F_AUTH;
+
+		if (!pskb_may_pull(skb, nhoff + sizeof(*iph)))
+			return false;
+		iph = (const void *)(skb->data + nhoff);
+		if (iph->version != 6)
+			return false;
+		*thoff = nhoff + sizeof(*iph);
+		*ip_proto = iph->nexthdr;
+		if (*ip_proto != IPPROTO_TCP && *ip_proto != IPPROTO_UDP) {
+			*thoff = nhoff;
+			/* Find the transport header behind any
+			 * extension headers. IP6_FH_F_AUTH stops
+			 * the walk at AH, so frames with AH are rejected.
+			 */
+			*ip_proto = ipv6_find_hdr(skb, thoff, -1, NULL,
+						  &flags);
+			if (*ip_proto < 0 || flags & IP6_FH_F_FRAG)
+				return false;
+		}
+		return true;
+	}
+
+	return false;
+}
+
 static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,
 					  const struct virtio_net_hdr *hdr,
 					  bool little_endian, u8 hdr_gso_type)
@@ -282,9 +424,33 @@ static inline unsigned int virtio_l3min(bool is_ipv6)
 	return is_ipv6 ? sizeof(struct ipv6hdr) : sizeof(struct iphdr);
 }
 
+/* Do the tunnel offsets match the outer headers in the frame?
+ * The L3 header the tunnel claims must be the one the frame
+ * carries, with UDP following it at @outer_th.
+ */
+static inline bool
+virtio_net_tnl_offsets_ok(struct sk_buff *skb, bool has_eth, bool outer_isv6,
+			  unsigned int outer_th)
+{
+	int nhoff;
+	__be16 proto;
+	unsigned int thoff;
+	int ip_proto;
+
+	if (!virtio_net_l3_start(skb, has_eth, &nhoff, &proto))
+		return false;
+	if (outer_isv6 != (proto == htons(ETH_P_IPV6)))
+		return false;
+	if (!virtio_net_l4_start(skb, nhoff, proto, &thoff, &ip_proto))
+		return false;
+
+	return ip_proto == IPPROTO_UDP && outer_th == thoff;
+}
+
 static inline int
 virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
 			  const struct virtio_net_hdr_v1_hash_tunnel *vhdr,
+			  bool has_eth,
 			  bool tnl_hdr_negotiated,
 			  bool tnl_csum_negotiated,
 			  bool little_endian)
@@ -338,6 +504,9 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
 	if (ret)
 		return ret;
 
+	if (!virtio_net_tnl_offsets_ok(skb, has_eth, outer_isv6, outer_th))
+		return -EINVAL;
+
 	/* In case of USO, the inner protocol is still unknown and
 	 * `inner_isv6` is just a guess, additional parsing is needed.
 	 * The previous validation ensures that accessing an ipv4 inner
-- 
2.53.0

Re: [PATCH net v6 3/3] net: virtio: validate outer protocol of UDP tunnel GSO packets

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 (1) · Medium: 1 · 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).

- [Medium] The new has_eth=false path in virtio_net_hdr_tnl_to_skb()
  (include/linux/virtio_net.h) rejects almost every valid frame, so it…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it is a
  sibling of the injection the patch closes.

--

net: virtio: validate outer protocol of UDP tunnel GSO packets

This patch makes virtio_net_hdr_tnl_to_skb() check that a UDP tunnel GSO
frame really carries the outer IPv4 or IPv6 header it claims, with UDP at
outer_th_offset. Callers now pass a has_eth argument that says whether the
frame starts with an Ethernet header. tun_get_user() passes false for
IFF_TUN devices.

Link: https://lore.kernel.org/netdev/93d0e37c99711bebdf60c120af03010455c2f2e2.1793276116.git.mst@redhat.com/

> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index d1fcb25ab026b..ced5a1596e609 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -1950,7 +1950,8 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
>  		break;
>  	}
>  
> -	if (tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, &hdr)) {
> +	if (tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, &hdr,
> +				    (tun->flags & TUN_TYPE_MASK) == IFF_TAP)) {
>  		atomic_long_inc(&tun->rx_frame_errors);
>  		err = -EINVAL;
>  		goto free_skb;

[ ... ]

> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> index 83f801ff57006..fc9667176f23c 100644
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h

[ ... ]

> @@ -338,6 +504,9 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
>  	if (ret)
>  		return ret;
>  
> +	if (!virtio_net_tnl_offsets_ok(skb, has_eth, outer_isv6, outer_th))
> +		return -EINVAL;
> +

[Severity: Medium]
Can the has_eth == false path be reached by ordinary IFF_TUN frames?

On IFF_TUN, tun_get_user() resets the network header to offset 0 and now
passes has_eth == false. virtio_net_l3_start() then takes the L3 header
from skb_network_offset(), so virtio_net_tnl_offsets_ok() needs outer_th
to be 20 for a plain IPv4 outer header, or 40 for plain IPv6.

Earlier in virtio_net_hdr_tnl_to_skb(), the offset floor still assumes a
14-byte Ethernet header, whatever has_eth says:

	outer_l3min = ETH_HLEN + virtio_l3min(outer_isv6);
	...
	if (outer_th < outer_l3min ||
	    inner_nh < outer_th + sizeof(struct udphdr) ||
	    inner_th < inner_nh + inner_l3min)
		return -EINVAL;

So an outer_th of 20 or 40 fails against 34 or 54 before the new check
runs. The only IFF_TUN frames that can pass both checks have IPv4 options
(ihl >= 9) or at least 14 bytes of IPv6 extension headers.

On the TX side, virtio_net_hdr_tnl_from_skb() computes outer_th relative
to skb->data:

	outer_th = skb->transport_header - skb_headroom(skb) + vlan_hlen;

On IFF_TUN, skb->data points at the IP header, so this emits 20 or 40.
set_offload() also enables NETIF_F_GSO_UDP_TUNNEL without checking the
device type. As a result, IFF_TUN devices advertise tunnel GSO, but their
ordinary tunnel GSO frames get -EINVAL and increment rx_frame_errors.

The ETH_HLEN floor predates this patch and comes from a2fb4bc4e2a6.
However, the commit message relies on the has_eth == false path:

  "tun_get_user() passes whether the device is IFF_TAP."

Should outer_l3min follow has_eth, for example:

	outer_l3min = (has_eth ? ETH_HLEN : 0) + virtio_l3min(outer_isv6);

[Severity: High]
This is a pre-existing issue, not one introduced by this patch, but it is
closely related to the injection fixed here. Plain (non-tunnel) GSO
frames return early and never reach the new check:

	gso_tunnel_type = hdr->gso_type & VIRTIO_NET_HDR_GSO_UDP_TUNNEL;
	if (!gso_tunnel_type)
		return virtio_net_hdr_to_skb(skb, hdr, little_endian);

In __virtio_net_hdr_to_skb(), the VIRTIO_NET_HDR_F_NEEDS_CSUM branch only
checks pskb_may_pull(), skb_partial_csum_set() and the transport offset
against nh_min_len. virtio_net_hdr_match_proto() and the flow dissector
run only in the branch without NEEDS_CSUM.

So can a tap writer, or a guest via vhost-net, still send an Ethernet
frame with ethertype ETH_P_NSH, gso_type TCPV4, TCPV6 or UDP_L4, and
NEEDS_CSUM set? eth_type_trans() would then set skb->protocol to
ETH_P_NSH.

Suppose such a frame is forwarded, by a bridge or tc mirred, to a device
that segments in software. Could the segmentation then recurse without
limit?

skb_mac_gso_segment()
  nsh_gso_segment()
    proto = tun_p_to_eth_p(nsh_hdr(skb)->np);   /* TUN_P_NSH -> ETH_P_NSH */
    __skb_pull(skb, nsh_len);
    skb->mac_len = proto == htons(ETH_P_TEB) ? ETH_HLEN : 0;
    skb_mac_gso_segment()
      nsh_gso_segment()
        ...

Neither nsh_gso_segment() nor skb_mac_gso_segment() limits the depth.
Commit 006c8b834aed ("net: gso: limit recursive IP-in-IP segmentation")
added the SKB_GSO_CB(skb)->recursion_counter check in
gso_recursion_inc_test(). It is called only from inet_gso_segment() and
ipv6_gso_segment().

A frame of about 64KB made of stacked 8-byte NSH headers could recurse
thousands of levels deep and overflow the kernel stack.
TUN_P_ETHERNET -> ETH_P_TEB -> NSH appears to be a second route into the
same recursion.

Should nsh_gso_segment() use gso_recursion_inc_test()? Or should the
plain NEEDS_CSUM path also validate the outer protocol?

>  	/* In case of USO, the inner protocol is still unknown and
>  	 * `inner_isv6` is just a guess, additional parsing is needed.
>  	 * The previous validation ensures that accessing an ipv4 inner

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261110213628.3476051-1-edumazet%40kernel.org

Re: [PATCH net v6 3/3] net: virtio: validate outer protocol of UDP tunnel GSO packets

[Eric Dumazet]

Le mer. 11 nov. 2026 à 23:41, <netdev-bot+sashiko@kernel.org> a écrit :
>
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
>
> Critical: 0 · High: 0 (1) · Medium: 1 · 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).
>
> - [Medium] The new has_eth=false path in virtio_net_hdr_tnl_to_skb()
>   (include/linux/virtio_net.h) rejects almost every valid frame, so it…
>
> Pre-existing issues:
> - [High] This problem wasn't introduced by this patch, but it is a
>   sibling of the injection the patch closes.
>
> --
>
> net: virtio: validate outer protocol of UDP tunnel GSO packets
>
> This patch makes virtio_net_hdr_tnl_to_skb() check that a UDP tunnel GSO
> frame really carries the outer IPv4 or IPv6 header it claims, with UDP at
> outer_th_offset. Callers now pass a has_eth argument that says whether the
> frame starts with an Ethernet header. tun_get_user() passes false for
> IFF_TUN devices.
>
> Link: https://lore.kernel.org/netdev/93d0e37c99711bebdf60c120af03010455c2f2e2.1793276116.git.mst@redhat.com/
>
> > diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> > index d1fcb25ab026b..ced5a1596e609 100644
> > --- a/drivers/net/tun.c
> > +++ b/drivers/net/tun.c
> > @@ -1950,7 +1950,8 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
> >               break;
> >       }
> >
> > -     if (tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, &hdr)) {
> > +     if (tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, &hdr,
> > +                                 (tun->flags & TUN_TYPE_MASK) == IFF_TAP)) {
> >               atomic_long_inc(&tun->rx_frame_errors);
> >               err = -EINVAL;
> >               goto free_skb;
>
> [ ... ]
>
> > diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> > index 83f801ff57006..fc9667176f23c 100644
> > --- a/include/linux/virtio_net.h
> > +++ b/include/linux/virtio_net.h
>
> [ ... ]
>
> > @@ -338,6 +504,9 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
> >       if (ret)
> >               return ret;
> >
> > +     if (!virtio_net_tnl_offsets_ok(skb, has_eth, outer_isv6, outer_th))
> > +             return -EINVAL;
> > +
>
> [Severity: Medium]
> Can the has_eth == false path be reached by ordinary IFF_TUN frames?
>
> On IFF_TUN, tun_get_user() resets the network header to offset 0 and now
> passes has_eth == false. virtio_net_l3_start() then takes the L3 header
> from skb_network_offset(), so virtio_net_tnl_offsets_ok() needs outer_th
> to be 20 for a plain IPv4 outer header, or 40 for plain IPv6.
>
> Earlier in virtio_net_hdr_tnl_to_skb(), the offset floor still assumes a
> 14-byte Ethernet header, whatever has_eth says:
>
>         outer_l3min = ETH_HLEN + virtio_l3min(outer_isv6);
>         ...
>         if (outer_th < outer_l3min ||
>             inner_nh < outer_th + sizeof(struct udphdr) ||
>             inner_th < inner_nh + inner_l3min)
>                 return -EINVAL;
>
> So an outer_th of 20 or 40 fails against 34 or 54 before the new check
> runs. The only IFF_TUN frames that can pass both checks have IPv4 options
> (ihl >= 9) or at least 14 bytes of IPv6 extension headers.
>
> On the TX side, virtio_net_hdr_tnl_from_skb() computes outer_th relative
> to skb->data:
>
>         outer_th = skb->transport_header - skb_headroom(skb) + vlan_hlen;
>
> On IFF_TUN, skb->data points at the IP header, so this emits 20 or 40.
> set_offload() also enables NETIF_F_GSO_UDP_TUNNEL without checking the
> device type. As a result, IFF_TUN devices advertise tunnel GSO, but their
> ordinary tunnel GSO frames get -EINVAL and increment rx_frame_errors.
>
> The ETH_HLEN floor predates this patch and comes from a2fb4bc4e2a6.
> However, the commit message relies on the has_eth == false path:
>
>   "tun_get_user() passes whether the device is IFF_TAP."
>
> Should outer_l3min follow has_eth, for example:
>
>         outer_l3min = (has_eth ? ETH_HLEN : 0) + virtio_l3min(outer_isv6);

Same answer as for v4 and v5: UDP tunnel GSO over IFF_TUN has never
worked since a2fb4bc4e2a6, and this series does not change that.
has_eth only makes sure IFF_TUN frames are not parsed as Ethernet.

Relaxing the floor enables a path that has never run. I prefer to do
that in a separate patch, with selftests covering IFF_TUN in both
directions, and not in this painful series.

>
> [Severity: High]
> This is a pre-existing issue, not one introduced by this patch, but it is
> closely related to the injection fixed here. Plain (non-tunnel) GSO
> frames return early and never reach the new check:
>
>         gso_tunnel_type = hdr->gso_type & VIRTIO_NET_HDR_GSO_UDP_TUNNEL;
>         if (!gso_tunnel_type)
>                 return virtio_net_hdr_to_skb(skb, hdr, little_endian);
>
> In __virtio_net_hdr_to_skb(), the VIRTIO_NET_HDR_F_NEEDS_CSUM branch only
> checks pskb_may_pull(), skb_partial_csum_set() and the transport offset
> against nh_min_len. virtio_net_hdr_match_proto() and the flow dissector
> run only in the branch without NEEDS_CSUM.
>
> So can a tap writer, or a guest via vhost-net, still send an Ethernet
> frame with ethertype ETH_P_NSH, gso_type TCPV4, TCPV6 or UDP_L4, and
> NEEDS_CSUM set? eth_type_trans() would then set skb->protocol to
> ETH_P_NSH.
>
> Suppose such a frame is forwarded, by a bridge or tc mirred, to a device
> that segments in software. Could the segmentation then recurse without
> limit?
>
> skb_mac_gso_segment()
>   nsh_gso_segment()
>     proto = tun_p_to_eth_p(nsh_hdr(skb)->np);   /* TUN_P_NSH -> ETH_P_NSH */
>     __skb_pull(skb, nsh_len);
>     skb->mac_len = proto == htons(ETH_P_TEB) ? ETH_HLEN : 0;
>     skb_mac_gso_segment()
>       nsh_gso_segment()
>         ...
>
> Neither nsh_gso_segment() nor skb_mac_gso_segment() limits the depth.
> Commit 006c8b834aed ("net: gso: limit recursive IP-in-IP segmentation")
> added the SKB_GSO_CB(skb)->recursion_counter check in
> gso_recursion_inc_test(). It is called only from inet_gso_segment() and
> ipv6_gso_segment().
>
> A frame of about 64KB made of stacked 8-byte NSH headers could recurse
> thousands of levels deep and overflow the kernel stack.
> TUN_P_ETHERNET -> ETH_P_TEB -> NSH appears to be a second route into the
> same recursion.
>
> Should nsh_gso_segment() use gso_recursion_inc_test()? Or should the
> plain NEEDS_CSUM path also validate the outer protocol?


This one looks real, and is independent of this series:
nsh_gso_segment() can call itself through skb_mac_gso_segment() with
no depth limit, whatever the way the packet entered the stack.

I will send a separate fix for nsh_gso_segment(), with a selftest.

The remark on the changelog of patch 2 is noted, I do not plan to
respin for it.

Let's merge this series and move on.