[syzbot] [virt?] [net?] KMSAN: uninit-value in vsock_assign_transport (2)

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit: 8cd26fd90c1a Merge tag 'for-6.9-rc4-tag' of git://git.kern..
git tree: upstream
console+strace: https://syzkaller.appspot.com/x/log.txt?x=102d27cd180000
kernel config: https://syzkaller.appspot.com/x/.config?x=87a805e655619c64
dashboard link: https://syzkaller.appspot.com/bug?extid=6c21aeb59d0e82eb2782
compiler: Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=16e38c3b180000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=10e62fed180000

Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/488822aee24a/disk-8cd26fd9.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/ba40e322ba00/vmlinux-8cd26fd9.xz
kernel image: https://storage.googleapis.com/syzbot-assets/f30af1dfbc30/bzImage-8cd26fd9.xz

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+6c21ae...@syzkaller.appspotmail.com

=====================================================
BUG: KMSAN: uninit-value in vsock_assign_transport+0xb2a/0xb90 net/vmw_vsock/af_vsock.c:500
vsock_assign_transport+0xb2a/0xb90 net/vmw_vsock/af_vsock.c:500
vsock_connect+0x544/0x1560 net/vmw_vsock/af_vsock.c:1393
__sys_connect_file net/socket.c:2048 [inline]
__sys_connect+0x606/0x690 net/socket.c:2065
__do_sys_connect net/socket.c:2075 [inline]
__se_sys_connect net/socket.c:2072 [inline]
__x64_sys_connect+0x91/0xe0 net/socket.c:2072
x64_sys_call+0x3356/0x3b50 arch/x86/include/generated/asm/syscalls_64.h:43
do_syscall_x64 arch/x86/entry/common.c:52 [inline]
do_syscall_64+0xcf/0x1e0 arch/x86/entry/common.c:83
entry_SYSCALL_64_after_hwframe+0x77/0x7f

Uninit was created at:
__kmalloc_large_node+0x231/0x370 mm/slub.c:3921
__do_kmalloc_node mm/slub.c:3954 [inline]
__kmalloc_node+0xb07/0x1060 mm/slub.c:3973
kmalloc_node include/linux/slab.h:648 [inline]
kvmalloc_node+0xc0/0x2d0 mm/util.c:634
kvmalloc include/linux/slab.h:766 [inline]
vhost_vsock_dev_open+0x44/0x510 drivers/vhost/vsock.c:659
misc_open+0x66b/0x760 drivers/char/misc.c:165
chrdev_open+0xa5f/0xb80 fs/char_dev.c:414
do_dentry_open+0x11f1/0x2120 fs/open.c:955
vfs_open+0x7e/0xa0 fs/open.c:1089
do_open fs/namei.c:3642 [inline]
path_openat+0x4a3c/0x5b00 fs/namei.c:3799
do_filp_open+0x20e/0x590 fs/namei.c:3826
do_sys_openat2+0x1bf/0x2f0 fs/open.c:1406
do_sys_open fs/open.c:1421 [inline]
__do_sys_openat fs/open.c:1437 [inline]
__se_sys_openat fs/open.c:1432 [inline]
__x64_sys_openat+0x2a1/0x310 fs/open.c:1432
x64_sys_call+0x3a64/0x3b50 arch/x86/include/generated/asm/syscalls_64.h:258
do_syscall_x64 arch/x86/entry/common.c:52 [inline]
do_syscall_64+0xcf/0x1e0 arch/x86/entry/common.c:83
entry_SYSCALL_64_after_hwframe+0x77/0x7f

CPU: 1 PID: 5021 Comm: syz-executor390 Not tainted 6.9.0-rc4-syzkaller-00038-g8cd26fd90c1a #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 03/27/2024
=====================================================


---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzk...@googlegroups.com.

syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.

If the report is already addressed, let syzbot know by replying with:
#syz fix: exact-commit-title

If you want syzbot to run the reproducer, reply with:
#syz test: git://repo/address.git branch-or-commit-hash
If you attach or paste a git patch, syzbot will apply it before testing.

If you want to overwrite report's subsystems, reply with:
#syz set subsystems: new-subsystem
(See the list of subsystem names on the web dashboard)

If the report is a duplicate of another one, reply with:
#syz dup: exact-subject-of-another-report

If you want to undo deduplication, reply with:
#syz undup

[Jeongjun Park]

please test uninit-value in vsock_assign_transport

#syz test git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master

---
drivers/vhost/vsock.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
index ec20ecff85c7..652ef97a444b 100644
--- a/drivers/vhost/vsock.c
+++ b/drivers/vhost/vsock.c
@@ -656,7 +656,7 @@ static int vhost_vsock_dev_open(struct inode *inode, struct file *file)
/* This struct is large and allocation could fail, fall back to vmalloc
* if there is no other way.
*/
- vsock = kvmalloc(sizeof(*vsock), GFP_KERNEL | __GFP_RETRY_MAYFAIL);
+ vsock = kvzmalloc(sizeof(*vsock), GFP_KERNEL | __GFP_RETRY_MAYFAIL);
if (!vsock)
return -ENOMEM;

--
2.34.1

[syzbot]

Hello,

syzbot tried to test the proposed patch but the build/boot failed:

drivers/vhost/vsock.c:659:10: error: call to undeclared function 'kvzmalloc'; ISO C99 and later do not support implicit function declarations [-Werror,-Wimplicit-function-declaration]
drivers/vhost/vsock.c:659:8: error: incompatible integer to pointer conversion assigning to 'struct vhost_vsock *' from 'int' [-Wint-conversion]


Tested on:

commit: 2668e3ae Merge tag 'scsi-fixes' of git://git.kernel.or..
git tree: upstream

kernel config: https://syzkaller.appspot.com/x/.config?x=87a805e655619c64
dashboard link: https://syzkaller.appspot.com/bug?extid=6c21aeb59d0e82eb2782
compiler: Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40

patch: https://syzkaller.appspot.com/x/patch.diff?x=120f5af5180000

[Jeongjun Park]

please test uninit-value in vsock_assign_transport

#syz test git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master

---
drivers/vhost/vsock.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
index ec20ecff85c7..652ef97a444b 100644
--- a/drivers/vhost/vsock.c
+++ b/drivers/vhost/vsock.c
@@ -656,7 +656,7 @@ static int vhost_vsock_dev_open(struct inode *inode, struct file *file)
/* This struct is large and allocation could fail, fall back to vmalloc
* if there is no other way.
*/
- vsock = kvmalloc(sizeof(*vsock), GFP_KERNEL | __GFP_RETRY_MAYFAIL);

+ vsock = kvzalloc(sizeof(*vsock), GFP_KERNEL | __GFP_RETRY_MAYFAIL);

[syzbot]

Hello,

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

Reported-and-tested-by: syzbot+6c21ae...@syzkaller.appspotmail.com

Tested on:

commit: 2668e3ae Merge tag 'scsi-fixes' of git://git.kernel.or..
git tree: upstream

console output: https://syzkaller.appspot.com/x/log.txt?x=1629bf67180000

kernel config: https://syzkaller.appspot.com/x/.config?x=87a805e655619c64
dashboard link: https://syzkaller.appspot.com/bug?extid=6c21aeb59d0e82eb2782
compiler: Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40

patch: https://syzkaller.appspot.com/x/patch.diff?x=16ef09d7180000

Note: testing is done by a robot and is best-effort only.

[Jeongjun Park]

Change vhost_vsock_dev_open() to use kvzalloc() instead of kvmalloc()
to avoid uninit state.

Reported-by: syzbot+6c21ae...@syzkaller.appspotmail.com
Fixes: dcda9b04713c ("mm, tree wide: replace __GFP_REPEAT by __GFP_RETRY_MAYFAIL with more useful semantic")
Signed-off-by: Jeongjun Park <aha3...@gmail.com>

[Michael S. Tsirkin]

On Sat, Apr 20, 2024 at 05:57:50PM +0900, Jeongjun Park wrote:
> Change vhost_vsock_dev_open() to use kvzalloc() instead of kvmalloc()
> to avoid uninit state.
>
> Reported-by: syzbot+6c21ae...@syzkaller.appspotmail.com
> Fixes: dcda9b04713c ("mm, tree wide: replace __GFP_REPEAT by __GFP_RETRY_MAYFAIL with more useful semantic")
> Signed-off-by: Jeongjun Park <aha3...@gmail.com>

What value exactly is used uninitialized?

[Jeongjun Park]

static bool vhost_transport_seqpacket_allow(u32 remote_cid)
{
....
vsock = vhost_vsock_get(remote_cid);

if (vsock)
seqpacket_allow = vsock->seqpacket_allow;
....
}

I think this is due to reading a previously created uninitialized
vsock->seqpacket_allow inside vhost_transport_seqpacket_allow(),
which is executed by the function pointer present in the if statement.

Thanks

[Stefan Hajnoczi]

CCing Arseny, author of commit ced7b713711f ("vhost/vsock: support
SEQPACKET for transport").

Looks like a genuine bug in the commit. vhost_vsock_set_features() sets
seqpacket_allow to true when the feature is negotiated. The assumption
is that the field defaults to false.

The rest of the vhost_vsock.ko code is written to initialize the
vhost_vsock fields, so you could argue seqpacket_allow should just be
explicitly initialized to false.

However, eliminating this class of errors by zeroing seems reasonable in
this code path. vhost_vsock_dev_open() is not performance-critical.

Acked-by: Stefan Hajnoczi <stef...@redhat.com>

[Michael S. Tsirkin]

#syz test: https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git bcc17a060d93b198d8a17a9b87b593f41337ee28

[Michael S. Tsirkin]

But now that it's explained, the bugfix as proposed is incomplete:
userspace can set features twice and the second time will leak
old VIRTIO_VSOCK_F_SEQPACKET bit value.

And I am pretty sure the Fixes tag is wrong.

So I wrote this, but I actually don't have a set for
seqpacket to test this. Arseny could you help test maybe?
Thanks!


commit bcc17a060d93b198d8a17a9b87b593f41337ee28
Author: Michael S. Tsirkin <m...@redhat.com>
Date: Mon Apr 22 10:03:13 2024 -0400

vhost/vsock: always initialize seqpacket_allow

There are two issues around seqpacket_allow:
1. seqpacket_allow is not initialized when socket is
created. Thus if features are never set, it will be
read uninitialized.
2. if VIRTIO_VSOCK_F_SEQPACKET is set and then cleared,
then seqpacket_allow will not be cleared appropriately
(existing apps I know about don't usually do this but
it's legal and there's no way to be sure no one relies
on this).

To fix:
- initialize seqpacket_allow after allocation
- set it unconditionally in set_features

Reported-by: syzbot+6c21ae...@syzkaller.appspotmail.com
Reported-by: Jeongjun Park <aha3...@gmail.com>
Fixes: ced7b713711f ("vhost/vsock: support SEQPACKET for transport").
Cc: Arseny Krasnov <arseny....@kaspersky.com>
Cc: David S. Miller <da...@davemloft.net>
Cc: Stefan Hajnoczi <stef...@redhat.com>
Signed-off-by: Michael S. Tsirkin <m...@redhat.com>

diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
index ec20ecff85c7..bf664ec9341b 100644
--- a/drivers/vhost/vsock.c
+++ b/drivers/vhost/vsock.c
@@ -667,6 +667,7 @@ static int vhost_vsock_dev_open(struct inode *inode, struct file *file)
}

vsock->guest_cid = 0; /* no CID assigned yet */
+ vsock->seqpacket_allow = false;

atomic_set(&vsock->queued_replies, 0);

@@ -810,8 +811,7 @@ static int vhost_vsock_set_features(struct vhost_vsock *vsock, u64 features)
goto err;
}

- if (features & (1ULL << VIRTIO_VSOCK_F_SEQPACKET))
- vsock->seqpacket_allow = true;
+ vsock->seqpacket_allow = features & (1ULL << VIRTIO_VSOCK_F_SEQPACKET);

for (i = 0; i < ARRAY_SIZE(vsock->vqs); i++) {
vq = &vsock->vqs[i];

[syzbot]

Hello,

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

Reported-and-tested-by: syzbot+6c21ae...@syzkaller.appspotmail.com

Tested on:

commit: bcc17a06 vhost/vsock: always initialize seqpacket_allow
git tree: https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git
console output: https://syzkaller.appspot.com/x/log.txt?x=12b58abb180000

kernel config: https://syzkaller.appspot.com/x/.config?x=87a805e655619c64
dashboard link: https://syzkaller.appspot.com/bug?extid=6c21aeb59d0e82eb2782
compiler: Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40

Note: no patches were applied.