Skip to content

Commit

Permalink
vhost_net: fix possible infinite loop
Browse files Browse the repository at this point in the history
When the rx buffer is too small for a packet, we will discard the vq
descriptor and retry it for the next packet:

while ((sock_len = vhost_net_rx_peek_head_len(net, sock->sk,
					      &busyloop_intr))) {
...
	/* On overrun, truncate and discard */
	if (unlikely(headcount > UIO_MAXIOV)) {
		iov_iter_init(&msg.msg_iter, READ, vq->iov, 1, 1);
		err = sock->ops->recvmsg(sock, &msg,
					 1, MSG_DONTWAIT | MSG_TRUNC);
		pr_debug("Discarded rx packet: len %zd\n", sock_len);
		continue;
	}
...
}

This makes it possible to trigger a infinite while..continue loop
through the co-opreation of two VMs like:

1) Malicious VM1 allocate 1 byte rx buffer and try to slow down the
   vhost process as much as possible e.g using indirect descriptors or
   other.
2) Malicious VM2 generate packets to VM1 as fast as possible

Fixing this by checking against weight at the end of RX and TX
loop. This also eliminate other similar cases when:

- userspace is consuming the packets in the meanwhile
- theoretical TOCTOU attack if guest moving avail index back and forth
  to hit the continue after vhost find guest just add new buffers

This addresses CVE-2019-3900.

Fixes: d8316f3 ("vhost: fix total length when packets are too short")
Fixes: 3a4d5c9 ("vhost_net: a kernel-level virtio server")
Signed-off-by: Jason Wang <jasowang@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
  • Loading branch information
Jason Wang authored and Michael S. Tsirkin committed May 27, 2019
1 parent e82b9b0 commit e2412c0
Showing 1 changed file with 13 additions and 16 deletions.
29 changes: 13 additions & 16 deletions drivers/vhost/net.c
Original file line number Diff line number Diff line change
Expand Up @@ -773,7 +773,7 @@ static void handle_tx_copy(struct vhost_net *net, struct socket *sock)
int sent_pkts = 0;
bool sock_can_batch = (sock->sk->sk_sndbuf == INT_MAX);

for (;;) {
do {
bool busyloop_intr = false;

if (nvq->done_idx == VHOST_NET_BATCH)
Expand Down Expand Up @@ -839,9 +839,7 @@ static void handle_tx_copy(struct vhost_net *net, struct socket *sock)
vq->heads[nvq->done_idx].id = cpu_to_vhost32(vq, head);
vq->heads[nvq->done_idx].len = 0;
++nvq->done_idx;
if (vhost_exceeds_weight(vq, ++sent_pkts, total_len))
break;
}
} while (likely(!vhost_exceeds_weight(vq, ++sent_pkts, total_len)));

vhost_tx_batch(net, nvq, sock, &msg);
}
Expand All @@ -866,7 +864,7 @@ static void handle_tx_zerocopy(struct vhost_net *net, struct socket *sock)
bool zcopy_used;
int sent_pkts = 0;

for (;;) {
do {
bool busyloop_intr;

/* Release DMAs done buffers first */
Expand Down Expand Up @@ -943,10 +941,7 @@ static void handle_tx_zerocopy(struct vhost_net *net, struct socket *sock)
else
vhost_zerocopy_signal_used(net, vq);
vhost_net_tx_packet(net);
if (unlikely(vhost_exceeds_weight(vq, ++sent_pkts,
total_len)))
break;
}
} while (likely(!vhost_exceeds_weight(vq, ++sent_pkts, total_len)));
}

/* Expects to be always run from workqueue - which acts as
Expand Down Expand Up @@ -1144,8 +1139,11 @@ static void handle_rx(struct vhost_net *net)
vq->log : NULL;
mergeable = vhost_has_feature(vq, VIRTIO_NET_F_MRG_RXBUF);

while ((sock_len = vhost_net_rx_peek_head_len(net, sock->sk,
&busyloop_intr))) {
do {
sock_len = vhost_net_rx_peek_head_len(net, sock->sk,
&busyloop_intr);
if (!sock_len)
break;
sock_len += sock_hlen;
vhost_len = sock_len + vhost_hlen;
headcount = get_rx_bufs(vq, vq->heads + nvq->done_idx,
Expand Down Expand Up @@ -1230,12 +1228,11 @@ static void handle_rx(struct vhost_net *net)
vhost_log_write(vq, vq_log, log, vhost_len,
vq->iov, in);
total_len += vhost_len;
if (unlikely(vhost_exceeds_weight(vq, ++recv_pkts, total_len)))
goto out;
}
} while (likely(!vhost_exceeds_weight(vq, ++recv_pkts, total_len)));

if (unlikely(busyloop_intr))
vhost_poll_queue(&vq->poll);
else
else if (!sock_len)
vhost_net_enable_vq(net, vq);
out:
vhost_net_signal_used(nvq);
Expand Down Expand Up @@ -1328,7 +1325,7 @@ static int vhost_net_open(struct inode *inode, struct file *f)
}
vhost_dev_init(dev, vqs, VHOST_NET_VQ_MAX,
UIO_MAXIOV + VHOST_NET_BATCH,
VHOST_NET_WEIGHT, VHOST_NET_PKT_WEIGHT);
VHOST_NET_PKT_WEIGHT, VHOST_NET_WEIGHT);

vhost_poll_init(n->poll + VHOST_NET_VQ_TX, handle_tx_net, EPOLLOUT, dev);
vhost_poll_init(n->poll + VHOST_NET_VQ_RX, handle_rx_net, EPOLLIN, dev);
Expand Down

0 comments on commit e2412c0

Please sign in to comment.