Re: [PATCH RFC v3 3/8] vsock/virtio: implement the datagram data path

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] vsock/virtio: OOB read on filtered nonlinear vhost dgram skb
- [Medium] vsock/virtio: empty datagrams skipped on repeated MSG_PEEK
--

commit 7bb89ea8d31712feb1c6f76854206e10a5fd07b5
Author: Sergio Lopez <slp@redhat.com>

vsock/virtio: implement the datagram data path

This commit adds the VIRTIO_VSOCK_F_DGRAM feature bit and
VIRTIO_VSOCK_TYPE_DGRAM packet type, and implements the common virtio
transport operations to bind, send, and receive datagrams.

> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
> index 1e762a480df49..08baa1181ba6d 100644
> --- a/net/vmw_vsock/virtio_transport_common.c
> +++ b/net/vmw_vsock/virtio_transport_common.c

[ ... ]

> @@ -999,7 +1018,47 @@ virtio_transport_dgram_dequeue(struct vsock_sock *vsk,
>  			       struct msghdr *msg,
>  			       size_t len, int flags)
>  {
> -	return -EOPNOTSUPP;
> +	struct sock *sk = sk_vsock(vsk);
> +	struct virtio_vsock_hdr *hdr;
> +	struct sk_buff *skb;
> +	size_t payload_len;
> +	int err;
> +
> +	if (flags & (MSG_OOB | MSG_ERRQUEUE))
> +		return -EOPNOTSUPP;
> +
> +	skb = skb_recv_datagram(sk, flags, &err);
> +	if (!skb)
> +		return err;

[Severity: Medium]
Does calling skb_recv_datagram() skip an unread empty datagram when
userspace calls recvmsg() with MSG_PEEK more than once?

Because virtio_vsock_hdr lives in skb headroom, an empty datagram has
skb->len == 0.  skb_recv_datagram() passes off = 0 to
__skb_recv_datagram(), which sets peek_at_off = true in
__skb_try_recv_from_queue():

net/core/datagram.c:__skb_try_recv_from_queue() {
	...
		if (flags & MSG_PEEK) {
			if (peek_at_off && _off >= skb->len &&
			    (_off || skb->peeked)) {
				_off -= skb->len;
				continue;
			}
			if (!skb->len) {
				skb = skb_set_peeked(skb);
				if (unlikely(!skb)) {
					*err = -ENOMEM;
					return NULL;
				}
			}
			*peeked = 1;
			refcount_inc(&skb->users);
		} else
	...
}

The first MSG_PEEK sets skb->peeked = 1 on the 0-length skb, and the next
MSG_PEEK sees _off >= skb->len && skb->peeked and skips the unread empty
datagram, returning the following datagram out of order or failing with
-EAGAIN.  Calling __skb_recv_datagram() with sk_peek_offset(sk, flags)
avoids this, as in commit a0917e0bc6ef ("datagram: When peeking datagrams
with offset < 0 don't skip empty skbs").

[ ... ]

> @@ -1805,6 +1875,38 @@ void virtio_transport_recv_pkt(struct virtio_transport *t,
>  		goto free_pkt;
>  	}
>  
> +	/* Dgram packets are delivered directly to bound sockets without
> +	 * going through the connection state machine. Undeliverable
> +	 * datagrams are silently dropped (no RST).
> +	 */
> +	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_dgram_socket_net(&dst, net);
> +
> +		if (!sk)
> +			goto free_pkt;
> +
> +		vsk = vsock_sk(sk);
> +		lock_sock(sk);
> +		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);

[Severity: High]
Can sock_queue_rcv_skb() corrupt a nonlinear vhost-vsock skb when a socket
filter trims the packet, leading to an out-of-bounds read in
virtio_transport_dgram_dequeue()?

In vhost_vsock_alloc_skb(), the skb is allocated from the guest TX
descriptor chain length before hdr->len is inspected, and
virtio_vsock_skb_put() then sets skb->len = payload_len without adjusting
skb->data_len:

include/linux/virtio_vsock.h:virtio_vsock_skb_put() {
	if (skb_is_nonlinear(skb))
		skb->len = len;
	else
		skb_put(skb, len);
}

When a guest sends a TX descriptor chain larger than 32 KiB with hdr->len
smaller than the descriptor payload space, the resulting nonlinear skb has
skb->len < skb->data_len and a negative skb_headlen(skb).  If the receiving
host datagram socket has a filter attached, sock_queue_rcv_skb() ->
sk_filter_trim_cap() -> pskb_trim() -> ___pskb_trim() treats the negative
skb_headlen(skb) as a large unsigned offset, drops all page frags, zeroes
skb->data_len, and sets skb->tail past skb->end.  The subsequent
skb_copy_datagram_iter() in virtio_transport_dgram_dequeue() then copies
skb_shared_info and out-of-bounds kernel heap memory after skb->end to
userspace.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-vsock-dgram-support-v3-0-1523c8e9b27e@redhat.com?part=3