KMSAN: uninit-value in _copy_to_iter (2)

[syzbot]

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+87cfa0...@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 syzk...@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.

[syzbot]

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+87cfa0...@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
==================================================================

[Kevin Easton]

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 <ke...@guarana.org>
Reported-by: syzbot+87cfa0...@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

[Michael S. Tsirkin]

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 <ke...@guarana.org>
> Reported-by: syzbot+87cfa0...@syzkaller.appspotmail.com

Does it help if a patch naming the padding is applied,
and then we init just the relevant field?
Just curious.

[Dmitry Vyukov]

On Fri, Apr 27, 2018 at 6:05 PM, Michael S. Tsirkin <m...@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 <ke...@guarana.org>
>> Reported-by: syzbot+87cfa0...@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-bug...@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.

[Michael S. Tsirkin]

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 <m...@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 <ke...@guarana.org>
> >> Reported-by: syzbot+87cfa0...@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?

[Dmitry Vyukov]

On Fri, Apr 27, 2018 at 6:15 PM, Michael S. Tsirkin <m...@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 <ke...@guarana.org>
>> >> Reported-by: syzbot+87cfa0...@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.

[Dmitry Vyukov]

On Fri, Apr 27, 2018 at 6:25 PM, Dmitry Vyukov <dvy...@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 <ke...@guarana.org>
>>> >> Reported-by: syzbot+87cfa0...@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.

[Michael S. Tsirkin]

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 <m...@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 <ke...@guarana.org>
> >> Reported-by: syzbot+87cfa0...@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?

[Kevin Easton]

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 <ke...@guarana.org>
> > Reported-by: syzbot+87cfa0...@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

[Kevin Easton]

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 <ke...@guarana.org>
> > > Reported-by: syzbot+87cfa0...@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

[Jason Wang]

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

[Dmitry Vyukov]

On Fri, Apr 27, 2018 at 9:36 PM, Michael S. Tsirkin <m...@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 <ke...@guarana.org>
>> >> Reported-by: syzbot+87cfa0...@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.

[Michael S. Tsirkin]

On Fri, Apr 27, 2018 at 11:45:02AM -0400, Kevin Easton wrote:

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 <m...@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;

[Dmitry Vyukov]

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 <m...@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-bug...@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.

[Kevin Easton]

Yes, the reproducer doesn't trigger the error with that patch applied.

- Kevin

[Guenter Roeck]

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 <ke...@guarana.org>
> Reported-by: syzbot+87cfa0...@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

[Michael S. Tsirkin]

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 <m...@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);

[Guenter Roeck]

Sorry, I don't have the means to test the fix.

Guenter

[Dmitry Vyukov]

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 <m...@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-bug...@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.

[Michael S. Tsirkin]

#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 <m...@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)

[syzbot]

> #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.

[Dmitry Vyukov]

On Thu, Jun 7, 2018 at 5:38 PM, Michael S. Tsirkin <m...@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 <m...@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-bug...@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.

[syzbot]

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger
crash:

Reported-and-tested-by:
syzbot+87cfa0...@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.

[Michael S. Tsirkin]

#syz test: https://github.com/google/kmsan.git master

[syzbot]

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger
crash:

Reported-and-tested-by:
syzbot+87cfa0...@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

[Al Viro]

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?

[Michael S. Tsirkin]

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.

[Al Viro]

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...

[Michael S. Tsirkin]

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