[PATCH net] net: add more sanity check in virtio_net_hdr_to_skb()

[PATCH net] net: add more sanity check in virtio_net_hdr_to_skb()

[Eric Dumazet]

syzbot/KMSAN reports access to uninitialized data from gso_features_check() [1]

The repro use af_packet, injecting a gso packet and hdrlen == 0.

We could fix the issue making gso_features_check() more careful
while dealing with NETIF_F_TSO_MANGLEID in fast path.

Or we can make sure virtio_net_hdr_to_skb() pulls minimal network and
transport headers as intended.

Note that for GSO packets coming from untrusted sources, SKB_GSO_DODGY
bit forces a proper header validation (and pull) before the packet can
hit any device ndo_start_xmit(), thus we do not need a precise disection
at virtio_net_hdr_to_skb() stage.

[1]
BUG: KMSAN: uninit-value in skb_gso_segment include/net/gso.h:83 [inline]
BUG: KMSAN: uninit-value in validate_xmit_skb+0x10f2/0x1930 net/core/dev.c:3629
 skb_gso_segment include/net/gso.h:83 [inline]
 validate_xmit_skb+0x10f2/0x1930 net/core/dev.c:3629
 __dev_queue_xmit+0x1eac/0x5130 net/core/dev.c:4341
 dev_queue_xmit include/linux/netdevice.h:3134 [inline]
 packet_xmit+0x9c/0x6b0 net/packet/af_packet.c:276
 packet_snd net/packet/af_packet.c:3087 [inline]
 packet_sendmsg+0x8b1d/0x9f30 net/packet/af_packet.c:3119
 sock_sendmsg_nosec net/socket.c:730 [inline]
 __sock_sendmsg net/socket.c:745 [inline]
 ____sys_sendmsg+0x9c2/0xd60 net/socket.c:2584
 ___sys_sendmsg+0x28d/0x3c0 net/socket.c:2638
 __sys_sendmsg net/socket.c:2667 [inline]
 __do_sys_sendmsg net/socket.c:2676 [inline]
 __se_sys_sendmsg net/socket.c:2674 [inline]
 __x64_sys_sendmsg+0x307/0x490 net/socket.c:2674
 do_syscall_x64 arch/x86/entry/common.c:52 [inline]
 do_syscall_64+0x44/0x110 arch/x86/entry/common.c:83
 entry_SYSCALL_64_after_hwframe+0x63/0x6b

Uninit was created at:
 slab_post_alloc_hook+0x129/0xa70 mm/slab.h:768
 slab_alloc_node mm/slub.c:3478 [inline]
 kmem_cache_alloc_node+0x5e9/0xb10 mm/slub.c:3523
 kmalloc_reserve+0x13d/0x4a0 net/core/skbuff.c:560
 __alloc_skb+0x318/0x740 net/core/skbuff.c:651
 alloc_skb include/linux/skbuff.h:1286 [inline]
 alloc_skb_with_frags+0xc8/0xbd0 net/core/skbuff.c:6334
 sock_alloc_send_pskb+0xa80/0xbf0 net/core/sock.c:2780
 packet_alloc_skb net/packet/af_packet.c:2936 [inline]
 packet_snd net/packet/af_packet.c:3030 [inline]
 packet_sendmsg+0x70e8/0x9f30 net/packet/af_packet.c:3119
 sock_sendmsg_nosec net/socket.c:730 [inline]
 __sock_sendmsg net/socket.c:745 [inline]
 ____sys_sendmsg+0x9c2/0xd60 net/socket.c:2584
 ___sys_sendmsg+0x28d/0x3c0 net/socket.c:2638
 __sys_sendmsg net/socket.c:2667 [inline]
 __do_sys_sendmsg net/socket.c:2676 [inline]
 __se_sys_sendmsg net/socket.c:2674 [inline]
 __x64_sys_sendmsg+0x307/0x490 net/socket.c:2674
 do_syscall_x64 arch/x86/entry/common.c:52 [inline]
 do_syscall_64+0x44/0x110 arch/x86/entry/common.c:83
 entry_SYSCALL_64_after_hwframe+0x63/0x6b

CPU: 0 PID: 5025 Comm: syz-executor279 Not tainted 6.7.0-rc7-syzkaller-00003-gfbafc3e621c3 #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 11/17/2023

Reported-by: syzbot+7f4d0ea3df4d4fa9a65f@syzkaller.appspotmail.com
Link: https://lore.kernel.org/netdev/0000000000005abd7b060eb160cd@google.com/
Fixes: 9274124f023b ("net: stricter validation of untrusted gso packets")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Willem de Bruijn <willemb@google.com>
---
 include/linux/virtio_net.h | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
index 27cc1d4643219a44c01a2404124cd45ef46f7f3d..4dfa9b69ca8d95d43e44831bc166eadbe5715d3c 100644
--- a/include/linux/virtio_net.h
+++ b/include/linux/virtio_net.h
@@ -3,6 +3,8 @@
 #define _LINUX_VIRTIO_NET_H
 
 #include <linux/if_vlan.h>
+#include <linux/ip.h>
+#include <linux/ipv6.h>
 #include <linux/udp.h>
 #include <uapi/linux/tcp.h>
 #include <uapi/linux/virtio_net.h>
@@ -49,6 +51,7 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
 					const struct virtio_net_hdr *hdr,
 					bool little_endian)
 {
+	unsigned int nh_min_len = sizeof(struct iphdr);
 	unsigned int gso_type = 0;
 	unsigned int thlen = 0;
 	unsigned int p_off = 0;
@@ -65,6 +68,7 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
 			gso_type = SKB_GSO_TCPV6;
 			ip_proto = IPPROTO_TCP;
 			thlen = sizeof(struct tcphdr);
+			nh_min_len = sizeof(struct ipv6hdr);
 			break;
 		case VIRTIO_NET_HDR_GSO_UDP:
 			gso_type = SKB_GSO_UDP;
@@ -100,7 +104,8 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
 		if (!skb_partial_csum_set(skb, start, off))
 			return -EINVAL;
 
-		p_off = skb_transport_offset(skb) + thlen;
+		nh_min_len = max_t(u32, nh_min_len, skb_transport_offset(skb));
+		p_off = nh_min_len + thlen;
 		if (!pskb_may_pull(skb, p_off))
 			return -EINVAL;
 	} else {
@@ -140,7 +145,7 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
 
 			skb_set_transport_header(skb, keys.control.thoff);
 		} else if (gso_type) {
-			p_off = thlen;
+			p_off = nh_min_len + thlen;
 			if (!pskb_may_pull(skb, p_off))
 				return -EINVAL;
 		}
-- 
2.43.0.275.g3460e3d667-goog

Re: [PATCH net] net: add more sanity check in virtio_net_hdr_to_skb()

[Jiri Pirko]

Fri, Jan 12, 2024 at 01:28:16PM CET, edumazet@google.com wrote:
>syzbot/KMSAN reports access to uninitialized data from gso_features_check() [1]
>
>The repro use af_packet, injecting a gso packet and hdrlen == 0.
>
>We could fix the issue making gso_features_check() more careful
>while dealing with NETIF_F_TSO_MANGLEID in fast path.
>
>Or we can make sure virtio_net_hdr_to_skb() pulls minimal network and
>transport headers as intended.

You describe "either or", but don't really say what to do. Bit
confusing :/


>
>Note that for GSO packets coming from untrusted sources, SKB_GSO_DODGY
>bit forces a proper header validation (and pull) before the packet can
>hit any device ndo_start_xmit(), thus we do not need a precise disection
>at virtio_net_hdr_to_skb() stage.
>
>[1]
>BUG: KMSAN: uninit-value in skb_gso_segment include/net/gso.h:83 [inline]
>BUG: KMSAN: uninit-value in validate_xmit_skb+0x10f2/0x1930 net/core/dev.c:3629
> skb_gso_segment include/net/gso.h:83 [inline]
> validate_xmit_skb+0x10f2/0x1930 net/core/dev.c:3629
> __dev_queue_xmit+0x1eac/0x5130 net/core/dev.c:4341
> dev_queue_xmit include/linux/netdevice.h:3134 [inline]
> packet_xmit+0x9c/0x6b0 net/packet/af_packet.c:276
> packet_snd net/packet/af_packet.c:3087 [inline]
> packet_sendmsg+0x8b1d/0x9f30 net/packet/af_packet.c:3119
> sock_sendmsg_nosec net/socket.c:730 [inline]
> __sock_sendmsg net/socket.c:745 [inline]
> ____sys_sendmsg+0x9c2/0xd60 net/socket.c:2584
> ___sys_sendmsg+0x28d/0x3c0 net/socket.c:2638
> __sys_sendmsg net/socket.c:2667 [inline]
> __do_sys_sendmsg net/socket.c:2676 [inline]
> __se_sys_sendmsg net/socket.c:2674 [inline]
> __x64_sys_sendmsg+0x307/0x490 net/socket.c:2674
> do_syscall_x64 arch/x86/entry/common.c:52 [inline]
> do_syscall_64+0x44/0x110 arch/x86/entry/common.c:83
> entry_SYSCALL_64_after_hwframe+0x63/0x6b
>
>Uninit was created at:
> slab_post_alloc_hook+0x129/0xa70 mm/slab.h:768
> slab_alloc_node mm/slub.c:3478 [inline]
> kmem_cache_alloc_node+0x5e9/0xb10 mm/slub.c:3523
> kmalloc_reserve+0x13d/0x4a0 net/core/skbuff.c:560
> __alloc_skb+0x318/0x740 net/core/skbuff.c:651
> alloc_skb include/linux/skbuff.h:1286 [inline]
> alloc_skb_with_frags+0xc8/0xbd0 net/core/skbuff.c:6334
> sock_alloc_send_pskb+0xa80/0xbf0 net/core/sock.c:2780
> packet_alloc_skb net/packet/af_packet.c:2936 [inline]
> packet_snd net/packet/af_packet.c:3030 [inline]
> packet_sendmsg+0x70e8/0x9f30 net/packet/af_packet.c:3119
> sock_sendmsg_nosec net/socket.c:730 [inline]
> __sock_sendmsg net/socket.c:745 [inline]
> ____sys_sendmsg+0x9c2/0xd60 net/socket.c:2584
> ___sys_sendmsg+0x28d/0x3c0 net/socket.c:2638
> __sys_sendmsg net/socket.c:2667 [inline]
> __do_sys_sendmsg net/socket.c:2676 [inline]
> __se_sys_sendmsg net/socket.c:2674 [inline]
> __x64_sys_sendmsg+0x307/0x490 net/socket.c:2674
> do_syscall_x64 arch/x86/entry/common.c:52 [inline]
> do_syscall_64+0x44/0x110 arch/x86/entry/common.c:83
> entry_SYSCALL_64_after_hwframe+0x63/0x6b
>
>CPU: 0 PID: 5025 Comm: syz-executor279 Not tainted 6.7.0-rc7-syzkaller-00003-gfbafc3e621c3 #0
>Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 11/17/2023
>
>Reported-by: syzbot+7f4d0ea3df4d4fa9a65f@syzkaller.appspotmail.com
>Link: https://lore.kernel.org/netdev/0000000000005abd7b060eb160cd@google.com/
>Fixes: 9274124f023b ("net: stricter validation of untrusted gso packets")
>Signed-off-by: Eric Dumazet <edumazet@google.com>
>Cc: Willem de Bruijn <willemb@google.com>
>---
> include/linux/virtio_net.h | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
>diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
>index 27cc1d4643219a44c01a2404124cd45ef46f7f3d..4dfa9b69ca8d95d43e44831bc166eadbe5715d3c 100644
>--- a/include/linux/virtio_net.h
>+++ b/include/linux/virtio_net.h
>@@ -3,6 +3,8 @@
> #define _LINUX_VIRTIO_NET_H
> 
> #include <linux/if_vlan.h>
>+#include <linux/ip.h>
>+#include <linux/ipv6.h>
> #include <linux/udp.h>
> #include <uapi/linux/tcp.h>
> #include <uapi/linux/virtio_net.h>
>@@ -49,6 +51,7 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
> 					const struct virtio_net_hdr *hdr,
> 					bool little_endian)
> {
>+	unsigned int nh_min_len = sizeof(struct iphdr);
> 	unsigned int gso_type = 0;
> 	unsigned int thlen = 0;
> 	unsigned int p_off = 0;
>@@ -65,6 +68,7 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
> 			gso_type = SKB_GSO_TCPV6;
> 			ip_proto = IPPROTO_TCP;
> 			thlen = sizeof(struct tcphdr);
>+			nh_min_len = sizeof(struct ipv6hdr);
> 			break;
> 		case VIRTIO_NET_HDR_GSO_UDP:
> 			gso_type = SKB_GSO_UDP;
>@@ -100,7 +104,8 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
> 		if (!skb_partial_csum_set(skb, start, off))
> 			return -EINVAL;
> 
>-		p_off = skb_transport_offset(skb) + thlen;
>+		nh_min_len = max_t(u32, nh_min_len, skb_transport_offset(skb));
>+		p_off = nh_min_len + thlen;
> 		if (!pskb_may_pull(skb, p_off))
> 			return -EINVAL;
> 	} else {
>@@ -140,7 +145,7 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
> 
> 			skb_set_transport_header(skb, keys.control.thoff);
> 		} else if (gso_type) {
>-			p_off = thlen;
>+			p_off = nh_min_len + thlen;
> 			if (!pskb_may_pull(skb, p_off))
> 				return -EINVAL;
> 		}
>-- 
>2.43.0.275.g3460e3d667-goog
>
>

Re: [PATCH net] net: add more sanity check in virtio_net_hdr_to_skb()

[Eric Dumazet]

On Fri, Jan 12, 2024 at 2:00 PM Jiri Pirko <jiri@resnulli.us> wrote:
>
> Fri, Jan 12, 2024 at 01:28:16PM CET, edumazet@google.com wrote:
> >syzbot/KMSAN reports access to uninitialized data from gso_features_check() [1]
> >
> >The repro use af_packet, injecting a gso packet and hdrlen == 0.
> >
> >We could fix the issue making gso_features_check() more careful
> >while dealing with NETIF_F_TSO_MANGLEID in fast path.
> >
> >Or we can make sure virtio_net_hdr_to_skb() pulls minimal network and
> >transport headers as intended.
>
> You describe "either or", but don't really say what to do. Bit
> confusing :/

Not sure I understand your point?

 Patch title is " net: add more sanity check in virtio_net_hdr_to_skb() ",
and the change is implementing that option.

I am saying I prefer not touching gso_features_check(), even if we
could just do this.

Had I been silent about that option, I am sure some reviewers would
have raised the question,
given the stack trace ?

Apparently you are saying these kinds of things should not be ever mentioned,
because of some "imperative mood" request that you often raise with my patches.

I have not written a novel, only one sentence, admittedly not written
in perfect English.

Re: [PATCH net] net: add more sanity check in virtio_net_hdr_to_skb()

[Jiri Pirko]

Fri, Jan 12, 2024 at 02:11:43PM CET, edumazet@google.com wrote:
>On Fri, Jan 12, 2024 at 2:00 PM Jiri Pirko <jiri@resnulli.us> wrote:
>>
>> Fri, Jan 12, 2024 at 01:28:16PM CET, edumazet@google.com wrote:
>> >syzbot/KMSAN reports access to uninitialized data from gso_features_check() [1]
>> >
>> >The repro use af_packet, injecting a gso packet and hdrlen == 0.
>> >
>> >We could fix the issue making gso_features_check() more careful
>> >while dealing with NETIF_F_TSO_MANGLEID in fast path.
>> >
>> >Or we can make sure virtio_net_hdr_to_skb() pulls minimal network and
>> >transport headers as intended.
>>
>> You describe "either or", but don't really say what to do. Bit
>> confusing :/
>
>Not sure I understand your point?
>
> Patch title is " net: add more sanity check in virtio_net_hdr_to_skb() ",
>and the change is implementing that option.

Right. Patch desctiption does not clearly say it, that's what made me
wonder.


>
>I am saying I prefer not touching gso_features_check(), even if we
>could just do this.
>
>Had I been silent about that option, I am sure some reviewers would
>have raised the question,
>given the stack trace ?
>
>Apparently you are saying these kinds of things should not be ever mentioned,
>because of some "imperative mood" request that you often raise with my patches.

I woudn't dare :) I just find it hard to understand from time to time,
sorry about that.


>
>I have not written a novel, only one sentence, admittedly not written
>in perfect English.

Re: [PATCH net] net: add more sanity check in virtio_net_hdr_to_skb()

[Willem de Bruijn]

Eric Dumazet wrote:
> syzbot/KMSAN reports access to uninitialized data from gso_features_check() [1]
> 
> The repro use af_packet, injecting a gso packet and hdrlen == 0.
> 
> We could fix the issue making gso_features_check() more careful
> while dealing with NETIF_F_TSO_MANGLEID in fast path.
> 
> Or we can make sure virtio_net_hdr_to_skb() pulls minimal network and
> transport headers as intended.
> 
> Note that for GSO packets coming from untrusted sources, SKB_GSO_DODGY
> bit forces a proper header validation (and pull) before the packet can
> hit any device ndo_start_xmit(), thus we do not need a precise disection
> at virtio_net_hdr_to_skb() stage.
> 
> [1]
> BUG: KMSAN: uninit-value in skb_gso_segment include/net/gso.h:83 [inline]
> BUG: KMSAN: uninit-value in validate_xmit_skb+0x10f2/0x1930 net/core/dev.c:3629
>  skb_gso_segment include/net/gso.h:83 [inline]
>  validate_xmit_skb+0x10f2/0x1930 net/core/dev.c:3629
>  __dev_queue_xmit+0x1eac/0x5130 net/core/dev.c:4341
>  dev_queue_xmit include/linux/netdevice.h:3134 [inline]
>  packet_xmit+0x9c/0x6b0 net/packet/af_packet.c:276
>  packet_snd net/packet/af_packet.c:3087 [inline]
>  packet_sendmsg+0x8b1d/0x9f30 net/packet/af_packet.c:3119
>  sock_sendmsg_nosec net/socket.c:730 [inline]
>  __sock_sendmsg net/socket.c:745 [inline]
>  ____sys_sendmsg+0x9c2/0xd60 net/socket.c:2584
>  ___sys_sendmsg+0x28d/0x3c0 net/socket.c:2638
>  __sys_sendmsg net/socket.c:2667 [inline]
>  __do_sys_sendmsg net/socket.c:2676 [inline]
>  __se_sys_sendmsg net/socket.c:2674 [inline]
>  __x64_sys_sendmsg+0x307/0x490 net/socket.c:2674
>  do_syscall_x64 arch/x86/entry/common.c:52 [inline]
>  do_syscall_64+0x44/0x110 arch/x86/entry/common.c:83
>  entry_SYSCALL_64_after_hwframe+0x63/0x6b
> 
> Uninit was created at:
>  slab_post_alloc_hook+0x129/0xa70 mm/slab.h:768
>  slab_alloc_node mm/slub.c:3478 [inline]
>  kmem_cache_alloc_node+0x5e9/0xb10 mm/slub.c:3523
>  kmalloc_reserve+0x13d/0x4a0 net/core/skbuff.c:560
>  __alloc_skb+0x318/0x740 net/core/skbuff.c:651
>  alloc_skb include/linux/skbuff.h:1286 [inline]
>  alloc_skb_with_frags+0xc8/0xbd0 net/core/skbuff.c:6334
>  sock_alloc_send_pskb+0xa80/0xbf0 net/core/sock.c:2780
>  packet_alloc_skb net/packet/af_packet.c:2936 [inline]
>  packet_snd net/packet/af_packet.c:3030 [inline]
>  packet_sendmsg+0x70e8/0x9f30 net/packet/af_packet.c:3119
>  sock_sendmsg_nosec net/socket.c:730 [inline]
>  __sock_sendmsg net/socket.c:745 [inline]
>  ____sys_sendmsg+0x9c2/0xd60 net/socket.c:2584
>  ___sys_sendmsg+0x28d/0x3c0 net/socket.c:2638
>  __sys_sendmsg net/socket.c:2667 [inline]
>  __do_sys_sendmsg net/socket.c:2676 [inline]
>  __se_sys_sendmsg net/socket.c:2674 [inline]
>  __x64_sys_sendmsg+0x307/0x490 net/socket.c:2674
>  do_syscall_x64 arch/x86/entry/common.c:52 [inline]
>  do_syscall_64+0x44/0x110 arch/x86/entry/common.c:83
>  entry_SYSCALL_64_after_hwframe+0x63/0x6b
> 
> CPU: 0 PID: 5025 Comm: syz-executor279 Not tainted 6.7.0-rc7-syzkaller-00003-gfbafc3e621c3 #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 11/17/2023
> 
> Reported-by: syzbot+7f4d0ea3df4d4fa9a65f@syzkaller.appspotmail.com
> Link: https://lore.kernel.org/netdev/0000000000005abd7b060eb160cd@google.com/
> Fixes: 9274124f023b ("net: stricter validation of untrusted gso packets")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Willem de Bruijn <willemb@google.com>

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

Re: [PATCH net] net: add more sanity check in virtio_net_hdr_to_skb()

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by David S. Miller <davem@davemloft.net>:

On Fri, 12 Jan 2024 12:28:16 +0000 you wrote:
> syzbot/KMSAN reports access to uninitialized data from gso_features_check() [1]
> 
> The repro use af_packet, injecting a gso packet and hdrlen == 0.
> 
> We could fix the issue making gso_features_check() more careful
> while dealing with NETIF_F_TSO_MANGLEID in fast path.
> 
> [...]

Here is the summary with links:
  - [net] net: add more sanity check in virtio_net_hdr_to_skb()
    https://git.kernel.org/netdev/net/c/9181d6f8a2bb

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html