patches for DPDK stable branches
 help / color / mirror / Atom feed
* [dpdk-stable] [PATCH 1/7] vhost: fix virtqueues metadata allocation
       [not found] <20201019173415.582407-1-maxime.coquelin@redhat.com>
@ 2020-10-19 17:34 ` Maxime Coquelin
  2020-10-21 11:10   ` Xia, Chenbo
  2020-10-19 17:34 ` [dpdk-stable] [PATCH 2/7] vhost: validate index in available entries API Maxime Coquelin
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 14+ messages in thread
From: Maxime Coquelin @ 2020-10-19 17:34 UTC (permalink / raw)
  To: dev, chenbo.xia, amorenoz; +Cc: Maxime Coquelin, stable

The Vhost-user backend implementation assumes there will be
no holes in the device's array of virtqueues metadata
pointers.

It can happen though, and would cause segmentation faults,
memory leaks or undefined behaviour.

This patch keep the assumption that there is no holes in this
array, and allocate all uninitialized virtqueues metadata up
to requested index.

Fixes: 160cbc815b41 ("vhost: remove a hack on queue allocation")
Cc: stable@dpdk.org

Suggested-by: Adrian Moreno <amorenoz@redhat.com>
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/librte_vhost/vhost.c | 33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
index 6068c38ec6..0c9ba3b3af 100644
--- a/lib/librte_vhost/vhost.c
+++ b/lib/librte_vhost/vhost.c
@@ -579,22 +579,29 @@ int
 alloc_vring_queue(struct virtio_net *dev, uint32_t vring_idx)
 {
 	struct vhost_virtqueue *vq;
+	uint32_t i;
 
-	vq = rte_malloc(NULL, sizeof(struct vhost_virtqueue), 0);
-	if (vq == NULL) {
-		VHOST_LOG_CONFIG(ERR,
-			"Failed to allocate memory for vring:%u.\n", vring_idx);
-		return -1;
-	}
+	/* Also allocate holes, if any, up to requested vring index. */
+	for (i = 0; i <= vring_idx; i++) {
+		if (dev->virtqueue[i])
+			continue;
 
-	dev->virtqueue[vring_idx] = vq;
-	init_vring_queue(dev, vring_idx);
-	rte_spinlock_init(&vq->access_lock);
-	vq->avail_wrap_counter = 1;
-	vq->used_wrap_counter = 1;
-	vq->signalled_used_valid = false;
+		vq = rte_malloc(NULL, sizeof(struct vhost_virtqueue), 0);
+		if (vq == NULL) {
+			VHOST_LOG_CONFIG(ERR,
+				"Failed to allocate memory for vring:%u.\n", i);
+			return -1;
+		}
+
+		dev->virtqueue[i] = vq;
+		init_vring_queue(dev, vring_idx);
+		rte_spinlock_init(&vq->access_lock);
+		vq->avail_wrap_counter = 1;
+		vq->used_wrap_counter = 1;
+		vq->signalled_used_valid = false;
+	}
 
-	dev->nr_vring += 1;
+	dev->nr_vring = RTE_MAX(dev->nr_vring, vring_idx + 1);
 
 	return 0;
 }
-- 
2.26.2


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

* [dpdk-stable] [PATCH 2/7] vhost: validate index in available entries API
       [not found] <20201019173415.582407-1-maxime.coquelin@redhat.com>
  2020-10-19 17:34 ` [dpdk-stable] [PATCH 1/7] vhost: fix virtqueues metadata allocation Maxime Coquelin
@ 2020-10-19 17:34 ` Maxime Coquelin
  2020-10-21 11:28   ` Xia, Chenbo
  2020-10-19 17:34 ` [dpdk-stable] [PATCH 3/7] vhost: validate index in guest notification API Maxime Coquelin
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 14+ messages in thread
From: Maxime Coquelin @ 2020-10-19 17:34 UTC (permalink / raw)
  To: dev, chenbo.xia, amorenoz; +Cc: Maxime Coquelin, stable

This patch validates the queue index parameter, in order
to ensure neither out-of-bound accesses nor NULL pointer
dereferencing happen.

Fixes: a67f286a6596 ("vhost: export queue free entries")
Cc: stable@dpdk.org

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/librte_vhost/vhost.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
index 0c9ba3b3af..193dafc369 100644
--- a/lib/librte_vhost/vhost.c
+++ b/lib/librte_vhost/vhost.c
@@ -1260,7 +1260,12 @@ rte_vhost_avail_entries(int vid, uint16_t queue_id)
 	if (!dev)
 		return 0;
 
+	if (queue_id >= VHOST_MAX_VRING)
+		return 0;
+
 	vq = dev->virtqueue[queue_id];
+	if (!vq)
+		return 0;
 
 	rte_spinlock_lock(&vq->access_lock);
 
-- 
2.26.2


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

* [dpdk-stable] [PATCH 3/7] vhost: validate index in guest notification API
       [not found] <20201019173415.582407-1-maxime.coquelin@redhat.com>
  2020-10-19 17:34 ` [dpdk-stable] [PATCH 1/7] vhost: fix virtqueues metadata allocation Maxime Coquelin
  2020-10-19 17:34 ` [dpdk-stable] [PATCH 2/7] vhost: validate index in available entries API Maxime Coquelin
@ 2020-10-19 17:34 ` Maxime Coquelin
  2020-10-21 11:30   ` Xia, Chenbo
  2020-10-19 17:34 ` [dpdk-stable] [PATCH 4/7] vhost: validate index in live-migration API Maxime Coquelin
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 14+ messages in thread
From: Maxime Coquelin @ 2020-10-19 17:34 UTC (permalink / raw)
  To: dev, chenbo.xia, amorenoz; +Cc: Maxime Coquelin, stable

This patch validates the queue index parameter, in order
to ensure neither out-of-bound accesses nor NULL pointer
dereferencing happen.

Fixes: 9eed6bfd2efb ("vhost: allow to enable or disable features")
Cc: stable@dpdk.org

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/librte_vhost/vhost.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
index 193dafc369..801a1a5098 100644
--- a/lib/librte_vhost/vhost.c
+++ b/lib/librte_vhost/vhost.c
@@ -1352,7 +1352,12 @@ rte_vhost_enable_guest_notification(int vid, uint16_t queue_id, int enable)
 	if (!dev)
 		return -1;
 
+	if (queue_id >= VHOST_MAX_VRING)
+		return -1;
+
 	vq = dev->virtqueue[queue_id];
+	if (!vq)
+		return -1;
 
 	rte_spinlock_lock(&vq->access_lock);
 
-- 
2.26.2


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

* [dpdk-stable] [PATCH 4/7] vhost: validate index in live-migration API
       [not found] <20201019173415.582407-1-maxime.coquelin@redhat.com>
                   ` (2 preceding siblings ...)
  2020-10-19 17:34 ` [dpdk-stable] [PATCH 3/7] vhost: validate index in guest notification API Maxime Coquelin
@ 2020-10-19 17:34 ` Maxime Coquelin
  2020-10-21 11:30   ` Xia, Chenbo
  2020-10-19 17:34 ` [dpdk-stable] [PATCH 5/7] vhost: validate index in inflight API Maxime Coquelin
  2020-10-19 17:34 ` [dpdk-stable] [PATCH 6/7] vhost: validate index in async API Maxime Coquelin
  5 siblings, 1 reply; 14+ messages in thread
From: Maxime Coquelin @ 2020-10-19 17:34 UTC (permalink / raw)
  To: dev, chenbo.xia, amorenoz; +Cc: Maxime Coquelin, stable

This patch validates the queue index parameter, in order
to ensure no out-of-bound accesses happen.

Fixes: bd2e0c3fe5ac ("vhost: add APIs for live migration")
Cc: stable@dpdk.org

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/librte_vhost/vhost.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
index 801a1a5098..b9afe46ca2 100644
--- a/lib/librte_vhost/vhost.c
+++ b/lib/librte_vhost/vhost.c
@@ -1467,6 +1467,9 @@ int rte_vhost_get_vring_base(int vid, uint16_t queue_id,
 	if (dev == NULL || last_avail_idx == NULL || last_used_idx == NULL)
 		return -1;
 
+	if (queue_id >= VHOST_MAX_VRING)
+		return -1;
+
 	vq = dev->virtqueue[queue_id];
 	if (!vq)
 		return -1;
@@ -1493,6 +1496,9 @@ int rte_vhost_set_vring_base(int vid, uint16_t queue_id,
 	if (!dev)
 		return -1;
 
+	if (queue_id >= VHOST_MAX_VRING)
+		return -1;
+
 	vq = dev->virtqueue[queue_id];
 	if (!vq)
 		return -1;
-- 
2.26.2


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

* [dpdk-stable] [PATCH 5/7] vhost: validate index in inflight API
       [not found] <20201019173415.582407-1-maxime.coquelin@redhat.com>
                   ` (3 preceding siblings ...)
  2020-10-19 17:34 ` [dpdk-stable] [PATCH 4/7] vhost: validate index in live-migration API Maxime Coquelin
@ 2020-10-19 17:34 ` Maxime Coquelin
  2020-10-21 11:30   ` Xia, Chenbo
  2020-10-19 17:34 ` [dpdk-stable] [PATCH 6/7] vhost: validate index in async API Maxime Coquelin
  5 siblings, 1 reply; 14+ messages in thread
From: Maxime Coquelin @ 2020-10-19 17:34 UTC (permalink / raw)
  To: dev, chenbo.xia, amorenoz; +Cc: Maxime Coquelin, stable

This patch validates the queue index parameter, in order
to ensure neither out-of-bound accesses nor NULL pointer
dereferencing happen.

Fixes: 4d891f77ddfa ("vhost: add APIs to get inflight ring")
Cc: stable@dpdk.org

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/librte_vhost/vhost.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
index b9afe46ca2..f78bdfcc94 100644
--- a/lib/librte_vhost/vhost.c
+++ b/lib/librte_vhost/vhost.c
@@ -1523,15 +1523,23 @@ rte_vhost_get_vring_base_from_inflight(int vid,
 				       uint16_t *last_used_idx)
 {
 	struct rte_vhost_inflight_info_packed *inflight_info;
+	struct vhost_virtqueue *vq;
 	struct virtio_net *dev = get_device(vid);
 
 	if (dev == NULL || last_avail_idx == NULL || last_used_idx == NULL)
 		return -1;
 
+	if (queue_id >= VHOST_MAX_VRING)
+		return -1;
+
+	vq = dev->virtqueue[queue_id];
+	if (!vq)
+		return -1;
+
 	if (!vq_is_packed(dev))
 		return -1;
 
-	inflight_info = dev->virtqueue[queue_id]->inflight_packed;
+	inflight_info = vq->inflight_packed;
 	if (!inflight_info)
 		return -1;
 
-- 
2.26.2


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

* [dpdk-stable] [PATCH 6/7] vhost: validate index in async API
       [not found] <20201019173415.582407-1-maxime.coquelin@redhat.com>
                   ` (4 preceding siblings ...)
  2020-10-19 17:34 ` [dpdk-stable] [PATCH 5/7] vhost: validate index in inflight API Maxime Coquelin
@ 2020-10-19 17:34 ` Maxime Coquelin
  2020-10-21 11:31   ` Xia, Chenbo
  5 siblings, 1 reply; 14+ messages in thread
From: Maxime Coquelin @ 2020-10-19 17:34 UTC (permalink / raw)
  To: dev, chenbo.xia, amorenoz; +Cc: Maxime Coquelin, stable

This patch validates the queue index parameter, in order
to ensure no out-of-bound accesses happen.

Fixes: 9eed6bfd2efb ("vhost: allow to enable or disable features")
Cc: stable@dpdk.org

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/librte_vhost/vhost.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
index f78bdfcc94..e92ff618ac 100644
--- a/lib/librte_vhost/vhost.c
+++ b/lib/librte_vhost/vhost.c
@@ -1577,6 +1577,9 @@ int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
 
 	f.intval = features;
 
+	if (queue_id >= VHOST_MAX_VRING)
+		return -1;
+
 	vq = dev->virtqueue[queue_id];
 
 	if (unlikely(vq == NULL || !dev->async_copy))
@@ -1658,6 +1661,9 @@ int rte_vhost_async_channel_unregister(int vid, uint16_t queue_id)
 	if (dev == NULL)
 		return ret;
 
+	if (queue_id >= VHOST_MAX_VRING)
+		return ret;
+
 	vq = dev->virtqueue[queue_id];
 
 	if (vq == NULL)
-- 
2.26.2


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

* Re: [dpdk-stable] [PATCH 1/7] vhost: fix virtqueues metadata allocation
  2020-10-19 17:34 ` [dpdk-stable] [PATCH 1/7] vhost: fix virtqueues metadata allocation Maxime Coquelin
@ 2020-10-21 11:10   ` Xia, Chenbo
  2020-10-21 12:06     ` Maxime Coquelin
  0 siblings, 1 reply; 14+ messages in thread
From: Xia, Chenbo @ 2020-10-21 11:10 UTC (permalink / raw)
  To: Maxime Coquelin, dev, amorenoz; +Cc: stable

Hi Maxime,

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Tuesday, October 20, 2020 1:34 AM
> To: dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; amorenoz@redhat.com
> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>; stable@dpdk.org
> Subject: [PATCH 1/7] vhost: fix virtqueues metadata allocation
> 
> The Vhost-user backend implementation assumes there will be
> no holes in the device's array of virtqueues metadata
> pointers.
> 
> It can happen though, and would cause segmentation faults,
> memory leaks or undefined behaviour.

Could I ask when will this happen? 

When QEMU does not configure all virtqueues? I'm not very sure.
Could you point that out for me?

Thanks!
Chenbo

> 
> This patch keep the assumption that there is no holes in this
> array, and allocate all uninitialized virtqueues metadata up
> to requested index.
> 
> Fixes: 160cbc815b41 ("vhost: remove a hack on queue allocation")
> Cc: stable@dpdk.org
> 
> Suggested-by: Adrian Moreno <amorenoz@redhat.com>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  lib/librte_vhost/vhost.c | 33 ++++++++++++++++++++-------------
>  1 file changed, 20 insertions(+), 13 deletions(-)
> 
> diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
> index 6068c38ec6..0c9ba3b3af 100644
> --- a/lib/librte_vhost/vhost.c
> +++ b/lib/librte_vhost/vhost.c
> @@ -579,22 +579,29 @@ int
>  alloc_vring_queue(struct virtio_net *dev, uint32_t vring_idx)
>  {
>  	struct vhost_virtqueue *vq;
> +	uint32_t i;
> 
> -	vq = rte_malloc(NULL, sizeof(struct vhost_virtqueue), 0);
> -	if (vq == NULL) {
> -		VHOST_LOG_CONFIG(ERR,
> -			"Failed to allocate memory for vring:%u.\n", vring_idx);
> -		return -1;
> -	}
> +	/* Also allocate holes, if any, up to requested vring index. */
> +	for (i = 0; i <= vring_idx; i++) {
> +		if (dev->virtqueue[i])
> +			continue;
> 
> -	dev->virtqueue[vring_idx] = vq;
> -	init_vring_queue(dev, vring_idx);
> -	rte_spinlock_init(&vq->access_lock);
> -	vq->avail_wrap_counter = 1;
> -	vq->used_wrap_counter = 1;
> -	vq->signalled_used_valid = false;
> +		vq = rte_malloc(NULL, sizeof(struct vhost_virtqueue), 0);
> +		if (vq == NULL) {
> +			VHOST_LOG_CONFIG(ERR,
> +				"Failed to allocate memory for vring:%u.\n", i);
> +			return -1;
> +		}
> +
> +		dev->virtqueue[i] = vq;
> +		init_vring_queue(dev, vring_idx);
> +		rte_spinlock_init(&vq->access_lock);
> +		vq->avail_wrap_counter = 1;
> +		vq->used_wrap_counter = 1;
> +		vq->signalled_used_valid = false;
> +	}
> 
> -	dev->nr_vring += 1;
> +	dev->nr_vring = RTE_MAX(dev->nr_vring, vring_idx + 1);
> 
>  	return 0;
>  }
> --
> 2.26.2


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

* Re: [dpdk-stable] [PATCH 2/7] vhost: validate index in available entries API
  2020-10-19 17:34 ` [dpdk-stable] [PATCH 2/7] vhost: validate index in available entries API Maxime Coquelin
@ 2020-10-21 11:28   ` Xia, Chenbo
  0 siblings, 0 replies; 14+ messages in thread
From: Xia, Chenbo @ 2020-10-21 11:28 UTC (permalink / raw)
  To: Maxime Coquelin, dev, amorenoz; +Cc: stable

Hi Maxime,

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Tuesday, October 20, 2020 1:34 AM
> To: dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; amorenoz@redhat.com
> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>; stable@dpdk.org
> Subject: [PATCH 2/7] vhost: validate index in available entries API
> 
> This patch validates the queue index parameter, in order
> to ensure neither out-of-bound accesses nor NULL pointer
> dereferencing happen.
> 
> Fixes: a67f286a6596 ("vhost: export queue free entries")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  lib/librte_vhost/vhost.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
> index 0c9ba3b3af..193dafc369 100644
> --- a/lib/librte_vhost/vhost.c
> +++ b/lib/librte_vhost/vhost.c
> @@ -1260,7 +1260,12 @@ rte_vhost_avail_entries(int vid, uint16_t queue_id)
>  	if (!dev)
>  		return 0;
> 
> +	if (queue_id >= VHOST_MAX_VRING)
> +		return 0;
> +
>  	vq = dev->virtqueue[queue_id];
> +	if (!vq)
> +		return 0;
> 
>  	rte_spinlock_lock(&vq->access_lock);
> 
> --
> 2.26.2

Looking at the API again, I don't know if it is good to return 0 when there are no
available entries or other errors.

For this patch:

Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>

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

* Re: [dpdk-stable] [PATCH 3/7] vhost: validate index in guest notification API
  2020-10-19 17:34 ` [dpdk-stable] [PATCH 3/7] vhost: validate index in guest notification API Maxime Coquelin
@ 2020-10-21 11:30   ` Xia, Chenbo
  0 siblings, 0 replies; 14+ messages in thread
From: Xia, Chenbo @ 2020-10-21 11:30 UTC (permalink / raw)
  To: Maxime Coquelin, dev, amorenoz; +Cc: stable

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Tuesday, October 20, 2020 1:34 AM
> To: dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; amorenoz@redhat.com
> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>; stable@dpdk.org
> Subject: [PATCH 3/7] vhost: validate index in guest notification API
> 
> This patch validates the queue index parameter, in order
> to ensure neither out-of-bound accesses nor NULL pointer
> dereferencing happen.
> 
> Fixes: 9eed6bfd2efb ("vhost: allow to enable or disable features")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  lib/librte_vhost/vhost.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
> index 193dafc369..801a1a5098 100644
> --- a/lib/librte_vhost/vhost.c
> +++ b/lib/librte_vhost/vhost.c
> @@ -1352,7 +1352,12 @@ rte_vhost_enable_guest_notification(int vid,
> uint16_t queue_id, int enable)
>  	if (!dev)
>  		return -1;
> 
> +	if (queue_id >= VHOST_MAX_VRING)
> +		return -1;
> +
>  	vq = dev->virtqueue[queue_id];
> +	if (!vq)
> +		return -1;
> 
>  	rte_spinlock_lock(&vq->access_lock);
> 
> --
> 2.26.2

Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>

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

* Re: [dpdk-stable] [PATCH 4/7] vhost: validate index in live-migration API
  2020-10-19 17:34 ` [dpdk-stable] [PATCH 4/7] vhost: validate index in live-migration API Maxime Coquelin
@ 2020-10-21 11:30   ` Xia, Chenbo
  0 siblings, 0 replies; 14+ messages in thread
From: Xia, Chenbo @ 2020-10-21 11:30 UTC (permalink / raw)
  To: Maxime Coquelin, dev, amorenoz; +Cc: stable

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Tuesday, October 20, 2020 1:34 AM
> To: dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; amorenoz@redhat.com
> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>; stable@dpdk.org
> Subject: [PATCH 4/7] vhost: validate index in live-migration API
> 
> This patch validates the queue index parameter, in order
> to ensure no out-of-bound accesses happen.
> 
> Fixes: bd2e0c3fe5ac ("vhost: add APIs for live migration")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  lib/librte_vhost/vhost.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
> index 801a1a5098..b9afe46ca2 100644
> --- a/lib/librte_vhost/vhost.c
> +++ b/lib/librte_vhost/vhost.c
> @@ -1467,6 +1467,9 @@ int rte_vhost_get_vring_base(int vid, uint16_t
> queue_id,
>  	if (dev == NULL || last_avail_idx == NULL || last_used_idx == NULL)
>  		return -1;
> 
> +	if (queue_id >= VHOST_MAX_VRING)
> +		return -1;
> +
>  	vq = dev->virtqueue[queue_id];
>  	if (!vq)
>  		return -1;
> @@ -1493,6 +1496,9 @@ int rte_vhost_set_vring_base(int vid, uint16_t
> queue_id,
>  	if (!dev)
>  		return -1;
> 
> +	if (queue_id >= VHOST_MAX_VRING)
> +		return -1;
> +
>  	vq = dev->virtqueue[queue_id];
>  	if (!vq)
>  		return -1;
> --
> 2.26.2

Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>

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

* Re: [dpdk-stable] [PATCH 5/7] vhost: validate index in inflight API
  2020-10-19 17:34 ` [dpdk-stable] [PATCH 5/7] vhost: validate index in inflight API Maxime Coquelin
@ 2020-10-21 11:30   ` Xia, Chenbo
  0 siblings, 0 replies; 14+ messages in thread
From: Xia, Chenbo @ 2020-10-21 11:30 UTC (permalink / raw)
  To: Maxime Coquelin, dev, amorenoz; +Cc: stable

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Tuesday, October 20, 2020 1:34 AM
> To: dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; amorenoz@redhat.com
> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>; stable@dpdk.org
> Subject: [PATCH 5/7] vhost: validate index in inflight API
> 
> This patch validates the queue index parameter, in order
> to ensure neither out-of-bound accesses nor NULL pointer
> dereferencing happen.
> 
> Fixes: 4d891f77ddfa ("vhost: add APIs to get inflight ring")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  lib/librte_vhost/vhost.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
> index b9afe46ca2..f78bdfcc94 100644
> --- a/lib/librte_vhost/vhost.c
> +++ b/lib/librte_vhost/vhost.c
> @@ -1523,15 +1523,23 @@ rte_vhost_get_vring_base_from_inflight(int vid,
>  				       uint16_t *last_used_idx)
>  {
>  	struct rte_vhost_inflight_info_packed *inflight_info;
> +	struct vhost_virtqueue *vq;
>  	struct virtio_net *dev = get_device(vid);
> 
>  	if (dev == NULL || last_avail_idx == NULL || last_used_idx == NULL)
>  		return -1;
> 
> +	if (queue_id >= VHOST_MAX_VRING)
> +		return -1;
> +
> +	vq = dev->virtqueue[queue_id];
> +	if (!vq)
> +		return -1;
> +
>  	if (!vq_is_packed(dev))
>  		return -1;
> 
> -	inflight_info = dev->virtqueue[queue_id]->inflight_packed;
> +	inflight_info = vq->inflight_packed;
>  	if (!inflight_info)
>  		return -1;
> 
> --
> 2.26.2

Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>

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

* Re: [dpdk-stable] [PATCH 6/7] vhost: validate index in async API
  2020-10-19 17:34 ` [dpdk-stable] [PATCH 6/7] vhost: validate index in async API Maxime Coquelin
@ 2020-10-21 11:31   ` Xia, Chenbo
  0 siblings, 0 replies; 14+ messages in thread
From: Xia, Chenbo @ 2020-10-21 11:31 UTC (permalink / raw)
  To: Maxime Coquelin, dev, amorenoz; +Cc: stable

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Tuesday, October 20, 2020 1:34 AM
> To: dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; amorenoz@redhat.com
> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>; stable@dpdk.org
> Subject: [PATCH 6/7] vhost: validate index in async API
> 
> This patch validates the queue index parameter, in order
> to ensure no out-of-bound accesses happen.
> 
> Fixes: 9eed6bfd2efb ("vhost: allow to enable or disable features")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  lib/librte_vhost/vhost.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
> index f78bdfcc94..e92ff618ac 100644
> --- a/lib/librte_vhost/vhost.c
> +++ b/lib/librte_vhost/vhost.c
> @@ -1577,6 +1577,9 @@ int rte_vhost_async_channel_register(int vid,
> uint16_t queue_id,
> 
>  	f.intval = features;
> 
> +	if (queue_id >= VHOST_MAX_VRING)
> +		return -1;
> +
>  	vq = dev->virtqueue[queue_id];
> 
>  	if (unlikely(vq == NULL || !dev->async_copy))
> @@ -1658,6 +1661,9 @@ int rte_vhost_async_channel_unregister(int vid,
> uint16_t queue_id)
>  	if (dev == NULL)
>  		return ret;
> 
> +	if (queue_id >= VHOST_MAX_VRING)
> +		return ret;
> +
>  	vq = dev->virtqueue[queue_id];
> 
>  	if (vq == NULL)
> --
> 2.26.2

Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>

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

* Re: [dpdk-stable] [PATCH 1/7] vhost: fix virtqueues metadata allocation
  2020-10-21 11:10   ` Xia, Chenbo
@ 2020-10-21 12:06     ` Maxime Coquelin
  2020-10-22 11:00       ` Xia, Chenbo
  0 siblings, 1 reply; 14+ messages in thread
From: Maxime Coquelin @ 2020-10-21 12:06 UTC (permalink / raw)
  To: Xia, Chenbo, dev, amorenoz; +Cc: stable

Hi Chenbon

On 10/21/20 1:10 PM, Xia, Chenbo wrote:
> Hi Maxime,
> 
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: Tuesday, October 20, 2020 1:34 AM
>> To: dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; amorenoz@redhat.com
>> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>; stable@dpdk.org
>> Subject: [PATCH 1/7] vhost: fix virtqueues metadata allocation
>>
>> The Vhost-user backend implementation assumes there will be
>> no holes in the device's array of virtqueues metadata
>> pointers.
>>
>> It can happen though, and would cause segmentation faults,
>> memory leaks or undefined behaviour.
> 
> Could I ask when will this happen? 
> 
> When QEMU does not configure all virtqueues? I'm not very sure.
> Could you point that out for me?

It has been reported by our QE when doing reconnect with multiqueue with
vIOMMU enabled:
https://bugzilla.redhat.com/show_bug.cgi?id=1880299

Regards,
Maxime

> Thanks!
> Chenbo
> 
>>
>> This patch keep the assumption that there is no holes in this
>> array, and allocate all uninitialized virtqueues metadata up
>> to requested index.
>>
>> Fixes: 160cbc815b41 ("vhost: remove a hack on queue allocation")
>> Cc: stable@dpdk.org
>>
>> Suggested-by: Adrian Moreno <amorenoz@redhat.com>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> ---
>>  lib/librte_vhost/vhost.c | 33 ++++++++++++++++++++-------------
>>  1 file changed, 20 insertions(+), 13 deletions(-)
>>
>> diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
>> index 6068c38ec6..0c9ba3b3af 100644
>> --- a/lib/librte_vhost/vhost.c
>> +++ b/lib/librte_vhost/vhost.c
>> @@ -579,22 +579,29 @@ int
>>  alloc_vring_queue(struct virtio_net *dev, uint32_t vring_idx)
>>  {
>>  	struct vhost_virtqueue *vq;
>> +	uint32_t i;
>>
>> -	vq = rte_malloc(NULL, sizeof(struct vhost_virtqueue), 0);
>> -	if (vq == NULL) {
>> -		VHOST_LOG_CONFIG(ERR,
>> -			"Failed to allocate memory for vring:%u.\n", vring_idx);
>> -		return -1;
>> -	}
>> +	/* Also allocate holes, if any, up to requested vring index. */
>> +	for (i = 0; i <= vring_idx; i++) {
>> +		if (dev->virtqueue[i])
>> +			continue;
>>
>> -	dev->virtqueue[vring_idx] = vq;
>> -	init_vring_queue(dev, vring_idx);
>> -	rte_spinlock_init(&vq->access_lock);
>> -	vq->avail_wrap_counter = 1;
>> -	vq->used_wrap_counter = 1;
>> -	vq->signalled_used_valid = false;
>> +		vq = rte_malloc(NULL, sizeof(struct vhost_virtqueue), 0);
>> +		if (vq == NULL) {
>> +			VHOST_LOG_CONFIG(ERR,
>> +				"Failed to allocate memory for vring:%u.\n", i);
>> +			return -1;
>> +		}
>> +
>> +		dev->virtqueue[i] = vq;
>> +		init_vring_queue(dev, vring_idx);
>> +		rte_spinlock_init(&vq->access_lock);
>> +		vq->avail_wrap_counter = 1;
>> +		vq->used_wrap_counter = 1;
>> +		vq->signalled_used_valid = false;
>> +	}
>>
>> -	dev->nr_vring += 1;
>> +	dev->nr_vring = RTE_MAX(dev->nr_vring, vring_idx + 1);
>>
>>  	return 0;
>>  }
>> --
>> 2.26.2
> 


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

* Re: [dpdk-stable] [PATCH 1/7] vhost: fix virtqueues metadata allocation
  2020-10-21 12:06     ` Maxime Coquelin
@ 2020-10-22 11:00       ` Xia, Chenbo
  0 siblings, 0 replies; 14+ messages in thread
From: Xia, Chenbo @ 2020-10-22 11:00 UTC (permalink / raw)
  To: Maxime Coquelin, dev, amorenoz; +Cc: stable

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Wednesday, October 21, 2020 8:07 PM
> To: Xia, Chenbo <chenbo.xia@intel.com>; dev@dpdk.org; amorenoz@redhat.com
> Cc: stable@dpdk.org
> Subject: Re: [PATCH 1/7] vhost: fix virtqueues metadata allocation
> 
> Hi Chenbon
> 
> On 10/21/20 1:10 PM, Xia, Chenbo wrote:
> > Hi Maxime,
> >
> >> -----Original Message-----
> >> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >> Sent: Tuesday, October 20, 2020 1:34 AM
> >> To: dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>;
> amorenoz@redhat.com
> >> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>; stable@dpdk.org
> >> Subject: [PATCH 1/7] vhost: fix virtqueues metadata allocation
> >>
> >> The Vhost-user backend implementation assumes there will be
> >> no holes in the device's array of virtqueues metadata
> >> pointers.
> >>
> >> It can happen though, and would cause segmentation faults,
> >> memory leaks or undefined behaviour.
> >
> > Could I ask when will this happen?
> >
> > When QEMU does not configure all virtqueues? I'm not very sure.
> > Could you point that out for me?
> 
> It has been reported by our QE when doing reconnect with multiqueue with
> vIOMMU enabled:
> https://bugzilla.redhat.com/show_bug.cgi?id=1880299
> 
> Regards,
> Maxime
> 
> > Thanks!
> > Chenbo
> >
> >>
> >> This patch keep the assumption that there is no holes in this
> >> array, and allocate all uninitialized virtqueues metadata up
> >> to requested index.
> >>
> >> Fixes: 160cbc815b41 ("vhost: remove a hack on queue allocation")
> >> Cc: stable@dpdk.org
> >>
> >> Suggested-by: Adrian Moreno <amorenoz@redhat.com>
> >> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> >> ---
> >>  lib/librte_vhost/vhost.c | 33 ++++++++++++++++++++-------------
> >>  1 file changed, 20 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
> >> index 6068c38ec6..0c9ba3b3af 100644
> >> --- a/lib/librte_vhost/vhost.c
> >> +++ b/lib/librte_vhost/vhost.c
> >> @@ -579,22 +579,29 @@ int
> >>  alloc_vring_queue(struct virtio_net *dev, uint32_t vring_idx)
> >>  {
> >>  	struct vhost_virtqueue *vq;
> >> +	uint32_t i;
> >>
> >> -	vq = rte_malloc(NULL, sizeof(struct vhost_virtqueue), 0);
> >> -	if (vq == NULL) {
> >> -		VHOST_LOG_CONFIG(ERR,
> >> -			"Failed to allocate memory for vring:%u.\n", vring_idx);
> >> -		return -1;
> >> -	}
> >> +	/* Also allocate holes, if any, up to requested vring index. */
> >> +	for (i = 0; i <= vring_idx; i++) {
> >> +		if (dev->virtqueue[i])
> >> +			continue;
> >>
> >> -	dev->virtqueue[vring_idx] = vq;
> >> -	init_vring_queue(dev, vring_idx);
> >> -	rte_spinlock_init(&vq->access_lock);
> >> -	vq->avail_wrap_counter = 1;
> >> -	vq->used_wrap_counter = 1;
> >> -	vq->signalled_used_valid = false;
> >> +		vq = rte_malloc(NULL, sizeof(struct vhost_virtqueue), 0);
> >> +		if (vq == NULL) {
> >> +			VHOST_LOG_CONFIG(ERR,
> >> +				"Failed to allocate memory for vring:%u.\n", i);
> >> +			return -1;
> >> +		}
> >> +
> >> +		dev->virtqueue[i] = vq;
> >> +		init_vring_queue(dev, vring_idx);
> >> +		rte_spinlock_init(&vq->access_lock);
> >> +		vq->avail_wrap_counter = 1;
> >> +		vq->used_wrap_counter = 1;
> >> +		vq->signalled_used_valid = false;
> >> +	}
> >>
> >> -	dev->nr_vring += 1;
> >> +	dev->nr_vring = RTE_MAX(dev->nr_vring, vring_idx + 1);
> >>
> >>  	return 0;
> >>  }
> >> --
> >> 2.26.2
> >

Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>

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

end of thread, other threads:[~2020-10-22 11:00 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20201019173415.582407-1-maxime.coquelin@redhat.com>
2020-10-19 17:34 ` [dpdk-stable] [PATCH 1/7] vhost: fix virtqueues metadata allocation Maxime Coquelin
2020-10-21 11:10   ` Xia, Chenbo
2020-10-21 12:06     ` Maxime Coquelin
2020-10-22 11:00       ` Xia, Chenbo
2020-10-19 17:34 ` [dpdk-stable] [PATCH 2/7] vhost: validate index in available entries API Maxime Coquelin
2020-10-21 11:28   ` Xia, Chenbo
2020-10-19 17:34 ` [dpdk-stable] [PATCH 3/7] vhost: validate index in guest notification API Maxime Coquelin
2020-10-21 11:30   ` Xia, Chenbo
2020-10-19 17:34 ` [dpdk-stable] [PATCH 4/7] vhost: validate index in live-migration API Maxime Coquelin
2020-10-21 11:30   ` Xia, Chenbo
2020-10-19 17:34 ` [dpdk-stable] [PATCH 5/7] vhost: validate index in inflight API Maxime Coquelin
2020-10-21 11:30   ` Xia, Chenbo
2020-10-19 17:34 ` [dpdk-stable] [PATCH 6/7] vhost: validate index in async API Maxime Coquelin
2020-10-21 11:31   ` Xia, Chenbo

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).