[PATCH] vhost: fix segfault on bad descriptor address.

* [PATCH] vhost: fix segfault on bad descriptor address.
@ 2016-05-20 12:50 Ilya Maximets
  2016-05-23 10:57 ` Yuanhan Liu
                   ` (5 more replies)
  0 siblings, 6 replies; 39+ messages in thread
From: Ilya Maximets @ 2016-05-20 12:50 UTC (permalink / raw)
  To: dev, Huawei Xie, Yuanhan Liu
  Cc: Dyasly Sergey, Heetae Ahn, Jianfeng Tan, Ilya Maximets

In current implementation guest application can reinitialize vrings
by executing start after stop. In the same time host application
can still poll virtqueue while device stopped in guest and it will
crash with segmentation fault while vring reinitialization because
of dereferencing of bad descriptor addresses.

OVS crash for example:
<------------------------------------------------------------------------>
[test-pmd inside guest VM]

	testpmd> port stop all
	    Stopping ports...
	    Checking link statuses...
	    Port 0 Link Up - speed 10000 Mbps - full-duplex
	    Done
	testpmd> port config all rxq 2
	testpmd> port config all txq 2
	testpmd> port start all
	    Configuring Port 0 (socket 0)
	    Port 0: 52:54:00:CB:44:C8
	    Checking link statuses...
	    Port 0 Link Up - speed 10000 Mbps - full-duplex
	    Done

[OVS on host]
	Program received signal SIGSEGV, Segmentation fault.
	rte_memcpy (n=2056, src=0xc, dst=0x7ff4d5247000) at rte_memcpy.h

	(gdb) bt
	    #0  rte_memcpy (n=2056, src=0xc, dst=0x7ff4d5247000)
	    #1  copy_desc_to_mbuf
	    #2  rte_vhost_dequeue_burst
	    #3  netdev_dpdk_vhost_rxq_recv
	    ...

	(gdb) bt full
	    #0  rte_memcpy
	        ...
	    #1  copy_desc_to_mbuf
	        desc_addr = 0
	        mbuf_offset = 0
	        desc_offset = 12
	        ...
<------------------------------------------------------------------------>

Fix that by checking addresses of descriptors before using them.

Note: For mergeable buffers this patch checks only guest's address for
zero, but in non-meargeable case host's address checked. This is done
because checking of host's address in mergeable case requires additional
refactoring to keep virtqueue in consistent state in case of error.

Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
---

Actually, current virtio implementation looks broken for me. Because
'virtio_dev_start' breaks virtqueue while it still available from the vhost
side.

There was 2 patches about this behaviour:

	1. a85786dc816f ("virtio: fix states handling during initialization")
	2. 9a0615af7746 ("virtio: fix restart")

The second patch fixes somehow issue intoduced in the first patch, but actually
also breaks vhost in the way described above.
It's not pretty clear for me what to do in current situation with virtio,
because it will be broken for guest application even if vhost will not crash.

May be it'll be better to forbid stopping of virtio device and force user to
exit and start again (may be implemented in hidden from user way)?

This patch adds additional sane checks, so it should be applied anyway, IMHO.

 lib/librte_vhost/vhost_rxtx.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/lib/librte_vhost/vhost_rxtx.c b/lib/librte_vhost/vhost_rxtx.c
index 750821a..9d05739 100644
--- a/lib/librte_vhost/vhost_rxtx.c
+++ b/lib/librte_vhost/vhost_rxtx.c
@@ -147,10 +147,10 @@ copy_mbuf_to_desc(struct virtio_net *dev, struct vhost_virtqueue *vq,
 	struct virtio_net_hdr_mrg_rxbuf virtio_hdr = {{0, 0, 0, 0, 0, 0}, 0};
 
 	desc = &vq->desc[desc_idx];
-	if (unlikely(desc->len < vq->vhost_hlen))
+	desc_addr = gpa_to_vva(dev, desc->addr);
+	if (unlikely(desc->len < vq->vhost_hlen || !desc_addr))
 		return -1;
 
-	desc_addr = gpa_to_vva(dev, desc->addr);
 	rte_prefetch0((void *)(uintptr_t)desc_addr);
 
 	virtio_enqueue_offload(m, &virtio_hdr.hdr);
@@ -184,6 +184,9 @@ copy_mbuf_to_desc(struct virtio_net *dev, struct vhost_virtqueue *vq,
 
 			desc = &vq->desc[desc->next];
 			desc_addr   = gpa_to_vva(dev, desc->addr);
+			if (unlikely(!desc_addr))
+				return -1;
+
 			desc_offset = 0;
 			desc_avail  = desc->len;
 		}
@@ -344,7 +347,8 @@ fill_vec_buf(struct vhost_virtqueue *vq, uint32_t avail_idx,
 	uint32_t len    = *allocated;
 
 	while (1) {
-		if (unlikely(vec_id >= BUF_VECTOR_MAX || idx >= vq->size))
+		if (unlikely(vec_id >= BUF_VECTOR_MAX || idx >= vq->size
+			|| !vq->desc[idx].addr))
 			return -1;
 
 		len += vq->desc[idx].len;
@@ -747,10 +751,10 @@ copy_desc_to_mbuf(struct virtio_net *dev, struct vhost_virtqueue *vq,
 	uint32_t nr_desc = 1;
 
 	desc = &vq->desc[desc_idx];
-	if (unlikely(desc->len < vq->vhost_hlen))
+	desc_addr = gpa_to_vva(dev, desc->addr);
+	if (unlikely(desc->len < vq->vhost_hlen || !desc_addr))
 		return -1;
 
-	desc_addr = gpa_to_vva(dev, desc->addr);
 	rte_prefetch0((void *)(uintptr_t)desc_addr);
 
 	/* Retrieve virtio net header */
@@ -769,6 +773,9 @@ copy_desc_to_mbuf(struct virtio_net *dev, struct vhost_virtqueue *vq,
 			desc = &vq->desc[desc->next];
 
 			desc_addr = gpa_to_vva(dev, desc->addr);
+			if (unlikely(!desc_addr))
+				return -1;
+
 			rte_prefetch0((void *)(uintptr_t)desc_addr);
 
 			desc_offset = 0;
-- 
2.5.0

^ permalink raw reply related	[flat|nested] 39+ messages in thread