Re: [PATCH RFC 6/8] vsock: allow datagrams to use directional transports

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] vsock/virtio: bypass of checks in virtio_transport_dgram_source_allow()
- [Medium] vsock: vsock_dgram_sendmsg() mutates connected vsk->remote_addr on fallback
--

commit 634f5c11de9d307a1a88027855c76c28c15f7d96
Author: Sergio Lopez <slp@redhat.com>

vsock: allow datagrams to use directional transports

This commit enables AF_VSOCK datagram sockets to initialize through and
route across directional transports (such as virtio, vhost, and loopback)
when a dedicated datagram transport is not registered.

> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> index dc1add2d2e896..6419498a6fe27 100644
> --- a/net/vmw_vsock/af_vsock.c
> +++ b/net/vmw_vsock/af_vsock.c
[ ... ]
> @@ -1493,7 +1606,8 @@ static int vsock_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
>  		remote_addr = &vsk->remote_addr;
>  
>  		if (remote_addr->svm_cid == VMADDR_CID_ANY)
> -			remote_addr->svm_cid = transport->get_local_cid();
> +			remote_addr->svm_cid = transport->dgram_transport_match ?
> +				VMADDR_CID_LOCAL : transport->get_local_cid();
>  
>  		/* XXX Should connect() or this function ensure remote_addr is
>  		 * bound?
> @@ -1507,13 +1621,14 @@ static int vsock_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
>  		goto out;
>  	}
>  
> -	if (!transport->dgram_allow(vsk, remote_addr->svm_cid,
> -				    remote_addr->svm_port)) {
> -		err = -EINVAL;
> +	transport = vsock_dgram_get_transport(vsk, remote_addr);
> +	if (IS_ERR(transport)) {
> +		err = PTR_ERR(transport);
>  		goto out;
>  	}

[Severity: Medium]
When vsock_dgram_sendmsg() is called without msg_name on a connected
socket, remote_addr points directly to &vsk->remote_addr.

If the socket connected to an active H2G guest CID (leaving
vsk->remote_addr.svm_flags without VMADDR_FLAG_TO_HOST) and that guest is
temporarily unavailable while g2h_fallback is enabled,
vsock_dgram_get_transport() takes the fallback path and mutates
remote_addr in place:

net/vmw_vsock/af_vsock.c:vsock_dgram_get_transport() {
	...
	} else if (sock_net(sk)->vsock.g2h_fallback && transport_g2h &&
		   transport_g2h->has_remote_cid &&
		   transport_g2h->has_remote_cid(vsk, cid)) {
		transport = transport_g2h;
		to_host = true;
	}
	...
	if (to_host)
		remote_addr->svm_flags |= VMADDR_FLAG_TO_HOST;
}

Because vsock_dgram_get_transport() checks VMADDR_FLAG_TO_HOST before
transport_h2g->has_remote_cid(), won't this permanently pin
vsk->remote_addr to transport_g2h for subsequent sends, receive source
checks, and getpeername() even after the H2G guest restarts?

> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
> index ae33630d99e71..7c1ec8feb853c 100644
> --- a/net/vmw_vsock/virtio_transport_common.c
> +++ b/net/vmw_vsock/virtio_transport_common.c
> @@ -322,13 +322,18 @@ static int virtio_transport_send_pkt_info(struct vsock_sock *vsk,
>  
>  	info->type = virtio_transport_get_type(sk_vsock(vsk));
>  
> -	t_ops = virtio_transport_get_ops(vsk);
> +	t_ops = info->t_ops ?: virtio_transport_get_ops(vsk);
>  	if (unlikely(!t_ops))
>  		return -EFAULT;
>  
>  	src_cid = t_ops->transport.get_local_cid();
> +	/* Preserve explicit datagram bindings so loopback replies can find them. */
> +	if (info->type == VIRTIO_VSOCK_TYPE_DGRAM &&
> +	    src_cid == VMADDR_CID_LOCAL &&
> +	    vsk->local_addr.svm_cid != VMADDR_CID_ANY)
> +		src_cid = vsk->local_addr.svm_cid;
>  	src_port = vsk->local_addr.svm_port;
[ ... ]
> @@ -1861,6 +1877,33 @@ static bool virtio_transport_valid_type(u16 type)
>  	       (type == VIRTIO_VSOCK_TYPE_DGRAM);
>  }
>  
> +static bool
> +virtio_transport_dgram_source_allow(struct vsock_sock *vsk,
> +				    const struct vsock_transport *transport,
> +				    const struct sockaddr_vm *src)
> +{
> +	const struct vsock_transport *peer_transport;
> +	struct sockaddr_vm peer = vsk->remote_addr;
> +	bool allow;
> +
> +	if (!virtio_transport_dgram_transport_match(vsk->transport) ||
> +	    !virtio_transport_dgram_transport_match(transport))
> +		return false;
> +	/* Shutdown preserves the peer; only disconnect removes it. */
> +	if (sk_vsock(vsk)->sk_socket->state == SS_UNCONNECTED)
> +		return true;
> +
> +	peer_transport = vsock_dgram_get_transport(vsk, &peer);
> +	if (IS_ERR(peer_transport))
> +		return false;
> +	allow = peer_transport == transport &&
> +		src->svm_port == vsk->remote_addr.svm_port &&
> +		(src->svm_cid == vsk->remote_addr.svm_cid ||
> +		 transport->get_local_cid() == VMADDR_CID_LOCAL);
> +	module_put(peer_transport->module);
> +	return allow;
> +}
> +
>  /* We are under the virtio-vsock's vsock->rx_lock or vhost-vsock's vq->mutex
>   * lock.
>   */
> @@ -1911,14 +1954,14 @@ void virtio_transport_recv_pkt(struct virtio_transport *t,
>  		vsk = vsock_sk(sk);
>  		lock_sock(sk);
>  		if (sock_flag(sk, SOCK_DEAD) ||
> -		    vsk->transport != &t->transport ||
> -		    (sk->sk_socket->state != SS_UNCONNECTED &&
> -		     !vsock_check_source(vsk, &t->transport, &src))) {
> +		    !virtio_transport_dgram_source_allow(vsk, &t->transport, &src)) {
>  			release_sock(sk);
>  			sock_put(sk);
>  			goto free_pkt;
>  		}

[Severity: High]
Can virtio_transport_dgram_source_allow() bypass feature negotiation,
local CID transport isolation, and connected peer CID checks?

First, for SS_UNCONNECTED sockets, virtio_transport_dgram_source_allow()
returns true immediately after virtio_transport_dgram_transport_match(),
which is a static callback comparison that succeeds for virtio_transport,
vhost_transport, and loopback_transport without calling
transport->dgram_allow(vsk, src->svm_cid, src->svm_port).

Since neither vhost_vsock_handle_tx_kick() nor virtio_transport_rx_work()
checks vsock->dgram_allow before calling virtio_transport_recv_pkt(), a
peer that has not negotiated VIRTIO_VSOCK_F_DGRAM can deliver
VIRTIO_VSOCK_TYPE_DGRAM packets to any unconnected bound socket on the
host (bypassing vhost_transport_dgram_allow()) or on a guest initialized
via the vhost_transport fallback (bypassing
virtio_transport_dgram_allow_check()).

Second, returning true early for SS_UNCONNECTED sockets skips checking
whether vsk->local_addr.svm_cid is compatible with the receiving
transport. Because virtio_transport_rx_work() does not validate
hdr->dst_cid against the guest CID and __vsock_find_bound_socket_net()
matches dst.svm_cid == vsk->local_addr.svm_cid or VMADDR_CID_ANY, an
external peer on virtio_transport can send datagrams addressed to
VMADDR_CID_LOCAL, VMADDR_CID_HOST, or VMADDR_CID_ANY and reach an
unconnected socket bound to VMADDR_CID_LOCAL or VMADDR_CID_HOST.

Third, on loopback_transport, virtio_transport_send_pkt_info() now
preserves explicit vsk->local_addr.svm_cid values in hdr->src_cid, and
__vsock_find_bound_socket_net() allows distinct local sockets to bind to
the same port on different local CIDs (such as VMADDR_CID_LOCAL and the
guest or host CID). Because transport->get_local_cid() == VMADDR_CID_LOCAL
is always true for loopback_transport, won't a local socket bound to
(VMADDR_CID_LOCAL, port) be accepted by a socket connected to (guest_cid,
port) despite having a mismatched src->svm_cid?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028-vsock-dgram-support-v1-0-e457cf7cee78@sinrega.org?part=6