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.
