* [PATCH v2] net: missing check virtio
@ 2024-06-13 9:54 Denis Arefev
2024-06-13 14:49 ` Jiri Pirko
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Denis Arefev @ 2024-06-13 9:54 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Jason Wang, Xuan Zhuo, Eugenio Pérez, virtualization,
linux-kernel, netdev, Eric Dumazet, lvc-project
Two missing check in virtio_net_hdr_to_skb() allowed syzbot
to crash kernels again
1. After the skb_segment function the buffer may become non-linear
(nr_frags != 0), but since the SKBTX_SHARED_FRAG flag is not set anywhere
the __skb_linearize function will not be executed, then the buffer will
remain non-linear. Then the condition (offset >= skb_headlen(skb))
becomes true, which causes WARN_ON_ONCE in skb_checksum_help.
2. The struct sk_buff and struct virtio_net_hdr members must be
mathematically related.
(gso_size) must be greater than (needed) otherwise WARN_ON_ONCE.
(remainder) must be greater than (needed) otherwise WARN_ON_ONCE.
(remainder) may be 0 if division is without remainder.
offset+2 (4191) > skb_headlen() (1116)
WARNING: CPU: 1 PID: 5084 at net/core/dev.c:3303 skb_checksum_help+0x5e2/0x740 net/core/dev.c:3303
Modules linked in:
CPU: 1 PID: 5084 Comm: syz-executor336 Not tainted 6.7.0-rc3-syzkaller-00014-gdf60cee26a2e #0
Hardware name: Google Compute Engine/Google Compute Engine, BIOS Google 11/10/2023
RIP: 0010:skb_checksum_help+0x5e2/0x740 net/core/dev.c:3303
Code: 89 e8 83 e0 07 83 c0 03 38 d0 7c 08 84 d2 0f 85 52 01 00 00 44 89 e2 2b 53 74 4c 89 ee 48 c7 c7 40 57 e9 8b e8 af 8f dd f8 90 <0f> 0b 90 90 e9 87 fe ff ff e8 40 0f 6e f9 e9 4b fa ff ff 48 89 ef
RSP: 0018:ffffc90003a9f338 EFLAGS: 00010286
RAX: 0000000000000000 RBX: ffff888025125780 RCX: ffffffff814db209
RDX: ffff888015393b80 RSI: ffffffff814db216 RDI: 0000000000000001
RBP: ffff8880251257f4 R08: 0000000000000001 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000001 R12: 000000000000045c
R13: 000000000000105f R14: ffff8880251257f0 R15: 000000000000105d
FS: 0000555555c24380(0000) GS:ffff8880b9900000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 000000002000f000 CR3: 0000000023151000 CR4: 00000000003506f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
<TASK>
ip_do_fragment+0xa1b/0x18b0 net/ipv4/ip_output.c:777
ip_fragment.constprop.0+0x161/0x230 net/ipv4/ip_output.c:584
ip_finish_output_gso net/ipv4/ip_output.c:286 [inline]
__ip_finish_output net/ipv4/ip_output.c:308 [inline]
__ip_finish_output+0x49c/0x650 net/ipv4/ip_output.c:295
ip_finish_output+0x31/0x310 net/ipv4/ip_output.c:323
NF_HOOK_COND include/linux/netfilter.h:303 [inline]
ip_output+0x13b/0x2a0 net/ipv4/ip_output.c:433
dst_output include/net/dst.h:451 [inline]
ip_local_out+0xaf/0x1a0 net/ipv4/ip_output.c:129
iptunnel_xmit+0x5b4/0x9b0 net/ipv4/ip_tunnel_core.c:82
ipip6_tunnel_xmit net/ipv6/sit.c:1034 [inline]
sit_tunnel_xmit+0xed2/0x28f0 net/ipv6/sit.c:1076
__netdev_start_xmit include/linux/netdevice.h:4940 [inline]
netdev_start_xmit include/linux/netdevice.h:4954 [inline]
xmit_one net/core/dev.c:3545 [inline]
dev_hard_start_xmit+0x13d/0x6d0 net/core/dev.c:3561
__dev_queue_xmit+0x7c1/0x3d60 net/core/dev.c:4346
dev_queue_xmit include/linux/netdevice.h:3134 [inline]
packet_xmit+0x257/0x380 net/packet/af_packet.c:276
packet_snd net/packet/af_packet.c:3087 [inline]
packet_sendmsg+0x24ca/0x5240 net/packet/af_packet.c:3119
sock_sendmsg_nosec net/socket.c:730 [inline]
__sock_sendmsg+0xd5/0x180 net/socket.c:745
__sys_sendto+0x255/0x340 net/socket.c:2190
__do_sys_sendto net/socket.c:2202 [inline]
__se_sys_sendto net/socket.c:2198 [inline]
__x64_sys_sendto+0xe0/0x1b0 net/socket.c:2198
do_syscall_x64 arch/x86/entry/common.c:51 [inline]
do_syscall_64+0x40/0x110 arch/x86/entry/common.c:82
entry_SYSCALL_64_after_hwframe+0x63/0x6b
Found by Linux Verification Center (linuxtesting.org) with Syzkaller
Signed-off-by: Denis Arefev <arefev@swemel.ru>
---
V1 -> V2: incorrect type in argument 2
include/linux/virtio_net.h | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
index 4dfa9b69ca8d..d1d7825318c3 100644
--- a/include/linux/virtio_net.h
+++ b/include/linux/virtio_net.h
@@ -56,6 +56,7 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
unsigned int thlen = 0;
unsigned int p_off = 0;
unsigned int ip_proto;
+ u64 ret, remainder, gso_size;
if (hdr->gso_type != VIRTIO_NET_HDR_GSO_NONE) {
switch (hdr->gso_type & ~VIRTIO_NET_HDR_GSO_ECN) {
@@ -98,6 +99,16 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
u32 off = __virtio16_to_cpu(little_endian, hdr->csum_offset);
u32 needed = start + max_t(u32, thlen, off + sizeof(__sum16));
+ if (hdr->gso_size) {
+ gso_size = __virtio16_to_cpu(little_endian, hdr->gso_size);
+ ret = div64_u64_rem(skb->len, gso_size, &remainder);
+ if (!(ret && (hdr->gso_size > needed) &&
+ ((remainder > needed) || (remainder == 0)))) {
+ return -EINVAL;
+ }
+ skb_shinfo(skb)->tx_flags |= SKBFL_SHARED_FRAG;
+ }
+
if (!pskb_may_pull(skb, needed))
return -EINVAL;
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] net: missing check virtio
2024-06-13 9:54 [PATCH v2] net: missing check virtio Denis Arefev
@ 2024-06-13 14:49 ` Jiri Pirko
2024-06-14 10:18 ` Denis Arefev
2024-06-19 7:51 ` Michael S. Tsirkin
2024-07-03 16:40 ` Michael S. Tsirkin
2 siblings, 1 reply; 6+ messages in thread
From: Jiri Pirko @ 2024-06-13 14:49 UTC (permalink / raw)
To: Denis Arefev
Cc: Michael S. Tsirkin, Jason Wang, Xuan Zhuo, Eugenio Pérez,
virtualization, linux-kernel, netdev, Eric Dumazet, lvc-project
Thu, Jun 13, 2024 at 11:54:48AM CEST, arefev@swemel.ru wrote:
>Two missing check in virtio_net_hdr_to_skb() allowed syzbot
>to crash kernels again
>
>1. After the skb_segment function the buffer may become non-linear
>(nr_frags != 0), but since the SKBTX_SHARED_FRAG flag is not set anywhere
>the __skb_linearize function will not be executed, then the buffer will
>remain non-linear. Then the condition (offset >= skb_headlen(skb))
>becomes true, which causes WARN_ON_ONCE in skb_checksum_help.
>
>2. The struct sk_buff and struct virtio_net_hdr members must be
>mathematically related.
>(gso_size) must be greater than (needed) otherwise WARN_ON_ONCE.
>(remainder) must be greater than (needed) otherwise WARN_ON_ONCE.
>(remainder) may be 0 if division is without remainder.
>
>offset+2 (4191) > skb_headlen() (1116)
>WARNING: CPU: 1 PID: 5084 at net/core/dev.c:3303 skb_checksum_help+0x5e2/0x740 net/core/dev.c:3303
>Modules linked in:
>CPU: 1 PID: 5084 Comm: syz-executor336 Not tainted 6.7.0-rc3-syzkaller-00014-gdf60cee26a2e #0
>Hardware name: Google Compute Engine/Google Compute Engine, BIOS Google 11/10/2023
>RIP: 0010:skb_checksum_help+0x5e2/0x740 net/core/dev.c:3303
>Code: 89 e8 83 e0 07 83 c0 03 38 d0 7c 08 84 d2 0f 85 52 01 00 00 44 89 e2 2b 53 74 4c 89 ee 48 c7 c7 40 57 e9 8b e8 af 8f dd f8 90 <0f> 0b 90 90 e9 87 fe ff ff e8 40 0f 6e f9 e9 4b fa ff ff 48 89 ef
>RSP: 0018:ffffc90003a9f338 EFLAGS: 00010286
>RAX: 0000000000000000 RBX: ffff888025125780 RCX: ffffffff814db209
>RDX: ffff888015393b80 RSI: ffffffff814db216 RDI: 0000000000000001
>RBP: ffff8880251257f4 R08: 0000000000000001 R09: 0000000000000000
>R10: 0000000000000000 R11: 0000000000000001 R12: 000000000000045c
>R13: 000000000000105f R14: ffff8880251257f0 R15: 000000000000105d
>FS: 0000555555c24380(0000) GS:ffff8880b9900000(0000) knlGS:0000000000000000
>CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>CR2: 000000002000f000 CR3: 0000000023151000 CR4: 00000000003506f0
>DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>Call Trace:
> <TASK>
> ip_do_fragment+0xa1b/0x18b0 net/ipv4/ip_output.c:777
> ip_fragment.constprop.0+0x161/0x230 net/ipv4/ip_output.c:584
> ip_finish_output_gso net/ipv4/ip_output.c:286 [inline]
> __ip_finish_output net/ipv4/ip_output.c:308 [inline]
> __ip_finish_output+0x49c/0x650 net/ipv4/ip_output.c:295
> ip_finish_output+0x31/0x310 net/ipv4/ip_output.c:323
> NF_HOOK_COND include/linux/netfilter.h:303 [inline]
> ip_output+0x13b/0x2a0 net/ipv4/ip_output.c:433
> dst_output include/net/dst.h:451 [inline]
> ip_local_out+0xaf/0x1a0 net/ipv4/ip_output.c:129
> iptunnel_xmit+0x5b4/0x9b0 net/ipv4/ip_tunnel_core.c:82
> ipip6_tunnel_xmit net/ipv6/sit.c:1034 [inline]
> sit_tunnel_xmit+0xed2/0x28f0 net/ipv6/sit.c:1076
> __netdev_start_xmit include/linux/netdevice.h:4940 [inline]
> netdev_start_xmit include/linux/netdevice.h:4954 [inline]
> xmit_one net/core/dev.c:3545 [inline]
> dev_hard_start_xmit+0x13d/0x6d0 net/core/dev.c:3561
> __dev_queue_xmit+0x7c1/0x3d60 net/core/dev.c:4346
> dev_queue_xmit include/linux/netdevice.h:3134 [inline]
> packet_xmit+0x257/0x380 net/packet/af_packet.c:276
> packet_snd net/packet/af_packet.c:3087 [inline]
> packet_sendmsg+0x24ca/0x5240 net/packet/af_packet.c:3119
> sock_sendmsg_nosec net/socket.c:730 [inline]
> __sock_sendmsg+0xd5/0x180 net/socket.c:745
> __sys_sendto+0x255/0x340 net/socket.c:2190
> __do_sys_sendto net/socket.c:2202 [inline]
> __se_sys_sendto net/socket.c:2198 [inline]
> __x64_sys_sendto+0xe0/0x1b0 net/socket.c:2198
> do_syscall_x64 arch/x86/entry/common.c:51 [inline]
> do_syscall_64+0x40/0x110 arch/x86/entry/common.c:82
> entry_SYSCALL_64_after_hwframe+0x63/0x6b
>
>Found by Linux Verification Center (linuxtesting.org) with Syzkaller
>
>Signed-off-by: Denis Arefev <arefev@swemel.ru>
Could you please provide "Fixes" blaming the commit which itroduced the
bug?
>---
> V1 -> V2: incorrect type in argument 2
> include/linux/virtio_net.h | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
>diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
>index 4dfa9b69ca8d..d1d7825318c3 100644
>--- a/include/linux/virtio_net.h
>+++ b/include/linux/virtio_net.h
>@@ -56,6 +56,7 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
> unsigned int thlen = 0;
> unsigned int p_off = 0;
> unsigned int ip_proto;
>+ u64 ret, remainder, gso_size;
>
> if (hdr->gso_type != VIRTIO_NET_HDR_GSO_NONE) {
> switch (hdr->gso_type & ~VIRTIO_NET_HDR_GSO_ECN) {
>@@ -98,6 +99,16 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
> u32 off = __virtio16_to_cpu(little_endian, hdr->csum_offset);
> u32 needed = start + max_t(u32, thlen, off + sizeof(__sum16));
>
>+ if (hdr->gso_size) {
>+ gso_size = __virtio16_to_cpu(little_endian, hdr->gso_size);
>+ ret = div64_u64_rem(skb->len, gso_size, &remainder);
>+ if (!(ret && (hdr->gso_size > needed) &&
>+ ((remainder > needed) || (remainder == 0)))) {
>+ return -EINVAL;
>+ }
>+ skb_shinfo(skb)->tx_flags |= SKBFL_SHARED_FRAG;
>+ }
>+
> if (!pskb_may_pull(skb, needed))
> return -EINVAL;
>
>--
>2.25.1
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] net: missing check virtio
2024-06-13 14:49 ` Jiri Pirko
@ 2024-06-14 10:18 ` Denis Arefev
2024-06-19 0:45 ` Jakub Kicinski
0 siblings, 1 reply; 6+ messages in thread
From: Denis Arefev @ 2024-06-14 10:18 UTC (permalink / raw)
To: jiri
Cc: arefev, edumazet, eperezma, jasowang, linux-kernel, lvc-project,
mst, netdev, virtualization, xuanzhuo
Yeah, I was thinking of adding Fixes:
But this code is new, it complements what is done.
1. check (!(ret && (hdr->gso_size > needed) &&
((remainder > needed) || (remainder == 0))))
complements comit 0f6925b3e8da0
2. The setting of the SKBFL_SHARED_FRAG flag can be associated with this comit cef401de7be8c.
In the skb_checksum_help function, a check for skb_has_shared_frag has been added.
If the flag is not set, the skb buffer will remain non-linear, which is not good.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] net: missing check virtio
2024-06-14 10:18 ` Denis Arefev
@ 2024-06-19 0:45 ` Jakub Kicinski
0 siblings, 0 replies; 6+ messages in thread
From: Jakub Kicinski @ 2024-06-19 0:45 UTC (permalink / raw)
To: Denis Arefev
Cc: jiri, edumazet, eperezma, jasowang, linux-kernel, lvc-project,
mst, netdev, virtualization, xuanzhuo
On Fri, 14 Jun 2024 13:18:26 +0300 Denis Arefev wrote:
> Yeah, I was thinking of adding Fixes:
>
> But this code is new, it complements what is done.
> 1. check (!(ret && (hdr->gso_size > needed) &&
> ((remainder > needed) || (remainder == 0))))
> complements comit 0f6925b3e8da0
>
> 2. The setting of the SKBFL_SHARED_FRAG flag can be associated with this comit cef401de7be8c.
> In the skb_checksum_help function, a check for skb_has_shared_frag has been added.
> If the flag is not set, the skb buffer will remain non-linear, which is not good.
The Fixes tag indicates how far back the bug would trigger.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] net: missing check virtio
2024-06-13 9:54 [PATCH v2] net: missing check virtio Denis Arefev
2024-06-13 14:49 ` Jiri Pirko
@ 2024-06-19 7:51 ` Michael S. Tsirkin
2024-07-03 16:40 ` Michael S. Tsirkin
2 siblings, 0 replies; 6+ messages in thread
From: Michael S. Tsirkin @ 2024-06-19 7:51 UTC (permalink / raw)
To: Denis Arefev
Cc: Jason Wang, Xuan Zhuo, Eugenio Pérez, virtualization,
linux-kernel, netdev, Eric Dumazet, lvc-project
On Thu, Jun 13, 2024 at 12:54:48PM +0300, Denis Arefev wrote:
> Two missing check in virtio_net_hdr_to_skb() allowed syzbot
> to crash kernels again
>
> 1. After the skb_segment function the buffer may become non-linear
> (nr_frags != 0), but since the SKBTX_SHARED_FRAG flag is not set anywhere
> the __skb_linearize function will not be executed, then the buffer will
> remain non-linear. Then the condition (offset >= skb_headlen(skb))
> becomes true, which causes WARN_ON_ONCE in skb_checksum_help.
>
> 2. The struct sk_buff and struct virtio_net_hdr members must be
> mathematically related.
> (gso_size) must be greater than (needed) otherwise WARN_ON_ONCE.
> (remainder) must be greater than (needed) otherwise WARN_ON_ONCE.
> (remainder) may be 0 if division is without remainder.
>
> offset+2 (4191) > skb_headlen() (1116)
> WARNING: CPU: 1 PID: 5084 at net/core/dev.c:3303 skb_checksum_help+0x5e2/0x740 net/core/dev.c:3303
> Modules linked in:
> CPU: 1 PID: 5084 Comm: syz-executor336 Not tainted 6.7.0-rc3-syzkaller-00014-gdf60cee26a2e #0
> Hardware name: Google Compute Engine/Google Compute Engine, BIOS Google 11/10/2023
> RIP: 0010:skb_checksum_help+0x5e2/0x740 net/core/dev.c:3303
> Code: 89 e8 83 e0 07 83 c0 03 38 d0 7c 08 84 d2 0f 85 52 01 00 00 44 89 e2 2b 53 74 4c 89 ee 48 c7 c7 40 57 e9 8b e8 af 8f dd f8 90 <0f> 0b 90 90 e9 87 fe ff ff e8 40 0f 6e f9 e9 4b fa ff ff 48 89 ef
> RSP: 0018:ffffc90003a9f338 EFLAGS: 00010286
> RAX: 0000000000000000 RBX: ffff888025125780 RCX: ffffffff814db209
> RDX: ffff888015393b80 RSI: ffffffff814db216 RDI: 0000000000000001
> RBP: ffff8880251257f4 R08: 0000000000000001 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000001 R12: 000000000000045c
> R13: 000000000000105f R14: ffff8880251257f0 R15: 000000000000105d
> FS: 0000555555c24380(0000) GS:ffff8880b9900000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 000000002000f000 CR3: 0000000023151000 CR4: 00000000003506f0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
> <TASK>
> ip_do_fragment+0xa1b/0x18b0 net/ipv4/ip_output.c:777
> ip_fragment.constprop.0+0x161/0x230 net/ipv4/ip_output.c:584
> ip_finish_output_gso net/ipv4/ip_output.c:286 [inline]
> __ip_finish_output net/ipv4/ip_output.c:308 [inline]
> __ip_finish_output+0x49c/0x650 net/ipv4/ip_output.c:295
> ip_finish_output+0x31/0x310 net/ipv4/ip_output.c:323
> NF_HOOK_COND include/linux/netfilter.h:303 [inline]
> ip_output+0x13b/0x2a0 net/ipv4/ip_output.c:433
> dst_output include/net/dst.h:451 [inline]
> ip_local_out+0xaf/0x1a0 net/ipv4/ip_output.c:129
> iptunnel_xmit+0x5b4/0x9b0 net/ipv4/ip_tunnel_core.c:82
> ipip6_tunnel_xmit net/ipv6/sit.c:1034 [inline]
> sit_tunnel_xmit+0xed2/0x28f0 net/ipv6/sit.c:1076
> __netdev_start_xmit include/linux/netdevice.h:4940 [inline]
> netdev_start_xmit include/linux/netdevice.h:4954 [inline]
> xmit_one net/core/dev.c:3545 [inline]
> dev_hard_start_xmit+0x13d/0x6d0 net/core/dev.c:3561
> __dev_queue_xmit+0x7c1/0x3d60 net/core/dev.c:4346
> dev_queue_xmit include/linux/netdevice.h:3134 [inline]
> packet_xmit+0x257/0x380 net/packet/af_packet.c:276
> packet_snd net/packet/af_packet.c:3087 [inline]
> packet_sendmsg+0x24ca/0x5240 net/packet/af_packet.c:3119
> sock_sendmsg_nosec net/socket.c:730 [inline]
> __sock_sendmsg+0xd5/0x180 net/socket.c:745
> __sys_sendto+0x255/0x340 net/socket.c:2190
> __do_sys_sendto net/socket.c:2202 [inline]
> __se_sys_sendto net/socket.c:2198 [inline]
> __x64_sys_sendto+0xe0/0x1b0 net/socket.c:2198
> do_syscall_x64 arch/x86/entry/common.c:51 [inline]
> do_syscall_64+0x40/0x110 arch/x86/entry/common.c:82
> entry_SYSCALL_64_after_hwframe+0x63/0x6b
>
> Found by Linux Verification Center (linuxtesting.org) with Syzkaller
>
> Signed-off-by: Denis Arefev <arefev@swemel.ru>
> ---
> V1 -> V2: incorrect type in argument 2
> include/linux/virtio_net.h | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> index 4dfa9b69ca8d..d1d7825318c3 100644
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h
> @@ -56,6 +56,7 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
> unsigned int thlen = 0;
> unsigned int p_off = 0;
> unsigned int ip_proto;
> + u64 ret, remainder, gso_size;
>
> if (hdr->gso_type != VIRTIO_NET_HDR_GSO_NONE) {
> switch (hdr->gso_type & ~VIRTIO_NET_HDR_GSO_ECN) {
> @@ -98,6 +99,16 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
> u32 off = __virtio16_to_cpu(little_endian, hdr->csum_offset);
> u32 needed = start + max_t(u32, thlen, off + sizeof(__sum16));
>
> + if (hdr->gso_size) {
> + gso_size = __virtio16_to_cpu(little_endian, hdr->gso_size);
> + ret = div64_u64_rem(skb->len, gso_size, &remainder);
> + if (!(ret && (hdr->gso_size > needed) &&
> + ((remainder > needed) || (remainder == 0)))) {
> + return -EINVAL;
> + }
Could you please add some comments explaining the logic here?
And do we really need u64 math? All the values here are 32 bit ATM ...
> + skb_shinfo(skb)->tx_flags |= SKBFL_SHARED_FRAG;
> + }
> +
> if (!pskb_may_pull(skb, needed))
> return -EINVAL;
>
> --
> 2.25.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] net: missing check virtio
2024-06-13 9:54 [PATCH v2] net: missing check virtio Denis Arefev
2024-06-13 14:49 ` Jiri Pirko
2024-06-19 7:51 ` Michael S. Tsirkin
@ 2024-07-03 16:40 ` Michael S. Tsirkin
2 siblings, 0 replies; 6+ messages in thread
From: Michael S. Tsirkin @ 2024-07-03 16:40 UTC (permalink / raw)
To: Denis Arefev
Cc: Jason Wang, Xuan Zhuo, Eugenio Pérez, virtualization,
linux-kernel, netdev, Eric Dumazet, lvc-project
On Thu, Jun 13, 2024 at 12:54:48PM +0300, Denis Arefev wrote:
> Two missing check in virtio_net_hdr_to_skb() allowed syzbot
> to crash kernels again
>
> 1. After the skb_segment function the buffer may become non-linear
> (nr_frags != 0), but since the SKBTX_SHARED_FRAG flag is not set anywhere
> the __skb_linearize function will not be executed, then the buffer will
> remain non-linear. Then the condition (offset >= skb_headlen(skb))
> becomes true, which causes WARN_ON_ONCE in skb_checksum_help.
>
> 2. The struct sk_buff and struct virtio_net_hdr members must be
> mathematically related.
> (gso_size) must be greater than (needed) otherwise WARN_ON_ONCE.
> (remainder) must be greater than (needed) otherwise WARN_ON_ONCE.
> (remainder) may be 0 if division is without remainder.
>
> offset+2 (4191) > skb_headlen() (1116)
> WARNING: CPU: 1 PID: 5084 at net/core/dev.c:3303 skb_checksum_help+0x5e2/0x740 net/core/dev.c:3303
> Modules linked in:
> CPU: 1 PID: 5084 Comm: syz-executor336 Not tainted 6.7.0-rc3-syzkaller-00014-gdf60cee26a2e #0
> Hardware name: Google Compute Engine/Google Compute Engine, BIOS Google 11/10/2023
> RIP: 0010:skb_checksum_help+0x5e2/0x740 net/core/dev.c:3303
> Code: 89 e8 83 e0 07 83 c0 03 38 d0 7c 08 84 d2 0f 85 52 01 00 00 44 89 e2 2b 53 74 4c 89 ee 48 c7 c7 40 57 e9 8b e8 af 8f dd f8 90 <0f> 0b 90 90 e9 87 fe ff ff e8 40 0f 6e f9 e9 4b fa ff ff 48 89 ef
> RSP: 0018:ffffc90003a9f338 EFLAGS: 00010286
> RAX: 0000000000000000 RBX: ffff888025125780 RCX: ffffffff814db209
> RDX: ffff888015393b80 RSI: ffffffff814db216 RDI: 0000000000000001
> RBP: ffff8880251257f4 R08: 0000000000000001 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000001 R12: 000000000000045c
> R13: 000000000000105f R14: ffff8880251257f0 R15: 000000000000105d
> FS: 0000555555c24380(0000) GS:ffff8880b9900000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 000000002000f000 CR3: 0000000023151000 CR4: 00000000003506f0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
> <TASK>
> ip_do_fragment+0xa1b/0x18b0 net/ipv4/ip_output.c:777
> ip_fragment.constprop.0+0x161/0x230 net/ipv4/ip_output.c:584
> ip_finish_output_gso net/ipv4/ip_output.c:286 [inline]
> __ip_finish_output net/ipv4/ip_output.c:308 [inline]
> __ip_finish_output+0x49c/0x650 net/ipv4/ip_output.c:295
> ip_finish_output+0x31/0x310 net/ipv4/ip_output.c:323
> NF_HOOK_COND include/linux/netfilter.h:303 [inline]
> ip_output+0x13b/0x2a0 net/ipv4/ip_output.c:433
> dst_output include/net/dst.h:451 [inline]
> ip_local_out+0xaf/0x1a0 net/ipv4/ip_output.c:129
> iptunnel_xmit+0x5b4/0x9b0 net/ipv4/ip_tunnel_core.c:82
> ipip6_tunnel_xmit net/ipv6/sit.c:1034 [inline]
> sit_tunnel_xmit+0xed2/0x28f0 net/ipv6/sit.c:1076
> __netdev_start_xmit include/linux/netdevice.h:4940 [inline]
> netdev_start_xmit include/linux/netdevice.h:4954 [inline]
> xmit_one net/core/dev.c:3545 [inline]
> dev_hard_start_xmit+0x13d/0x6d0 net/core/dev.c:3561
> __dev_queue_xmit+0x7c1/0x3d60 net/core/dev.c:4346
> dev_queue_xmit include/linux/netdevice.h:3134 [inline]
> packet_xmit+0x257/0x380 net/packet/af_packet.c:276
> packet_snd net/packet/af_packet.c:3087 [inline]
> packet_sendmsg+0x24ca/0x5240 net/packet/af_packet.c:3119
> sock_sendmsg_nosec net/socket.c:730 [inline]
> __sock_sendmsg+0xd5/0x180 net/socket.c:745
> __sys_sendto+0x255/0x340 net/socket.c:2190
> __do_sys_sendto net/socket.c:2202 [inline]
> __se_sys_sendto net/socket.c:2198 [inline]
> __x64_sys_sendto+0xe0/0x1b0 net/socket.c:2198
> do_syscall_x64 arch/x86/entry/common.c:51 [inline]
> do_syscall_64+0x40/0x110 arch/x86/entry/common.c:82
> entry_SYSCALL_64_after_hwframe+0x63/0x6b
>
> Found by Linux Verification Center (linuxtesting.org) with Syzkaller
>
> Signed-off-by: Denis Arefev <arefev@swemel.ru>
I suspect it's this one:
Fixes: 0f6925b3e8da ("virtio_net: Do not pull payload in skb->head")
Though if you can test kernel before that to make sure, that
would be nice.
I'm inclined to merge, crashing syzkaller is not nice.
Acked-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> V1 -> V2: incorrect type in argument 2
> include/linux/virtio_net.h | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> index 4dfa9b69ca8d..d1d7825318c3 100644
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h
> @@ -56,6 +56,7 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
> unsigned int thlen = 0;
> unsigned int p_off = 0;
> unsigned int ip_proto;
> + u64 ret, remainder, gso_size;
>
> if (hdr->gso_type != VIRTIO_NET_HDR_GSO_NONE) {
> switch (hdr->gso_type & ~VIRTIO_NET_HDR_GSO_ECN) {
> @@ -98,6 +99,16 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
> u32 off = __virtio16_to_cpu(little_endian, hdr->csum_offset);
> u32 needed = start + max_t(u32, thlen, off + sizeof(__sum16));
>
> + if (hdr->gso_size) {
> + gso_size = __virtio16_to_cpu(little_endian, hdr->gso_size);
> + ret = div64_u64_rem(skb->len, gso_size, &remainder);
> + if (!(ret && (hdr->gso_size > needed) &&
> + ((remainder > needed) || (remainder == 0)))) {
> + return -EINVAL;
> + }
> + skb_shinfo(skb)->tx_flags |= SKBFL_SHARED_FRAG;
> + }
> +
> if (!pskb_may_pull(skb, needed))
> return -EINVAL;
>
> --
> 2.25.1
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-07-03 16:40 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-06-13 9:54 [PATCH v2] net: missing check virtio Denis Arefev
2024-06-13 14:49 ` Jiri Pirko
2024-06-14 10:18 ` Denis Arefev
2024-06-19 0:45 ` Jakub Kicinski
2024-06-19 7:51 ` Michael S. Tsirkin
2024-07-03 16:40 ` Michael S. Tsirkin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).