geneve: fix header validation in geneve[6]_xmit_skb

PATCH Superseded Eric Dumazet 2 participants 6 messages Apr 3, 2024

Subsystems (3)

Networking Network drivers IPv4/IPv6

lore

Share & tools

lore

Eric Dumazet

author

2024-04-03 11:38 UTC #0 lore raw

PATCH 1/1 geneve: fix header validation in geneve[6]_xmit_skb

syzbot is able to trigger an uninit-value in geneve_xmit() [1]

Problem : While most ip tunnel helpers (like ip_tunnel_get_dsfield()) uses skb_protocol(skb, true), pskb_inet_may_pull() is only using skb->protocol.

If anything else than ETH_P_IPV6 or ETH_P_IP is found in skb->protocol, pskb_inet_may_pull() does nothing at all.

If a vlan tag was provided by the caller (af_packet in the syzbot case), the network header might not point to the correct location, and skb linear part could be smaller than expected.

Add skb_vlan_inet_prepare() to perform a complete validation and pull. If no IPv4/IPv6 header is found, it returns 0.

Use this in geneve for the moment, I suspect we need to adopt this more broadly.

[1]

BUG: KMSAN: uninit-value in geneve_xmit_skb drivers/net/geneve.c:910 [inline]
 BUG: KMSAN: uninit-value in geneve_xmit+0x302d/0x5420 drivers/net/geneve.c:1030
  geneve_xmit_skb drivers/net/geneve.c:910 [inline]
  geneve_xmit+0x302d/0x5420 drivers/net/geneve.c:1030
  __netdev_start_xmit include/linux/netdevice.h:4903 [inline]
  netdev_start_xmit include/linux/netdevice.h:4917 [inline]
  xmit_one net/core/dev.c:3531 [inline]
  dev_hard_start_xmit+0x247/0xa20 net/core/dev.c:3547
  __dev_queue_xmit+0x348d/0x52c0 net/core/dev.c:4335
  dev_queue_xmit include/linux/netdevice.h:3091 [inline]
  packet_xmit+0x9c/0x6c0 net/packet/af_packet.c:276
  packet_snd net/packet/af_packet.c:3081 [inline]
  packet_sendmsg+0x8bb0/0x9ef0 net/packet/af_packet.c:3113
  sock_sendmsg_nosec net/socket.c:730 [inline]
  __sock_sendmsg+0x30f/0x380 net/socket.c:745
  __sys_sendto+0x685/0x830 net/socket.c:2191
  __do_sys_sendto net/socket.c:2203 [inline]
  __se_sys_sendto net/socket.c:2199 [inline]
  __x64_sys_sendto+0x125/0x1d0 net/socket.c:2199
 do_syscall_64+0xd5/0x1f0
 entry_SYSCALL_64_after_hwframe+0x6d/0x75

Uninit was created at:

  slab_post_alloc_hook mm/slub.c:3804 [inline]
  slab_alloc_node mm/slub.c:3845 [inline]
  kmem_cache_alloc_node+0x613/0xc50 mm/slub.c:3888
  kmalloc_reserve+0x13d/0x4a0 net/core/skbuff.c:577
  __alloc_skb+0x35b/0x7a0 net/core/skbuff.c:668
  alloc_skb include/linux/skbuff.h:1318 [inline]
  alloc_skb_with_frags+0xc8/0xbf0 net/core/skbuff.c:6504
  sock_alloc_send_pskb+0xa81/0xbf0 net/core/sock.c:2795
  packet_alloc_skb net/packet/af_packet.c:2930 [inline]
  packet_snd net/packet/af_packet.c:3024 [inline]
  packet_sendmsg+0x722d/0x9ef0 net/packet/af_packet.c:3113
  sock_sendmsg_nosec net/socket.c:730 [inline]
  __sock_sendmsg+0x30f/0x380 net/socket.c:745
  __sys_sendto+0x685/0x830 net/socket.c:2191
  __do_sys_sendto net/socket.c:2203 [inline]
  __se_sys_sendto net/socket.c:2199 [inline]
  __x64_sys_sendto+0x125/0x1d0 net/socket.c:2199
 do_syscall_64+0xd5/0x1f0
 entry_SYSCALL_64_after_hwframe+0x6d/0x75

CPU: 0 PID: 5033 Comm: syz-executor346 Not tainted 6.9.0-rc1-syzkaller-00005-g928a87efa423 #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 02/29/2024

Fixes d13f048dd40e ("net: geneve: modify IP header check in geneve6_xmit_skb and geneve_xmit_skb")

Reported-by [email protected]

Closes https://lore.kernel.org/netdev/[email protected]/ open here

Signed-off-by Eric Dumazet <[email protected]>

Cc Phillip Potter <[email protected]>

---
 drivers/net/geneve.c     |  4 ++--
 include/net/ip_tunnels.h | 31 +++++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 2 deletions(-)

drivers/net/geneve.c +2 -2

index 2f6739fe78af2e8e90c0a3b474c2e99c83e02994..6c2835086b57eacbcddb44a3c507e26d5a944427 100644
@@ -822,7 +822,7 @@ static int geneve_xmit_skb(struct sk_buff *skb, struct net_device *dev,
 	__be16 sport;
 	int err;
 
-	if (!pskb_inet_may_pull(skb))
+	if (!skb_vlan_inet_prepare(skb))
 		return -EINVAL;
 
 	if (!gs4)

@@ -929,7 +929,7 @@ static int geneve6_xmit_skb(struct sk_buff *skb, struct net_device *dev,
 	__be16 sport;
 	int err;
 
-	if (!pskb_inet_may_pull(skb))
+	if (!skb_vlan_inet_prepare(skb))
 		return -EINVAL;
 
 	if (!gs6)

include/net/ip_tunnels.h +31 -0

index 5cd64bb2104df389250fb3c518ba00a3826c53f7..41537d5dce52412e15d7871ec604546582b10098 100644
@@ -361,6 +361,37 @@ static inline bool pskb_inet_may_pull(struct sk_buff *skb)
 	return pskb_network_may_pull(skb, nhlen);
 }
 
+/* Strict version of pskb_inet_may_pull().
+ * Once vlan headers are skipped, only accept
+ * ETH_P_IPV6 and ETH_P_IP.
+ */
+static inline __be16 skb_vlan_inet_prepare(struct sk_buff *skb)
+{
+	int nhlen, maclen;
+	__be16 type;
+
+	type = __vlan_get_protocol(skb, type, &maclen);
+
+	switch (type) {
+#if IS_ENABLED(CONFIG_IPV6)
+	case htons(ETH_P_IPV6):
+		nhlen = sizeof(struct ipv6hdr);
+		break;
+#endif
+	case htons(ETH_P_IP):
+		nhlen = sizeof(struct iphdr);
+		break;
+
+	default:
+		return 0;
+	}
+	if (pskb_may_pull(skb, maclen + nhlen))
+		return 0;
+
+	skb_set_network_header(skb, maclen);
+	return type;
+}
+
 static inline int ip_encap_hlen(struct ip_tunnel_encap *e)
 {
 	const struct ip_tunnel_encap_ops *ops;

Signature · 1 line

2.44.0.478.gd926399ef9-goog

Sabrina Dubroca

2024-04-03 14:28 UTC #1 lore raw

Quoted 9 lines · Eric Dumazet

syzbot is able to trigger an uninit-value in geneve_xmit() [1]
Problem : While most ip tunnel helpers (like ip_tunnel_get_dsfield()) uses skb_protocol(skb, true), pskb_inet_may_pull() is only using skb->protocol.
If anything else than ETH_P_IPV6 or ETH_P_IP is found in skb->protocol, pskb_inet_may_pull() does nothing at all.

Quoted 6 lines

If a vlan tag was provided by the caller (af_packet in the syzbot case), the network header might not point to the correct location, and skb linear part could be smaller than expected.
Add skb_vlan_inet_prepare() to perform a complete validation and pull. If no IPv4/IPv6 header is found, it returns 0.

And then geneve_xmit_skb/geneve6_xmit_skb drops the packet, which breaks ARP over a geneve tunnel, and other valid things like macsec.

Quoted 6 lines

include/net/ip_tunnels.h +0 -0

index 5cd64bb2104df389250fb3c518ba00a3826c53f7..41537d5dce52412e15d7871ec604546582b10098 100644
@@ -361,6 +361,37 @@ static inline bool pskb_inet_may_pull(struct sk_buff *skb)
 	return pskb_network_may_pull(skb, nhlen);

include/net/ip_tunnels.h static inline bool pskb_inet_may_pull(struct sk_buff *skb) · line 371

Quoted 10 lines

include/net/ip_tunnels.h +8 -0

 }
 
+/* Strict version of pskb_inet_may_pull().
+ * Once vlan headers are skipped, only accept
+ * ETH_P_IPV6 and ETH_P_IP.
+ */
+static inline __be16 skb_vlan_inet_prepare(struct sk_buff *skb)
+{
+	int nhlen, maclen;
+	__be16 type;

Should that be:

    type = skb->protocol

Otherwise it's used uninitialized here:

Quoted 2 lines

include/net/ip_tunnels.h +2 -0
+
+	type = __vlan_get_protocol(skb, type, &maclen);

Signature · 1 line

Sabrina

Eric Dumazet

author

2024-04-03 14:25 UTC #2 lore raw

Quoted 15 lines · Sabrina Dubroca

Quoted 13 lines · Eric Dumazet
syzbot is able to trigger an uninit-value in geneve_xmit() [1]
Problem : While most ip tunnel helpers (like ip_tunnel_get_dsfield()) uses skb_protocol(skb, true), pskb_inet_may_pull() is only using skb->protocol.
If anything else than ETH_P_IPV6 or ETH_P_IP is found in skb->protocol, pskb_inet_may_pull() does nothing at all.
If a vlan tag was provided by the caller (af_packet in the syzbot case), the network header might not point to the correct location, and skb linear part could be smaller than expected.

Quoted 5 lines

Quoted 2 lines
Add skb_vlan_inet_prepare() to perform a complete validation and pull. If no IPv4/IPv6 header is found, it returns 0.
And then geneve_xmit_skb/geneve6_xmit_skb drops the packet, which breaks ARP over a geneve tunnel, and other valid things like macsec.

geneve_xmit_skb() uses ip_hdr() blindly.

How can we cope properly with this mess ?

Quoted 20 lines

Quoted 7 lines

include/net/ip_tunnels.h +0 -0

index 5cd64bb2104df389250fb3c518ba00a3826c53f7..41537d5dce52412e15d7871ec604546582b10098 100644
@@ -361,6 +361,37 @@ static inline bool pskb_inet_may_pull(struct sk_buff *skb)
      return pskb_network_may_pull(skb, nhlen);

include/net/ip_tunnels.h static inline bool pskb_inet_may_pull(struct sk_buff *skb) · line 371

Quoted 10 lines

include/net/ip_tunnels.h +8 -0

 }
 
+/* Strict version of pskb_inet_may_pull().
+ * Once vlan headers are skipped, only accept
+ * ETH_P_IPV6 and ETH_P_IP.
+ */
+static inline __be16 skb_vlan_inet_prepare(struct sk_buff *skb)
+{
+     int nhlen, maclen;
+     __be16 type;

Should that be:

Quoted 8 lines

    type = skb->protocol
?
Otherwise it's used uninitialized here:
Quoted 2 lines
include/net/ip_tunnels.h +2 -0
+
+     type = __vlan_get_protocol(skb, type, &maclen);

Arg, a last minute change did not make it.

Quoted 4 lines

--
Sabrina

Sabrina Dubroca

2024-04-03 14:55 UTC #3 lore raw

Quoted 16 lines · Eric Dumazet

Quoted 15 lines · Sabrina Dubroca
Quoted 13 lines · Eric Dumazet
syzbot is able to trigger an uninit-value in geneve_xmit() [1]
Problem : While most ip tunnel helpers (like ip_tunnel_get_dsfield()) uses skb_protocol(skb, true), pskb_inet_may_pull() is only using skb->protocol.
If anything else than ETH_P_IPV6 or ETH_P_IP is found in skb->protocol, pskb_inet_may_pull() does nothing at all.
If a vlan tag was provided by the caller (af_packet in the syzbot case), the network header might not point to the correct location, and skb linear part could be smaller than expected.

Quoted 7 lines

Quoted 5 lines
Quoted 2 lines
Add skb_vlan_inet_prepare() to perform a complete validation and pull. If no IPv4/IPv6 header is found, it returns 0.
And then geneve_xmit_skb/geneve6_xmit_skb drops the packet, which breaks ARP over a geneve tunnel, and other valid things like macsec.
geneve_xmit_skb() uses ip_hdr() blindly.

Do those actually end up getting used? They get passed to {ip_tunnel_ecn_encap,ip_tunnel_get_ttl,ip_tunnel_get_dsfield}, and those helpers only look at their iph argument when skb_protocol(skb, true) is ETH_P_IP or ETH_P_IPV6. So, definitely not pretty, but I don't see a bug there. Am I missing something?

From a quick look, most users of those helpers seem to pass ip_hdr(skb) (except for ip_tunnel_ecn_encap called from ip_md_tunnel_xmit and ip_tunnel_xmit -- vxlan_xmit_one uses a cached version but I don't think it's needed). Would it be less confusing if we removed that argument and let the helper fetch ip_hdr?

Signature · 1 line

Sabrina

Eric Dumazet

author

2024-04-03 15:00 UTC #4 lore raw

Quoted 24 lines · Sabrina Dubroca

Quoted 22 lines · Eric Dumazet
Quoted 20 lines · Sabrina Dubroca
Quoted 9 lines · Eric Dumazet
syzbot is able to trigger an uninit-value in geneve_xmit() [1]
Problem : While most ip tunnel helpers (like ip_tunnel_get_dsfield()) uses skb_protocol(skb, true), pskb_inet_may_pull() is only using skb->protocol.
If anything else than ETH_P_IPV6 or ETH_P_IP is found in skb->protocol, pskb_inet_may_pull() does nothing at all.
Quoted 6 lines
If a vlan tag was provided by the caller (af_packet in the syzbot case), the network header might not point to the correct location, and skb linear part could be smaller than expected.
Add skb_vlan_inet_prepare() to perform a complete validation and pull. If no IPv4/IPv6 header is found, it returns 0.
And then geneve_xmit_skb/geneve6_xmit_skb drops the packet, which breaks ARP over a geneve tunnel, and other valid things like macsec.

Quoted 7 lines

Quoted 1 line
geneve_xmit_skb() uses ip_hdr() blindly.
Do those actually end up getting used? They get passed to {ip_tunnel_ecn_encap,ip_tunnel_get_ttl,ip_tunnel_get_dsfield}, and those helpers only look at their iph argument when skb_protocol(skb, true) is ETH_P_IP or ETH_P_IPV6. So, definitely not pretty, but I don't see a bug there. Am I missing something?

Please read my changelog, I explained that skb_protocol(skb, true) is parsing the Ethernet header up to the non vlan proto.

syzbot buillt a vlan packet with final proto being IPv4.

So the helpers who are using skb_protocol() do not understand the IP header has not been pulled.

Quoted 6 lines

From a quick look, most users of those helpers seem to pass ip_hdr(skb) (except for ip_tunnel_ecn_encap called from ip_md_tunnel_xmit and ip_tunnel_xmit -- vxlan_xmit_one uses a cached version but I don't think it's needed). Would it be less confusing if we removed that argument and let the helper fetch ip_hdr?

If you look at the syzbot report, the ip header is definitely dereferenced.

Sabrina Dubroca

2024-04-03 16:05 UTC #5 lore raw

Quoted 27 lines · Eric Dumazet

Quoted 26 lines · Sabrina Dubroca
Quoted 23 lines · Eric Dumazet
Quoted 15 lines · Sabrina Dubroca
Quoted 13 lines · Eric Dumazet
syzbot is able to trigger an uninit-value in geneve_xmit() [1]
Problem : While most ip tunnel helpers (like ip_tunnel_get_dsfield()) uses skb_protocol(skb, true), pskb_inet_may_pull() is only using skb->protocol.
If anything else than ETH_P_IPV6 or ETH_P_IP is found in skb->protocol, pskb_inet_may_pull() does nothing at all.
If a vlan tag was provided by the caller (af_packet in the syzbot case), the network header might not point to the correct location, and skb linear part could be smaller than expected.
Quoted 5 lines
Quoted 2 lines
Add skb_vlan_inet_prepare() to perform a complete validation and pull. If no IPv4/IPv6 header is found, it returns 0.
And then geneve_xmit_skb/geneve6_xmit_skb drops the packet, which breaks ARP over a geneve tunnel, and other valid things like macsec.
geneve_xmit_skb() uses ip_hdr() blindly.

Quoted 8 lines

Quoted 5 lines
Do those actually end up getting used? They get passed to {ip_tunnel_ecn_encap,ip_tunnel_get_ttl,ip_tunnel_get_dsfield}, and those helpers only look at their iph argument when skb_protocol(skb, true) is ETH_P_IP or ETH_P_IPV6. So, definitely not pretty, but I don't see a bug there. Am I missing something?
Please read my changelog, I explained that skb_protocol(skb, true) is parsing the Ethernet header up to the non vlan proto.

I meant in the ARP/MACsec/whatever else case, using ip_hdr is ugly but won't do anything wrong.

Quoted 6 lines

syzbot buillt a vlan packet with final proto being IPv4.
So the helpers who are using skb_protocol() do not understand the IP header has not been pulled.
Quoted 1 line

Quoted 7 lines

Quoted 5 lines
From a quick look, most users of those helpers seem to pass ip_hdr(skb) (except for ip_tunnel_ecn_encap called from ip_md_tunnel_xmit and ip_tunnel_xmit -- vxlan_xmit_one uses a cached version but I don't think it's needed). Would it be less confusing if we removed that argument and let the helper fetch ip_hdr?
If you look at the syzbot report, the ip header is definitely dereferenced.

Sure, something is clearly needed to fix what syzbot found (and pulling more headers is the reasonable way to deal with it).

But AFAICT, if the skb inner contents are not IP/IPv6, the IP header won't get looked at, even if we have ip_hdr() calls everywhere. That's what I tried to say with:

those helpers only look at their iph argument when skb_protocol(skb, true) is ETH_P_IP or ETH_P_IPV6

Signature · 1 line

Sabrina