* KMSAN: uninit-value in _copy_to_iter (2) @ 2018-04-23 16:56 syzbot 2018-04-25 19:19 ` syzbot 2018-04-27 15:45 ` [PATCH net] vhost: Use kzalloc() to allocate vhost_msg_node Kevin Easton 0 siblings, 2 replies; 30+ messages in thread From: syzbot @ 2018-04-23 16:56 UTC (permalink / raw) To: avagin, davem, dingtianhong, edumazet, elena.reshetova, linux-kernel, matthew, mingo, netdev, pabeni, syzkaller-bugs, viro, willemb Hello, syzbot hit the following crash on https://github.com/google/kmsan.git/master commit d2d741e5d1898dfde1a75ea3d29a9a3e2edf0617 (Sun Apr 22 15:05:22 2018 +0000) kmsan: add initialization for shmem pages syzbot dashboard link: https://syzkaller.appspot.com/bug?extid=87cfa083e727a224754b Unfortunately, I don't have any reproducer for this crash yet. Raw console output: https://syzkaller.appspot.com/x/log.txt?id=6616554548494336 Kernel config: https://syzkaller.appspot.com/x/.config?id=328654897048964367 compiler: clang version 7.0.0 (trunk 329391) IMPORTANT: if you fix the bug, please add the following tag to the commit: Reported-by: syzbot+87cfa083e727a224754b@syzkaller.appspotmail.com It will help syzbot understand when the bug is fixed. See footer for details. If you forward the report, please keep this part and the footer. ================================================================== BUG: KMSAN: uninit-value in copyout lib/iov_iter.c:140 [inline] BUG: KMSAN: uninit-value in _copy_to_iter+0x1bb3/0x28f0 lib/iov_iter.c:571 CPU: 0 PID: 7670 Comm: syz-executor7 Not tainted 4.16.0+ #86 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 Call Trace: __dump_stack lib/dump_stack.c:17 [inline] dump_stack+0x185/0x1d0 lib/dump_stack.c:53 kmsan_report+0x142/0x240 mm/kmsan/kmsan.c:1067 kmsan_internal_check_memory+0x135/0x1e0 mm/kmsan/kmsan.c:1157 kmsan_copy_to_user+0x69/0x160 mm/kmsan/kmsan.c:1199 copyout lib/iov_iter.c:140 [inline] _copy_to_iter+0x1bb3/0x28f0 lib/iov_iter.c:571 copy_to_iter include/linux/uio.h:106 [inline] skb_copy_datagram_iter+0x443/0xf70 net/core/datagram.c:431 skb_copy_datagram_msg include/linux/skbuff.h:3264 [inline] netlink_recvmsg+0x6f1/0x1900 net/netlink/af_netlink.c:1958 sock_recvmsg_nosec net/socket.c:803 [inline] sock_recvmsg+0x1d0/0x230 net/socket.c:810 ___sys_recvmsg+0x3fb/0x810 net/socket.c:2205 __sys_recvmmsg+0x54e/0xdb0 net/socket.c:2313 SYSC_recvmmsg+0x29b/0x3e0 net/socket.c:2394 SyS_recvmmsg+0x76/0xa0 net/socket.c:2378 do_syscall_64+0x309/0x430 arch/x86/entry/common.c:287 entry_SYSCALL_64_after_hwframe+0x3d/0xa2 RIP: 0033:0x455389 RSP: 002b:00007f0281d3dc68 EFLAGS: 00000246 ORIG_RAX: 000000000000012b RAX: ffffffffffffffda RBX: 00007f0281d3e6d4 RCX: 0000000000455389 RDX: 0000000000000003 RSI: 0000000020001f80 RDI: 0000000000000014 RBP: 000000000072bea0 R08: 0000000020002040 R09: 0000000000000000 R10: 0000000000000000 R11: 0000000000000246 R12: 00000000ffffffff R13: 000000000000049e R14: 00000000006f9f70 R15: 0000000000000000 Uninit was stored to memory at: kmsan_save_stack_with_flags mm/kmsan/kmsan.c:278 [inline] kmsan_save_stack mm/kmsan/kmsan.c:293 [inline] kmsan_internal_chain_origin+0x12b/0x210 mm/kmsan/kmsan.c:684 kmsan_memcpy_origins+0x11d/0x170 mm/kmsan/kmsan.c:526 __msan_memcpy+0x109/0x160 mm/kmsan/kmsan_instr.c:477 __nla_put lib/nlattr.c:569 [inline] nla_put+0x276/0x340 lib/nlattr.c:627 copy_to_user_policy_type net/xfrm/xfrm_user.c:1678 [inline] build_acquire net/xfrm/xfrm_user.c:2850 [inline] xfrm_send_acquire+0x1068/0x1690 net/xfrm/xfrm_user.c:2873 km_query net/xfrm/xfrm_state.c:1953 [inline] xfrm_state_find+0x3ad8/0x4f40 net/xfrm/xfrm_state.c:1021 xfrm_tmpl_resolve_one net/xfrm/xfrm_policy.c:1393 [inline] xfrm_tmpl_resolve net/xfrm/xfrm_policy.c:1437 [inline] xfrm_resolve_and_create_bundle+0xc31/0x5270 net/xfrm/xfrm_policy.c:1833 xfrm_lookup+0x606/0x39d0 net/xfrm/xfrm_policy.c:2163 xfrm_lookup_route+0xfa/0x360 net/xfrm/xfrm_policy.c:2283 ip6_dst_lookup_flow+0x221/0x270 net/ipv6/ip6_output.c:1099 ip6_datagram_dst_update+0x93a/0x1470 net/ipv6/datagram.c:91 __ip6_datagram_connect+0x14f6/0x1a20 net/ipv6/datagram.c:257 ip6_datagram_connect net/ipv6/datagram.c:280 [inline] ip6_datagram_connect_v6_only+0x104/0x180 net/ipv6/datagram.c:292 inet_dgram_connect+0x2e8/0x4d0 net/ipv4/af_inet.c:542 SYSC_connect+0x41a/0x510 net/socket.c:1639 SyS_connect+0x54/0x80 net/socket.c:1620 do_syscall_64+0x309/0x430 arch/x86/entry/common.c:287 entry_SYSCALL_64_after_hwframe+0x3d/0xa2 Local variable description: ----upt.i.i@xfrm_send_acquire Variable was created at: xfrm_send_acquire+0x73/0x1690 net/xfrm/xfrm_user.c:2864 km_query net/xfrm/xfrm_state.c:1953 [inline] xfrm_state_find+0x3ad8/0x4f40 net/xfrm/xfrm_state.c:1021 Byte 200 of 207 is uninitialized FAULT_INJECTION: forcing a failure. name failslab, interval 1, probability 0, space 0, times 0 ================================================================== CPU: 1 PID: 7675 Comm: syz-executor3 Not tainted 4.16.0+ #86 --- This bug is generated by a dumb bot. It may contain errors. See https://goo.gl/tpsmEJ for details. Direct all questions to syzkaller@googlegroups.com. syzbot will keep track of this bug report. If you forgot to add the Reported-by tag, once the fix for this bug is merged into any tree, please reply to this email with: #syz fix: exact-commit-title To mark this as a duplicate of another syzbot report, please reply with: #syz dup: exact-subject-of-another-report If it's a one-off invalid bug report, please reply with: #syz invalid Note: if the crash happens again, it will cause creation of a new bug report. Note: all commands must start from beginning of the line in the email body. ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: KMSAN: uninit-value in _copy_to_iter (2) 2018-04-23 16:56 KMSAN: uninit-value in _copy_to_iter (2) syzbot @ 2018-04-25 19:19 ` syzbot 2018-06-07 15:38 ` Michael S. Tsirkin 2018-06-07 17:10 ` Michael S. Tsirkin 2018-04-27 15:45 ` [PATCH net] vhost: Use kzalloc() to allocate vhost_msg_node Kevin Easton 1 sibling, 2 replies; 30+ messages in thread From: syzbot @ 2018-04-25 19:19 UTC (permalink / raw) To: avagin, davem, dingtianhong, edumazet, elena.reshetova, jasowang, kvm, linux-kernel, matthew, mingo, mst, netdev, pabeni, syzkaller-bugs, viro, virtualization, willemb syzbot has found reproducer for the following crash on https://github.com/google/kmsan.git/master commit d2d741e5d1898dfde1a75ea3d29a9a3e2edf0617 (Sun Apr 22 15:05:22 2018 +0000) kmsan: add initialization for shmem pages syzbot dashboard link: https://syzkaller.appspot.com/bug?extid=87cfa083e727a224754b So far this crash happened 3 times on https://github.com/google/kmsan.git/master. C reproducer: https://syzkaller.appspot.com/x/repro.c?id=5122017598636032 syzkaller reproducer: https://syzkaller.appspot.com/x/repro.syz?id=6680049734385664 Raw console output: https://syzkaller.appspot.com/x/log.txt?id=5920461749747712 Kernel config: https://syzkaller.appspot.com/x/.config?id=328654897048964367 compiler: clang version 7.0.0 (trunk 329391) IMPORTANT: if you fix the bug, please add the following tag to the commit: Reported-by: syzbot+87cfa083e727a224754b@syzkaller.appspotmail.com It will help syzbot understand when the bug is fixed. ================================================================== BUG: KMSAN: uninit-value in copyout lib/iov_iter.c:140 [inline] BUG: KMSAN: uninit-value in _copy_to_iter+0x46d/0x28f0 lib/iov_iter.c:571 CPU: 1 PID: 4516 Comm: syz-executor879 Not tainted 4.16.0+ #87 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 Call Trace: __dump_stack lib/dump_stack.c:17 [inline] dump_stack+0x185/0x1d0 lib/dump_stack.c:53 kmsan_report+0x142/0x240 mm/kmsan/kmsan.c:1067 kmsan_internal_check_memory+0x135/0x1e0 mm/kmsan/kmsan.c:1157 kmsan_copy_to_user+0x69/0x160 mm/kmsan/kmsan.c:1199 copyout lib/iov_iter.c:140 [inline] _copy_to_iter+0x46d/0x28f0 lib/iov_iter.c:571 copy_to_iter include/linux/uio.h:106 [inline] vhost_chr_read_iter+0x7ac/0xc50 drivers/vhost/vhost.c:1104 vhost_net_chr_read_iter+0xf6/0x130 drivers/vhost/net.c:1365 call_read_iter include/linux/fs.h:1776 [inline] aio_read+0x5c1/0x6f0 fs/aio.c:1517 io_submit_one fs/aio.c:1633 [inline] do_io_submit+0x1bb4/0x2f60 fs/aio.c:1698 SYSC_io_submit+0x98/0xb0 fs/aio.c:1723 SyS_io_submit+0x56/0x80 fs/aio.c:1720 do_syscall_64+0x309/0x430 arch/x86/entry/common.c:287 entry_SYSCALL_64_after_hwframe+0x3d/0xa2 RIP: 0033:0x4457b9 RSP: 002b:00007ff9343e4da8 EFLAGS: 00000293 ORIG_RAX: 00000000000000d1 RAX: ffffffffffffffda RBX: 00000000006dac44 RCX: 00000000004457b9 RDX: 00000000200001c0 RSI: 0000000000000001 RDI: 00007ff93439a000 RBP: 00000000006dac40 R08: 0000000000000000 R09: 0000000000000000 R10: 0000000000000000 R11: 0000000000000293 R12: 901aeeff3a98f9ab R13: 98c94b26f489688e R14: ae1b2dfa3c87200a R15: 0000000000000001 Uninit was created at: kmsan_save_stack_with_flags mm/kmsan/kmsan.c:278 [inline] kmsan_internal_poison_shadow+0xb8/0x1b0 mm/kmsan/kmsan.c:188 kmsan_kmalloc+0x94/0x100 mm/kmsan/kmsan.c:314 __kmalloc+0x23c/0x350 mm/slub.c:3791 kmalloc include/linux/slab.h:517 [inline] vhost_new_msg drivers/vhost/vhost.c:2340 [inline] vhost_iotlb_miss drivers/vhost/vhost.c:1124 [inline] translate_desc+0xbef/0x1120 drivers/vhost/vhost.c:1829 __vhost_get_user_slow drivers/vhost/vhost.c:812 [inline] __vhost_get_user drivers/vhost/vhost.c:846 [inline] vhost_update_used_flags+0x469/0x8d0 drivers/vhost/vhost.c:1715 vhost_vq_init_access+0x173/0xa20 drivers/vhost/vhost.c:1763 vhost_net_set_backend drivers/vhost/net.c:1166 [inline] vhost_net_ioctl+0x22b0/0x3480 drivers/vhost/net.c:1322 vfs_ioctl fs/ioctl.c:46 [inline] do_vfs_ioctl+0xaf0/0x2440 fs/ioctl.c:686 SYSC_ioctl+0x1d2/0x260 fs/ioctl.c:701 SyS_ioctl+0x54/0x80 fs/ioctl.c:692 do_syscall_64+0x309/0x430 arch/x86/entry/common.c:287 entry_SYSCALL_64_after_hwframe+0x3d/0xa2 Bytes 4-7 of 72 are uninitialized ================================================================== ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: KMSAN: uninit-value in _copy_to_iter (2) 2018-04-25 19:19 ` syzbot @ 2018-06-07 15:38 ` Michael S. Tsirkin 2018-06-07 15:38 ` syzbot ` (2 more replies) 2018-06-07 17:10 ` Michael S. Tsirkin 1 sibling, 3 replies; 30+ messages in thread From: Michael S. Tsirkin @ 2018-06-07 15:38 UTC (permalink / raw) To: syzbot Cc: willemb, avagin, kvm, netdev, matthew, linux-kernel, mingo, syzkaller-bugs, edumazet, viro, dingtianhong, pabeni, virtualization, davem, elena.reshetova #syz test: https://github.com/google/kmsan.git/master d2d741e5d1898dfde1a75ea3d29a9a3e2edf0617 Subject: vhost: fix info leak Fixes: CVE-2018-1118 Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c index f0be5f35ab28..9beefa6ed1ce 100644 --- a/drivers/vhost/vhost.c +++ b/drivers/vhost/vhost.c @@ -2345,6 +2345,9 @@ struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type) struct vhost_msg_node *node = kmalloc(sizeof *node, GFP_KERNEL); if (!node) return NULL; + + /* Make sure all padding within the structure is initialized. */ + memset(&node->msg, 0, sizeof node->msg); node->vq = vq; node->msg.type = type; return node; ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: Re: KMSAN: uninit-value in _copy_to_iter (2) 2018-06-07 15:38 ` Michael S. Tsirkin @ 2018-06-07 15:38 ` syzbot 2018-06-07 16:25 ` Dmitry Vyukov via Virtualization 2018-06-07 17:43 ` Al Viro 2 siblings, 0 replies; 30+ messages in thread From: syzbot @ 2018-06-07 15:38 UTC (permalink / raw) To: Michael S. Tsirkin Cc: avagin, davem, dingtianhong, edumazet, elena.reshetova, jasowang, kvm, linux-kernel, matthew, mingo, mst, netdev, pabeni, syzkaller-bugs, viro, virtualization, willemb > #syz test: https://github.com/google/kmsan.git/master > d2d741e5d1898dfde1a75ea3d29a9a3e2edf0617 "https://github.com/google/kmsan.git/master" does not look like a valid git repo address. > Subject: vhost: fix info leak > Fixes: CVE-2018-1118 > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > index f0be5f35ab28..9beefa6ed1ce 100644 > --- a/drivers/vhost/vhost.c > +++ b/drivers/vhost/vhost.c > @@ -2345,6 +2345,9 @@ struct vhost_msg_node *vhost_new_msg(struct > vhost_virtqueue *vq, int type) > struct vhost_msg_node *node = kmalloc(sizeof *node, GFP_KERNEL); > if (!node) > return NULL; > + > + /* Make sure all padding within the structure is initialized. */ > + memset(&node->msg, 0, sizeof node->msg); > node->vq = vq; > node->msg.type = type; > return node; ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: KMSAN: uninit-value in _copy_to_iter (2) 2018-06-07 15:38 ` Michael S. Tsirkin 2018-06-07 15:38 ` syzbot @ 2018-06-07 16:25 ` Dmitry Vyukov via Virtualization 2018-06-07 17:04 ` syzbot 2018-06-07 17:43 ` Al Viro 2 siblings, 1 reply; 30+ messages in thread From: Dmitry Vyukov via Virtualization @ 2018-06-07 16:25 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Willem de Bruijn, avagin, KVM list, netdev, Matthew Dawson, LKML, Ingo Molnar, syzkaller-bugs, Eric Dumazet, Al Viro, Ding Tianhong, syzbot, Paolo Abeni, virtualization, David Miller, Reshetova, Elena [-- Attachment #1: Type: text/plain, Size: 1599 bytes --] On Thu, Jun 7, 2018 at 5:38 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > #syz test: https://github.com/google/kmsan.git/master d2d741e5d1898dfde1a75ea3d29a9a3e2edf0617 Hi Michael, We need: #syz test: https://github.com/google/kmsan.git master here. Please see https://github.com/google/syzkaller/blob/master/docs/syzbot.md#testing-patches for more info. Please also add the Reported-by tag when mailing the patch for review. Thanks > Subject: vhost: fix info leak > > Fixes: CVE-2018-1118 > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > index f0be5f35ab28..9beefa6ed1ce 100644 > --- a/drivers/vhost/vhost.c > +++ b/drivers/vhost/vhost.c > @@ -2345,6 +2345,9 @@ struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type) > struct vhost_msg_node *node = kmalloc(sizeof *node, GFP_KERNEL); > if (!node) > return NULL; > + > + /* Make sure all padding within the structure is initialized. */ > + memset(&node->msg, 0, sizeof node->msg); > node->vq = vq; > node->msg.type = type; > return node; > > -- > You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group. > To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller-bugs+unsubscribe@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/20180607183627-mutt-send-email-mst%40kernel.org. > For more options, visit https://groups.google.com/d/optout. [-- Attachment #2: patch --] [-- Type: application/octet-stream, Size: 518 bytes --] diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c index f0be5f35ab28..9beefa6ed1ce 100644 --- a/drivers/vhost/vhost.c +++ b/drivers/vhost/vhost.c @@ -2345,6 +2345,9 @@ struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type) struct vhost_msg_node *node = kmalloc(sizeof *node, GFP_KERNEL); if (!node) return NULL; + + /* Make sure all padding within the structure is initialized. */ + memset(&node->msg, 0, sizeof node->msg); node->vq = vq; node->msg.type = type; return node; [-- Attachment #3: Type: text/plain, Size: 183 bytes --] _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: KMSAN: uninit-value in _copy_to_iter (2) 2018-06-07 16:25 ` Dmitry Vyukov via Virtualization @ 2018-06-07 17:04 ` syzbot 0 siblings, 0 replies; 30+ messages in thread From: syzbot @ 2018-06-07 17:04 UTC (permalink / raw) To: avagin, davem, dingtianhong, dvyukov, edumazet, elena.reshetova, jasowang, kvm, linux-kernel, matthew, mingo, mst, netdev, pabeni, syzkaller-bugs, viro, virtualization, willemb Hello, syzbot has tested the proposed patch and the reproducer did not trigger crash: Reported-and-tested-by: syzbot+87cfa083e727a224754b@syzkaller.appspotmail.com Tested on: commit: c6a6aed994b6 kmsan: remove dead code to trigger syzbot build git tree: https://github.com/google/kmsan.git/master kernel config: https://syzkaller.appspot.com/x/.config?x=848e40757852af3e compiler: clang version 7.0.0 (trunk 334104) patch: https://syzkaller.appspot.com/x/patch.diff?x=17dc3a8f800000 Note: testing is done by a robot and is best-effort only. ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: KMSAN: uninit-value in _copy_to_iter (2) 2018-06-07 15:38 ` Michael S. Tsirkin 2018-06-07 15:38 ` syzbot 2018-06-07 16:25 ` Dmitry Vyukov via Virtualization @ 2018-06-07 17:43 ` Al Viro 2018-06-07 17:59 ` Michael S. Tsirkin 2 siblings, 1 reply; 30+ messages in thread From: Al Viro @ 2018-06-07 17:43 UTC (permalink / raw) To: Michael S. Tsirkin Cc: syzbot, avagin, davem, dingtianhong, edumazet, elena.reshetova, jasowang, kvm, linux-kernel, matthew, mingo, netdev, pabeni, syzkaller-bugs, virtualization, willemb On Thu, Jun 07, 2018 at 06:38:48PM +0300, Michael S. Tsirkin wrote: > #syz test: https://github.com/google/kmsan.git/master d2d741e5d1898dfde1a75ea3d29a9a3e2edf0617 > > Subject: vhost: fix info leak > > Fixes: CVE-2018-1118 > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > index f0be5f35ab28..9beefa6ed1ce 100644 > --- a/drivers/vhost/vhost.c > +++ b/drivers/vhost/vhost.c > @@ -2345,6 +2345,9 @@ struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type) > struct vhost_msg_node *node = kmalloc(sizeof *node, GFP_KERNEL); > if (!node) > return NULL; > + > + /* Make sure all padding within the structure is initialized. */ > + memset(&node->msg, 0, sizeof node->msg); Umm... Maybe kzalloc(), then? You have struct vhost_msg_node { struct vhost_msg msg; struct vhost_virtqueue *vq; struct list_head node; }; and that's what, 68 bytes in msg, then either 4 bytes pointer or 4 bytes padding + 8 bytes pointer, then two pointers? How much does explicit partial memset() save you here? > node->vq = vq; > node->msg.type = type; > return node; ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: KMSAN: uninit-value in _copy_to_iter (2) 2018-06-07 17:43 ` Al Viro @ 2018-06-07 17:59 ` Michael S. Tsirkin 2018-06-07 18:04 ` Al Viro 0 siblings, 1 reply; 30+ messages in thread From: Michael S. Tsirkin @ 2018-06-07 17:59 UTC (permalink / raw) To: Al Viro Cc: syzbot, avagin, davem, dingtianhong, edumazet, elena.reshetova, jasowang, kvm, linux-kernel, matthew, mingo, netdev, pabeni, syzkaller-bugs, virtualization, willemb On Thu, Jun 07, 2018 at 06:43:55PM +0100, Al Viro wrote: > On Thu, Jun 07, 2018 at 06:38:48PM +0300, Michael S. Tsirkin wrote: > > #syz test: https://github.com/google/kmsan.git/master d2d741e5d1898dfde1a75ea3d29a9a3e2edf0617 > > > > Subject: vhost: fix info leak > > > > Fixes: CVE-2018-1118 > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > --- > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > > index f0be5f35ab28..9beefa6ed1ce 100644 > > --- a/drivers/vhost/vhost.c > > +++ b/drivers/vhost/vhost.c > > @@ -2345,6 +2345,9 @@ struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type) > > struct vhost_msg_node *node = kmalloc(sizeof *node, GFP_KERNEL); > > if (!node) > > return NULL; > > + > > + /* Make sure all padding within the structure is initialized. */ > > + memset(&node->msg, 0, sizeof node->msg); > > Umm... Maybe kzalloc(), then? You have > > struct vhost_msg_node { > struct vhost_msg msg; > struct vhost_virtqueue *vq; > struct list_head node; > }; > > and that's what, 68 bytes in msg, then either 4 bytes pointer or > 4 bytes padding + 8 bytes pointer, then two pointers? How much > does explicit partial memset() save you here? Yes but 0 isn't a nop here so if this struct is used without a sensible initialization, it will crash elsewhere. I prefer KASAN to catch such uses. > > node->vq = vq; > > node->msg.type = type; > > return node; ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: KMSAN: uninit-value in _copy_to_iter (2) 2018-06-07 17:59 ` Michael S. Tsirkin @ 2018-06-07 18:04 ` Al Viro 2018-06-07 19:29 ` Michael S. Tsirkin 0 siblings, 1 reply; 30+ messages in thread From: Al Viro @ 2018-06-07 18:04 UTC (permalink / raw) To: Michael S. Tsirkin Cc: syzbot, avagin, davem, dingtianhong, edumazet, elena.reshetova, jasowang, kvm, linux-kernel, matthew, mingo, netdev, pabeni, syzkaller-bugs, virtualization, willemb On Thu, Jun 07, 2018 at 08:59:06PM +0300, Michael S. Tsirkin wrote: > On Thu, Jun 07, 2018 at 06:43:55PM +0100, Al Viro wrote: > > On Thu, Jun 07, 2018 at 06:38:48PM +0300, Michael S. Tsirkin wrote: > > > #syz test: https://github.com/google/kmsan.git/master d2d741e5d1898dfde1a75ea3d29a9a3e2edf0617 > > > > > > Subject: vhost: fix info leak > > > > > > Fixes: CVE-2018-1118 > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > > --- > > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > > > index f0be5f35ab28..9beefa6ed1ce 100644 > > > --- a/drivers/vhost/vhost.c > > > +++ b/drivers/vhost/vhost.c > > > @@ -2345,6 +2345,9 @@ struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type) > > > struct vhost_msg_node *node = kmalloc(sizeof *node, GFP_KERNEL); > > > if (!node) > > > return NULL; > > > + > > > + /* Make sure all padding within the structure is initialized. */ > > > + memset(&node->msg, 0, sizeof node->msg); > > > > Umm... Maybe kzalloc(), then? You have > > > > struct vhost_msg_node { > > struct vhost_msg msg; > > struct vhost_virtqueue *vq; > > struct list_head node; > > }; > > > > and that's what, 68 bytes in msg, then either 4 bytes pointer or > > 4 bytes padding + 8 bytes pointer, then two pointers? How much > > does explicit partial memset() save you here? > > Yes but 0 isn't a nop here so if this struct is used without > a sensible initialization, it will crash elsewhere. > I prefer KASAN to catch such uses. > > > > > node->vq = vq; > > > node->msg.type = type; IDGI - what would your variant catch that kzalloc + 2 assignments won't? Accesses to uninitialized ->node? Because that's the only difference in what is and is not initialized between those variants... ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: KMSAN: uninit-value in _copy_to_iter (2) 2018-06-07 18:04 ` Al Viro @ 2018-06-07 19:29 ` Michael S. Tsirkin 0 siblings, 0 replies; 30+ messages in thread From: Michael S. Tsirkin @ 2018-06-07 19:29 UTC (permalink / raw) To: Al Viro Cc: syzbot, avagin, davem, dingtianhong, edumazet, elena.reshetova, jasowang, kvm, linux-kernel, matthew, mingo, netdev, pabeni, syzkaller-bugs, virtualization, willemb On Thu, Jun 07, 2018 at 07:04:49PM +0100, Al Viro wrote: > On Thu, Jun 07, 2018 at 08:59:06PM +0300, Michael S. Tsirkin wrote: > > On Thu, Jun 07, 2018 at 06:43:55PM +0100, Al Viro wrote: > > > On Thu, Jun 07, 2018 at 06:38:48PM +0300, Michael S. Tsirkin wrote: > > > > #syz test: https://github.com/google/kmsan.git/master d2d741e5d1898dfde1a75ea3d29a9a3e2edf0617 > > > > > > > > Subject: vhost: fix info leak > > > > > > > > Fixes: CVE-2018-1118 > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > > > --- > > > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > > > > index f0be5f35ab28..9beefa6ed1ce 100644 > > > > --- a/drivers/vhost/vhost.c > > > > +++ b/drivers/vhost/vhost.c > > > > @@ -2345,6 +2345,9 @@ struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type) > > > > struct vhost_msg_node *node = kmalloc(sizeof *node, GFP_KERNEL); > > > > if (!node) > > > > return NULL; > > > > + > > > > + /* Make sure all padding within the structure is initialized. */ > > > > + memset(&node->msg, 0, sizeof node->msg); > > > > > > Umm... Maybe kzalloc(), then? You have > > > > > > struct vhost_msg_node { > > > struct vhost_msg msg; > > > struct vhost_virtqueue *vq; > > > struct list_head node; > > > }; > > > > > > and that's what, 68 bytes in msg, then either 4 bytes pointer or > > > 4 bytes padding + 8 bytes pointer, then two pointers? How much > > > does explicit partial memset() save you here? > > > > Yes but 0 isn't a nop here so if this struct is used without > > a sensible initialization, it will crash elsewhere. > > I prefer KASAN to catch such uses. > > > > > > > > node->vq = vq; > > > > node->msg.type = type; > > IDGI - what would your variant catch that kzalloc + 2 assignments won't? > Accesses to uninitialized ->node? Because that's the only difference in > what is and is not initialized between those variants... For now yes but we'll likely add more fields in this structure down the road, which is where I'd expect new bugs to come from. -- MST ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: KMSAN: uninit-value in _copy_to_iter (2) 2018-04-25 19:19 ` syzbot 2018-06-07 15:38 ` Michael S. Tsirkin @ 2018-06-07 17:10 ` Michael S. Tsirkin 2018-06-07 17:31 ` syzbot 1 sibling, 1 reply; 30+ messages in thread From: Michael S. Tsirkin @ 2018-06-07 17:10 UTC (permalink / raw) To: syzbot Cc: avagin, davem, dingtianhong, edumazet, elena.reshetova, jasowang, kvm, linux-kernel, matthew, mingo, netdev, pabeni, syzkaller-bugs, viro, virtualization, willemb #syz test: https://github.com/google/kmsan.git master Subject: vhost: fix info leak Fixes: CVE-2018-1118 Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c index f0be5f35ab28..9beefa6ed1ce 100644 --- a/drivers/vhost/vhost.c +++ b/drivers/vhost/vhost.c @@ -2345,6 +2345,9 @@ struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type) struct vhost_msg_node *node = kmalloc(sizeof *node, GFP_KERNEL); if (!node) return NULL; + + /* Make sure all padding within the structure is initialized. */ + memset(&node->msg, 0, sizeof node->msg); node->vq = vq; node->msg.type = type; return node; ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: KMSAN: uninit-value in _copy_to_iter (2) 2018-06-07 17:10 ` Michael S. Tsirkin @ 2018-06-07 17:31 ` syzbot 0 siblings, 0 replies; 30+ messages in thread From: syzbot @ 2018-06-07 17:31 UTC (permalink / raw) To: avagin, davem, dingtianhong, edumazet, elena.reshetova, jasowang, kvm, linux-kernel, matthew, mingo, mst, netdev, pabeni, syzkaller-bugs, viro, virtualization, willemb Hello, syzbot has tested the proposed patch and the reproducer did not trigger crash: Reported-and-tested-by: syzbot+87cfa083e727a224754b@syzkaller.appspotmail.com Tested on: commit: c6a6aed994b6 kmsan: remove dead code to trigger syzbot build git tree: https://github.com/google/kmsan.git/master kernel config: https://syzkaller.appspot.com/x/.config?x=848e40757852af3e compiler: clang version 7.0.0 (trunk 334104) patch: https://syzkaller.appspot.com/x/patch.diff?x=1119eddf800000 Note: testing is done by a robot and is best-effort only. ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH net] vhost: Use kzalloc() to allocate vhost_msg_node 2018-04-23 16:56 KMSAN: uninit-value in _copy_to_iter (2) syzbot 2018-04-25 19:19 ` syzbot @ 2018-04-27 15:45 ` Kevin Easton 2018-04-27 16:05 ` Michael S. Tsirkin ` (2 more replies) 1 sibling, 3 replies; 30+ messages in thread From: Kevin Easton @ 2018-04-27 15:45 UTC (permalink / raw) To: Michael S. Tsirkin, Jason Wang, kvm, virtualization, netdev, linux-kernel, syzkaller-bugs The struct vhost_msg within struct vhost_msg_node is copied to userspace, so it should be allocated with kzalloc() to ensure all structure padding is zeroed. Signed-off-by: Kevin Easton <kevin@guarana.org> Reported-by: syzbot+87cfa083e727a224754b@syzkaller.appspotmail.com --- drivers/vhost/vhost.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c index f3bd8e9..1b84dcff 100644 --- a/drivers/vhost/vhost.c +++ b/drivers/vhost/vhost.c @@ -2339,7 +2339,7 @@ EXPORT_SYMBOL_GPL(vhost_disable_notify); /* Create a new message. */ struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type) { - struct vhost_msg_node *node = kmalloc(sizeof *node, GFP_KERNEL); + struct vhost_msg_node *node = kzalloc(sizeof *node, GFP_KERNEL); if (!node) return NULL; node->vq = vq; -- 2.8.1 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH net] vhost: Use kzalloc() to allocate vhost_msg_node 2018-04-27 15:45 ` [PATCH net] vhost: Use kzalloc() to allocate vhost_msg_node Kevin Easton @ 2018-04-27 16:05 ` Michael S. Tsirkin 2018-04-27 16:11 ` Dmitry Vyukov via Virtualization 2018-04-28 1:07 ` Kevin Easton 2018-05-07 13:03 ` Michael S. Tsirkin 2018-05-29 22:19 ` [net] " Guenter Roeck 2 siblings, 2 replies; 30+ messages in thread From: Michael S. Tsirkin @ 2018-04-27 16:05 UTC (permalink / raw) To: Kevin Easton Cc: Jason Wang, kvm, virtualization, netdev, linux-kernel, syzkaller-bugs On Fri, Apr 27, 2018 at 11:45:02AM -0400, Kevin Easton wrote: > The struct vhost_msg within struct vhost_msg_node is copied to userspace, > so it should be allocated with kzalloc() to ensure all structure padding > is zeroed. > > Signed-off-by: Kevin Easton <kevin@guarana.org> > Reported-by: syzbot+87cfa083e727a224754b@syzkaller.appspotmail.com Does it help if a patch naming the padding is applied, and then we init just the relevant field? Just curious. > --- > drivers/vhost/vhost.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > index f3bd8e9..1b84dcff 100644 > --- a/drivers/vhost/vhost.c > +++ b/drivers/vhost/vhost.c > @@ -2339,7 +2339,7 @@ EXPORT_SYMBOL_GPL(vhost_disable_notify); > /* Create a new message. */ > struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type) > { > - struct vhost_msg_node *node = kmalloc(sizeof *node, GFP_KERNEL); > + struct vhost_msg_node *node = kzalloc(sizeof *node, GFP_KERNEL); > if (!node) > return NULL; > node->vq = vq; > -- > 2.8.1 ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH net] vhost: Use kzalloc() to allocate vhost_msg_node 2018-04-27 16:05 ` Michael S. Tsirkin @ 2018-04-27 16:11 ` Dmitry Vyukov via Virtualization 2018-04-27 16:15 ` Michael S. Tsirkin 2018-04-27 19:36 ` Michael S. Tsirkin 2018-04-28 1:07 ` Kevin Easton 1 sibling, 2 replies; 30+ messages in thread From: Dmitry Vyukov via Virtualization @ 2018-04-27 16:11 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Kevin Easton, KVM list, netdev, syzkaller-bugs, LKML, virtualization On Fri, Apr 27, 2018 at 6:05 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > On Fri, Apr 27, 2018 at 11:45:02AM -0400, Kevin Easton wrote: >> The struct vhost_msg within struct vhost_msg_node is copied to userspace, >> so it should be allocated with kzalloc() to ensure all structure padding >> is zeroed. >> >> Signed-off-by: Kevin Easton <kevin@guarana.org> >> Reported-by: syzbot+87cfa083e727a224754b@syzkaller.appspotmail.com > > Does it help if a patch naming the padding is applied, > and then we init just the relevant field? > Just curious. Yes, it would help. >> --- >> drivers/vhost/vhost.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c >> index f3bd8e9..1b84dcff 100644 >> --- a/drivers/vhost/vhost.c >> +++ b/drivers/vhost/vhost.c >> @@ -2339,7 +2339,7 @@ EXPORT_SYMBOL_GPL(vhost_disable_notify); >> /* Create a new message. */ >> struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type) >> { >> - struct vhost_msg_node *node = kmalloc(sizeof *node, GFP_KERNEL); >> + struct vhost_msg_node *node = kzalloc(sizeof *node, GFP_KERNEL); >> if (!node) >> return NULL; >> node->vq = vq; >> -- >> 2.8.1 > > -- > You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group. > To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller-bugs+unsubscribe@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/20180427185501-mutt-send-email-mst%40kernel.org. > For more options, visit https://groups.google.com/d/optout. ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH net] vhost: Use kzalloc() to allocate vhost_msg_node 2018-04-27 16:11 ` Dmitry Vyukov via Virtualization @ 2018-04-27 16:15 ` Michael S. Tsirkin 2018-04-27 16:25 ` Dmitry Vyukov 2018-04-27 19:36 ` Michael S. Tsirkin 1 sibling, 1 reply; 30+ messages in thread From: Michael S. Tsirkin @ 2018-04-27 16:15 UTC (permalink / raw) To: Dmitry Vyukov Cc: Kevin Easton, Jason Wang, KVM list, virtualization, netdev, LKML, syzkaller-bugs On Fri, Apr 27, 2018 at 06:11:31PM +0200, Dmitry Vyukov wrote: > On Fri, Apr 27, 2018 at 6:05 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Fri, Apr 27, 2018 at 11:45:02AM -0400, Kevin Easton wrote: > >> The struct vhost_msg within struct vhost_msg_node is copied to userspace, > >> so it should be allocated with kzalloc() to ensure all structure padding > >> is zeroed. > >> > >> Signed-off-by: Kevin Easton <kevin@guarana.org> > >> Reported-by: syzbot+87cfa083e727a224754b@syzkaller.appspotmail.com > > > > Does it help if a patch naming the padding is applied, > > and then we init just the relevant field? > > Just curious. > > Yes, it would help. I think it's slightly better that way then. node has a lot of internal stuff we don't care to init. Would you mind taking my patch and building on top of that then? > >> --- > >> drivers/vhost/vhost.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > >> index f3bd8e9..1b84dcff 100644 > >> --- a/drivers/vhost/vhost.c > >> +++ b/drivers/vhost/vhost.c > >> @@ -2339,7 +2339,7 @@ EXPORT_SYMBOL_GPL(vhost_disable_notify); > >> /* Create a new message. */ > >> struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type) > >> { > >> - struct vhost_msg_node *node = kmalloc(sizeof *node, GFP_KERNEL); > >> + struct vhost_msg_node *node = kzalloc(sizeof *node, GFP_KERNEL); > >> if (!node) > >> return NULL; > >> node->vq = vq; > >> -- > >> 2.8.1 > > > > -- > > You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group. > > To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller-bugs+unsubscribe@googlegroups.com. > > To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/20180427185501-mutt-send-email-mst%40kernel.org. > > For more options, visit https://groups.google.com/d/optout. ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH net] vhost: Use kzalloc() to allocate vhost_msg_node 2018-04-27 16:15 ` Michael S. Tsirkin @ 2018-04-27 16:25 ` Dmitry Vyukov 2018-04-27 16:29 ` Dmitry Vyukov 0 siblings, 1 reply; 30+ messages in thread From: Dmitry Vyukov @ 2018-04-27 16:25 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Kevin Easton, Jason Wang, KVM list, virtualization, netdev, LKML, syzkaller-bugs On Fri, Apr 27, 2018 at 6:15 PM, Michael S. Tsirkin <mst@redhat.com> wrote: >> >> The struct vhost_msg within struct vhost_msg_node is copied to userspace, >> >> so it should be allocated with kzalloc() to ensure all structure padding >> >> is zeroed. >> >> >> >> Signed-off-by: Kevin Easton <kevin@guarana.org> >> >> Reported-by: syzbot+87cfa083e727a224754b@syzkaller.appspotmail.com >> > >> > Does it help if a patch naming the padding is applied, >> > and then we init just the relevant field? >> > Just curious. >> >> Yes, it would help. > > I think it's slightly better that way then. node has a lot of internal > stuff we don't care to init. Would you mind taking my patch and building > on top of that then? But it's asking for more information leaks in future. This looks like work for compiler. >> >> --- >> >> drivers/vhost/vhost.c | 2 +- >> >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> >> >> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c >> >> index f3bd8e9..1b84dcff 100644 >> >> --- a/drivers/vhost/vhost.c >> >> +++ b/drivers/vhost/vhost.c >> >> @@ -2339,7 +2339,7 @@ EXPORT_SYMBOL_GPL(vhost_disable_notify); >> >> /* Create a new message. */ >> >> struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type) >> >> { >> >> - struct vhost_msg_node *node = kmalloc(sizeof *node, GFP_KERNEL); >> >> + struct vhost_msg_node *node = kzalloc(sizeof *node, GFP_KERNEL); >> >> if (!node) >> >> return NULL; >> >> node->vq = vq; >> >> -- >> >> 2.8.1 ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH net] vhost: Use kzalloc() to allocate vhost_msg_node 2018-04-27 16:25 ` Dmitry Vyukov @ 2018-04-27 16:29 ` Dmitry Vyukov 0 siblings, 0 replies; 30+ messages in thread From: Dmitry Vyukov @ 2018-04-27 16:29 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Kevin Easton, Jason Wang, KVM list, virtualization, netdev, LKML, syzkaller-bugs On Fri, Apr 27, 2018 at 6:25 PM, Dmitry Vyukov <dvyukov@google.com> wrote: >>> >> The struct vhost_msg within struct vhost_msg_node is copied to userspace, >>> >> so it should be allocated with kzalloc() to ensure all structure padding >>> >> is zeroed. >>> >> >>> >> Signed-off-by: Kevin Easton <kevin@guarana.org> >>> >> Reported-by: syzbot+87cfa083e727a224754b@syzkaller.appspotmail.com >>> > >>> > Does it help if a patch naming the padding is applied, >>> > and then we init just the relevant field? >>> > Just curious. >>> >>> Yes, it would help. >> >> I think it's slightly better that way then. node has a lot of internal >> stuff we don't care to init. Would you mind taking my patch and building >> on top of that then? > > > But it's asking for more information leaks in future. This looks like > work for compiler. Modern compilers are perfectly capable of doing this: #include <memory.h> #include <unistd.h> int main() { int x[10]; memset(&x, 0, sizeof(x)); x[0] = 0; x[2] = 2; x[3] = 3; x[4] = 4; x[5] = 5; x[6] = 6; x[7] = 7; x[8] = 8; x[9] = 9; write(0, x, sizeof(x)); return 0; } gcc 7.2 -O3 0000000000000540 <main>: 540: sub $0x38,%rsp 544: mov $0x28,%edx 549: xor %edi,%edi 54b: movdqa 0x1cd(%rip),%xmm0 # 720 <_IO_stdin_used+0x10> 553: mov %rsp,%rsi 556: movq $0x0,(%rsp) 55e: movups %xmm0,0x8(%rsp) 563: movdqa 0x1c5(%rip),%xmm0 # 730 <_IO_stdin_used+0x20> 56b: movups %xmm0,0x18(%rsp) 570: callq 520 <write@plt> 575: xor %eax,%eax 577: add $0x38,%rsp 57b: retq 57c: nopl 0x0(%rax) But they will not put a security hole next time fields are shuffled. >>> >> --- >>> >> drivers/vhost/vhost.c | 2 +- >>> >> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >> >>> >> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c >>> >> index f3bd8e9..1b84dcff 100644 >>> >> --- a/drivers/vhost/vhost.c >>> >> +++ b/drivers/vhost/vhost.c >>> >> @@ -2339,7 +2339,7 @@ EXPORT_SYMBOL_GPL(vhost_disable_notify); >>> >> /* Create a new message. */ >>> >> struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type) >>> >> { >>> >> - struct vhost_msg_node *node = kmalloc(sizeof *node, GFP_KERNEL); >>> >> + struct vhost_msg_node *node = kzalloc(sizeof *node, GFP_KERNEL); >>> >> if (!node) >>> >> return NULL; >>> >> node->vq = vq; >>> >> -- >>> >> 2.8.1 ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH net] vhost: Use kzalloc() to allocate vhost_msg_node 2018-04-27 16:11 ` Dmitry Vyukov via Virtualization 2018-04-27 16:15 ` Michael S. Tsirkin @ 2018-04-27 19:36 ` Michael S. Tsirkin 2018-04-29 8:10 ` Dmitry Vyukov 1 sibling, 1 reply; 30+ messages in thread From: Michael S. Tsirkin @ 2018-04-27 19:36 UTC (permalink / raw) To: Dmitry Vyukov Cc: Kevin Easton, Jason Wang, KVM list, virtualization, netdev, LKML, syzkaller-bugs On Fri, Apr 27, 2018 at 06:11:31PM +0200, Dmitry Vyukov wrote: > On Fri, Apr 27, 2018 at 6:05 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Fri, Apr 27, 2018 at 11:45:02AM -0400, Kevin Easton wrote: > >> The struct vhost_msg within struct vhost_msg_node is copied to userspace, > >> so it should be allocated with kzalloc() to ensure all structure padding > >> is zeroed. > >> > >> Signed-off-by: Kevin Easton <kevin@guarana.org> > >> Reported-by: syzbot+87cfa083e727a224754b@syzkaller.appspotmail.com > > > > Does it help if a patch naming the padding is applied, > > and then we init just the relevant field? > > Just curious. > > Yes, it would help. How about a Tested-by tag then? > >> --- > >> drivers/vhost/vhost.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > >> index f3bd8e9..1b84dcff 100644 > >> --- a/drivers/vhost/vhost.c > >> +++ b/drivers/vhost/vhost.c > >> @@ -2339,7 +2339,7 @@ EXPORT_SYMBOL_GPL(vhost_disable_notify); > >> /* Create a new message. */ > >> struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type) > >> { > >> - struct vhost_msg_node *node = kmalloc(sizeof *node, GFP_KERNEL); > >> + struct vhost_msg_node *node = kzalloc(sizeof *node, GFP_KERNEL); > >> if (!node) > >> return NULL; > >> node->vq = vq; > >> -- > >> 2.8.1 > > > > -- > > You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group. > > To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller-bugs+unsubscribe@googlegroups.com. > > To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/20180427185501-mutt-send-email-mst%40kernel.org. > > For more options, visit https://groups.google.com/d/optout. ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH net] vhost: Use kzalloc() to allocate vhost_msg_node 2018-04-27 19:36 ` Michael S. Tsirkin @ 2018-04-29 8:10 ` Dmitry Vyukov 0 siblings, 0 replies; 30+ messages in thread From: Dmitry Vyukov @ 2018-04-29 8:10 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Kevin Easton, Jason Wang, KVM list, virtualization, netdev, LKML, syzkaller-bugs On Fri, Apr 27, 2018 at 9:36 PM, Michael S. Tsirkin <mst@redhat.com> wrote: >> >> The struct vhost_msg within struct vhost_msg_node is copied to userspace, >> >> so it should be allocated with kzalloc() to ensure all structure padding >> >> is zeroed. >> >> >> >> Signed-off-by: Kevin Easton <kevin@guarana.org> >> >> Reported-by: syzbot+87cfa083e727a224754b@syzkaller.appspotmail.com >> > >> > Does it help if a patch naming the padding is applied, >> > and then we init just the relevant field? >> > Just curious. >> >> Yes, it would help. > > How about a Tested-by tag then? I didn't test either patch. >> >> --- >> >> drivers/vhost/vhost.c | 2 +- >> >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> >> >> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c >> >> index f3bd8e9..1b84dcff 100644 >> >> --- a/drivers/vhost/vhost.c >> >> +++ b/drivers/vhost/vhost.c >> >> @@ -2339,7 +2339,7 @@ EXPORT_SYMBOL_GPL(vhost_disable_notify); >> >> /* Create a new message. */ >> >> struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type) >> >> { >> >> - struct vhost_msg_node *node = kmalloc(sizeof *node, GFP_KERNEL); >> >> + struct vhost_msg_node *node = kzalloc(sizeof *node, GFP_KERNEL); >> >> if (!node) >> >> return NULL; >> >> node->vq = vq; >> >> -- >> >> 2.8.1 >> > >> > -- >> > You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group. >> > To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller-bugs+unsubscribe@googlegroups.com. >> > To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/20180427185501-mutt-send-email-mst%40kernel.org. >> > For more options, visit https://groups.google.com/d/optout. ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH net] vhost: Use kzalloc() to allocate vhost_msg_node 2018-04-27 16:05 ` Michael S. Tsirkin 2018-04-27 16:11 ` Dmitry Vyukov via Virtualization @ 2018-04-28 1:07 ` Kevin Easton 2018-04-28 1:51 ` Kevin Easton 1 sibling, 1 reply; 30+ messages in thread From: Kevin Easton @ 2018-04-28 1:07 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Jason Wang, kvm, virtualization, netdev, linux-kernel, syzkaller-bugs On Fri, Apr 27, 2018 at 07:05:45PM +0300, Michael S. Tsirkin wrote: > On Fri, Apr 27, 2018 at 11:45:02AM -0400, Kevin Easton wrote: > > The struct vhost_msg within struct vhost_msg_node is copied to userspace, > > so it should be allocated with kzalloc() to ensure all structure padding > > is zeroed. > > > > Signed-off-by: Kevin Easton <kevin@guarana.org> > > Reported-by: syzbot+87cfa083e727a224754b@syzkaller.appspotmail.com > > Does it help if a patch naming the padding is applied, > and then we init just the relevant field? > Just curious. No, I don't believe that is sufficient to fix the problem. The structure is allocated by kmalloc(), then individual fields are initialised. The named adding would be forced to be initialised if it were initialised with a struct initialiser, but that's not the case. The compiler is free to leave padding0 with whatever junk kmalloc() left there. Having said that, naming the padding *does* help - technically, the compiler is allowed to put whatever it likes in the padding every time you modify the struct. It really needs both. I didn't name the padding in my original patch because I wasn't sure if the padding actually exists on 32 bit architectures? - Kevin ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH net] vhost: Use kzalloc() to allocate vhost_msg_node 2018-04-28 1:07 ` Kevin Easton @ 2018-04-28 1:51 ` Kevin Easton 2018-04-28 2:23 ` Jason Wang 0 siblings, 1 reply; 30+ messages in thread From: Kevin Easton @ 2018-04-28 1:51 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Jason Wang, kvm, virtualization, netdev, linux-kernel, syzkaller-bugs On Fri, Apr 27, 2018 at 09:07:56PM -0400, Kevin Easton wrote: > On Fri, Apr 27, 2018 at 07:05:45PM +0300, Michael S. Tsirkin wrote: > > On Fri, Apr 27, 2018 at 11:45:02AM -0400, Kevin Easton wrote: > > > The struct vhost_msg within struct vhost_msg_node is copied to userspace, > > > so it should be allocated with kzalloc() to ensure all structure padding > > > is zeroed. > > > > > > Signed-off-by: Kevin Easton <kevin@guarana.org> > > > Reported-by: syzbot+87cfa083e727a224754b@syzkaller.appspotmail.com > > > > Does it help if a patch naming the padding is applied, > > and then we init just the relevant field? > > Just curious. > > No, I don't believe that is sufficient to fix the problem. Scratch that, somehow I missed the "..and then we init just the relevant field" part, sorry. There's still the padding after the vhost_iotlb_msg to consider. It's named in the union but I don't think that's guaranteed to be initialised when the iotlb member of the union is used to initialise things. > I didn't name the padding in my original patch because I wasn't sure > if the padding actually exists on 32 bit architectures? This might still be a concern, too? At the end of the day, zeroing 96 bytes (the full size of vhost_msg_node) is pretty quick. - Kevin ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH net] vhost: Use kzalloc() to allocate vhost_msg_node 2018-04-28 1:51 ` Kevin Easton @ 2018-04-28 2:23 ` Jason Wang 0 siblings, 0 replies; 30+ messages in thread From: Jason Wang @ 2018-04-28 2:23 UTC (permalink / raw) To: Kevin Easton, Michael S. Tsirkin Cc: kvm, virtualization, netdev, linux-kernel, syzkaller-bugs On 2018年04月28日 09:51, Kevin Easton wrote: > On Fri, Apr 27, 2018 at 09:07:56PM -0400, Kevin Easton wrote: >> On Fri, Apr 27, 2018 at 07:05:45PM +0300, Michael S. Tsirkin wrote: >>> On Fri, Apr 27, 2018 at 11:45:02AM -0400, Kevin Easton wrote: >>>> The struct vhost_msg within struct vhost_msg_node is copied to userspace, >>>> so it should be allocated with kzalloc() to ensure all structure padding >>>> is zeroed. >>>> >>>> Signed-off-by: Kevin Easton <kevin@guarana.org> >>>> Reported-by: syzbot+87cfa083e727a224754b@syzkaller.appspotmail.com >>> Does it help if a patch naming the padding is applied, >>> and then we init just the relevant field? >>> Just curious. >> No, I don't believe that is sufficient to fix the problem. > Scratch that, somehow I missed the "..and then we init just the > relevant field" part, sorry. > > There's still the padding after the vhost_iotlb_msg to consider. It's > named in the union but I don't think that's guaranteed to be initialised > when the iotlb member of the union is used to initialise things. > >> I didn't name the padding in my original patch because I wasn't sure >> if the padding actually exists on 32 bit architectures? > This might still be a conce Yes. print &((struct vhost_msg *)0)->iotlb $3 = (struct vhost_iotlb_msg *) 0x4 > > At the end of the day, zeroing 96 bytes (the full size of vhost_msg_node) > is pretty quick. > > - Kevin Right, and even if it may be used heavily in the data-path, zeroing is not the main delay in that path. Thanks ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH net] vhost: Use kzalloc() to allocate vhost_msg_node 2018-04-27 15:45 ` [PATCH net] vhost: Use kzalloc() to allocate vhost_msg_node Kevin Easton 2018-04-27 16:05 ` Michael S. Tsirkin @ 2018-05-07 13:03 ` Michael S. Tsirkin 2018-05-07 13:12 ` Dmitry Vyukov 2018-05-08 8:27 ` Kevin Easton 2018-05-29 22:19 ` [net] " Guenter Roeck 2 siblings, 2 replies; 30+ messages in thread From: Michael S. Tsirkin @ 2018-05-07 13:03 UTC (permalink / raw) To: Kevin Easton Cc: Jason Wang, kvm, virtualization, netdev, linux-kernel, syzkaller-bugs On Fri, Apr 27, 2018 at 11:45:02AM -0400, Kevin Easton wrote: > The struct vhost_msg within struct vhost_msg_node is copied to userspace, > so it should be allocated with kzalloc() to ensure all structure padding > is zeroed. > > Signed-off-by: Kevin Easton <kevin@guarana.org> > Reported-by: syzbot+87cfa083e727a224754b@syzkaller.appspotmail.com > --- > drivers/vhost/vhost.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > index f3bd8e9..1b84dcff 100644 > --- a/drivers/vhost/vhost.c > +++ b/drivers/vhost/vhost.c > @@ -2339,7 +2339,7 @@ EXPORT_SYMBOL_GPL(vhost_disable_notify); > /* Create a new message. */ > struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type) > { > - struct vhost_msg_node *node = kmalloc(sizeof *node, GFP_KERNEL); > + struct vhost_msg_node *node = kzalloc(sizeof *node, GFP_KERNEL); > if (!node) > return NULL; > node->vq = vq; Let's just init the msg though. OK it seems this is the best we can do for now, we need a new feature bit to fix it for 32 bit userspace on 64 bit kernels. Does the following help? Signed-off-by: Michael S. Tsirkin <mst@redhat.com> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c index f3bd8e9..58d9aec 100644 --- a/drivers/vhost/vhost.c +++ b/drivers/vhost/vhost.c @@ -2342,6 +2342,9 @@ struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type) struct vhost_msg_node *node = kmalloc(sizeof *node, GFP_KERNEL); if (!node) return NULL; + + /* Make sure all padding within the structure is initialized. */ + memset(&node->msg, 0, sizeof node->msg); node->vq = vq; node->msg.type = type; return node; ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH net] vhost: Use kzalloc() to allocate vhost_msg_node 2018-05-07 13:03 ` Michael S. Tsirkin @ 2018-05-07 13:12 ` Dmitry Vyukov 2018-05-08 8:27 ` Kevin Easton 1 sibling, 0 replies; 30+ messages in thread From: Dmitry Vyukov @ 2018-05-07 13:12 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Kevin Easton, Jason Wang, KVM list, virtualization, netdev, LKML, syzkaller-bugs On Mon, May 7, 2018 at 3:03 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > On Fri, Apr 27, 2018 at 11:45:02AM -0400, Kevin Easton wrote: >> The struct vhost_msg within struct vhost_msg_node is copied to userspace, >> so it should be allocated with kzalloc() to ensure all structure padding >> is zeroed. >> >> Signed-off-by: Kevin Easton <kevin@guarana.org> >> Reported-by: syzbot+87cfa083e727a224754b@syzkaller.appspotmail.com >> --- >> drivers/vhost/vhost.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c >> index f3bd8e9..1b84dcff 100644 >> --- a/drivers/vhost/vhost.c >> +++ b/drivers/vhost/vhost.c >> @@ -2339,7 +2339,7 @@ EXPORT_SYMBOL_GPL(vhost_disable_notify); >> /* Create a new message. */ >> struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type) >> { >> - struct vhost_msg_node *node = kmalloc(sizeof *node, GFP_KERNEL); >> + struct vhost_msg_node *node = kzalloc(sizeof *node, GFP_KERNEL); >> if (!node) >> return NULL; >> node->vq = vq; > > > Let's just init the msg though. > > OK it seems this is the best we can do for now, > we need a new feature bit to fix it for 32 bit > userspace on 64 bit kernels. > > Does the following help? Hi Michael, You can ask reporter (syzbot) to test: https://github.com/google/syzkaller/blob/master/docs/syzbot.md#testing-patches https://github.com/google/syzkaller/blob/master/docs/syzbot.md#kmsan-bugs > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > index f3bd8e9..58d9aec 100644 > --- a/drivers/vhost/vhost.c > +++ b/drivers/vhost/vhost.c > @@ -2342,6 +2342,9 @@ struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type) > struct vhost_msg_node *node = kmalloc(sizeof *node, GFP_KERNEL); > if (!node) > return NULL; > + > + /* Make sure all padding within the structure is initialized. */ > + memset(&node->msg, 0, sizeof node->msg); > node->vq = vq; > node->msg.type = type; > return node; > > -- > You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group. > To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller-bugs+unsubscribe@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/20180507155534-mutt-send-email-mst%40kernel.org. > For more options, visit https://groups.google.com/d/optout. ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH net] vhost: Use kzalloc() to allocate vhost_msg_node 2018-05-07 13:03 ` Michael S. Tsirkin 2018-05-07 13:12 ` Dmitry Vyukov @ 2018-05-08 8:27 ` Kevin Easton 1 sibling, 0 replies; 30+ messages in thread From: Kevin Easton @ 2018-05-08 8:27 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Jason Wang, kvm, virtualization, netdev, linux-kernel, syzkaller-bugs On Mon, May 07, 2018 at 04:03:25PM +0300, Michael S. Tsirkin wrote: > On Fri, Apr 27, 2018 at 11:45:02AM -0400, Kevin Easton wrote: > > The struct vhost_msg within struct vhost_msg_node is copied to userspace, > > so it should be allocated with kzalloc() to ensure all structure padding > > is zeroed. > > > > Signed-off-by: Kevin Easton <kevin@guarana.org> > > Reported-by: syzbot+87cfa083e727a224754b@syzkaller.appspotmail.com > > --- > > drivers/vhost/vhost.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > > index f3bd8e9..1b84dcff 100644 > > --- a/drivers/vhost/vhost.c > > +++ b/drivers/vhost/vhost.c > > @@ -2339,7 +2339,7 @@ EXPORT_SYMBOL_GPL(vhost_disable_notify); > > /* Create a new message. */ > > struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type) > > { > > - struct vhost_msg_node *node = kmalloc(sizeof *node, GFP_KERNEL); > > + struct vhost_msg_node *node = kzalloc(sizeof *node, GFP_KERNEL); > > if (!node) > > return NULL; > > node->vq = vq; > > > Let's just init the msg though. > > OK it seems this is the best we can do for now, > we need a new feature bit to fix it for 32 bit > userspace on 64 bit kernels. > > Does the following help? Yes, the reproducer doesn't trigger the error with that patch applied. - Kevin ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [net] vhost: Use kzalloc() to allocate vhost_msg_node 2018-04-27 15:45 ` [PATCH net] vhost: Use kzalloc() to allocate vhost_msg_node Kevin Easton 2018-04-27 16:05 ` Michael S. Tsirkin 2018-05-07 13:03 ` Michael S. Tsirkin @ 2018-05-29 22:19 ` Guenter Roeck 2018-05-30 3:01 ` Michael S. Tsirkin 2 siblings, 1 reply; 30+ messages in thread From: Guenter Roeck @ 2018-05-29 22:19 UTC (permalink / raw) To: Kevin Easton Cc: Michael S. Tsirkin, Jason Wang, kvm, virtualization, netdev, linux-kernel, syzkaller-bugs On Fri, Apr 27, 2018 at 11:45:02AM -0400, Kevin Easton wrote: > The struct vhost_msg within struct vhost_msg_node is copied to userspace, > so it should be allocated with kzalloc() to ensure all structure padding > is zeroed. > > Signed-off-by: Kevin Easton <kevin@guarana.org> > Reported-by: syzbot+87cfa083e727a224754b@syzkaller.appspotmail.com Is this patch going anywhere ? The patch fixes CVE-2018-1118. It would be useful to understand if and when this problem is going to be fixed. Thanks, Guenter > --- > drivers/vhost/vhost.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > index f3bd8e9..1b84dcff 100644 > --- a/drivers/vhost/vhost.c > +++ b/drivers/vhost/vhost.c > @@ -2339,7 +2339,7 @@ EXPORT_SYMBOL_GPL(vhost_disable_notify); > /* Create a new message. */ > struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type) > { > - struct vhost_msg_node *node = kmalloc(sizeof *node, GFP_KERNEL); > + struct vhost_msg_node *node = kzalloc(sizeof *node, GFP_KERNEL); > if (!node) > return NULL; > node->vq = vq; ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [net] vhost: Use kzalloc() to allocate vhost_msg_node 2018-05-29 22:19 ` [net] " Guenter Roeck @ 2018-05-30 3:01 ` Michael S. Tsirkin 2018-05-30 3:42 ` Guenter Roeck 2018-06-04 12:34 ` Dmitry Vyukov via Virtualization 0 siblings, 2 replies; 30+ messages in thread From: Michael S. Tsirkin @ 2018-05-30 3:01 UTC (permalink / raw) To: Guenter Roeck Cc: Kevin Easton, kvm, netdev, syzkaller-bugs, linux-kernel, virtualization On Tue, May 29, 2018 at 03:19:08PM -0700, Guenter Roeck wrote: > On Fri, Apr 27, 2018 at 11:45:02AM -0400, Kevin Easton wrote: > > The struct vhost_msg within struct vhost_msg_node is copied to userspace, > > so it should be allocated with kzalloc() to ensure all structure padding > > is zeroed. > > > > Signed-off-by: Kevin Easton <kevin@guarana.org> > > Reported-by: syzbot+87cfa083e727a224754b@syzkaller.appspotmail.com > > Is this patch going anywhere ? > > The patch fixes CVE-2018-1118. It would be useful to understand if and when > this problem is going to be fixed. > > Thanks, > Guenter > > --- > > drivers/vhost/vhost.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > > index f3bd8e9..1b84dcff 100644 > > --- a/drivers/vhost/vhost.c > > +++ b/drivers/vhost/vhost.c > > @@ -2339,7 +2339,7 @@ EXPORT_SYMBOL_GPL(vhost_disable_notify); > > /* Create a new message. */ > > struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type) > > { > > - struct vhost_msg_node *node = kmalloc(sizeof *node, GFP_KERNEL); > > + struct vhost_msg_node *node = kzalloc(sizeof *node, GFP_KERNEL); > > if (!node) > > return NULL; > > node->vq = vq; As I pointed out, we don't need to init the whole structure. The proper fix is thus (I think) below. Could you use your testing infrastructure to confirm this fixes the issue? Thanks! Signed-off-by: Michael S. Tsirkin <mst@redhat.com> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c index f3bd8e941224..58d9aec90afb 100644 --- a/drivers/vhost/vhost.c +++ b/drivers/vhost/vhost.c @@ -2342,6 +2342,9 @@ struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type) struct vhost_msg_node *node = kmalloc(sizeof *node, GFP_KERNEL); if (!node) return NULL; + + /* Make sure all padding within the structure is initialized. */ + memset(&node->msg, 0, sizeof node->msg); node->vq = vq; node->msg.type = type; return node; ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [net] vhost: Use kzalloc() to allocate vhost_msg_node 2018-05-30 3:01 ` Michael S. Tsirkin @ 2018-05-30 3:42 ` Guenter Roeck 2018-06-04 12:34 ` Dmitry Vyukov via Virtualization 1 sibling, 0 replies; 30+ messages in thread From: Guenter Roeck @ 2018-05-30 3:42 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Kevin Easton, Jason Wang, kvm, virtualization, netdev, linux-kernel, syzkaller-bugs On 05/29/2018 08:01 PM, Michael S. Tsirkin wrote: > On Tue, May 29, 2018 at 03:19:08PM -0700, Guenter Roeck wrote: >> On Fri, Apr 27, 2018 at 11:45:02AM -0400, Kevin Easton wrote: >>> The struct vhost_msg within struct vhost_msg_node is copied to userspace, >>> so it should be allocated with kzalloc() to ensure all structure padding >>> is zeroed. >>> >>> Signed-off-by: Kevin Easton <kevin@guarana.org> >>> Reported-by: syzbot+87cfa083e727a224754b@syzkaller.appspotmail.com >> >> Is this patch going anywhere ? >> >> The patch fixes CVE-2018-1118. It would be useful to understand if and when >> this problem is going to be fixed. >> >> Thanks, >> Guenter >>> --- >>> drivers/vhost/vhost.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c >>> index f3bd8e9..1b84dcff 100644 >>> --- a/drivers/vhost/vhost.c >>> +++ b/drivers/vhost/vhost.c >>> @@ -2339,7 +2339,7 @@ EXPORT_SYMBOL_GPL(vhost_disable_notify); >>> /* Create a new message. */ >>> struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type) >>> { >>> - struct vhost_msg_node *node = kmalloc(sizeof *node, GFP_KERNEL); >>> + struct vhost_msg_node *node = kzalloc(sizeof *node, GFP_KERNEL); >>> if (!node) >>> return NULL; >>> node->vq = vq; > > As I pointed out, we don't need to init the whole structure. The proper > fix is thus (I think) below. > > Could you use your testing infrastructure to confirm this fixes the issue? > Sorry, I don't have the means to test the fix. Guenter > Thanks! > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > index f3bd8e941224..58d9aec90afb 100644 > --- a/drivers/vhost/vhost.c > +++ b/drivers/vhost/vhost.c > @@ -2342,6 +2342,9 @@ struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type) > struct vhost_msg_node *node = kmalloc(sizeof *node, GFP_KERNEL); > if (!node) > return NULL; > + > + /* Make sure all padding within the structure is initialized. */ > + memset(&node->msg, 0, sizeof node->msg); > node->vq = vq; > node->msg.type = type; > return node; > ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [net] vhost: Use kzalloc() to allocate vhost_msg_node 2018-05-30 3:01 ` Michael S. Tsirkin 2018-05-30 3:42 ` Guenter Roeck @ 2018-06-04 12:34 ` Dmitry Vyukov via Virtualization 1 sibling, 0 replies; 30+ messages in thread From: Dmitry Vyukov via Virtualization @ 2018-06-04 12:34 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Kevin Easton, KVM list, netdev, syzkaller-bugs, LKML, virtualization, Guenter Roeck On Wed, May 30, 2018 at 5:01 AM, Michael S. Tsirkin <mst@redhat.com> wrote: > On Tue, May 29, 2018 at 03:19:08PM -0700, Guenter Roeck wrote: >> On Fri, Apr 27, 2018 at 11:45:02AM -0400, Kevin Easton wrote: >> > The struct vhost_msg within struct vhost_msg_node is copied to userspace, >> > so it should be allocated with kzalloc() to ensure all structure padding >> > is zeroed. >> > >> > Signed-off-by: Kevin Easton <kevin@guarana.org> >> > Reported-by: syzbot+87cfa083e727a224754b@syzkaller.appspotmail.com >> >> Is this patch going anywhere ? >> >> The patch fixes CVE-2018-1118. It would be useful to understand if and when >> this problem is going to be fixed. >> >> Thanks, >> Guenter >> > --- >> > drivers/vhost/vhost.c | 2 +- >> > 1 file changed, 1 insertion(+), 1 deletion(-) >> > >> > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c >> > index f3bd8e9..1b84dcff 100644 >> > --- a/drivers/vhost/vhost.c >> > +++ b/drivers/vhost/vhost.c >> > @@ -2339,7 +2339,7 @@ EXPORT_SYMBOL_GPL(vhost_disable_notify); >> > /* Create a new message. */ >> > struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type) >> > { >> > - struct vhost_msg_node *node = kmalloc(sizeof *node, GFP_KERNEL); >> > + struct vhost_msg_node *node = kzalloc(sizeof *node, GFP_KERNEL); >> > if (!node) >> > return NULL; >> > node->vq = vq; > > As I pointed out, we don't need to init the whole structure. The proper > fix is thus (I think) below. > > Could you use your testing infrastructure to confirm this fixes the issue? Hi Michael, syzbot is self-service, see: https://github.com/google/syzkaller/blob/master/docs/syzbot.md#testing-patches > Thanks! > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > index f3bd8e941224..58d9aec90afb 100644 > --- a/drivers/vhost/vhost.c > +++ b/drivers/vhost/vhost.c > @@ -2342,6 +2342,9 @@ struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type) > struct vhost_msg_node *node = kmalloc(sizeof *node, GFP_KERNEL); > if (!node) > return NULL; > + > + /* Make sure all padding within the structure is initialized. */ > + memset(&node->msg, 0, sizeof node->msg); > node->vq = vq; > node->msg.type = type; > return node; > > -- > You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group. > To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller-bugs+unsubscribe@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/20180530055704-mutt-send-email-mst%40kernel.org. > For more options, visit https://groups.google.com/d/optout. ^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2018-06-07 19:29 UTC | newest] Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-04-23 16:56 KMSAN: uninit-value in _copy_to_iter (2) syzbot 2018-04-25 19:19 ` syzbot 2018-06-07 15:38 ` Michael S. Tsirkin 2018-06-07 15:38 ` syzbot 2018-06-07 16:25 ` Dmitry Vyukov via Virtualization 2018-06-07 17:04 ` syzbot 2018-06-07 17:43 ` Al Viro 2018-06-07 17:59 ` Michael S. Tsirkin 2018-06-07 18:04 ` Al Viro 2018-06-07 19:29 ` Michael S. Tsirkin 2018-06-07 17:10 ` Michael S. Tsirkin 2018-06-07 17:31 ` syzbot 2018-04-27 15:45 ` [PATCH net] vhost: Use kzalloc() to allocate vhost_msg_node Kevin Easton 2018-04-27 16:05 ` Michael S. Tsirkin 2018-04-27 16:11 ` Dmitry Vyukov via Virtualization 2018-04-27 16:15 ` Michael S. Tsirkin 2018-04-27 16:25 ` Dmitry Vyukov 2018-04-27 16:29 ` Dmitry Vyukov 2018-04-27 19:36 ` Michael S. Tsirkin 2018-04-29 8:10 ` Dmitry Vyukov 2018-04-28 1:07 ` Kevin Easton 2018-04-28 1:51 ` Kevin Easton 2018-04-28 2:23 ` Jason Wang 2018-05-07 13:03 ` Michael S. Tsirkin 2018-05-07 13:12 ` Dmitry Vyukov 2018-05-08 8:27 ` Kevin Easton 2018-05-29 22:19 ` [net] " Guenter Roeck 2018-05-30 3:01 ` Michael S. Tsirkin 2018-05-30 3:42 ` Guenter Roeck 2018-06-04 12:34 ` Dmitry Vyukov via Virtualization
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).