Re: [PATCH RFC 5/8] vhost/vsock: negotiate datagram support with guests

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] vhost/vsock: missing dgram_allow check in vhost_vsock_handle_tx_kick()
--

commit 635912e90dc603f9b7384d44cf174e18056f799d
Author: Sergio Lopez <slp@redhat.com>

vhost/vsock: negotiate datagram support with guests

This commit exposes VIRTIO_VSOCK_F_DGRAM in the vhost feature bitmap and
tracks whether datagram support was negotiated per guest via
vsock->dgram_allow. It also wires up vhost_transport_dgram_allow() to check
the negotiated feature on the target guest for outbound datagrams.

> diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
> index 0cb87971d7491..eb2fbdb721aa9 100644
> --- a/drivers/vhost/vsock.c
> +++ b/drivers/vhost/vsock.c

[ ... ]

> @@ -516,6 +520,24 @@ static bool vhost_transport_seqpacket_allow(struct vsock_sock *vsk,
>  	return seqpacket_allow;
>  }
>  
> +static bool vhost_transport_dgram_allow(struct vsock_sock *vsk,
> +					u32 cid, u32 port)
> +{
> +	struct net *net = sock_net(sk_vsock(vsk));
> +	struct vhost_vsock *vsock;
> +	bool dgram_allow = false;
> +
> +	rcu_read_lock();
> +	vsock = vhost_vsock_get(cid, net);
> +
> +	if (vsock)
> +		dgram_allow = vsock->dgram_allow;
> +
> +	rcu_read_unlock();
> +
> +	return dgram_allow;
> +}
> +
>  static void vhost_vsock_handle_tx_kick(struct vhost_work *work)
>  {
>  	struct vhost_virtqueue *vq = container_of(work, struct vhost_virtqueue,

[Severity: Medium]
Can a guest that has not negotiated VIRTIO_VSOCK_F_DGRAM still send
VIRTIO_VSOCK_TYPE_DGRAM packets to unconnected bound SOCK_DGRAM sockets on
the host?

vhost_transport_dgram_allow() checks vsock->dgram_allow on outbound host
connect() and sendmsg() paths, but on the guest-to-host path in
drivers/vhost/vsock.c:vhost_vsock_handle_tx_kick(), incoming packets are
passed to virtio_transport_recv_pkt() without checking vsock->dgram_allow:

drivers/vhost/vsock.c:vhost_vsock_handle_tx_kick() {
	...
		/* Only accept correctly addressed packets */
		if (le64_to_cpu(hdr->src_cid) == vsock->guest_cid &&
		    le64_to_cpu(hdr->dst_cid) ==
		    vhost_transport_get_local_cid())
			virtio_transport_recv_pkt(&vhost_transport, skb,
						  vsock->net);
		else
			kfree_skb(skb);
	...
}

Unlike VIRTIO_VSOCK_TYPE_SEQPACKET, where inbound connection requests go
through virtio_transport_recv_listen() -> vsock_assign_transport() ->
vhost_transport_seqpacket_allow(), inbound VIRTIO_VSOCK_TYPE_DGRAM packets
in net/vmw_vsock/virtio_transport_common.c:virtio_transport_recv_pkt() are
queued directly onto bound unconnected SOCK_DGRAM sockets without calling
dgram_allow():

net/vmw_vsock/virtio_transport_common.c:virtio_transport_recv_pkt() {
	...
	if (le16_to_cpu(hdr->type) == VIRTIO_VSOCK_TYPE_DGRAM) {
		if (le16_to_cpu(hdr->op) != VIRTIO_VSOCK_OP_RW)
			goto free_pkt;

		sk = vsock_find_bound_socket_net(&dst, net);
		...
		if (sock_flag(sk, SOCK_DEAD) ||
		    vsk->transport != &t->transport ||
		    (sk->sk_socket->state != SS_UNCONNECTED &&
		     !vsock_check_source(vsk, &t->transport, &src))) {
			release_sock(sk);
			sock_put(sk);
			goto free_pkt;
		}

		if (sock_queue_rcv_skb(sk, skb))
			kfree_skb(skb);
	...
}

Even with the later virtio_transport_dgram_source_allow() check, unconnected
bound SOCK_DGRAM sockets return true before consulting dgram_allow().

Should vhost_vsock_handle_tx_kick() drop VIRTIO_VSOCK_TYPE_DGRAM packets
when !vsock->dgram_allow, or should virtio_transport_recv_pkt() validate
dgram_allow() for incoming datagrams regardless of whether the receiving
socket is connected?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028-vsock-dgram-support-v1-0-e457cf7cee78@sinrega.org?part=5