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 <redacted>
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/ (local)
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.
quoted hunk ↗ jump to hunk
---
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