On Thu, Jan 09, 2025 at 02:34:28PM +0100, Michal Luczaj wrote:

On 1/8/25 19:06, Stefano Garzarella wrote:

quoted

If the socket has been de-assigned or assigned to another transport,
we must discard any packets received because they are not expected
and would cause issues when we access vsk->transport.

A possible scenario is described by Hyunwoo Kim in the attached link,
where after a first connect() interrupted by a signal, and a second
connect() failed, we can find `vsk->transport` at NULL, leading to a
NULL pointer dereference.

Fixes: c0cfa2d8a788 ("vsock: add multi-transports support")
Reported-by: Hyunwoo Kim <redacted>
Reported-by: Wongi Lee <redacted>
Closes: https://lore.kernel.org/netdev/Z2LvdTTQR7dBmPb5@v4bel-B760M-AORUS-ELITE-AX/ (local)
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 net/vmw_vsock/virtio_transport_common.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
index 9acc13ab3f82..51a494b69be8 100644
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c

@@ -1628,8 +1628,11 @@ void virtio_transport_recv_pkt(struct virtio_transport *t,

 	lock_sock(sk);

-	/* Check if sk has been closed before lock_sock */
-	if (sock_flag(sk, SOCK_DONE)) {
+	/* Check if sk has been closed or assigned to another transport before
+	 * lock_sock (note: listener sockets are not assigned to any transport)
+	 */
+	if (sock_flag(sk, SOCK_DONE) ||
+	    (sk->sk_state != TCP_LISTEN && vsk->transport != &t->transport)) {
 		(void)virtio_transport_reset_no_sock(t, skb);
 		release_sock(sk);
 		sock_put(sk);

FWIW, I've tried simplifying Hyunwoo's repro to toy with some tests. Ended
up with

from threading import *
from socket import *
from signal import *

def listener(tid):
while True:
	s = socket(AF_VSOCK, SOCK_SEQPACKET)
	s.bind((1, 1234))
	s.listen()
	pthread_kill(tid, SIGUSR1)

signal(SIGUSR1, lambda *args: None)
Thread(target=listener, args=[get_ident()]).start()

while True:
c = socket(AF_VSOCK, SOCK_SEQPACKET)
c.connect_ex((1, 1234))
c.connect_ex((42, 1234))

which gives me splats with or without this patch.

That said, when I apply this patch, but drop the `sk->sk_state !=
TCP_LISTEN &&`: no more splats.

We can't drop `sk->sk_state != TCP_LISTEN &&` because listener socket 
doesn't have any transport (vsk->transport == NULL), so every connection 
request will receive an error, so maybe this is the reason of no splats.

I'm cooking some more patches to fix Hyunwoo's scenario handling better 
the close work when the virtio destructor is called.

I'll run your reproduces to test it, thanks for that!

Stefano