Re: [PATCH net] vsock/virtio: validate packet source for connected sockets

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The identical destination-only fallback vulnerability fixed in `virtio_transport` exists unpatched in `vmci_transport.c`.
--

--- Patch [1]: [PATCH net] vsock/virtio: validate packet source for connected sockets ---
commit cb88c0daae92eec924188a4684efd077dd9df9cd
Author: Daehyeon Ko <[email protected]>
Subject: vsock/virtio: validate packet source for connected sockets

This commit fixes a vulnerability in the virtio socket transport where
packets from an unrelated source could be incorrectly dispatched to a
non-listening socket via a destination-only fallback lookup.

> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
> index 8becad81279c8..f73e0406a790a 100644
> --- a/net/vmw_vsock/virtio_transport_common.c
> +++ b/net/vmw_vsock/virtio_transport_common.c

[ ... ]

> @@ -1822,11 +1822,15 @@ void virtio_transport_recv_pkt(struct virtio_transport *t,
>  
>  	lock_sock(sk);
>  
> -	/* Check if sk has been closed or assigned to another transport before
> -	 * lock_sock (note: listener sockets are not assigned to any transport)
> +	/* Check if sk has been closed, assigned to another transport, or if the
> +	 * packet is from a different peer than the one connected to sk.  These
> +	 * properties could have changed before lock_sock.  Listener sockets are
> +	 * not assigned to any transport and accept packets from any peer.
>  	 */
>  	if (sock_flag(sk, SOCK_DONE) ||
> -	    (sk->sk_state != TCP_LISTEN && vsk->transport != &t->transport)) {
> +	    (sk->sk_state != TCP_LISTEN &&
> +	     (vsk->transport != &t->transport ||
> +	      !vsock_addr_equals_addr(&src, &vsk->remote_addr)))) {

[Severity: High]
This isn't a bug introduced by this patch, but does the same vulnerability
exist unpatched in the VMCI transport?

Looking at net/vmw_vsock/vmci_transport.c:vmci_transport_recv_stream_cb(),
it appears to use the same fallback logic where it can return an established
socket if the connected socket lookup misses:

net/vmw_vsock/vmci_transport.c:vmci_transport_recv_stream_cb() {
    ...
    sk = vsock_find_bound_socket(&dst);
    ...
}

The packet is then processed by vmci_transport_recv_pkt_work() which
calls vmci_transport_recv_connected() for TCP_ESTABLISHED sockets:

net/vmw_vsock/vmci_transport.c:vmci_transport_recv_pkt_work() {
    ...
    case TCP_ESTABLISHED:
        vmci_transport_recv_connected(sk, pkt);
        break;
    ...
}

Neither of these functions appears to verify that the packet's source
address matches the socket's remote_addr before processing it.

Could this allow an attacker to inject control packets into an established
connection and cause a denial of service?

net/vmw_vsock/vmci_transport.c:vmci_transport_recv_connected() {
    ...
    case VMCI_TRANSPORT_PACKET_TYPE_RST:
        vsk = vsock_sk(sk);
        sock_set_flag(sk, SOCK_DONE);
        WRITE_ONCE(vsk->peer_shutdown, SHUTDOWN_MASK);
    ...
}

Should a similar validation be added to vmci_transport_recv_pkt_work() or
the VMCI handlers?

>  		(void)virtio_transport_reset_no_sock(t, skb, net);
>  		release_sock(sk);
>  		sock_put(sk);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1