On Thu, Jan 09, 2025 at 10:01:31AM +0100, Stefano Garzarella wrote:

On Wed, Jan 08, 2025 at 02:31:19PM -0500, Hyunwoo Kim wrote:

quoted

On Wed, Jan 08, 2025 at 07:06:16PM +0100, 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)) {

If a race scenario with vsock_listen() is added to the existing
race scenario, the patch can be bypassed.

In addition to the existing scenario:

                    cpu0                                                      cpu1

                                                              socket(A)

                                                              bind(A, {cid: VMADDR_CID_LOCAL, port: 1024})
                                                                vsock_bind()

                                                              listen(A)
                                                                vsock_listen()
 socket(B)

 connect(B, {cid: VMADDR_CID_LOCAL, port: 1024})
   vsock_connect()
     lock_sock(sk);
     virtio_transport_connect()
       virtio_transport_connect()
         virtio_transport_send_pkt_info()
           vsock_loopback_send_pkt(VIRTIO_VSOCK_OP_REQUEST)
             queue_work(vsock_loopback_work)
     sk->sk_state = TCP_SYN_SENT;
     release_sock(sk);
                                                              vsock_loopback_work()
                                                                virtio_transport_recv_pkt(VIRTIO_VSOCK_OP_REQUEST)
                                                                  sk = vsock_find_bound_socket(&dst);
                                                                  virtio_transport_recv_listen(sk, skb)
                                                                    child = vsock_create_connected(sk);
                                                                    vsock_assign_transport()
                                                                      vvs = kzalloc(sizeof(*vvs), GFP_KERNEL);
                                                                    vsock_insert_connected(vchild);
                                                                      list_add(&vsk->connected_table, list);
                                                                    virtio_transport_send_response(vchild, skb);
                                                                      virtio_transport_send_pkt_info()
                                                                        vsock_loopback_send_pkt(VIRTIO_VSOCK_OP_RESPONSE)
                                                                          queue_work(vsock_loopback_work)

                                                              vsock_loopback_work()
                                                                virtio_transport_recv_pkt(VIRTIO_VSOCK_OP_RESPONSE)
                                                                  sk = vsock_find_bound_socket(&dst);
                                                                  lock_sock(sk);
                                                                  case TCP_SYN_SENT:
                                                                  virtio_transport_recv_connecting()
                                                                    sk->sk_state = TCP_ESTABLISHED;
                                                                  release_sock(sk);

                                                              kill(connect(B));
     lock_sock(sk);
     if (signal_pending(current)) {
     sk->sk_state = sk->sk_state == TCP_ESTABLISHED ? TCP_CLOSING : TCP_CLOSE;
     sock->state = SS_UNCONNECTED;    // [1]
     release_sock(sk);

 connect(B, {cid: VMADDR_CID_HYPERVISOR, port: 1024})
   vsock_connect(B)
     lock_sock(sk);
     vsock_assign_transport()
       virtio_transport_release()
         virtio_transport_close()
           if (!(sk->sk_state == TCP_ESTABLISHED || sk->sk_state == TCP_CLOSING))
           virtio_transport_shutdown()
             virtio_transport_send_pkt_info()
               vsock_loopback_send_pkt(VIRTIO_VSOCK_OP_SHUTDOWN)
                 queue_work(vsock_loopback_work)
           schedule_delayed_work(&vsk->close_work, VSOCK_CLOSE_TIMEOUT);	// [5]
       vsock_deassign_transport()
         vsk->transport = NULL;
       return -ESOCKTNOSUPPORT;
     release_sock(sk);
                                                              vsock_loopback_work()
                                                                virtio_transport_recv_pkt(VIRTIO_VSOCK_OP_SHUTDOWN)
                                                                  virtio_transport_recv_connected()
                                                                    virtio_transport_reset()
                                                                      virtio_transport_send_pkt_info()
                                                                        vsock_loopback_send_pkt(VIRTIO_VSOCK_OP_RST)
                                                                          queue_work(vsock_loopback_work)
 listen(B)
   vsock_listen()
     if (sock->state != SS_UNCONNECTED)    // [2]
     sk->sk_state = TCP_LISTEN;    // [3]

                                                              vsock_loopback_work()
                                                                virtio_transport_recv_pkt(VIRTIO_VSOCK_OP_RST)
								   if ((sk->sk_state != TCP_LISTEN && vsk->transport != &t->transport)) {    // [4]
								   ...
							
 virtio_transport_close_timeout()
   virtio_transport_do_close()
     vsock_stream_has_data()
       return vsk->transport->stream_has_data(vsk);    // null-ptr-deref

(Yes, This is quite a crazy scenario, but it can actually be induced)

Since sock->state is set to SS_UNCONNECTED during the first connect()[1],
it can pass the sock->state check[2] in vsock_listen() and set
sk->sk_state to TCP_LISTEN[3].
If this happens, the check in the patch with
`sk->sk_state != TCP_LISTEN` will pass[4], and a null-ptr-deref can
still occur.)

More specifically, because the sk_state has changed to TCP_LISTEN,
virtio_transport_recv_disconnecting() will not be called by the
loopback worker. However, a null-ptr-deref may occur in
virtio_transport_close_timeout(), which is scheduled by
virtio_transport_close() called in the flow of the second connect()[5].
(The patch no longer cancels the virtio_transport_close_timeout() worker)

And even if the `sk->sk_state != TCP_LISTEN` check is removed from the
patch, it seems that a null-ptr-deref will still occur due to
virtio_transport_close_timeout().
It might be necessary to add worker cancellation at the
appropriate location.

Thanks for the analysis!

Do you have time to cook a proper patch to cover this scenario?
Or we should mix this fix together with your patch (return 0 in
vsock_stream_has_data()) while we investigate a better handling?

Thanks,
Stefano

better combine them imho.