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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Legitimate connection requests to bound but non-listening sockets (TCP_CLOSE) or concurrently closing sockets are silently dropped instead of generating a connection reset (RST), causing clients to hang until timeout.

Pre-existing issues:
- [High] Dynamically allocated `recv_pkt_info` work items are queued to the global `system_wq` but are not flushed or canceled when the module is unloaded, leading to a kernel panic.
--

--- Patch [2]: [PATCH net v3 2/2] vsock/vmci: validate packet source for connected sockets ---
commit 0c47d3fbd6e3da162ec0e88ab935d6073c395e19
Author: Daehyeon Ko <[email protected]>
vsock/vmci: validate packet source for connected sockets

This commit adds source validation for connected sockets in the VMCI
transport to prevent packets from unverified sources from being processed.
It updates both the bottom-half fast path and the workqueue path to verify
the packet's source.

Link: 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 {

[Severity: High]
Does this code silently drop connection requests to bound but non-listening
sockets (TCP_CLOSE)?

If a client attempts to connect to a bound port that isn't listening yet,
the socket is found in the TCP_CLOSE state. Since the state is not TCP_LISTEN,
it calls vsock_check_source(), which checks against the socket's remote_addr.
Because the socket is not connected, the port is 0 and won't match the
client's ephemeral source port, causing vsock_check_source() to return false.

By setting drop_pkt = true, the packet is completely dropped. Could this
prevent the client from receiving the expected connection reset (RST), causing
them to hang indefinitely instead of seeing ECONNREFUSED?

> +			/* 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;
> +
>  	if (!bh_process_pkt) {
>  		struct vmci_transport_recv_pkt_info *recv_pkt_info;

[Severity: High]
This is a pre-existing issue, but does queuing these work items to the global
system_wq create a use-after-free risk during module unload?

Looking at vmci_transport_recv_stream_cb(), it dynamically allocates a work
item and schedules it:

	INIT_WORK(&recv_pkt_info->work, vmci_transport_recv_pkt_work);
	schedule_work(&recv_pkt_info->work);

However, vmci_transport_exit() cancels a specific cleanup work item but does
not flush system_wq or track these pending recv_pkt_info items before
unloading.

If packets arrive for a port bound by a listening socket of a different
transport (e.g., virtio_transport), this code bypasses vsock_check_source() and
schedules the work. Since the target socket belongs to a different transport,
the vmci_transport module reference count can remain at zero.

If the module is unloaded while this work is still pending, could the kernel
worker thread attempt to execute the unmapped vmci_transport_recv_pkt_work
function and panic?

[ ... ]

> @@ -900,6 +911,7 @@ static void vmci_transport_recv_pkt_work(struct work_struct *work)
>  {
>  	struct vmci_transport_recv_pkt_info *recv_pkt_info;
>  	struct vmci_transport_packet *pkt;
> +	struct sockaddr_vm src;
>  	struct sock *sk;
>  
>  	recv_pkt_info =
> @@ -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;

[Severity: High]
Similarly, does this early exit bypass the logic meant to send connection
resets for closed sockets?

The default case in the switch statement below this code was specifically
designed to send a reset:

	switch (sk->sk_state) {
        ...
	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;
	}

Will jumping to the out label bypass this reset logic when a socket closes
concurrently?

>  
>  	/* The local context ID may be out of date. */
>  	vsock_sk(sk)->local_addr.svm_cid = pkt->dg.dst.context;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2