Re: [PATCH RFC net] vsock: make local-transport CIDs conflict on the same port

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] vsock: incomplete local CID alias check in vsock_local_alias_cid()
--

commit 5d164f9e8d6139f39bfbdee4f02c52a0bdbf7d5a
Author: Yiran Qiu <eritque-arcus@ikuyo.dev>

vsock: make local-transport CIDs conflict on the same port

This commit updates __vsock_find_bound_socket_net() during connectible
socket bind() so that binding to a port on VMADDR_CID_LOCAL conflicts
with binding to the same port on the local alias CID.

> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> index d0508887a2fba..47fd00fdac6d8 100644
> --- a/net/vmw_vsock/af_vsock.c
> +++ b/net/vmw_vsock/af_vsock.c
> @@ -294,8 +294,31 @@ static void __vsock_remove_connected(struct vsock_sock *vsk)
>  	sock_put(&vsk->sk);
>  }
>  
> +/*
> + * CIDs served by the local transport are aliases of one another on the wire:
> + * an outgoing packet uses transport->get_local_cid() as its source CID, no
> + * matter which of those CIDs the socket was bound to.  Two binds of the same
> + * port on different local CIDs therefore reach the same peer and must
> + * conflict, just like a VMADDR_CID_ANY bind conflicts with any CID.
> + *
> + * @local_cid is the second local CID (the guest->host transport's own CID, or
> + * VMADDR_CID_HOST when there is no guest->host transport), or
> + * VMADDR_CID_ANY when the local transport is not loaded.
> + */
> +static bool vsock_addr_aliases_local(const struct sockaddr_vm *a,
> +				     const struct sockaddr_vm *b,
> +				     u32 local_cid)
> +{
> +	return local_cid != VMADDR_CID_ANY &&
> +	       a->svm_port == b->svm_port &&
> +	       a->svm_cid != b->svm_cid &&
> +	       (a->svm_cid == VMADDR_CID_LOCAL || a->svm_cid == local_cid) &&
> +	       (b->svm_cid == VMADDR_CID_LOCAL || b->svm_cid == local_cid);
> +}

[ ... ]

> @@ -727,6 +754,27 @@ static u32 vsock_registered_transport_cid(const struct vsock_transport **transpo
>  	return cid;
>  }
>  
> +/*
> + * Return the CID that the local transport aliases with VMADDR_CID_LOCAL, or
> + * VMADDR_CID_ANY when the local transport is not loaded or has no second CID.
> + * Must be called before taking vsock_table_lock.
> + */
> +static u32 vsock_local_alias_cid(void)
> +{
> +	u32 cid = VMADDR_CID_ANY;
> +
> +	mutex_lock(&vsock_register_mutex);
> +	if (transport_local) {
> +		if (transport_g2h)
> +			cid = transport_g2h->get_local_cid();
> +		else
> +			cid = VMADDR_CID_HOST;
> +	}
> +	mutex_unlock(&vsock_register_mutex);
> +
> +	return cid;
> +}

[Severity: Medium]
Can two sockets still bind to the same port on different CIDs that end up
using transport_local and colliding on the wire when VMADDR_CID_HOST is
bound alongside VMADDR_CID_LOCAL or the guest CID?

First, in vsock_local_alias_cid(), if a guest-to-host transport driver is
registered (for example, virtio_transport built-in or loaded on a host
where no virtio-vsock guest device is present), transport_g2h is non-NULL,
and transport_g2h->get_local_cid() returns VMADDR_CID_ANY:

net/vmw_vsock/virtio_transport.c:virtio_transport_get_local_cid() {
	rcu_read_lock();
	vsock = rcu_dereference(the_virtio_vsock);
	if (!vsock) {
		ret = VMADDR_CID_ANY;
		goto out_rcu;
	}
    ...
}

Because vsock_local_alias_cid() only checks whether transport_g2h is
non-NULL rather than whether get_local_cid() returned VMADDR_CID_ANY, it
returns VMADDR_CID_ANY instead of falling back to VMADDR_CID_HOST. When
transport_h2g and transport_local are also loaded on that host,
__vsock_bind() -> vsock_find_cid() allows binding to both VMADDR_CID_HOST
and VMADDR_CID_LOCAL, while vsock_addr_aliases_local() sees
local_cid == VMADDR_CID_ANY and skips the conflict check completely.

Second, if both transport_g2h (with a valid guest CID) and transport_h2g
are registered alongside transport_local, vsock_find_cid() allows
__vsock_bind() to bind sockets to all three CIDs:

net/vmw_vsock/af_vsock.c:vsock_find_cid() {
	if (cid == vsock_registered_transport_cid(&transport_g2h))
		return true;

	if (transport_h2g && cid == VMADDR_CID_HOST)
		return true;

	if (transport_local && cid == VMADDR_CID_LOCAL)
		return true;

	return false;
}

However, vsock_local_alias_cid() only returns the single G2H CID and
vsock_addr_aliases_local() only checks VMADDR_CID_LOCAL against that single
local_cid. A socket can therefore still bind to (VMADDR_CID_HOST, port)
while another socket is bound to (VMADDR_CID_LOCAL, port) or
(guest_cid, port).

When the socket bound to (VMADDR_CID_HOST, port) connects to
VMADDR_CID_LOCAL (or guest_cid), vsock_connect() -> vsock_assign_transport()
selects transport_local based only on remote_cid:

net/vmw_vsock/af_vsock.c:vsock_assign_transport() {
	case SOCK_STREAM:
	case SOCK_SEQPACKET:
		if (vsock_use_local_transport(remote_cid))
			new_transport = transport_local;
    ...
}

And virtio_transport_send_pkt_info() then sends packets with source CID
VMADDR_CID_LOCAL and vsk->local_addr.svm_port, hitting the same port
collision on the loopback wire.

Should vsock_addr_aliases_local() treat all locally bindable CIDs (where
vsock_find_cid() is true) as conflicting with each other whenever
transport_local is registered, or at least fall back when
transport_g2h->get_local_cid() returns VMADDR_CID_ANY?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028065231.655701-1-eritque-arcus@ikuyo.dev?part=1