2024-04-19 09:39:36

by syzbot

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

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: [email protected]

=====================================================
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 [email protected].

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


2024-04-19 12:40:13

by Jeongjun Park

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


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


2024-04-19 14:44:10

by syzbot

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

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


2024-04-19 14:56:30

by Jeongjun Park

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


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);
if (!vsock)
return -ENOMEM;

--
2.34.1

2024-04-19 15:39:12

by syzbot

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

Hello,

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

Reported-and-tested-by: [email protected]

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.

2024-04-20 08:58:22

by Jeongjun Park

[permalink] [raw]
Subject: [PATCH virt] virt: fix uninit-value in vhost_vsock_dev_open

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

Reported-by: [email protected]
Fixes: dcda9b04713c ("mm, tree wide: replace __GFP_REPEAT by __GFP_RETRY_MAYFAIL with more useful semantic")
Signed-off-by: Jeongjun Park <[email protected]>
---
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);
if (!vsock)
return -ENOMEM;

--
2.34.1

2024-04-20 10:05:36

by Michael S. Tsirkin

[permalink] [raw]
Subject: Re: [PATCH virt] virt: fix uninit-value in vhost_vsock_dev_open

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: [email protected]
> Fixes: dcda9b04713c ("mm, tree wide: replace __GFP_REPEAT by __GFP_RETRY_MAYFAIL with more useful semantic")
> Signed-off-by: Jeongjun Park <[email protected]>

What value exactly is used uninitialized?

> ---
> 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);
> if (!vsock)
> return -ENOMEM;
>
> --
> 2.34.1


2024-04-21 03:06:29

by Jeongjun Park

[permalink] [raw]
Subject: Re: [PATCH virt] virt: fix uninit-value in vhost_vsock_dev_open

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

2024-04-22 13:00:50

by Stefan Hajnoczi

[permalink] [raw]
Subject: Re: [PATCH virt] virt: fix uninit-value in vhost_vsock_dev_open

On Sun, Apr 21, 2024 at 12:06:06PM +0900, Jeongjun Park wrote:
> 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.

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 <[email protected]>


Attachments:
(No filename) (1.10 kB)
signature.asc (499.00 B)
Download all attachments

2024-04-22 14:19:01

by Michael S. Tsirkin

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

On Fri, Apr 19, 2024 at 02:39:20AM -0700, syzbot wrote:
> 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: [email protected]
>
> =====================================================
> 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 [email protected].
>
> 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


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




2024-04-22 14:21:03

by Michael S. Tsirkin

[permalink] [raw]
Subject: Re: [PATCH virt] virt: fix uninit-value in vhost_vsock_dev_open

On Mon, Apr 22, 2024 at 09:00:31AM -0400, Stefan Hajnoczi wrote:
> On Sun, Apr 21, 2024 at 12:06:06PM +0900, Jeongjun Park wrote:
> > 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.
>
> 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 <[email protected]>



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 <[email protected]>
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: [email protected]
Reported-by: Jeongjun Park <[email protected]>
Fixes: ced7b713711f ("vhost/vsock: support SEQPACKET for transport").
Cc: Arseny Krasnov <[email protected]>
Cc: David S. Miller <[email protected]>
Cc: Stefan Hajnoczi <[email protected]>
Signed-off-by: Michael S. Tsirkin <[email protected]>

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];


2024-04-23 01:31:24

by syzbot

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

Hello,

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

Reported-and-tested-by: [email protected]

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.
Note: testing is done by a robot and is best-effort only.

2024-05-05 20:13:55

by Arseniy Krasnov

[permalink] [raw]
Subject: Re: [PATCH virt] virt: fix uninit-value in vhost_vsock_dev_open

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

Hi! Sorry for late reply! Just run vsock test suite with this patch -
seems everything is ok!

>
>
> commit bcc17a060d93b198d8a17a9b87b593f41337ee28
> Author: Michael S. Tsirkin <[email protected]>
> 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: [email protected]
> Reported-by: Jeongjun Park <[email protected]>
> Fixes: ced7b713711f ("vhost/vsock: support SEQPACKET for transport").
> Cc: Arseny Krasnov <[email protected]>
> Cc: David S. Miller <[email protected]>
> Cc: Stefan Hajnoczi <[email protected]>
> Signed-off-by: Michael S. Tsirkin <[email protected]>

Acked-by: Arseniy Krasnov <[email protected]>

>
> 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];