[dpdk-dev] [PATCH 0/6] Some fixes for vhost

[dpdk-dev] [PATCH 0/6] Some fixes for vhost

[Tiwei Bie]

Tiwei Bie (6):
  vhost: fix desc access in relay helpers
  vhost: fix possible out of bound access in relay helpers
  vhost: fix possible dead loop in relay helpers
  vhost: fix possible out of bound access in vector filling
  vhost: fix possible dead loop in vector filling
  vhost: ensure event idx is mapped when negotiated

 lib/librte_vhost/vdpa.c       | 55 ++++++++++++++++++++++++++++-------
 lib/librte_vhost/vhost_user.c | 15 ++++++----
 lib/librte_vhost/virtio_net.c | 16 ++++++++++
 3 files changed, 69 insertions(+), 17 deletions(-)

-- 
2.17.1

[dpdk-dev] [PATCH 1/6] vhost: fix desc access in relay helpers

[Tiwei Bie]

Descs in desc table should be indexed using the desc idx
instead of the idx of avail ring and used ring.

Fixes: b13ad2decc83 ("vhost: provide helpers for virtio ring relay")

Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
---
 lib/librte_vhost/vdpa.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/lib/librte_vhost/vdpa.c b/lib/librte_vhost/vdpa.c
index 240a1fe3a..77d92f9ad 100644
--- a/lib/librte_vhost/vdpa.c
+++ b/lib/librte_vhost/vdpa.c
@@ -190,10 +190,10 @@ rte_vdpa_relay_vring_avail(int vid, uint16_t qid, void *vring_m)
 			if (unlikely(!desc_ring))
 				return -1;
 
-			if (unlikely(dlen < vq->desc[idx].len)) {
+			if (unlikely(dlen < vq->desc[desc_id].len)) {
 				idesc = alloc_copy_ind_table(dev, vq,
-						vq->desc[idx].addr,
-						vq->desc[idx].len);
+						vq->desc[desc_id].addr,
+						vq->desc[desc_id].len);
 				if (unlikely(!idesc))
 					return -1;
 
@@ -279,10 +279,10 @@ rte_vdpa_relay_vring_used(int vid, uint16_t qid, void *vring_m)
 			if (unlikely(!desc_ring))
 				return -1;
 
-			if (unlikely(dlen < vq->desc[idx].len)) {
+			if (unlikely(dlen < vq->desc[desc_id].len)) {
 				idesc = alloc_copy_ind_table(dev, vq,
-						vq->desc[idx].addr,
-						vq->desc[idx].len);
+						vq->desc[desc_id].addr,
+						vq->desc[desc_id].len);
 				if (unlikely(!idesc))
 					return -1;
 
-- 
2.17.1

Re: [dpdk-dev] [PATCH 1/6] vhost: fix desc access in relay helpers

[Maxime Coquelin]

On 1/4/19 5:06 AM, Tiwei Bie wrote:
> Descs in desc table should be indexed using the desc idx
> instead of the idx of avail ring and used ring.
> 
> Fixes: b13ad2decc83 ("vhost: provide helpers for virtio ring relay")
> 
> Signed-off-by: Tiwei Bie<tiwei.bie@intel.com>
> ---
>   lib/librte_vhost/vdpa.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)

Good catch!

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

Thanks,
Maxime

[dpdk-dev] [PATCH 2/6] vhost: fix possible out of bound access in relay helpers

[Tiwei Bie]

Fixes: b13ad2decc83 ("vhost: provide helpers for virtio ring relay")

Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
---
 lib/librte_vhost/vdpa.c | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/lib/librte_vhost/vdpa.c b/lib/librte_vhost/vdpa.c
index 77d92f9ad..407d1c363 100644
--- a/lib/librte_vhost/vdpa.c
+++ b/lib/librte_vhost/vdpa.c
@@ -178,6 +178,9 @@ rte_vdpa_relay_vring_avail(int vid, uint16_t qid, void *vring_m)
 	while (idx_m != idx) {
 		/* avail entry copy */
 		desc_id = vq->avail->ring[idx_m & (vq->size - 1)];
+		if (unlikely(desc_id >= vq->size))
+			return -1;
+
 		s_vring->avail->ring[idx_m & (vq->size - 1)] = desc_id;
 		desc_ring = vq->desc;
 
@@ -205,15 +208,14 @@ rte_vdpa_relay_vring_avail(int vid, uint16_t qid, void *vring_m)
 
 		/* check if the buf addr is within the guest memory */
 		do {
+			if (unlikely(desc_id >= vq->size))
+				goto fail;
 			desc = desc_ring[desc_id];
 			perm = desc.flags & VRING_DESC_F_WRITE ?
 				VHOST_ACCESS_WO : VHOST_ACCESS_RO;
 			if (invalid_desc_check(dev, vq, desc.addr, desc.len,
-						perm)) {
-				if (unlikely(idesc))
-					free_ind_table(idesc);
-				return -1;
-			}
+						perm))
+				goto fail;
 			desc_id = desc.next;
 		} while (desc.flags & VRING_DESC_F_NEXT);
 
@@ -232,6 +234,11 @@ rte_vdpa_relay_vring_avail(int vid, uint16_t qid, void *vring_m)
 		vhost_avail_event(vq) = idx;
 
 	return ret;
+
+fail:
+	if (unlikely(idesc))
+		free_ind_table(idesc);
+	return -1;
 }
 
 int __rte_experimental
@@ -270,6 +277,9 @@ rte_vdpa_relay_vring_used(int vid, uint16_t qid, void *vring_m)
 		desc_id = vq->used->ring[idx & (vq->size - 1)].id;
 		desc_ring = vq->desc;
 
+		if (unlikely(desc_id >= vq->size))
+			return -1;
+
 		if (vq->desc[desc_id].flags & VRING_DESC_F_INDIRECT) {
 			dlen = vq->desc[desc_id].len;
 			desc_ring = (struct vring_desc *)(uintptr_t)
@@ -294,6 +304,8 @@ rte_vdpa_relay_vring_used(int vid, uint16_t qid, void *vring_m)
 
 		/* dirty page logging for DMA writeable buffer */
 		do {
+			if (unlikely(desc_id >= vq->size))
+				goto fail;
 			desc = desc_ring[desc_id];
 			if (desc.flags & VRING_DESC_F_WRITE)
 				vhost_log_write(dev, desc.addr, desc.len);
@@ -315,4 +327,9 @@ rte_vdpa_relay_vring_used(int vid, uint16_t qid, void *vring_m)
 		vring_used_event(s_vring) = idx_m;
 
 	return ret;
+
+fail:
+	if (unlikely(idesc))
+		free_ind_table(idesc);
+	return -1;
 }
-- 
2.17.1

Re: [dpdk-dev] [PATCH 2/6] vhost: fix possible out of bound access in relay helpers

[Maxime Coquelin]

On 1/4/19 5:06 AM, Tiwei Bie wrote:
> Fixes: b13ad2decc83 ("vhost: provide helpers for virtio ring relay")
> 
> Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> ---
>   lib/librte_vhost/vdpa.c | 27 ++++++++++++++++++++++-----
>   1 file changed, 22 insertions(+), 5 deletions(-)
> 

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

Thanks,
Maxime

[dpdk-dev] [PATCH 3/6] vhost: fix possible dead loop in relay helpers

[Tiwei Bie]

Fix a possible dead loop which may happen, e.g. when
driver created a loop in the desc list.

Fixes: b13ad2decc83 ("vhost: provide helpers for virtio ring relay")

Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
---
 lib/librte_vhost/vdpa.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/lib/librte_vhost/vdpa.c b/lib/librte_vhost/vdpa.c
index 407d1c363..02083bb8a 100644
--- a/lib/librte_vhost/vdpa.c
+++ b/lib/librte_vhost/vdpa.c
@@ -157,6 +157,7 @@ rte_vdpa_relay_vring_avail(int vid, uint16_t qid, void *vring_m)
 	struct vring_desc *idesc = NULL;
 	struct vring *s_vring;
 	uint64_t dlen;
+	uint32_t nr_descs;
 	int ret;
 	uint8_t perm;
 
@@ -183,9 +184,14 @@ rte_vdpa_relay_vring_avail(int vid, uint16_t qid, void *vring_m)
 
 		s_vring->avail->ring[idx_m & (vq->size - 1)] = desc_id;
 		desc_ring = vq->desc;
+		nr_descs = vq->size;
 
 		if (vq->desc[desc_id].flags & VRING_DESC_F_INDIRECT) {
 			dlen = vq->desc[desc_id].len;
+			nr_descs = dlen / sizeof(struct vring_desc);
+			if (unlikely(nr_descs > vq->size))
+				return -1;
+
 			desc_ring = (struct vring_desc *)(uintptr_t)
 				vhost_iova_to_vva(dev, vq,
 						vq->desc[desc_id].addr, &dlen,
@@ -210,6 +216,8 @@ rte_vdpa_relay_vring_avail(int vid, uint16_t qid, void *vring_m)
 		do {
 			if (unlikely(desc_id >= vq->size))
 				goto fail;
+			if (unlikely(nr_descs-- == 0))
+				goto fail;
 			desc = desc_ring[desc_id];
 			perm = desc.flags & VRING_DESC_F_WRITE ?
 				VHOST_ACCESS_WO : VHOST_ACCESS_RO;
@@ -252,6 +260,7 @@ rte_vdpa_relay_vring_used(int vid, uint16_t qid, void *vring_m)
 	struct vring_desc *idesc = NULL;
 	struct vring *s_vring;
 	uint64_t dlen;
+	uint32_t nr_descs;
 	int ret;
 
 	if (!dev || !vring_m)
@@ -276,12 +285,17 @@ rte_vdpa_relay_vring_used(int vid, uint16_t qid, void *vring_m)
 
 		desc_id = vq->used->ring[idx & (vq->size - 1)].id;
 		desc_ring = vq->desc;
+		nr_descs = vq->size;
 
 		if (unlikely(desc_id >= vq->size))
 			return -1;
 
 		if (vq->desc[desc_id].flags & VRING_DESC_F_INDIRECT) {
 			dlen = vq->desc[desc_id].len;
+			nr_descs = dlen / sizeof(struct vring_desc);
+			if (unlikely(nr_descs > vq->size))
+				return -1;
+
 			desc_ring = (struct vring_desc *)(uintptr_t)
 				vhost_iova_to_vva(dev, vq,
 						vq->desc[desc_id].addr, &dlen,
@@ -306,6 +320,8 @@ rte_vdpa_relay_vring_used(int vid, uint16_t qid, void *vring_m)
 		do {
 			if (unlikely(desc_id >= vq->size))
 				goto fail;
+			if (unlikely(nr_descs-- == 0))
+				goto fail;
 			desc = desc_ring[desc_id];
 			if (desc.flags & VRING_DESC_F_WRITE)
 				vhost_log_write(dev, desc.addr, desc.len);
-- 
2.17.1

Re: [dpdk-dev] [PATCH 3/6] vhost: fix possible dead loop in relay helpers

[Maxime Coquelin]

On 1/4/19 5:06 AM, Tiwei Bie wrote:
> Fix a possible dead loop which may happen, e.g. when
> driver created a loop in the desc list.
> 
> Fixes: b13ad2decc83 ("vhost: provide helpers for virtio ring relay")
> 
> Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> ---
>   lib/librte_vhost/vdpa.c | 16 ++++++++++++++++
>   1 file changed, 16 insertions(+)
> 

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

Thanks,
Maxime

[dpdk-dev] [PATCH 4/6] vhost: fix possible out of bound access in vector filling

[Tiwei Bie]

Fixes: 7f74b95c444f ("vhost: pre update used ring for Tx and Rx")
Cc: stable@dpdk.org

Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
---
 lib/librte_vhost/virtio_net.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
index 474acf64d..d64c355b9 100644
--- a/lib/librte_vhost/virtio_net.c
+++ b/lib/librte_vhost/virtio_net.c
@@ -312,6 +312,9 @@ fill_vec_buf_split(struct virtio_net *dev, struct vhost_virtqueue *vq,
 	struct vring_desc *descs = vq->desc;
 	struct vring_desc *idesc = NULL;
 
+	if (unlikely(idx >= vq->size))
+		return -1;
+
 	*desc_chain_head = idx;
 
 	if (vq->desc[idx].flags & VRING_DESC_F_INDIRECT) {
-- 
2.17.1

Re: [dpdk-dev] [PATCH 4/6] vhost: fix possible out of bound access in vector filling

[Maxime Coquelin]

On 1/4/19 5:06 AM, Tiwei Bie wrote:
> Fixes: 7f74b95c444f ("vhost: pre update used ring for Tx and Rx")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> ---
>   lib/librte_vhost/virtio_net.c | 3 +++
>   1 file changed, 3 insertions(+)
> 

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

Thanks,
Maxime

[dpdk-dev] [PATCH 5/6] vhost: fix possible dead loop in vector filling

[Tiwei Bie]

Fix a possible dead loop which may happen, e.g. when driver
created a loop in the desc list and lens in descs are zero.

Fixes: fd68b4739d2c ("vhost: use buffer vectors in dequeue path")
Fixes: 2f3225a7d69b ("vhost: add vector filling support for packed ring")
Cc: stable@dpdk.org

Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
---
 lib/librte_vhost/virtio_net.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
index d64c355b9..0893a1d04 100644
--- a/lib/librte_vhost/virtio_net.c
+++ b/lib/librte_vhost/virtio_net.c
@@ -309,6 +309,7 @@ fill_vec_buf_split(struct virtio_net *dev, struct vhost_virtqueue *vq,
 	uint16_t vec_id = *vec_idx;
 	uint32_t len    = 0;
 	uint64_t dlen;
+	uint32_t nr_descs = vq->size;
 	struct vring_desc *descs = vq->desc;
 	struct vring_desc *idesc = NULL;
 
@@ -319,6 +320,10 @@ fill_vec_buf_split(struct virtio_net *dev, struct vhost_virtqueue *vq,
 
 	if (vq->desc[idx].flags & VRING_DESC_F_INDIRECT) {
 		dlen = vq->desc[idx].len;
+		nr_descs = dlen / sizeof(struct vring_desc);
+		if (unlikely(nr_descs > vq->size))
+			return -1;
+
 		descs = (struct vring_desc *)(uintptr_t)
 			vhost_iova_to_vva(dev, vq, vq->desc[idx].addr,
 						&dlen,
@@ -348,6 +353,11 @@ fill_vec_buf_split(struct virtio_net *dev, struct vhost_virtqueue *vq,
 			return -1;
 		}
 
+		if (unlikely(nr_descs-- == 0)) {
+			free_ind_table(idesc);
+			return -1;
+		}
+
 		len += descs[idx].len;
 
 		if (unlikely(map_one_desc(dev, vq, buf_vec, &vec_id,
@@ -510,6 +520,9 @@ fill_vec_buf_packed(struct virtio_net *dev, struct vhost_virtqueue *vq,
 		if (unlikely(vec_id >= BUF_VECTOR_MAX))
 			return -1;
 
+		if (unlikely(*desc_count >= vq->size))
+			return -1;
+
 		*desc_count += 1;
 		*buf_id = descs[avail_idx].id;
 
-- 
2.17.1

Re: [dpdk-dev] [PATCH 5/6] vhost: fix possible dead loop in vector filling

[Maxime Coquelin]

On 1/4/19 5:06 AM, Tiwei Bie wrote:
> Fix a possible dead loop which may happen, e.g. when driver
> created a loop in the desc list and lens in descs are zero.
> 
> Fixes: fd68b4739d2c ("vhost: use buffer vectors in dequeue path")
> Fixes: 2f3225a7d69b ("vhost: add vector filling support for packed ring")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> ---
>   lib/librte_vhost/virtio_net.c | 13 +++++++++++++
>   1 file changed, 13 insertions(+)
> 

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

Thanks,
Maxime

[dpdk-dev] [PATCH 6/6] vhost: ensure event idx is mapped when negotiated

[Tiwei Bie]

Fixes: 30920b1e2b15 ("vhost: ensure all range is mapped when translating QVAs")
Cc: stable@dpdk.org

Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
---
 lib/librte_vhost/vhost_user.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 8fec773d5..1843e032f 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -541,7 +541,7 @@ translate_ring_addresses(struct virtio_net *dev, int vq_index)
 {
 	struct vhost_virtqueue *vq = dev->virtqueue[vq_index];
 	struct vhost_vring_addr *addr = &vq->ring_addrs;
-	uint64_t len;
+	uint64_t len, expected_len;
 
 	if (vq_is_packed(dev)) {
 		len = sizeof(struct vring_packed_desc) * vq->size;
@@ -607,11 +607,12 @@ translate_ring_addresses(struct virtio_net *dev, int vq_index)
 	addr = &vq->ring_addrs;
 
 	len = sizeof(struct vring_avail) + sizeof(uint16_t) * vq->size;
+	if (dev->features & (1ULL << VIRTIO_RING_F_EVENT_IDX))
+		len += sizeof(uint16_t);
+	expected_len = len;
 	vq->avail = (struct vring_avail *)(uintptr_t)ring_addr_to_vva(dev,
 			vq, addr->avail_user_addr, &len);
-	if (vq->avail == 0 ||
-			len != sizeof(struct vring_avail) +
-			sizeof(uint16_t) * vq->size) {
+	if (vq->avail == 0 || len != expected_len) {
 		RTE_LOG(DEBUG, VHOST_CONFIG,
 			"(%d) failed to map avail ring.\n",
 			dev->vid);
@@ -620,10 +621,12 @@ translate_ring_addresses(struct virtio_net *dev, int vq_index)
 
 	len = sizeof(struct vring_used) +
 		sizeof(struct vring_used_elem) * vq->size;
+	if (dev->features & (1ULL << VIRTIO_RING_F_EVENT_IDX))
+		len += sizeof(uint16_t);
+	expected_len = len;
 	vq->used = (struct vring_used *)(uintptr_t)ring_addr_to_vva(dev,
 			vq, addr->used_user_addr, &len);
-	if (vq->used == 0 || len != sizeof(struct vring_used) +
-			sizeof(struct vring_used_elem) * vq->size) {
+	if (vq->used == 0 || len != expected_len) {
 		RTE_LOG(DEBUG, VHOST_CONFIG,
 			"(%d) failed to map used ring.\n",
 			dev->vid);
-- 
2.17.1

Re: [dpdk-dev] [PATCH 6/6] vhost: ensure event idx is mapped when negotiated

[Maxime Coquelin]

On 1/4/19 5:06 AM, Tiwei Bie wrote:
> Fixes: 30920b1e2b15 ("vhost: ensure all range is mapped when translating QVAs")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> ---
>   lib/librte_vhost/vhost_user.c | 15 +++++++++------
>   1 file changed, 9 insertions(+), 6 deletions(-)
> 

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

Thanks,
Maxime

Re: [dpdk-dev] [PATCH 0/6] Some fixes for vhost

[Maxime Coquelin]

On 1/4/19 5:06 AM, Tiwei Bie wrote:
> Tiwei Bie (6):
>    vhost: fix desc access in relay helpers
>    vhost: fix possible out of bound access in relay helpers
>    vhost: fix possible dead loop in relay helpers
>    vhost: fix possible out of bound access in vector filling
>    vhost: fix possible dead loop in vector filling
>    vhost: ensure event idx is mapped when negotiated
> 
>   lib/librte_vhost/vdpa.c       | 55 ++++++++++++++++++++++++++++-------
>   lib/librte_vhost/vhost_user.c | 15 ++++++----
>   lib/librte_vhost/virtio_net.c | 16 ++++++++++
>   3 files changed, 69 insertions(+), 17 deletions(-)
> 
Applied to dpdk-next-virtio

Thanks,
Maxime