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/[email protected]/

> diff --git a/net/vmw_vsock/vmci_transport.c b/net/vmw_vsock/vmci_transport.c
> index 1c4ee039c1662..b612a9893856a 100644
> --- a/net/vmw_vsock/vmci_transport.c
> +++ b/net/vmw_vsock/vmci_transport.c

[ ... ]

> @@ -765,17 +767,26 @@ static int vmci_transport_recv_stream_cb(void *data, struct vmci_datagram *dg)
>  	bh_lock_sock(sk);
>  
>  	if (!sock_owned_by_user(sk)) {
> -		/* The local context ID may be out of date, update it. */
> -		vsk->local_addr.svm_cid = dst.svm_cid;
> +		if (sk->sk_state != TCP_LISTEN &&
> +		    !vsock_check_source(vsk, &vmci_transport, &src)) {
> +			drop_pkt = true;
> +			err = VMCI_ERROR_NO_ACCESS;
> +		} else {
> +			/* The local context ID may be out of date, update it. */
> +			vsk->local_addr.svm_cid = dst.svm_cid;
>  
> -		if (sk->sk_state == TCP_ESTABLISHED)
> -			vmci_trans(vsk)->notify_ops->handle_notify_pkt(
> -					sk, pkt, true, &dst, &src,
> -					&bh_process_pkt);
> +			if (sk->sk_state == TCP_ESTABLISHED)
> +				vmci_trans(vsk)->notify_ops->handle_notify_pkt(sk, pkt, true,
> +									       &dst, &src,
> +									       &bh_process_pkt);
> +		}
>  	}
>  
>  	bh_unlock_sock(sk);
>  
> +	if (drop_pkt)
> +		goto out;
> +

[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:

> @@ -908,6 +920,10 @@ static void vmci_transport_recv_pkt_work(struct work_struct *work)
>  	pkt = &recv_pkt_info->pkt;
>  
>  	lock_sock(sk);
> +	vsock_addr_init(&src, pkt->dg.src.context, pkt->src_port);
> +	if (sk->sk_state != TCP_LISTEN &&
> +	    !vsock_check_source(vsock_sk(sk), &vmci_transport, &src))
> +		goto out;
>  
>  	/* The local context ID may be out of date. */
>  	vsock_sk(sk)->local_addr.svm_cid = pkt->dg.dst.context;

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.