geneve: fix header validation in geneve[6]_xmit_skb

PATCH In review Eric Dumazet 2 participants 5 messages Apr 4, 2024

Subsystems (3)

Networking Network drivers IPv4/IPv6

lore

Share & tools

lore

Eric Dumazet

author

2024-04-04 13:11 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 mac validation.

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

v2,v3 - Addressed Sabrina comments on v1 and v2

Link https://lore.kernel.org/netdev/Zg1l9L2BNoZWZDZG@hog/ open here

[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]>

Cc Sabrina Dubroca <[email protected]>

---
 drivers/net/geneve.c     |  4 ++--
 include/net/ip_tunnels.h | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 37 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 +35 -0

index 5cd64bb2104df389250fb3c518ba00a3826c53f7..6898d0b8306f803c57065f6e134f96dcc8517cad 100644
@@ -361,6 +361,41 @@ static inline bool pskb_inet_may_pull(struct sk_buff *skb)
 	return pskb_network_may_pull(skb, nhlen);
 }
 
+/* Variant of pskb_inet_may_pull().
+ */
+static inline bool skb_vlan_inet_prepare(struct sk_buff *skb)
+{
+	int nhlen, maclen;
+	__be16 type;
+
+	/* Essentially this is skb_protocol(skb, true)
+	 * And we get MAC len.
+	 */
+	type = __vlan_get_protocol(skb, skb->protocol, &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:
+		nhlen = 0;
+	}
+	/* For ETH_P_IPV6/ETH_P_IP we make sure to pull
+	 * a base network header in skb->head.
+	 */
+	if (!pskb_may_pull(skb, maclen + nhlen))
+		return false;
+
+	skb_set_network_header(skb, maclen);
+	return true;
+}
+
 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

Jakub Kicinski

maintainer

2024-04-04 17:00 UTC #1 lore raw

Quoted 9 lines · On Thu, 4 Apr 2024 13:11:26 +0000 Eric Dumazet wrote:

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 8 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 mac validation.
Use this in geneve for the moment, I suspect we need to adopt this more broadly.

Something is cause the ttl test do break:

# │ geneve │ 4 │ 4 │ inherit 0x3c │ inherit 8 │ false │./l2_tos_ttl_inherit.sh: line 350: printf: 0xeaECT0: invalid hex number
ok 1 selftests: net: l2_tos_ttl_inherit.sh # SKIP

https://netdev-3.bots.linux.dev/vmksft-net-dbg/results/537382/6-l2-tos-ttl-inherit-sh/stdout

Is is possibly this change?

Eric Dumazet

author

2024-04-04 17:09 UTC #2 lore raw

Quoted 20 lines · Jakub Kicinski

Quoted 9 lines · On Thu, 4 Apr 2024 13:11:26 +0000 Eric Dumazet wrote:
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 8 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 mac validation.
Use this in geneve for the moment, I suspect we need to adopt this more broadly.

Quoted 8 lines

Something is cause the ttl test do break:
# │ geneve │ 4 │ 4 │ inherit 0x3c │ inherit 8 │ false │./l2_tos_ttl_inherit.sh: line 350: printf: 0xeaECT0: invalid hex number
ok 1 selftests: net: l2_tos_ttl_inherit.sh # SKIP
https://netdev-3.bots.linux.dev/vmksft-net-dbg/results/537382/6-l2-tos-ttl-inherit-sh/stdout
Is is possibly this change?

Let me check this, thanks !

Eric Dumazet

author

2024-04-04 17:47 UTC #3 lore raw

Quoted 24 lines · Eric Dumazet

Quoted 22 lines · Jakub Kicinski
Quoted 9 lines · On Thu, 4 Apr 2024 13:11:26 +0000 Eric Dumazet wrote:
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 8 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 mac validation.
Use this in geneve for the moment, I suspect we need to adopt this more broadly.
Something is cause the ttl test do break:

Quoted 8 lines

Quoted 6 lines
# │ geneve │ 4 │ 4 │ inherit 0x3c │ inherit 8 │ false │./l2_tos_ttl_inherit.sh: line 350: printf: 0xeaECT0: invalid hex number
ok 1 selftests: net: l2_tos_ttl_inherit.sh # SKIP
https://netdev-3.bots.linux.dev/vmksft-net-dbg/results/537382/6-l2-tos-ttl-inherit-sh/stdout
Is is possibly this change?
Let me check this, thanks !

Apparently __vlan_get_protocol() depends on skb->mac_len.

This field is not usually set, unless skb_reset_mac_len() has been called.

Eric Dumazet

author

2024-04-04 18:13 UTC #4 lore raw

Quoted 31 lines · Eric Dumazet

Quoted 29 lines · Eric Dumazet
Quoted 27 lines · Jakub Kicinski
Quoted 9 lines · On Thu, 4 Apr 2024 13:11:26 +0000 Eric Dumazet wrote:
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 8 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 mac validation.
Use this in geneve for the moment, I suspect we need to adopt this more broadly.
Something is cause the ttl test do break:
# │ geneve │ 4 │ 4 │ inherit 0x3c │ inherit 8 │ false │./l2_tos_ttl_inherit.sh: line 350: printf: 0xeaECT0: invalid hex number
ok 1 selftests: net: l2_tos_ttl_inherit.sh # SKIP
https://netdev-3.bots.linux.dev/vmksft-net-dbg/results/537382/6-l2-tos-ttl-inherit-sh/stdout

Quoted 7 lines

Quoted 3 lines
Quoted 1 line
Is is possibly this change?
Let me check this, thanks !
Apparently __vlan_get_protocol() depends on skb->mac_len.
This field is not usually set, unless skb_reset_mac_len() has been called.

I will squash tomorrow in v4 this fix, unless someone disagrees.

Thanks again !

include/net/ip_tunnels.h +0 -0

index 6898d0b8306f803c57065f6e134f96dcc8517cad..ec3950be851f2d5a04972222d8cf13b0d811be6f

100644

include/net/ip_tunnels.h +4 -3

@@ -365,13 +365,14 @@ static inline bool pskb_inet_may_pull(struct sk_buff *skb)
  */
 static inline bool skb_vlan_inet_prepare(struct sk_buff *skb)
 {
-       int nhlen, maclen;
-       __be16 type;
+       int nhlen, maclen = ETH_HLEN;
+       __be16 type = skb->protocol;
 
        /* Essentially this is skb_protocol(skb, true)
         * And we get MAC len.
         */
-       type = __vlan_get_protocol(skb, skb->protocol, &maclen);
+       if (eth_type_vlan(type))
+               type = __vlan_get_protocol(skb, type, &maclen);
 
        switch (type) {
 #if IS_ENABLED(CONFIG_IPV6)