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