[dpdk-stable] [PATCH 1/3] vhost: fix dead loop in enqueue path

[dpdk-stable] [PATCH 1/3] vhost: fix dead loop in enqueue path

[Yuanhan Liu]

If a malicious guest forges a dead loop desc chain (let desc->next point
to itself) and desc->len is zero, this could lead to a dead loop in
copy_mbuf_to_desc(following is a simplified code to show this issue
clearly):

    while (mbuf_is_not_totally_consumed) {
        if (desc_avail == 0) {
            desc = &descs[desc->next];
            desc_avail = desc->len;
        }

        COPY(desc, mbuf, desc_avail);
    }

I have actually fixed a same issue before: a436f53ebfeb ("vhost: avoid
dead loop chain"); it fixes the dequeue path though, leaving the enqueue
path still vulnerable.

The fix is the same. Add a var nr_desc to avoid the dead loop.

Fixes: f1a519ad981c ("vhost: fix enqueue/dequeue to handle chained vring descriptors")

Cc: stable@dpdk.org
Reported-by: Xieming Katty <katty.xieming@huawei.com>
Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
 lib/librte_vhost/virtio_net.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
index 595f67c..143c0fa 100644
--- a/lib/librte_vhost/virtio_net.c
+++ b/lib/librte_vhost/virtio_net.c
@@ -195,6 +195,8 @@ static inline int __attribute__((always_inline))
 	struct vring_desc *desc;
 	uint64_t desc_addr;
 	struct virtio_net_hdr_mrg_rxbuf virtio_hdr = {{0, 0, 0, 0, 0, 0}, 0};
+	/* A counter to avoid desc dead loop chain */
+	uint16_t nr_desc = 1;
 
 	desc = &descs[desc_idx];
 	desc_addr = gpa_to_vva(dev, desc->addr);
@@ -233,7 +235,7 @@ static inline int __attribute__((always_inline))
 				/* Room in vring buffer is not enough */
 				return -1;
 			}
-			if (unlikely(desc->next >= size))
+			if (unlikely(desc->next >= size || ++nr_desc > size))
 				return -1;
 
 			desc = &descs[desc->next];
-- 
1.9.0

Re: [dpdk-stable] [dpdk-dev] [PATCH 1/3] vhost: fix dead loop in enqueue path

[Maxime Coquelin]

On 01/22/2017 09:46 AM, Yuanhan Liu wrote:
> If a malicious guest forges a dead loop desc chain (let desc->next point
> to itself) and desc->len is zero, this could lead to a dead loop in
> copy_mbuf_to_desc(following is a simplified code to show this issue
> clearly):
>
>     while (mbuf_is_not_totally_consumed) {
>         if (desc_avail == 0) {
>             desc = &descs[desc->next];
>             desc_avail = desc->len;
>         }
>
>         COPY(desc, mbuf, desc_avail);
>     }
>
> I have actually fixed a same issue before: a436f53ebfeb ("vhost: avoid
> dead loop chain"); it fixes the dequeue path though, leaving the enqueue
> path still vulnerable.
>
> The fix is the same. Add a var nr_desc to avoid the dead loop.
>
> Fixes: f1a519ad981c ("vhost: fix enqueue/dequeue to handle chained vring descriptors")
>
> Cc: stable@dpdk.org
> Reported-by: Xieming Katty <katty.xieming@huawei.com>
> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> ---
>  lib/librte_vhost/virtio_net.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

Thanks for the fix:
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

  - Maxime

[dpdk-stable] [PATCH 2/3] vhost: fix long stall of vhost-user negotiation

[Yuanhan Liu]

Setting up the mapping from GPA (guest physical address) to HPA (guest
physical address) could be very time consuming when the guest memory is
backened with small pages (4K). The bigger the guest memory, the longer
it takes. This could lead a very long vhost-user negotiation.

Since the mapping is only needed in zero copy mode so far, we could
avoid such time consuming settup when zero copy is turned off (which is
the default case).

It's actually a workaround, a right fix might be to start a new thread,
and hide the big latency there.

Fixes: e246896178e6 ("vhost: get guest/host physical address mappings")

Cc: stable@dpdk.org
Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
 lib/librte_vhost/vhost_user.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 7343a00..ab7f3fc 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -567,7 +567,8 @@
 		reg->host_user_addr = (uint64_t)(uintptr_t)mmap_addr +
 				      mmap_offset;
 
-		add_guest_pages(dev, reg, alignment);
+		if (dev->dequeue_zero_copy)
+			add_guest_pages(dev, reg, alignment);
 
 		RTE_LOG(INFO, VHOST_CONFIG,
 			"guest memory region %u, size: 0x%" PRIx64 "\n"
-- 
1.9.0

Re: [dpdk-stable] [dpdk-dev] [PATCH 2/3] vhost: fix long stall of vhost-user negotiation

[Maxime Coquelin]

On 01/22/2017 09:46 AM, Yuanhan Liu wrote:
> Setting up the mapping from GPA (guest physical address) to HPA (guest
> physical address) could be very time consuming when the guest memory is
> backened with small pages (4K). The bigger the guest memory, the longer
> it takes. This could lead a very long vhost-user negotiation.
>
> Since the mapping is only needed in zero copy mode so far, we could
> avoid such time consuming settup when zero copy is turned off (which is
> the default case).
>
> It's actually a workaround, a right fix might be to start a new thread,
> and hide the big latency there.
>
> Fixes: e246896178e6 ("vhost: get guest/host physical address mappings")
>
> Cc: stable@dpdk.org
> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> ---
>  lib/librte_vhost/vhost_user.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime

[dpdk-stable] [PATCH 3/3] net/virtio: fix crash when number of virtio devices > 1

[Yuanhan Liu]

The vtpci_ops assignment needs the 'hw->port_id' as an input parameter.
That said, we should set 'hw->port_id' firstly, then do the vtpci_ops
assignment, while the code does reversely. That would result to a crash
when more than one virtio devices are used, because we keep assigning
proper vtpci_ops to virtio_hw_internal[0]->vtpci_ops, leaving the pointer
for other ports being NULL.

Reverse the order fixes this issue.

Fixes: 9470427c88e1 ("net/virtio: do not store PCI device pointer at shared memory")

Cc: stable@dpdk.org
Reported-by: Lei Yao <lei.a.yao@intel.com>
Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
 drivers/net/virtio/virtio_ethdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 1d51942..68dde08 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1515,6 +1515,7 @@ static int virtio_dev_xstats_get_names(struct rte_eth_dev *dev,
 		return -ENOMEM;
 	}
 
+	hw->port_id = eth_dev->data->port_id;
 	/* For virtio_user case the hw->virtio_user_dev is populated by
 	 * virtio_user_eth_dev_alloc() before eth_virtio_dev_init() is called.
 	 */
@@ -1525,7 +1526,6 @@ static int virtio_dev_xstats_get_names(struct rte_eth_dev *dev,
 			return ret;
 	}
 
-	hw->port_id = eth_dev->data->port_id;
 	eth_dev->data->dev_flags = dev_flags;
 
 	/* reset device and negotiate default features */
-- 
1.9.0

Re: [dpdk-stable] [dpdk-dev] [PATCH 3/3] net/virtio: fix crash when number of virtio devices > 1

[Maxime Coquelin]

On 01/22/2017 09:47 AM, Yuanhan Liu wrote:
> The vtpci_ops assignment needs the 'hw->port_id' as an input parameter.
> That said, we should set 'hw->port_id' firstly, then do the vtpci_ops
> assignment, while the code does reversely. That would result to a crash
> when more than one virtio devices are used, because we keep assigning
> proper vtpci_ops to virtio_hw_internal[0]->vtpci_ops, leaving the pointer
> for other ports being NULL.
>
> Reverse the order fixes this issue.
>
> Fixes: 9470427c88e1 ("net/virtio: do not store PCI device pointer at shared memory")
>
> Cc: stable@dpdk.org
> Reported-by: Lei Yao <lei.a.yao@intel.com>
> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> ---
>  drivers/net/virtio/virtio_ethdev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime