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

[PATCH net 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 an outer IPv4/IPv6 header 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 adds the missing check. Ethernet frames can carry VLAN tags
before the outer IP header; for IFF_TUN, skb->protocol is provided
by the caller.

Patch 2 fixes a selftest helper computing UDP checksums over a length
in network byte order.

Patch 3 adds tun selftests: valid outer IPv4 frames (plain and VLAN
tagged) are accepted, mismatched or non-IP outer headers are rejected.

Eric Dumazet (3):
  net: virtio: validate outer protocol of UDP tunnel GSO packets
  selftests: net: tuntap: fix UDP checksum length in
    build_udp_packet_csum()
  selftests: net: tun: test outer protocol of UDP tunnel GSO packets

 include/linux/virtio_net.h                   |  28 ++++
 tools/testing/selftests/net/tun.c            | 130 +++++++++++++++++++
 tools/testing/selftests/net/tuntap_helpers.h |   2 +-
 3 files changed, 159 insertions(+), 1 deletion(-)


base-commit: 647b81ddefc6364290e67bf33c89d54971f8995e
-- 
2.53.0

[PATCH net 1/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 an outer header. 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

Require the outer L3 protocol to match the declared tunnel type:
for Ethernet frames, look at the ethertype after optional VLAN tags,
and check that the outer network header fits before outer_th_offset.
For IFF_TUN, skb->protocol is already set by the caller.

Fixes: a2fb4bc4e2a6 ("net: implement virtio helpers to handle UDP GSO tunneling.")
Reported-by: Hassan Sultan <hassansultan@google.com>
Signed-off-by: Eric Dumazet <edumazet@kernel.org>
Cc: Willem de Bruijn <willemb@google.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
---
 include/linux/virtio_net.h | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
index d6466f96cdd0..d3f97d71d81c 100644
--- a/include/linux/virtio_net.h
+++ b/include/linux/virtio_net.h
@@ -282,6 +282,31 @@ static inline unsigned int virtio_l3min(bool is_ipv6)
 	return is_ipv6 ? sizeof(struct ipv6hdr) : sizeof(struct iphdr);
 }
 
+/* The outer network header of a UDP tunnel GSO packet must match the
+ * tunnel type declared in the virtio header, otherwise the GSO stack
+ * would parse the packet as something else (e.g. NSH).
+ */
+static inline bool virtio_net_tnl_outer_proto_ok(const struct sk_buff *skb,
+						 bool outer_isv6,
+						 unsigned int outer_th)
+{
+	__be16 expected = outer_isv6 ? htons(ETH_P_IPV6) : htons(ETH_P_IP);
+	__be16 proto = skb->protocol;
+	int nh_off = ETH_HLEN;
+
+	/* L3 device (IFF_TUN): the caller already set the protocol. */
+	if (proto)
+		return proto == expected;
+
+	/* Ethernet frame, possibly VLAN tagged. */
+	proto = eth_hdr(skb)->h_proto;
+	if (eth_type_vlan(proto))
+		proto = __vlan_get_protocol(skb, proto, &nh_off);
+
+	return proto == expected &&
+	       nh_off + virtio_l3min(outer_isv6) <= outer_th;
+}
+
 static inline int
 virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
 			  const struct virtio_net_hdr_v1_hash_tunnel *vhdr,
@@ -338,6 +363,9 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
 	if (ret)
 		return ret;
 
+	if (!virtio_net_tnl_outer_proto_ok(skb, 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

[PATCH net 2/3] selftests: net: tuntap: fix UDP checksum length in build_udp_packet_csum()

[Eric Dumazet]

build_udp_packet_csum() passes udph->len, which is in network byte
order, as the number of bytes to checksum. On little-endian hosts this
reads up to 64KB past the UDP header and computes a wrong checksum.

Existing callers did not crash only because they use packet buffers
of MAX_VNET_TUNNEL_PACKET_SZ bytes.

Use ntohs(udph->len), as tap.c does in its own build_udp_packet().

Fixes: a942fcd72e97 ("selftest: tun: Introduce tuntap_helpers.h header for TUN/TAP testing")
Signed-off-by: Eric Dumazet <edumazet@kernel.org>
---
 tools/testing/selftests/net/tuntap_helpers.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/tuntap_helpers.h b/tools/testing/selftests/net/tuntap_helpers.h
index d6c0437136ec..ac9b2da57a6d 100644
--- a/tools/testing/selftests/net/tuntap_helpers.h
+++ b/tools/testing/selftests/net/tuntap_helpers.h
@@ -335,7 +335,7 @@ static inline void build_udp_packet_csum(uint8_t *buf, int family,
 	sum += htons(IPPROTO_UDP) + udph->len;
 
 	if (!csum_off)
-		sum += add_csum(buf, udph->len);
+		sum += add_csum(buf, ntohs(udph->len));
 
 	udph->check = finish_ip_csum(sum);
 }
-- 
2.53.0

[PATCH net 3/3] selftests: net: tun: test outer protocol of UDP tunnel GSO packets

[Eric Dumazet]

Add a tun_vnet_udptnl_proto fixture writing single 4in4 Geneve GSO
frames to a tap device with UDP tunnel offloads enabled:

- outer_proto_ok: an outer IPv4 header, with or without an 802.1Q tag,
  is accepted.
- outer_proto_bad: an outer ethertype that does not match the declared
  tunnel type (IPv6 on an IPv4 tunnel, or NSH, with or without a VLAN
  tag) is rejected with EINVAL.

Without the previous virtio_net patch, outer_proto_bad fails because
the kernel accepts the frames.

Signed-off-by: Eric Dumazet <edumazet@kernel.org>
---
 tools/testing/selftests/net/tun.c | 130 ++++++++++++++++++++++++++++++
 1 file changed, 130 insertions(+)

diff --git a/tools/testing/selftests/net/tun.c b/tools/testing/selftests/net/tun.c
index 34de2580e32e..47a24b9c477b 100644
--- a/tools/testing/selftests/net/tun.c
+++ b/tools/testing/selftests/net/tun.c
@@ -683,6 +683,136 @@ TEST_F(tun_vnet_gso, vlan_tcpv4_gso_no_csum_64k)
 	ASSERT_EQ(ret, len);
 }
 
+#ifndef ETH_P_NSH
+#define ETH_P_NSH 0x894F
+#endif
+
+FIXTURE(tun_vnet_udptnl_proto)
+{
+	char ifname[IFNAMSIZ];
+	uint8_t *pkt;
+	int fd;
+};
+
+FIXTURE_SETUP(tun_vnet_udptnl_proto)
+{
+	int flags = IFF_TAP | IFF_NO_PI | IFF_VNET_HDR;
+	int features = TUN_F_CSUM | TUN_F_UDP_TUNNEL_GSO |
+		       TUN_F_UDP_TUNNEL_GSO_CSUM | TUN_F_USO4 | TUN_F_USO6;
+
+	self->fd = -1;
+	if (unshare(CLONE_NEWNET))
+		SKIP(return, "Cannot create network namespace: %s",
+		     strerror(errno));
+
+	self->pkt = calloc(1, MAX_VNET_TUNNEL_PACKET_SZ);
+	ASSERT_NE(self->pkt, NULL);
+
+	memset(self->ifname, 0, sizeof(self->ifname));
+	self->fd = tun_open(self->ifname, flags, TUN_VNET_TNL_SIZE, features,
+			    param_hwaddr_outer_src);
+	ASSERT_GE(self->fd, 0);
+}
+
+FIXTURE_TEARDOWN(tun_vnet_udptnl_proto)
+{
+	if (self->fd >= 0)
+		close(self->fd);
+	free(self->pkt);
+}
+
+/* Build a 4in4 Geneve GSO frame whose outer ethertype is @l3proto,
+ * optionally behind an 802.1Q tag.
+ */
+static int build_udptnl_frame(uint8_t *buf, bool vlan, uint16_t l3proto,
+			      int payload_len)
+{
+	struct virtio_net_hdr_v1_hash_tunnel *vh = (void *)buf;
+	int vlan_len = vlan ? 4 : 0;
+	int hlen = ETH_HLEN + vlan_len + UDP_TUNNEL_GENEVE_4IN4_HDRLEN;
+	uint8_t *outer_udph, *cur = buf;
+	int pktlen;
+
+	cur += build_virtio_net_hdr_v1_hash_tunnel(cur, true, hlen, 1400,
+						   AF_INET, AF_INET);
+	vh->outer_th_offset += vlan_len;
+	vh->inner_nh_offset += vlan_len;
+	vh->hash_hdr.hdr.csum_start += vlan_len;
+
+	if (vlan) {
+		uint16_t vlan_tag[2] = { htons(100), htons(l3proto) };
+
+		cur += build_eth(cur, ETH_P_8021Q, param_hwaddr_outer_dst,
+				 param_hwaddr_outer_src);
+		memcpy(cur, vlan_tag, sizeof(vlan_tag));
+		cur += sizeof(vlan_tag);
+	} else {
+		cur += build_eth(cur, l3proto, param_hwaddr_outer_dst,
+				 param_hwaddr_outer_src);
+	}
+
+	pktlen = UDP_TUNNEL_GENEVE_4IN4_HDRLEN - sizeof(struct iphdr) +
+		 payload_len;
+	cur += build_ipv4_header(cur, IPPROTO_UDP, pktlen,
+				 &param_ipaddr4_outer_dst,
+				 &param_ipaddr4_outer_src);
+
+	outer_udph = cur;
+	cur += build_udp_header(cur, UDP_SRC_PORT, VN_PORT,
+				pktlen - sizeof(struct udphdr));
+	cur += build_geneve_header(cur, VN_ID);
+	cur += build_eth(cur, ETH_P_IP, param_hwaddr_inner_dst,
+			 param_hwaddr_inner_src);
+	cur += build_ipv4_header(cur, IPPROTO_UDP,
+				 sizeof(struct udphdr) + payload_len,
+				 &param_ipaddr4_inner_dst,
+				 &param_ipaddr4_inner_src);
+	cur += build_udp_packet(cur, UDP_DST_PORT, UDP_SRC_PORT, payload_len,
+				AF_INET, false);
+	build_udp_packet_csum(outer_udph, AF_INET, false);
+
+	return cur - buf;
+}
+
+TEST_F(tun_vnet_udptnl_proto, outer_proto_ok)
+{
+	uint8_t *pkt = self->pkt;
+	int len, ret;
+
+	len = build_udptnl_frame(pkt, false, ETH_P_IP, 2800);
+	ret = write(self->fd, pkt, len);
+	ASSERT_EQ(ret, len);
+
+	/* The outer IPv4 header can follow a VLAN tag. */
+	len = build_udptnl_frame(pkt, true, ETH_P_IP, 2800);
+	ret = write(self->fd, pkt, len);
+	ASSERT_EQ(ret, len);
+}
+
+TEST_F(tun_vnet_udptnl_proto, outer_proto_bad)
+{
+	uint8_t *pkt = self->pkt;
+	int len, ret;
+
+	/* Outer ethertype does not match the declared IPv4 tunnel. */
+	len = build_udptnl_frame(pkt, false, ETH_P_IPV6, 2800);
+	ret = write(self->fd, pkt, len);
+	ASSERT_EQ(ret, -1);
+	ASSERT_EQ(errno, EINVAL);
+
+	/* Outer header is not IP at all. */
+	len = build_udptnl_frame(pkt, false, ETH_P_NSH, 2800);
+	ret = write(self->fd, pkt, len);
+	ASSERT_EQ(ret, -1);
+	ASSERT_EQ(errno, EINVAL);
+
+	/* Same, behind a VLAN tag. */
+	len = build_udptnl_frame(pkt, true, ETH_P_NSH, 2800);
+	ret = write(self->fd, pkt, len);
+	ASSERT_EQ(ret, -1);
+	ASSERT_EQ(errno, EINVAL);
+}
+
 FIXTURE(tun_vnet_udptnl)
 {
 	char ifname[IFNAMSIZ];
-- 
2.53.0

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

[Willem de Bruijn]

Eric Dumazet wrote:
> 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 an outer header. 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
> 
> Require the outer L3 protocol to match the declared tunnel type:
> for Ethernet frames, look at the ethertype after optional VLAN tags,
> and check that the outer network header fits before outer_th_offset.
> For IFF_TUN, skb->protocol is already set by the caller.
> 
> Fixes: a2fb4bc4e2a6 ("net: implement virtio helpers to handle UDP GSO tunneling.")
> Reported-by: Hassan Sultan <hassansultan@google.com>
> Signed-off-by: Eric Dumazet <edumazet@kernel.org>
> Cc: Willem de Bruijn <willemb@google.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>

Thanks for the fix. It overlaps with

commit "net: resolve uncommon partial checksums at ingress"
https://lore.kernel.org/netdev/93d0e37c99711bebdf60c120af03010455c2f2e2.1793276116.git.mst@redhat.com/

There are a bunch of subtle issues here. That series is up to v7.

Two specific issues are that
- skb->protocol in general cannot be trusted, and
- non-L3 sources cannot be assumed Ethernet

Would it make sense to take the relevant parts from the quoted commit,
which has hopefully resolved those edge cases, and send that on its own?
The rest of the series can come later.

> ---
>  include/linux/virtio_net.h | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> index d6466f96cdd0..d3f97d71d81c 100644
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h
> @@ -282,6 +282,31 @@ static inline unsigned int virtio_l3min(bool is_ipv6)
>  	return is_ipv6 ? sizeof(struct ipv6hdr) : sizeof(struct iphdr);
>  }
>  
> +/* The outer network header of a UDP tunnel GSO packet must match the
> + * tunnel type declared in the virtio header, otherwise the GSO stack
> + * would parse the packet as something else (e.g. NSH).
> + */
> +static inline bool virtio_net_tnl_outer_proto_ok(const struct sk_buff *skb,
> +						 bool outer_isv6,
> +						 unsigned int outer_th)
> +{
> +	__be16 expected = outer_isv6 ? htons(ETH_P_IPV6) : htons(ETH_P_IP);
> +	__be16 proto = skb->protocol;
> +	int nh_off = ETH_HLEN;
> +
> +	/* L3 device (IFF_TUN): the caller already set the protocol. */
> +	if (proto)
> +		return proto == expected;
> +
> +	/* Ethernet frame, possibly VLAN tagged. */
> +	proto = eth_hdr(skb)->h_proto;
> +	if (eth_type_vlan(proto))
> +		proto = __vlan_get_protocol(skb, proto, &nh_off);
> +
> +	return proto == expected &&
> +	       nh_off + virtio_l3min(outer_isv6) <= outer_th;
> +}
> +
>  static inline int
>  virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
>  			  const struct virtio_net_hdr_v1_hash_tunnel *vhdr,
> @@ -338,6 +363,9 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
>  	if (ret)
>  		return ret;
>  
> +	if (!virtio_net_tnl_outer_proto_ok(skb, 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 2/3] selftests: net: tuntap: fix UDP checksum length in build_udp_packet_csum()

[Willem de Bruijn]

Eric Dumazet wrote:
> build_udp_packet_csum() passes udph->len, which is in network byte
> order, as the number of bytes to checksum. On little-endian hosts this
> reads up to 64KB past the UDP header and computes a wrong checksum.
> 
> Existing callers did not crash only because they use packet buffers
> of MAX_VNET_TUNNEL_PACKET_SZ bytes.
> 
> Use ntohs(udph->len), as tap.c does in its own build_udp_packet().
> 
> Fixes: a942fcd72e97 ("selftest: tun: Introduce tuntap_helpers.h header for TUN/TAP testing")
> Signed-off-by: Eric Dumazet <edumazet@kernel.org>

Reviewed-by: Willem de Bruijn <willemb@google.com>

> ---
>  tools/testing/selftests/net/tuntap_helpers.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/net/tuntap_helpers.h b/tools/testing/selftests/net/tuntap_helpers.h
> index d6c0437136ec..ac9b2da57a6d 100644
> --- a/tools/testing/selftests/net/tuntap_helpers.h
> +++ b/tools/testing/selftests/net/tuntap_helpers.h
> @@ -335,7 +335,7 @@ static inline void build_udp_packet_csum(uint8_t *buf, int family,
>  	sum += htons(IPPROTO_UDP) + udph->len;
>  
>  	if (!csum_off)
> -		sum += add_csum(buf, udph->len);
> +		sum += add_csum(buf, ntohs(udph->len));
>  
>  	udph->check = finish_ip_csum(sum);

If respinning: we probably should also do CSUM_MANGLED_0 conversion here.

And to zero udph->check first. All existing callers do. But the calloc in
patch 3/3 does not.

>  }
> -- 
> 2.53.0
> 

Re: [PATCH net 3/3] selftests: net: tun: test outer protocol of UDP tunnel GSO packets

[Willem de Bruijn]

Eric Dumazet wrote:
> Add a tun_vnet_udptnl_proto fixture writing single 4in4 Geneve GSO
> frames to a tap device with UDP tunnel offloads enabled:
> 
> - outer_proto_ok: an outer IPv4 header, with or without an 802.1Q tag,
>   is accepted.
> - outer_proto_bad: an outer ethertype that does not match the declared
>   tunnel type (IPv6 on an IPv4 tunnel, or NSH, with or without a VLAN
>   tag) is rejected with EINVAL.
> 
> Without the previous virtio_net patch, outer_proto_bad fails because
> the kernel accepts the frames.
> 
> Signed-off-by: Eric Dumazet <edumazet@kernel.org>
> ---
>  tools/testing/selftests/net/tun.c | 130 ++++++++++++++++++++++++++++++
>  1 file changed, 130 insertions(+)
> 
> diff --git a/tools/testing/selftests/net/tun.c b/tools/testing/selftests/net/tun.c
> index 34de2580e32e..47a24b9c477b 100644
> --- a/tools/testing/selftests/net/tun.c
> +++ b/tools/testing/selftests/net/tun.c
> @@ -683,6 +683,136 @@ TEST_F(tun_vnet_gso, vlan_tcpv4_gso_no_csum_64k)
>  	ASSERT_EQ(ret, len);
>  }
>  
> +#ifndef ETH_P_NSH
> +#define ETH_P_NSH 0x894F
> +#endif
> +
> +FIXTURE(tun_vnet_udptnl_proto)
> +{
> +	char ifname[IFNAMSIZ];
> +	uint8_t *pkt;
> +	int fd;
> +};
> +
> +FIXTURE_SETUP(tun_vnet_udptnl_proto)
> +{
> +	int flags = IFF_TAP | IFF_NO_PI | IFF_VNET_HDR;
> +	int features = TUN_F_CSUM | TUN_F_UDP_TUNNEL_GSO |
> +		       TUN_F_UDP_TUNNEL_GSO_CSUM | TUN_F_USO4 | TUN_F_USO6;
> +
> +	self->fd = -1;
> +	if (unshare(CLONE_NEWNET))
> +		SKIP(return, "Cannot create network namespace: %s",
> +		     strerror(errno));
> +
> +	self->pkt = calloc(1, MAX_VNET_TUNNEL_PACKET_SZ);
> +	ASSERT_NE(self->pkt, NULL);
> +
> +	memset(self->ifname, 0, sizeof(self->ifname));
> +	self->fd = tun_open(self->ifname, flags, TUN_VNET_TNL_SIZE, features,
> +			    param_hwaddr_outer_src);
> +	ASSERT_GE(self->fd, 0);
> +}
> +
> +FIXTURE_TEARDOWN(tun_vnet_udptnl_proto)
> +{
> +	if (self->fd >= 0)
> +		close(self->fd);
> +	free(self->pkt);
> +}
> +
> +/* Build a 4in4 Geneve GSO frame whose outer ethertype is @l3proto,
> + * optionally behind an 802.1Q tag.
> + */
> +static int build_udptnl_frame(uint8_t *buf, bool vlan, uint16_t l3proto,
> +			      int payload_len)
> +{
> +	struct virtio_net_hdr_v1_hash_tunnel *vh = (void *)buf;
> +	int vlan_len = vlan ? 4 : 0;
> +	int hlen = ETH_HLEN + vlan_len + UDP_TUNNEL_GENEVE_4IN4_HDRLEN;
> +	uint8_t *outer_udph, *cur = buf;
> +	int pktlen;
> +

zero buffer. It is zeroed by calloc, but reused for multiple packets.

> +	cur += build_virtio_net_hdr_v1_hash_tunnel(cur, true, hlen, 1400,
> +						   AF_INET, AF_INET);
> +	vh->outer_th_offset += vlan_len;
> +	vh->inner_nh_offset += vlan_len;
> +	vh->hash_hdr.hdr.csum_start += vlan_len;
> +
> +	if (vlan) {
> +		uint16_t vlan_tag[2] = { htons(100), htons(l3proto) };
> +
> +		cur += build_eth(cur, ETH_P_8021Q, param_hwaddr_outer_dst,
> +				 param_hwaddr_outer_src);
> +		memcpy(cur, vlan_tag, sizeof(vlan_tag));
> +		cur += sizeof(vlan_tag);
> +	} else {
> +		cur += build_eth(cur, l3proto, param_hwaddr_outer_dst,
> +				 param_hwaddr_outer_src);
> +	}
> +
> +	pktlen = UDP_TUNNEL_GENEVE_4IN4_HDRLEN - sizeof(struct iphdr) +
> +		 payload_len;
> +	cur += build_ipv4_header(cur, IPPROTO_UDP, pktlen,
> +				 &param_ipaddr4_outer_dst,
> +				 &param_ipaddr4_outer_src);
> +
> +	outer_udph = cur;
> +	cur += build_udp_header(cur, UDP_SRC_PORT, VN_PORT,
> +				pktlen - sizeof(struct udphdr));
> +	cur += build_geneve_header(cur, VN_ID);
> +	cur += build_eth(cur, ETH_P_IP, param_hwaddr_inner_dst,
> +			 param_hwaddr_inner_src);
> +	cur += build_ipv4_header(cur, IPPROTO_UDP,
> +				 sizeof(struct udphdr) + payload_len,
> +				 &param_ipaddr4_inner_dst,
> +				 &param_ipaddr4_inner_src);
> +	cur += build_udp_packet(cur, UDP_DST_PORT, UDP_SRC_PORT, payload_len,
> +				AF_INET, false);
> +	build_udp_packet_csum(outer_udph, AF_INET, false);
> +
> +	return cur - buf;
> +}
> +
> +TEST_F(tun_vnet_udptnl_proto, outer_proto_ok)
> +{
> +	uint8_t *pkt = self->pkt;
> +	int len, ret;
> +
> +	len = build_udptnl_frame(pkt, false, ETH_P_IP, 2800);
> +	ret = write(self->fd, pkt, len);
> +	ASSERT_EQ(ret, len);
> +
> +	/* The outer IPv4 header can follow a VLAN tag. */
> +	len = build_udptnl_frame(pkt, true, ETH_P_IP, 2800);
> +	ret = write(self->fd, pkt, len);
> +	ASSERT_EQ(ret, len);
> +}
> +
> +TEST_F(tun_vnet_udptnl_proto, outer_proto_bad)
> +{
> +	uint8_t *pkt = self->pkt;
> +	int len, ret;
> +
> +	/* Outer ethertype does not match the declared IPv4 tunnel. */
> +	len = build_udptnl_frame(pkt, false, ETH_P_IPV6, 2800);
> +	ret = write(self->fd, pkt, len);
> +	ASSERT_EQ(ret, -1);
> +	ASSERT_EQ(errno, EINVAL);
> +
> +	/* Outer header is not IP at all. */
> +	len = build_udptnl_frame(pkt, false, ETH_P_NSH, 2800);
> +	ret = write(self->fd, pkt, len);
> +	ASSERT_EQ(ret, -1);
> +	ASSERT_EQ(errno, EINVAL);
> +
> +	/* Same, behind a VLAN tag. */
> +	len = build_udptnl_frame(pkt, true, ETH_P_NSH, 2800);
> +	ret = write(self->fd, pkt, len);
> +	ASSERT_EQ(ret, -1);
> +	ASSERT_EQ(errno, EINVAL);
> +}
> +
>  FIXTURE(tun_vnet_udptnl)
>  {
>  	char ifname[IFNAMSIZ];
> -- 
> 2.53.0
> 

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

[Eric Dumazet]

Le mer. 4 nov. 2026 à 03:01, Willem de Bruijn
<willemdebruijn.kernel@gmail.com> a écrit :
>
> Eric Dumazet wrote:
> > 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 an outer header. 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
> >
> > Require the outer L3 protocol to match the declared tunnel type:
> > for Ethernet frames, look at the ethertype after optional VLAN tags,
> > and check that the outer network header fits before outer_th_offset.
> > For IFF_TUN, skb->protocol is already set by the caller.
> >
> > Fixes: a2fb4bc4e2a6 ("net: implement virtio helpers to handle UDP GSO tunneling.")
> > Reported-by: Hassan Sultan <hassansultan@google.com>
> > Signed-off-by: Eric Dumazet <edumazet@kernel.org>
> > Cc: Willem de Bruijn <willemb@google.com>
> > Cc: Michael S. Tsirkin <mst@redhat.com>
>
> Thanks for the fix. It overlaps with
>
> commit "net: resolve uncommon partial checksums at ingress"
> https://lore.kernel.org/netdev/93d0e37c99711bebdf60c120af03010455c2f2e2.1793276116.git.mst@redhat.com/
>
> There are a bunch of subtle issues here. That series is up to v7.
>
> Two specific issues are that
> - skb->protocol in general cannot be trusted, and
> - non-L3 sources cannot be assumed Ethernet

Agreed on both. With IFF_TUN and a tun_pi header, skb->protocol is
whatever userspace put in pi.proto, and v1 would accept an IPv4 tunnel
frame whose IP version says otherwise.

>
> Would it make sense to take the relevant parts from the quoted commit,
> which has hopefully resolved those edge cases, and send that on its own?
> The rest of the series can come later.

Done in v2: patch 1 now uses virtio_net_l3_start(), virtio_net_l4_start(),
virtio_net_ip4_parse() and virtio_net_tnl_offsets_ok() from Michael's
v7 4/5, unchanged except that ipv6_find_hdr() stops at AH, as you
suggested in your v7 review. The checksum validation/completion parts
and the non-GSO carve-out are left to Michael's series.

Michael, I added your Co-developed-by and Signed-off-by, since the code
is yours; please let me know if you prefer a different attribution.

v2 also adds IFF_TUN tests with a mismatching tun_pi protocol, and
addresses your comments on patches 2 and 3.

One side note: virtio_net_hdr_tnl_to_skb() uses
outer_l3min = ETH_HLEN + virtio_l3min(outer_isv6) for IFF_TUN too, so
tunnel GSO frames without an Ethernet header are rejected unless the
outer IP header carries at least 14 bytes of options (the tests pad
with IPv4 options for that reason). This predates the series, I left
it alone for now.

Thanks!

pw-bot: cr

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] net: virtio: unreliable L3 check in virtio_net_tnl_outer_proto_ok()
--

commit 02f420dee7a050b3832ecf552c44f9c2eeaaddc1
Author: Eric Dumazet <edumazet@kernel.org>

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

This commit validates that the outer L3 protocol of a UDP tunnel GSO packet
matches the tunnel type declared in the virtio header in
virtio_net_hdr_tnl_to_skb().

> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> index d6466f96cdd00..d3f97d71d81cc 100644
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h
> @@ -282,6 +282,31 @@ static inline unsigned int virtio_l3min(bool is_ipv6)
>  	return is_ipv6 ? sizeof(struct ipv6hdr) : sizeof(struct iphdr);
>  }
>  
> +/* The outer network header of a UDP tunnel GSO packet must match the
> + * tunnel type declared in the virtio header, otherwise the GSO stack
> + * would parse the packet as something else (e.g. NSH).
> + */
> +static inline bool virtio_net_tnl_outer_proto_ok(const struct sk_buff *skb,
> +						 bool outer_isv6,
> +						 unsigned int outer_th)
> +{
> +	__be16 expected = outer_isv6 ? htons(ETH_P_IPV6) : htons(ETH_P_IP);
> +	__be16 proto = skb->protocol;
> +	int nh_off = ETH_HLEN;
> +
> +	/* L3 device (IFF_TUN): the caller already set the protocol. */
> +	if (proto)
> +		return proto == expected;

[Severity: High]
Can testing if (proto) here be bypassed in tun_get_user() in two ways?

First, on an IFF_TAP device with IFF_NAPI_FRAGS enabled, tun_get_user()
allocates the skb via tun_napi_alloc_frags() -> napi_get_frags(). When a
prior packet was merged by GRO (GRO_MERGED_FREE in napi_frags_finish()),
napi_reuse_skb() caches napi->skb for reuse without clearing skb->protocol
(which napi_frags_skb() set to the previous frame's eth->h_proto, such as
htons(ETH_P_IP)).

In drivers/net/tun.c:tun_get_user(), tun_vnet_hdr_tnl_to_skb() runs before
eth_type_trans() updates skb->protocol for the new frame:

	if (tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, &hdr)) {
		atomic_long_inc(&tun->rx_frame_errors);
		err = -EINVAL;
		goto free_skb;
	}

	if ((tun->flags & TUN_TYPE_MASK) == IFF_TAP)
		skb->protocol = eth_type_trans(skb, tun->dev);

With a reused napi->skb still carrying skb->protocol == htons(ETH_P_IP),
virtio_net_tnl_outer_proto_ok() takes the if (proto) branch and returns true
without inspecting eth_hdr(skb)->h_proto or checking nh_off +
virtio_l3min(outer_isv6) <= outer_th, and eth_type_trans() then sets
skb->protocol to the new frame's actual EtherType (such as ETH_P_NSH).

Second, on an IFF_TUN device without IFF_NO_PI, tun_get_user() copies struct
tun_pi from userspace and sets skb->protocol = pi.proto:

		skb_reset_network_header(skb);
		skb->protocol = pi.proto;
		break;

If userspace passes pi.proto == 0, proto is 0, so
virtio_net_tnl_outer_proto_ok() falls through to the Ethernet path and checks
eth_hdr(skb)->h_proto inside the L3 payload, leaving skb->protocol == 0 with
SKB_GSO_UDP_TUNNEL set if those bytes match expected.

Since all callers set skb->network_header before calling
virtio_net_hdr_tnl_to_skb() (to 0 via skb_reset_network_header() for IFF_TUN,
and to ETH_HLEN via skb_set_network_header() for IFF_TAP and virtio_net),
would checking !skb_network_offset(skb) instead of if (proto) avoid both
issues?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103203113.550604-1-edumazet@kernel.org?part=1

Re: [PATCH net 2/3] selftests: net: tuntap: fix UDP checksum length in build_udp_packet_csum()

[sashiko-bot]

> build_udp_packet_csum() passes udph->len, which is in network byte
> order, as the number of bytes to checksum. On little-endian hosts this
> reads up to 64KB past the UDP header and computes a wrong checksum.
> 
> Existing callers did not crash only because they use packet buffers
> of MAX_VNET_TUNNEL_PACKET_SZ bytes.
> 
> Use ntohs(udph->len), as tap.c does in its own build_udp_packet().
> 
> Fixes: a942fcd72e97 ("selftest: tun: Introduce tuntap_helpers.h header for TUN/TAP testing")
> Signed-off-by: Eric Dumazet <edumazet@kernel.org>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103203113.550604-1-edumazet@kernel.org?part=2

Re: [PATCH net 3/3] selftests: net: tun: test outer protocol of UDP tunnel GSO packets

[sashiko-bot]

> Add a tun_vnet_udptnl_proto fixture writing single 4in4 Geneve GSO
> frames to a tap device with UDP tunnel offloads enabled:
> 
> - outer_proto_ok: an outer IPv4 header, with or without an 802.1Q tag,
>   is accepted.
> - outer_proto_bad: an outer ethertype that does not match the declared
>   tunnel type (IPv6 on an IPv4 tunnel, or NSH, with or without a VLAN
>   tag) is rejected with EINVAL.
> 
> Without the previous virtio_net patch, outer_proto_bad fails because
> the kernel accepts the frames.
> 
> Signed-off-by: Eric Dumazet <edumazet@kernel.org>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103203113.550604-1-edumazet@kernel.org?part=3

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 1 · Low: 1

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 validation in virtio_net_tnl_outer_proto_ok()
  (include/linux/virtio_net.h) can be bypassed on IFF_TAP devices that…
- [Medium] virtio_net_tnl_outer_proto_ok() only checks the outer
  ethertype (or skb->protocol) and that a minimum-size IPv4/IPv6 header…
- [Low] virtio_net_tnl_outer_proto_ok() treats skb->protocol == 0 as
  meaning "Ethernet frame", and its comment says that for IFF_TUN "the…

--

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

This patch adds virtio_net_tnl_outer_proto_ok(), called from
virtio_net_hdr_tnl_to_skb(). The new helper requires the outer L3
protocol of a UDP tunnel GSO packet to match the tunnel type declared
in the virtio header. For Ethernet frames it checks the ethertype after
optional VLAN tags, plus the outer network header size against
outer_th_offset. For IFF_TUN it relies on skb->protocol, which the
caller has already set.

> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> index d6466f96cdd00..d3f97d71d81cc 100644
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h
> @@ -282,6 +282,31 @@ static inline unsigned int virtio_l3min(bool is_ipv6)
>  	return is_ipv6 ? sizeof(struct ipv6hdr) : sizeof(struct iphdr);
>  }
>  
> +/* The outer network header of a UDP tunnel GSO packet must match the
> + * tunnel type declared in the virtio header, otherwise the GSO stack
> + * would parse the packet as something else (e.g. NSH).
> + */
> +static inline bool virtio_net_tnl_outer_proto_ok(const struct sk_buff *skb,
> +						 bool outer_isv6,
> +						 unsigned int outer_th)
> +{
> +	__be16 expected = outer_isv6 ? htons(ETH_P_IPV6) : htons(ETH_P_IP);
> +	__be16 proto = skb->protocol;
> +	int nh_off = ETH_HLEN;
> +
> +	/* L3 device (IFF_TUN): the caller already set the protocol. */
> +	if (proto)
> +		return proto == expected;

[Severity: High]
Can a stale skb->protocol on an IFF_TAP device get past this check?

On the IFF_TAP path, tun_get_user() does not clear skb->protocol before
it calls tun_vnet_hdr_tnl_to_skb(). eth_type_trans() only sets it
afterwards:

drivers/net/tun.c:tun_get_user() {
    ...
	if (tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, &hdr)) {
    ...
}

With IFF_NAPI_FRAGS, tun_napi_alloc_frags() gets its skb from
napi_get_frags(&tfile->napi), which returns the cached napi->skb. If GRO
merged the previous frame with GRO_MERGED_FREE, that skb was recycled
through:

napi_frags_finish()
  napi_reuse_skb()

napi_reuse_skb() resets dev, pkt_type, encapsulation, ip_summed, gso_type
and gso_size. It does not reset skb->protocol, which napi_frags_skb() had
set to the previous frame's eth->h_proto (for example ETH_P_IP).

On the next write, __virtio_net_hdr_to_skb() leaves skb->protocol alone
on the NEEDS_CSUM path. virtio_net_tnl_outer_proto_ok() then sees
htons(ETH_P_IP), takes the "L3 device" early return, and never reads the
real ethertype (for example ETH_P_NSH).

eth_type_trans() then sets skb->protocol to ETH_P_NSH on an skb that
carries SKB_GSO_UDP_TUNNEL. If that skb is forwarded (bridge to gretap)
and segmented in software, wouldn't nsh_gso_segment() run and hit the
same skb_under_panic() described in the commit message?

The setup needs a tap opened with IFF_TAP | IFF_NAPI_FRAGS
(CAP_NET_ADMIN). GRO also has to hold packets across writes, for
example with napi_defer_hard_irqs / gro_flush_timeout configured. Once
that is in place, a guest using vhost-net controls the frame contents.

Could the helper avoid inferring L2 vs L3 from skb->protocol? For
example, the caller could pass an explicit L2/L3 flag, or Ethernet
callers could always parse eth_hdr().

> +
> +	/* Ethernet frame, possibly VLAN tagged. */
> +	proto = eth_hdr(skb)->h_proto;
> +	if (eth_type_vlan(proto))
> +		proto = __vlan_get_protocol(skb, proto, &nh_off);

[Severity: Low]
Does IFF_TUN always set a non-zero skb->protocol, as the comment above
assumes?

For IFF_TUN without IFF_NO_PI, tun_get_user() copies the protocol
directly from the userspace struct tun_pi:

drivers/net/tun.c:tun_get_user() {
    ...
		skb_reset_network_header(skb);
		skb->protocol = pi.proto;
    ...
}

If pi.proto is 0, the L3 packet lands in this Ethernet branch.
eth_hdr(skb)->h_proto then reads bytes 12-13 of the IPv4/IPv6 header as
an ethertype, and the sender chooses those source address bytes. They
can also match a VLAN TPID and be parsed further by
__vlan_get_protocol().

With an IPv4 saddr that starts with 0x0800, the frame is accepted with
skb->protocol still 0 and SKB_GSO_UDP_TUNNEL set. The practical impact
looks small: no L3 handler takes protocol 0, and skb_mac_gso_segment()
would return -EPROTONOSUPPORT. Still, the write is accepted when it
presumably should fail with -EINVAL.

This is the same skb->protocol based L2/L3 test as above.

> +
> +	return proto == expected &&
> +	       nh_off + virtio_l3min(outer_isv6) <= outer_th;

[Severity: Medium]
Is checking only the ethertype and a minimum L3 size enough to stop GSO
from parsing the packet as something other than a UDP tunnel?

Nothing checks that the outer iph->protocol (or the IPv6 nexthdr chain)
is UDP. Nothing checks that the real outer header length (ihl,
extension headers) ends at outer_th either. GSO ignores the virtio
outer_th and dispatches from the IP header itself:

net/ipv4/af_inet.c:inet_gso_segment() {
    ...
	ops = rcu_dereference(inet_offloads[proto]);
    ...
}

For example, a frame with ethertype ETH_P_IP, iph->protocol =
IPPROTO_GRE and VIRTIO_NET_HDR_GSO_UDP_TUNNEL_IPV4 passes this check.
gre_gso_segment() then tests only skb->encapsulation, which
virtio_net_hdr_tnl_to_skb() sets, and not SKB_GSO_GRE:

net/ipv4/gre_offload.c:gre_gso_segment() {
    ...
	if (!skb->encapsulation)
		goto out;
    ...
}

With IPPROTO_TCP the frame would reach tcp4_gso_segment(), because
gso_type also carries the inner SKB_GSO_TCPV4 bit.

With ihl > 5, or with IPv6 extension headers, __skb_udp_tunnel_segment()
derives tnl_hlen from the transport header that GSO recomputed:

	int tnl_hlen = skb_inner_mac_header(skb) - skb_transport_header(skb);

That value differs from the outer_th layout validated here and can be
smaller than a udphdr. The UDP len/check writes could then land on the
inner IP header.

I could not show a crash from this: pskb_may_pull() catches a negative
tnl_hlen, and gre_gso_segment() recomputes its own offsets. Would it
make sense to also require the outer L4 protocol to be UDP, and the
outer L3 header to end exactly at outer_th?

> +}
> +

[ ... ]

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

Re: [PATCH net 3/3] selftests: net: tun: test outer protocol of UDP tunnel GSO packets

[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 new tests cover only part of the fix they are meant to
  validate.
- [Low] virtio_net_tnl_outer_proto_ok() in include/linux/virtio_net.h was
  added by the series commit "net: virtio: validate outer protocol of…
- [Low] Both tun_vnet_udptnl_proto tests build several frames in the same
  calloc'd buffer (self->pkt) without clearing it in between.

--

selftests: net: tun: test outer protocol of UDP tunnel GSO packets

This adds a tun_vnet_udptnl_proto fixture that writes single 4in4 Geneve
GSO frames to a tap device with UDP tunnel offloads enabled. outer_proto_ok
expects outer IPv4 frames, optionally VLAN tagged, to be accepted.
outer_proto_bad expects IPv6 or NSH outer ethertypes to be rejected with
EINVAL.

> diff --git a/tools/testing/selftests/net/tun.c b/tools/testing/selftests/net/tun.c
> index 34de2580e32ec..47a24b9c477bb 100644
> --- a/tools/testing/selftests/net/tun.c
> +++ b/tools/testing/selftests/net/tun.c
> @@ -683,6 +683,136 @@ TEST_F(tun_vnet_gso, vlan_tcpv4_gso_no_csum_64k)

[ ... ]

> +FIXTURE_SETUP(tun_vnet_udptnl_proto)
> +{
> +	int flags = IFF_TAP | IFF_NO_PI | IFF_VNET_HDR;

[Severity: Low]
This fixture only opens IFF_TAP devices. That means the IFF_TUN branch of
virtio_net_tnl_outer_proto_ok() from the previous patch is never run. Does
the assumption behind that branch hold?

include/linux/virtio_net.h:virtio_net_tnl_outer_proto_ok() {
    ...
	/* L3 device (IFF_TUN): the caller already set the protocol. */
	if (proto)
		return proto == expected;

	/* Ethernet frame, possibly VLAN tagged. */
	proto = eth_hdr(skb)->h_proto;
    ...
}

The previous patch's commit message also says "For IFF_TUN, skb->protocol
is already set by the caller."

For IFF_TUN without IFF_NO_PI, tun_get_user() in drivers/net/tun.c copies
struct tun_pi from userspace. It stores the value as-is before
tun_vnet_hdr_tnl_to_skb() runs:

		skb_reset_network_header(skb);
		skb->protocol = pi.proto;

pi.proto can be 0 here. In that case, would the helper take the Ethernet
branch for an L3 packet?

The mac header was reset to the start of the outer IP header. So h_proto
would be read from the first two bytes of the IPv4 saddr, and nh_off would
be measured from ETH_HLEN.

For example, a 4in4 frame with an outer saddr of 8.0.x.x and outer_th >= 34
(IPv4 options) would pass the check. The skb would then go into the stack
with SKB_GSO_UDP_TUNNEL set and skb->protocol == 0, which the comment says
can't happen. With other saddr values the same packet would be rejected.

The downstream impact looks limited, because skb_mac_gso_segment() returns
-EINVAL when skb_network_protocol() gives 0. Even so, should the helper or
its comment handle this case? Would an IFF_TUN case in this fixture be
useful?

[ ... ]

> +static int build_udptnl_frame(uint8_t *buf, bool vlan, uint16_t l3proto,
> +			      int payload_len)
> +{
> +	struct virtio_net_hdr_v1_hash_tunnel *vh = (void *)buf;
> +	int vlan_len = vlan ? 4 : 0;
> +	int hlen = ETH_HLEN + vlan_len + UDP_TUNNEL_GENEVE_4IN4_HDRLEN;
> +	uint8_t *outer_udph, *cur = buf;
> +	int pktlen;
> +
> +	cur += build_virtio_net_hdr_v1_hash_tunnel(cur, true, hlen, 1400,
> +						   AF_INET, AF_INET);
> +	vh->outer_th_offset += vlan_len;

[Severity: Low]
outer_th_offset is always shifted by vlan_len, and the outer family is
always AF_INET. Given that, do these tests reach all of the checks added by
"net: virtio: validate outer protocol of UDP tunnel GSO packets"?

virtio_net_tnl_outer_proto_ok() ends with:

	return proto == expected &&
	       nh_off + virtio_l3min(outer_isv6) <= outer_th;

The length clause can only fail when a VLAN tag moves nh_off to 18 or
more. Without a tag it is the same as the existing outer_l3min check.

No case here sends a VLAN tagged IPv4 frame with outer_th_offset left at
34. If that clause were removed or miscomputed (for example in a stable
backport), no test would fail.

Every frame also declares VIRTIO_NET_HDR_GSO_UDP_TUNNEL_IPV4. So the
outer_isv6 side of expected is never tested either, for example an IPv4
ethertype on a declared IPv6 tunnel.

Separately, the commit message says outer_proto_bad covers:

    (IPv6 on an IPv4 tunnel, or NSH, with or without a VLAN tag)

outer_proto_bad actually tests IPv6 untagged, NSH untagged and NSH behind a
VLAN tag. IPv6 behind a VLAN tag is not tested. Should the message be
narrowed, or should that case be added?

[ ... ]

> +TEST_F(tun_vnet_udptnl_proto, outer_proto_ok)
> +{
> +	uint8_t *pkt = self->pkt;
> +	int len, ret;
> +
> +	len = build_udptnl_frame(pkt, false, ETH_P_IP, 2800);
> +	ret = write(self->fd, pkt, len);
> +	ASSERT_EQ(ret, len);
> +
> +	/* The outer IPv4 header can follow a VLAN tag. */
> +	len = build_udptnl_frame(pkt, true, ETH_P_IP, 2800);

[Severity: Low]
Is the VLAN tagged frame here well formed? self->pkt is calloc'd once in
FIXTURE_SETUP and is not cleared between frames. The helpers in
tuntap_helpers.h expect a zeroed buffer.

build_ipv4_header() never writes tos or frag_off. It also sums over
whatever is already in the check field:

	iph->check = build_ip_csum(buf, iph->ihl << 2, 0);

build_udp_packet_csum() also sums over the existing udph->check. The
existing tun_vnet_udptnl test clears its buffer first in send_gso_packet():

	memset(pkt, 0, sizeof(pkt));

In this test, the second frame's outer IPv4 header starts 4 bytes after the
first frame's. As a result:

- frag_off holds the previous outer IPv4 checksum
- tos holds the low byte of the previous IP ID
- the checksum is computed starting from stale saddr bytes

The inner IPv4 header and the inner UDP checksum are shifted and corrupted
in the same way.

This only passes today because the kernel path checks just the offsets and
the ethertype. If that path ever validates the outer IPv4 header, for
example by rejecting fragments for GSO, outer_proto_ok would fail for an
unrelated reason. Would a memset at the start of build_udptnl_frame() avoid
this?

[ ... ]

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