[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