On Wed, Jan 08, 2025 at 07:06:16PM +0100, Stefano Garzarella wrote:
> 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 <v4bel@xxxxxxxxx>
> Reported-by: Wongi Lee <qwerty@xxxxxxxxx>
> Closes: https://lore.kernel.org/netdev/Z2LvdTTQR7dBmPb5@v4bel-B760M-AORUS-ELITE-AX/
> Signed-off-by: Stefano Garzarella <sgarzare@xxxxxxxxxx>
> ---
> 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.
