netdev - Re: [PATCH] vsock: initialize the_virtio

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Thu, 19 Oct 2023 10:54:21 +0200
From: Stefano Garzarella <sgarzare@...hat.com>
To: Alexandru Matei <alexandru.matei@...ath.com>
Cc: Stefan Hajnoczi <stefanha@...hat.com>, 
	"David S . Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, kvm@...r.kernel.org, 
	virtualization@...ts.linux-foundation.org, netdev@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Mihai Petrisor <mihai.petrisor@...ath.com>, Viorel Canja <viorel.canja@...ath.com>
Subject: Re: [PATCH] vsock: initialize the_virtio_vsock before using VQs

On Wed, Oct 18, 2023 at 09:32:47PM +0300, Alexandru Matei wrote:
>Once VQs are filled with empty buffers and we kick the host, it can send
>connection requests. If 'the_virtio_vsock' is not initialized before,
>replies are silently dropped and do not reach the host.

Are replies really dropped or we just miss the notification?

Could the reverse now happen, i.e., the guest wants to send a connection 
request, finds the pointer assigned but can't use virtqueues because 
they haven't been initialized yet?

Perhaps to avoid your problem, we could just queue vsock->rx_work at the 
bottom of the probe to see if anything was queued in the meantime.

Nit: please use "vsock/virtio" to point out that this problem is of the 
virtio transport.

Thanks,
Stefano

>
>Fixes: 0deab087b16a ("vsock/virtio: use RCU to avoid use-after-free on the_virtio_vsock")
>Signed-off-by: Alexandru Matei <alexandru.matei@...ath.com>
>---
> net/vmw_vsock/virtio_transport.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
>diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c
>index e95df847176b..eae0867133f8 100644
>--- a/net/vmw_vsock/virtio_transport.c
>+++ b/net/vmw_vsock/virtio_transport.c
>@@ -658,12 +658,13 @@ static int virtio_vsock_probe(struct virtio_device *vdev)
> 		vsock->seqpacket_allow = true;
>
> 	vdev->priv = vsock;
>+	rcu_assign_pointer(the_virtio_vsock, vsock);
>
> 	ret = virtio_vsock_vqs_init(vsock);
>-	if (ret < 0)
>+	if (ret < 0) {
>+		rcu_assign_pointer(the_virtio_vsock, NULL);
> 		goto out;
>-
>-	rcu_assign_pointer(the_virtio_vsock, vsock);
>+	}
>
> 	mutex_unlock(&the_virtio_vsock_mutex);
>
>-- 
>2.34.1
>
>