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 <[email protected]> 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/[email protected]?part=1
