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]>
Tested-by: Arseniy Krasnov <[email protected]>
---
Reposting now it's been tested.
drivers/vhost/vsock.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
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];
--
MST
On Wed, May 15, 2024 at 11:05 PM Michael S. Tsirkin <[email protected]> wrote:
>
> 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]>
> Tested-by: Arseniy Krasnov <[email protected]>
>
Acked-by: Jason Wang <[email protected]>
Thanks
On Wed, May 15, 2024 at 11:05:43AM GMT, Michael S. Tsirkin wrote:
>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]>
>Tested-by: Arseniy Krasnov <[email protected]>
>
>---
>
>
>Reposting now it's been tested.
>
> drivers/vhost/vsock.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
Thanks for fixing this issue!
Reviewed-by: Stefano Garzarella <[email protected]>
On Wed, May 15, 2024 at 5:05 PM Michael S. Tsirkin <[email protected]> wrote:
>
> 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]>
> Tested-by: Arseniy Krasnov <[email protected]>
>
Reviewed-by: Eugenio Pérez <[email protected]>
Thanks!
> ---
>
>
> Reposting now it's been tested.
>
> drivers/vhost/vsock.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> 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];
> --
> MST
>
On Wed, 15 May 2024 11:05:43 -0400 Michael S. Tsirkin wrote:
> 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).
Acked-by: Jakub Kicinski <[email protected]>
--
pw-bot: nap