Re: [PATCH net v3 2/2] vsock/vmci: validate packet source for connected sockets

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
vsock/vmci: validate packet source for connected sockets

This adds a vsock_check_source() test to the VMCI stream receive paths so
that a non-listening socket found by the destination-only bound-table
lookup no longer processes packets from a context other than its stored
peer. The check is applied both under bh_lock_sock() in
vmci_transport_recv_stream_cb() and after lock_sock() in
vmci_transport_recv_pkt_work().

Closes: https://lore.kernel.org/netdev/20260814121255.6B5001F000E9@smtp.kernel.org/ (local)

[ ... ]

[Severity: Medium]
Should these two new exits still emit an RST before dropping the packet?

Consider a stream socket that called bind() but never listen().
__vsock_bind_connectible() puts it in the bound table with sk_state
TCP_CLOSE, remote_addr left as {VMADDR_CID_ANY, VMADDR_PORT_ANY}, and
vsk->transport still NULL, because vsock_assign_transport() is only
called for connectible sockets from vsock_connect() (and at create time
for SOCK_DGRAM).

A REQUEST from a remote context to that port misses
vsock_find_connected_socket(), is matched by vsock_find_bound_socket(),
and then reaches the new check.  vsock_check_source() fails on its very
first test:

net/vmw_vsock/af_vsock.c:vsock_check_source() {
	if (vsk->transport != transport)
		return false;
	...
}

So drop_pkt is set and "goto out" skips the kmalloc/schedule_work()
handoff entirely.  vmci_transport_send_reset_bh() is only reached on the
"no socket found" and allocation-failure paths, so nothing is sent back.

The same happens on the deferred path below, where the new "goto out"
jumps over the whole sk_state switch, including the default: arm whose
in-tree comment says the reset is required:

net/vmw_vsock/vmci_transport.c:vmci_transport_recv_pkt_work() {
	...
	default:
		/* Because this function does not run in the same context as
		 * vmci_transport_recv_stream_cb it is possible that the
		 * socket has closed. We need to let the other side know or it
		 * could be sitting in a connect and hang forever. Send a
		 * reset to prevent that.
		 */
		vmci_transport_send_reset(sk, pkt);
		break;
	}

Before this change that arm did transmit an RST for exactly this case,
since vmci_transport_send_reset() handles an unbound remote_addr by
replying to the packet's own source:

net/vmw_vsock/vmci_transport.c:vmci_transport_send_reset() {
	...
	if (vsock_addr_bound(&vsk->remote_addr)) {
		dst_ptr = &vsk->remote_addr;
	} else {
		vsock_addr_init(&dst, pkt->dg.src.context,
				pkt->src_port);
		dst_ptr = &dst;
	}
	...
}

With the reply gone, does the remote connect() now block for
vsk->connect_timeout and return -ETIMEDOUT instead of -ECONNRESET?  The
same loss of the reply would apply to a socket disconnected via
connect(AF_UNSPEC), which resets remote_addr to {VMADDR_CID_ANY,
VMADDR_PORT_ANY}.

Returning VMCI_ERROR_NO_ACCESS from the datagram callback does not turn
into a protocol reply, so is there anything left that notifies the peer?

For comparison, the virtio side runs the same source test but resets the
sender before dropping:

net/vmw_vsock/virtio_transport_common.c:virtio_transport_recv_pkt() {
	...
	if (sock_flag(sk, SOCK_DONE) ||
	    (sk->sk_state != TCP_LISTEN &&
	     !vsock_check_source(vsk, &t->transport, &src))) {
		(void)virtio_transport_reset_no_sock(t, skb, net);
	...
}

Would it make sense to follow that convention here, and to mention the
new drop-without-reply semantics in the changelog?
-- 
This is an AI-generated review.