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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential
issues to consider:
- [High] vsock: Hyper-V stream connections broken in
  vsock_use_local_transport()
- [High] vsock: vsock_select_transport() routes VMADDR_CID_HOST
  datagrams to VMCI
- [High] vsock: explicit CID bind failure in vmci_transport_dgram_bind()
- [High] vsock/vmci: local netns isolation bypass in
  vmci_transport_dgram_bind()
- [Medium] vsock: broken VMADDR_CID_ANY handling in
  vsock_dgram_connect() and sendmsg()
- [Medium] vsock: bind rollback TOCTOU race in
  virtio_transport_recv_pkt()
- [Medium] vsock/virtio: virtio_transport_recv_pkt() sets TO_HOST on
  host datagrams
- [Low] vsock/virtio: unused to_host field in struct virtio_vsock_skb_cb
--

commit 99283d4e6bf0b4f58509834e3f59987834c1907c
Author: Sergio Lopez <slp@redhat.com>

vsock: allow datagrams to use directional transports

This commit routes AF_VSOCK datagrams across directional transports,
loopback, and the VMCI fallback transport while unifying datagram
receive queuing in the core socket layer.

Link: https://lore.kernel.org/all/DMGNAV67LK3F.2FNLTD5NVHDEA@gmail.com/

> diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h
> index f91704731057e..16b869085a0ab 100644
> --- a/include/linux/virtio_vsock.h
> +++ b/include/linux/virtio_vsock.h
> @@ -12,6 +12,7 @@
>  struct virtio_vsock_skb_cb {
>  	bool reply;
>  	bool tap_delivered;
> +	bool to_host;
>  	u32 offset;
>  };

[Severity: Low]
This isn't a bug, but struct virtio_vsock_skb_cb::to_host is never read or
written anywhere in the patch (receive-side VMADDR_FLAG_TO_HOST is stored
in VSOCK_DGRAM_SKB_CB(skb)->src.svm_flags instead).

> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> index aef5389eeca2c..7e52c7e5583c2 100644
> --- a/net/vmw_vsock/af_vsock.c
> +++ b/net/vmw_vsock/af_vsock.c

[ ... ]

> @@ -568,6 +569,8 @@ EXPORT_SYMBOL_GPL(vsock_enqueue_accept);
>  
>  static bool vsock_use_local_transport(unsigned int remote_cid)
>  {
> +	u32 local_cid;
> +
>  	lockdep_assert_held(&vsock_register_mutex);
>  
>  	if (!transport_local)
> @@ -576,11 +579,50 @@ static bool vsock_use_local_transport(unsigned int remote_cid)
>  	if (remote_cid == VMADDR_CID_LOCAL)
>  		return true;
>  
> -	if (transport_g2h) {
> -		return remote_cid == transport_g2h->get_local_cid();
> -	} else {
> -		return remote_cid == VMADDR_CID_HOST;
> -	}
> +	local_cid = transport_g2h ? transport_g2h->get_local_cid() :
> +		VMADDR_CID_ANY;
> +	if (local_cid == VMADDR_CID_ANY)
> +		local_cid = VMADDR_CID_HOST;
> +
> +	return remote_cid == local_cid;
> +}

[Severity: High]
Does replacing VMADDR_CID_ANY with VMADDR_CID_HOST here break Hyper-V
SOCK_STREAM connections when vsock_loopback is loaded?

hvs_transport registers as transport_g2h and its get_local_cid() callback
(hvs_get_local_cid()) always returns VMADDR_CID_ANY because Hyper-V guests
do not use a numeric guest CID.  With this change,
vsock_use_local_transport(VMADDR_CID_HOST) returns true whenever
transport_local is registered, so vsock_select_transport() routes
outgoing guest-to-host connect() calls to loopback_transport instead of
hvs_transport, and incoming host connections in hvs_open_connection()
(which sets vnew->remote_addr.svm_cid = VMADDR_CID_HOST) fail the
!hvs_check_transport(vnew) check and are dropped.

> +
> +/* Select a route without changing socket ownership. The caller holds
> + * vsock_register_mutex and pins the returned transport before releasing it.
> + */
> +static const struct vsock_transport *
> +vsock_select_transport(struct vsock_sock *vsk,
> +		       const struct sockaddr_vm *remote_addr,
> +		       const struct vsock_transport *dgram_owner,
> +		       bool *to_host)
> +{
> +	struct sock *sk = sk_vsock(vsk);
> +	u32 cid = remote_addr->svm_cid;
> +
> +	lockdep_assert_held(&vsock_register_mutex);
> +	*to_host = false;
> +
> +	if (vsock_use_local_transport(cid))
> +		return transport_local;
> +	/* VMCI may use a different local CID from the virtio transports. */
> +	if (dgram_owner && dgram_owner->dgram_fallback &&
> +	    cid == dgram_owner->get_local_cid())
> +		return dgram_owner;
> +	if (cid <= VMADDR_CID_HOST ||
> +	    (remote_addr->svm_flags & VMADDR_FLAG_TO_HOST))
> +		return transport_g2h;

[Severity: High]
On a virtio guest with vmw_vsock_vmci_transport loaded, doesn't checking
cid == dgram_owner->get_local_cid() before cid <= VMADDR_CID_HOST misroute
VMADDR_CID_HOST datagrams to VMCI instead of virtio_transport?

drivers/misc/vmw_vmci/vmci_driver.c:vmci_get_context_id() {
	if (vmci_guest_code_active())
		return vmci_get_vm_context_id();
	else if (vmci_host_code_active())
		return VMCI_HOST_CONTEXT_ID;

	return VMCI_INVALID_ID;
}

Because vmci_drv_init() initializes the host device unconditionally on
module load, vmci_host_code_active() is true on any guest without a VMware
PCI device, so vmci_transport_get_local_cid() returns VMCI_HOST_CONTEXT_ID
(2, which equals VMADDR_CID_HOST).  When a guest SOCK_DGRAM socket sends
or connects to VMADDR_CID_HOST, cid == dgram_owner->get_local_cid()
matches vmci_transport before reaching cid <= VMADDR_CID_HOST, causing
sends to fail in vmci_datagram_send() with -EHOSTUNREACH and connected
sockets to reject incoming virtio datagrams from the host in
virtio_transport_dgram_source_allow().

[ ... ]

> @@ -629,33 +673,28 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
>  	    vsk->remote_addr.svm_cid > VMADDR_CID_HOST)
>  		vsk->remote_addr.svm_flags |= VMADDR_FLAG_TO_HOST;
>  
> -	remote_flags = vsk->remote_addr.svm_flags;
> -
>  	mutex_lock(&vsock_register_mutex);
>  
>  	switch (sk->sk_type) {
>  	case SOCK_DGRAM:
> -		new_transport = transport_dgram;
> +		/* VMCI must own bind() to create its receive handle, even when
> +		 * directional transports serve individual destinations.
> +		 */
> +		if (transport_dgram_fallback)
> +			new_transport = transport_dgram_fallback;
> +		else if (transport_g2h)
> +			new_transport = transport_g2h;
> +		else if (transport_h2g)
> +			new_transport = transport_h2g;
> +		else
> +			new_transport = transport_local;
>  		break;

[Severity: High]
Should vsock_assign_transport() (or vmci_transport_dgram_bind()) avoid
allocating a global VMCI datagram handle when the socket's netns is in
VSOCK_NET_MODE_LOCAL?

Assigning transport_dgram_fallback to SOCK_DGRAM sockets in a
VSOCK_NET_MODE_LOCAL namespace causes vmci_transport_dgram_bind() to call
vmci_transport_datagram_create_hnd(), which reserves the port in the
system-wide vmci_resource_table and registers vmci_transport_recv_dgram_cb().
Because vmci_resource_table is global and vmci_transport_recv_dgram_cb()
does not check vsock_net_mode_global(vsk), sockets in local-mode
namespaces collide on ports with init_net and can receive global VMCI
datagrams across the namespace isolation boundary.

[ ... ]

> @@ -1498,7 +1671,8 @@ static int vsock_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
>  		 */
>  
>  		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();
>  
>  		if (!vsock_addr_bound(remote_addr)) {
>  			err = -EINVAL;
> @@ -1508,27 +1682,32 @@ 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?
>  		 */
> -		if (!vsock_addr_bound(&vsk->remote_addr)) {
> +		if (!vsock_addr_bound(remote_addr)) {
>  			err = -EINVAL;
>  			goto out;
>  		}
> +		/* A fallback must not pin subsequent sends to the host. */
> +		connected_addr = *remote_addr;
> +		remote_addr = &connected_addr;
>  	} else {
>  		err = -EINVAL;
>  		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;
>  	}
>  
> -	err = transport->dgram_enqueue(vsk, remote_addr, msg, len);
> +	err = transport->dgram_enqueue(vsk, transport, remote_addr, msg, len);
> +	module_put(transport->module);
>  
>  out:
>  	release_sock(sk);
> @@ -1542,6 +1721,7 @@ static int vsock_dgram_connect(struct socket *sock,
>  	struct sock *sk;
>  	struct vsock_sock *vsk;
>  	struct sockaddr_vm *remote_addr;
> +	const struct vsock_transport *transport;
>  
>  	sk = sock->sk;
>  	vsk = vsock_sk(sk);
> @@ -1563,11 +1743,16 @@ static int vsock_dgram_connect(struct socket *sock,
>  	if (err)
>  		goto out;
>  
> -	if (!vsk->transport->dgram_allow(vsk, remote_addr->svm_cid,
> -					 remote_addr->svm_port)) {
> -		err = -EINVAL;
> +	if (remote_addr->svm_cid == VMADDR_CID_ANY &&
> +	    vsk->transport->dgram_transport_match)
> +		remote_addr->svm_cid = VMADDR_CID_LOCAL;
> +
> +	transport = vsock_dgram_get_transport(vsk, remote_addr);
> +	if (IS_ERR(transport)) {
> +		err = PTR_ERR(transport);
>  		goto out;
>  	}
> +	module_put(transport->module);
>  
>  	memcpy(&vsk->remote_addr, remote_addr, sizeof(vsk->remote_addr));

[Severity: Medium]
What happens in vsock_dgram_connect() and vsock_dgram_sendmsg() when
remote_addr->svm_cid is VMADDR_CID_ANY and vmci_transport owns the socket
(so dgram_transport_match is NULL)?

In vsock_dgram_connect(), remote_addr->svm_cid is left as VMADDR_CID_ANY
(0xffffffffU) and passed to vsock_dgram_get_transport().  Because
0xffffffffU > VMADDR_CID_HOST, vsock_select_transport() takes the
g2h_fallback path when enabled and sets VMADDR_FLAG_TO_HOST on
vsk->remote_addr, while leaving vsk->remote_addr.svm_cid as VMADDR_CID_ANY
so virtio_transport_dgram_source_allow() drops all incoming packets until
sendmsg() is called.

In vsock_dgram_sendmsg(), the SS_CONNECTED path then mutates
vsk->remote_addr.svm_cid in place before copying it into connected_addr.
Moreover, if VMCI has no active personality, transport->get_local_cid()
returns VMCI_INVALID_ID (VMADDR_CID_ANY), causing vsock_addr_bound() in
vsock_dgram_sendmsg() to fail with -EINVAL and vsock_select_transport() in
vsock_dgram_connect() to match cid == dgram_owner->get_local_cid() instead
of loopback.

> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
> index 08baa1181ba6d..3ef86f5eaf362 100644
> --- a/net/vmw_vsock/virtio_transport_common.c
> +++ b/net/vmw_vsock/virtio_transport_common.c

[ ... ]

> @@ -1891,14 +1887,21 @@ 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: Medium]
Can virtio_transport_recv_pkt() deliver a datagram to an unbound socket or
the wrong port if vmci_transport_dgram_bind() rolls back the core bind?

CPU0                                  CPU1
__vsock_bind()                        virtio_transport_recv_pkt()
  lock_sock(sk)
  vmci_transport_dgram_bind()
    vsock_bind_common(vsk, addr)
                                        sk = vsock_find_bound_dgram_socket_net(&dst, ...)
                                        lock_sock(sk) (blocks)
    vmci_transport_datagram_create_hnd()
      (fails)
    vsock_unbind_common(vsk)
  release_sock(sk)
                                        (acquires lock_sock)
                                        virtio_transport_dgram_source_allow()
                                        sock_queue_rcv_skb(sk, skb) <-- queued on unbound socket

vsock_bind_common() publishes vsk in vsock_bind_table and drops
vsock_table_lock before vmci_transport_datagram_create_hnd() runs.  If
handle creation fails, vsock_unbind_common() removes vsk or retries on
another port, but virtio_transport_recv_pkt() does not re-check that
vsk->local_addr still matches dst after acquiring lock_sock(sk).

> +
> +		/* Loopback can hand the TX skb to its receiver. Release TX
> +		 * ownership before replacing its private control block.
> +		 */
> +		skb_orphan(skb);
> +		vsock_addr_init(&VSOCK_DGRAM_SKB_CB(skb)->src,
> +				src.svm_cid, src.svm_port);
> +		if (t->transport.get_local_cid() > VMADDR_CID_HOST)
> +			VSOCK_DGRAM_SKB_CB(skb)->src.svm_flags =
> +				VMADDR_FLAG_TO_HOST;

[Severity: Medium]
Should this also check src.svm_cid > VMADDR_CID_HOST before setting
VMADDR_FLAG_TO_HOST, matching vsock_assign_transport()?

On a guest using virtio_transport, t->transport.get_local_cid() is the
guest CID (> VMADDR_CID_HOST), so checking only get_local_cid() sets
VMADDR_FLAG_TO_HOST on every received datagram even when the sender is the
host itself (src.svm_cid == VMADDR_CID_HOST).  recvmsg() then reports
VMADDR_FLAG_TO_HOST in msg_name for direct host-to-guest traffic, whereas
getpeername() on a socket connected to VMADDR_CID_HOST reports
svm_flags == 0.

> diff --git a/net/vmw_vsock/vmci_transport.c b/net/vmw_vsock/vmci_transport.c
> index 2b9493e987d1e..3c02c9a5319b9 100644
> --- a/net/vmw_vsock/vmci_transport.c
> +++ b/net/vmw_vsock/vmci_transport.c

[ ... ]

> @@ -1690,39 +1693,49 @@ static void vmci_transport_release(struct vsock_sock *vsk)
>  	}
>  }
>  
> +#define VMCI_DGRAM_BIND_RETRIES 24
> +
>  static int vmci_transport_dgram_bind(struct vsock_sock *vsk,
>  				     struct sockaddr_vm *addr)
>  {
> +	const bool auto_port = addr->svm_port == VMADDR_PORT_ANY;
>  	u32 port;
>  	u32 flags;
> +	unsigned int i;
>  	int err;
>  
> -	/* VMCI will select a resource ID for us if we provide
> -	 * VMCI_INVALID_ID.
> -	 */
> -	port = addr->svm_port == VMADDR_PORT_ANY ?
> -			VMCI_INVALID_ID : addr->svm_port;
> -
> -	if (port <= LAST_RESERVED_PORT && !capable(CAP_NET_BIND_SERVICE))
> -		return -EACCES;
> -
>  	flags = addr->svm_cid == VMADDR_CID_ANY ?
>  				VMCI_FLAG_ANYCID_DG_HND : 0;
>  
> -	err = vmci_transport_datagram_create_hnd(port, flags,
> -						 vmci_transport_recv_dgram_cb,
> -						 &vsk->sk,
> -						 &vmci_trans(vsk)->dg_handle);
> -	if (err < VMCI_SUCCESS)
> -		return vmci_transport_error_to_vsock_error(err);
> -	vsock_addr_init(&vsk->local_addr, addr->svm_cid,
> -			vmci_trans(vsk)->dg_handle.resource);
> +	/* Reserve the same port for VMCI and directional receive. A VMCI
> +	 * resource may already occupy a core-selected ephemeral port.
> +	 */
> +	for (i = 0; i < VMCI_DGRAM_BIND_RETRIES; i++) {
> +		err = vsock_bind_common(vsk, addr);
> +		if (err)
> +			return err;
>  
> -	return 0;
> +		port = vsk->local_addr.svm_port;
> +		err = vmci_transport_datagram_create_hnd(port, flags,
> +							 vmci_transport_recv_dgram_cb,
> +							 &vsk->sk,
> +							 &vmci_trans(vsk)->dg_handle);

[Severity: High]
What happens here when a user binds a SOCK_DGRAM socket to an explicit
local CID (such as VMADDR_CID_LOCAL or a virtio guest CID) while
vmw_vsock_vmci_transport is loaded?

drivers/misc/vmw_vmci/vmci_datagram.c:dg_create_handle() {
    ...
	if ((flags & VMCI_FLAG_ANYCID_DG_HND) != 0) {
		context_id = VMCI_INVALID_ID;
	} else {
		context_id = vmci_get_context_id();
		if (context_id == VMCI_INVALID_ID)
			return VMCI_ERROR_NO_RESOURCES;
	}
    ...
}

Because addr->svm_cid != VMADDR_CID_ANY, flags is 0.  If VMCI has no
active guest or host personality (vmci_get_context_id() == VMCI_INVALID_ID),
dg_create_handle() returns VMCI_ERROR_NO_RESOURCES and every explicit-CID
bind() fails with -ENOBUFS.  If VMCI is active, dg_create_handle() binds
the VMCI handle to vmci_get_context_id() rather than addr->svm_cid, so a
socket bound to VMADDR_CID_LOCAL (or a virtio guest CID) still registers a
VMCI receiver on VMCI's context ID and can receive external VMCI datagrams.

Conversely, vsock_find_cid() only checks transport_g2h, transport_h2g, and
transport_local, so if VMCI's local CID differs from transport_g2h,
__vsock_bind() rejects binding to VMCI's own local CID with
-EADDRNOTAVAIL.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-vsock-dgram-support-v2-0-bacfc85b71ce@redhat.com?part=6