Re: [PATCH net v2] vsock/virtio: validate packet source for connected sockets

On Thu, Aug 20, 2026 at 09:15:17AM +0900, Daehyeon Ko wrote:
>virtio_transport_recv_pkt() looks up sockets first by the full source and
>destination tuple, then by destination only in the bound table.  The
>fallback is needed for listening and connecting sockets, but sockets remain
>in the bound table after connect(), so it can also return a non-listening
>socket.
>
>The fallback does not validate the source address.  In TCP_SYN_SENT, a
>RESPONSE from an unrelated source can transition the victim socket to
>TCP_ESTABLISHED while its stored remote address remains unchanged.
>Subsequent RW packets from that source are delivered through the same
>destination-only fallback.
>
>This was reproduced with capability-empty processes under different UIDs.
>The attacker discovered the target tuple through unprivileged AF_VSOCK
>sock_diag and caused the victim socket to read 16 attacker-chosen bytes;
>the intended peer-side socket read 0 of those 16 bytes.
>
>After lock_sock(), reject packets for non-listening sockets unless their
>source port matches the stored remote port.  Require the CID to match too,
>except that the loopback transport uses VMADDR_CID_LOCAL as the packet
>source for connections addressed through its valid CID aliases.
>
>Fixes: 06a8fc78367d ("VSOCK: Introduce virtio_vsock_common.ko")
>Closes: https://lore.kernel.org/netdev/[email protected]/
>Cc: [email protected]
>Assisted-by: Codex:gpt-5.6-sol
>Signed-off-by: Daehyeon Ko <[email protected]>
>---
>Changes in v2:
>- Preserve valid loopback CID aliases by matching the source port and
>  accepting VMADDR_CID_LOCAL only for the loopback transport.
>- Rewrite the commit message and receive-path comment for clarity.
>
>v1: https://lore.kernel.org/netdev/[email protected]/
>
> net/vmw_vsock/virtio_transport_common.c | 24 ++++++++++++++++++++++--
> 1 file changed, 22 insertions(+), 2 deletions(-)
>
>diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>index 8becad812..d8990f5f6 100644
>--- a/net/vmw_vsock/virtio_transport_common.c
>+++ b/net/vmw_vsock/virtio_transport_common.c
>@@ -1764,6 +1764,21 @@ static bool virtio_transport_valid_type(u16 type)
> 	       (type == VIRTIO_VSOCK_TYPE_SEQPACKET);
> }
>
>+static bool virtio_transport_source_matches(const struct virtio_transport *t,

This doesn't seem virtio-specific, so why introducing this function 
here?

IIRC we agreed that also VMCI has the same issue, so what about adding 
this function in af_vsock.c?

Also I don't see VMCI changes, why not doing them together in a series 
since they are strictly related?

>+					    const struct sockaddr_vm *src,
>+					    const struct sockaddr_vm *remote)
>+{

What about moving here also the transport check?

So we can have something like this (with some documentation):

bool vsock_check_source(const struct vsock_sock *vsk,
			const struct vsock_transport *transport,
			const struct sockaddr_vm *src)
{
	if (vsk->transport != transport)
		return false;

	if (src->svm_port != vsk->remote_addr.svm_port)
		return false;

etc.

>+	if (src->svm_port != remote->svm_port)
>+		return false;
>+
>+	if (src->svm_cid == remote->svm_cid)
>+		return true;
>+
>+	/* The loopback transport represents its peer as VMADDR_CID_LOCAL. */
>+	return t->transport.get_local_cid() == VMADDR_CID_LOCAL &&
>+	       src->svm_cid == VMADDR_CID_LOCAL;

Could this be just `t->transport.get_local_cid() == VMADDR_CID_LOCAL` ?

I guess the source is trusted and if we are in a local transport, it's 
always set to it, no?

Thanks,
Stefano

>+}
>+
> /* We are under the virtio-vsock's vsock->rx_lock or vhost-vsock's vq->mutex
>  * lock.
>  */
>@@ -1823,10 +1838,15 @@ void virtio_transport_recv_pkt(struct virtio_transport *t,
> 	lock_sock(sk);
>
> 	/* Check if sk has been closed or assigned to another transport before
>-	 * lock_sock (note: listener sockets are not assigned to any transport)
>+	 * lock_sock (note: listener sockets are not assigned to any transport).
>+	 * The bound-table fallback matches only the destination, so reject packets
>+	 * from a peer other than the one stored in the socket.
> 	 */
> 	if (sock_flag(sk, SOCK_DONE) ||
>-	    (sk->sk_state != TCP_LISTEN && vsk->transport != &t->transport)) {
>+	    (sk->sk_state != TCP_LISTEN &&
>+	     (vsk->transport != &t->transport ||
>+	      !virtio_transport_source_matches(t, &src,
>+					       &vsk->remote_addr)))) {
> 		(void)virtio_transport_reset_no_sock(t, skb, net);
> 		release_sock(sk);
> 		sock_put(sk);
>
>base-commit: e2466392a0b8496000e12181cb1ee1535eb0da25
>-- 
>2.54.0
>