[PATCH v2 0/7] vhost: ensure virtqueue access status is checked

[PATCH v2 0/7] vhost: ensure virtqueue access status is checked

[Maxime Coquelin]

Li Feng initially reported segmentation fault in rte_vhost_vring_call()
because of not checking the virtqueue metadata can be accessed.

This should be achieved by checking the access_ok status field of
the virtqueue.

This series also takes the opportunity to fix the other APIs.
This is split in multiple patches to ease LTS maintainers backports,
but could be squashed if preferred.

Changes in v2:
--------------
- Rebased to apply on -rc1 (David)
- Add Fixes tag in patch 1 (David)
- Fix various typos in commit logs (David)

Maxime Coquelin (7):
  vhost: fix missing vring call check on virtqueue access
  vhost: fix missing check on virtqueue access
  vhost: fix checking virtqueue access when notifying guest
  vhost: fix check on virtqueue access in async registration
  vhost: fix check on virtqueue access in in-flight getter
  vhost: fix missing lock protection in power monitor API
  vhost: fix checking virtqueue access in stats API

 lib/vhost/vhost.c | 92 +++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 85 insertions(+), 7 deletions(-)

-- 
2.41.0

[PATCH v2 1/7] vhost: fix missing vring call check on virtqueue access

[Maxime Coquelin]

Acquiring the access lock is not enough to ensure
virtqueue's metadata such as vring pointers are valid.

The access status must also be checked.

Fixes: 6c299bb7322f ("vhost: introduce vring call API")
Fixes: c5736998305d ("vhost: fix missing virtqueue lock protection")
Fixes: 830f7e790732 ("vhost: add non-blocking API for posting interrupt")
Cc: stable@dpdk.org

Reported-by: Li Feng <fengli@smartx.com>
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/vhost/vhost.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
index 7fde412ef3..0d2bc1a48b 100644
--- a/lib/vhost/vhost.c
+++ b/lib/vhost/vhost.c
@@ -1330,6 +1330,7 @@ rte_vhost_vring_call(int vid, uint16_t vring_idx)
 {
 	struct virtio_net *dev;
 	struct vhost_virtqueue *vq;
+	int ret = 0;
 
 	dev = get_device(vid);
 	if (!dev)
@@ -1344,14 +1345,20 @@ rte_vhost_vring_call(int vid, uint16_t vring_idx)
 
 	rte_rwlock_read_lock(&vq->access_lock);
 
+	if (unlikely(!vq->access_ok)) {
+		ret = -1;
+		goto out_unlock;
+	}
+
 	if (vq_is_packed(dev))
 		vhost_vring_call_packed(dev, vq);
 	else
 		vhost_vring_call_split(dev, vq);
 
+out_unlock:
 	rte_rwlock_read_unlock(&vq->access_lock);
 
-	return 0;
+	return ret;
 }
 
 int
@@ -1359,6 +1366,7 @@ rte_vhost_vring_call_nonblock(int vid, uint16_t vring_idx)
 {
 	struct virtio_net *dev;
 	struct vhost_virtqueue *vq;
+	int ret = 0;
 
 	dev = get_device(vid);
 	if (!dev)
@@ -1374,14 +1382,20 @@ rte_vhost_vring_call_nonblock(int vid, uint16_t vring_idx)
 	if (rte_rwlock_read_trylock(&vq->access_lock))
 		return -EAGAIN;
 
+	if (unlikely(!vq->access_ok)) {
+		ret = -1;
+		goto out_unlock;
+	}
+
 	if (vq_is_packed(dev))
 		vhost_vring_call_packed(dev, vq);
 	else
 		vhost_vring_call_split(dev, vq);
 
+out_unlock:
 	rte_rwlock_read_unlock(&vq->access_lock);
 
-	return 0;
+	return ret;
 }
 
 uint16_t
-- 
2.41.0

[PATCH v2 2/7] vhost: fix missing check on virtqueue access

[Maxime Coquelin]

Acquiring the access lock is not enough to ensure
virtqueue's metadata such as vring pointers are valid.

The access status must also be checked.

Fixes: 4e0de8dac853 ("vhost: protect vring access done by application")
Cc: stable@dpdk.org

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

diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
index 0d2bc1a48b..446bca1574 100644
--- a/lib/vhost/vhost.c
+++ b/lib/vhost/vhost.c
@@ -1418,7 +1418,10 @@ rte_vhost_avail_entries(int vid, uint16_t queue_id)
 
 	rte_rwlock_write_lock(&vq->access_lock);
 
-	if (unlikely(!vq->enabled || vq->avail == NULL))
+	if (unlikely(!vq->access_ok))
+		goto out;
+
+	if (unlikely(!vq->enabled))
 		goto out;
 
 	ret = *(volatile uint16_t *)&vq->avail->idx - vq->last_used_idx;
@@ -1510,9 +1513,15 @@ rte_vhost_enable_guest_notification(int vid, uint16_t queue_id, int enable)
 
 	rte_rwlock_write_lock(&vq->access_lock);
 
+	if (unlikely(!vq->access_ok)) {
+		ret = -1;
+		goto out_unlock;
+	}
+
 	vq->notif_enable = enable;
 	ret = vhost_enable_guest_notification(dev, vq, enable);
 
+out_unlock:
 	rte_rwlock_write_unlock(&vq->access_lock);
 
 	return ret;
@@ -1605,7 +1614,10 @@ rte_vhost_rx_queue_count(int vid, uint16_t qid)
 
 	rte_rwlock_write_lock(&vq->access_lock);
 
-	if (unlikely(!vq->enabled || vq->avail == NULL))
+	if (unlikely(!vq->access_ok))
+		goto out;
+
+	if (unlikely(!vq->enabled))
 		goto out;
 
 	ret = *((volatile uint16_t *)&vq->avail->idx) - vq->last_avail_idx;
-- 
2.41.0

[PATCH v2 3/7] vhost: fix checking virtqueue access when notifying guest

[Maxime Coquelin]

Acquiring the access lock is not enough to ensure
virtqueue's metadata such as vring pointers are valid.

The access status must also be checked.

Fixes: d761d455a0e4 ("vhost: add operation to offload the interrupt kick")
Cc: stable@dpdk.org

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

diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
index 446bca1574..5428ff4a25 100644
--- a/lib/vhost/vhost.c
+++ b/lib/vhost/vhost.c
@@ -1542,6 +1542,9 @@ rte_vhost_notify_guest(int vid, uint16_t queue_id)
 
 	rte_rwlock_read_lock(&vq->access_lock);
 
+	if (unlikely(!vq->access_ok))
+		goto out_unlock;
+
 	__atomic_store_n(&vq->irq_pending, false, __ATOMIC_RELEASE);
 
 	if (dev->backend_ops->inject_irq(dev, vq)) {
@@ -1556,6 +1559,7 @@ rte_vhost_notify_guest(int vid, uint16_t queue_id)
 			dev->notify_ops->guest_notified(dev->vid);
 	}
 
+out_unlock:
 	rte_rwlock_read_unlock(&vq->access_lock);
 }
 
-- 
2.41.0

[PATCH v2 4/7] vhost: fix check on virtqueue access in async registration

[Maxime Coquelin]

Acquiring the access lock is not enough to ensure
virtqueue's metadata such as vring pointers are valid.

The access status must also be checked.

Fixes: 78639d54563a ("vhost: introduce async enqueue registration API")
Cc: stable@dpdk.org

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

diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
index 5428ff4a25..ccd3c0e865 100644
--- a/lib/vhost/vhost.c
+++ b/lib/vhost/vhost.c
@@ -1857,7 +1857,15 @@ rte_vhost_async_channel_register(int vid, uint16_t queue_id)
 		return -1;
 
 	rte_rwlock_write_lock(&vq->access_lock);
+
+	if (unlikely(!vq->access_ok)) {
+		ret = -1;
+		goto out_unlock;
+	}
+
 	ret = async_channel_register(dev, vq);
+
+out_unlock:
 	rte_rwlock_write_unlock(&vq->access_lock);
 
 	return ret;
@@ -1909,6 +1917,11 @@ rte_vhost_async_channel_unregister(int vid, uint16_t queue_id)
 		return ret;
 	}
 
+	if (unlikely(!vq->access_ok)) {
+		ret = -1;
+		goto out_unlock;
+	}
+
 	if (!vq->async) {
 		ret = 0;
 	} else if (vq->async->pkts_inflight_n) {
@@ -1920,6 +1933,7 @@ rte_vhost_async_channel_unregister(int vid, uint16_t queue_id)
 		ret = 0;
 	}
 
+out_unlock:
 	rte_rwlock_write_unlock(&vq->access_lock);
 
 	return ret;
-- 
2.41.0

[PATCH v2 5/7] vhost: fix check on virtqueue access in in-flight getter

[Maxime Coquelin]

Acquiring the access lock is not enough to ensure
virtqueue's metadata such as vring pointers are valid.

The access status must also be checked.

Fixes: 0c0935c5f794 ("vhost: allow to check in-flight packets for async vhost")
Cc: stable@dpdk.org

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

diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
index ccd3c0e865..a243f88398 100644
--- a/lib/vhost/vhost.c
+++ b/lib/vhost/vhost.c
@@ -2073,9 +2073,15 @@ rte_vhost_async_get_inflight(int vid, uint16_t queue_id)
 		return ret;
 	}
 
+	if (unlikely(!vq->access_ok)) {
+		ret = -1;
+		goto out_unlock;
+	}
+
 	if (vq->async)
 		ret = vq->async->pkts_inflight_n;
 
+out_unlock:
 	rte_rwlock_write_unlock(&vq->access_lock);
 
 	return ret;
-- 
2.41.0

[PATCH v2 6/7] vhost: fix missing lock protection in power monitor API

[Maxime Coquelin]

The power monitor get API is missing both access lock
protection and access status check.

Fixes: 34fd4373ce76 ("vhost: add power monitor API")
Cc: stable@dpdk.org

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

diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
index a243f88398..d8d74623d4 100644
--- a/lib/vhost/vhost.c
+++ b/lib/vhost/vhost.c
@@ -2121,6 +2121,7 @@ rte_vhost_get_monitor_addr(int vid, uint16_t queue_id,
 {
 	struct virtio_net *dev = get_device(vid);
 	struct vhost_virtqueue *vq;
+	int ret = 0;
 
 	if (dev == NULL)
 		return -1;
@@ -2131,6 +2132,13 @@ rte_vhost_get_monitor_addr(int vid, uint16_t queue_id,
 	if (vq == NULL)
 		return -1;
 
+	rte_rwlock_read_lock(&vq->access_lock);
+
+	if (unlikely(!vq->access_ok)) {
+		ret = -1;
+		goto out_unlock;
+	}
+
 	if (vq_is_packed(dev)) {
 		struct vring_packed_desc *desc;
 		desc = vq->desc_packed;
@@ -2150,7 +2158,10 @@ rte_vhost_get_monitor_addr(int vid, uint16_t queue_id,
 		pmc->match = 0;
 	}
 
-	return 0;
+out_unlock:
+	rte_rwlock_read_unlock(&vq->access_lock);
+
+	return ret;
 }
 
 
-- 
2.41.0

[PATCH v2 7/7] vhost: fix checking virtqueue access in stats API

[Maxime Coquelin]

Acquiring the access lock is not enough to ensure
virtqueue's metadata such as vring pointers are valid.

The access status must also be checked.

Fixes: be75dc99ea1f ("vhost: support per-virtqueue statistics")
Cc: stable@dpdk.org

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

diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
index d8d74623d4..b03e3b391d 100644
--- a/lib/vhost/vhost.c
+++ b/lib/vhost/vhost.c
@@ -2199,6 +2199,7 @@ rte_vhost_vring_stats_get(int vid, uint16_t queue_id,
 	struct virtio_net *dev = get_device(vid);
 	struct vhost_virtqueue *vq;
 	unsigned int i;
+	int ret = VHOST_NB_VQ_STATS;
 
 	if (dev == NULL)
 		return -1;
@@ -2215,6 +2216,12 @@ rte_vhost_vring_stats_get(int vid, uint16_t queue_id,
 	vq = dev->virtqueue[queue_id];
 
 	rte_rwlock_write_lock(&vq->access_lock);
+
+	if (unlikely(!vq->access_ok)) {
+		ret = -1;
+		goto out_unlock;
+	}
+
 	for (i = 0; i < VHOST_NB_VQ_STATS; i++) {
 		/*
 		 * No need to the read atomic counters as such, due to the
@@ -2224,15 +2231,18 @@ rte_vhost_vring_stats_get(int vid, uint16_t queue_id,
 			*(uint64_t *)(((char *)vq) + vhost_vq_stat_strings[i].offset);
 		stats[i].id = i;
 	}
+
+out_unlock:
 	rte_rwlock_write_unlock(&vq->access_lock);
 
-	return VHOST_NB_VQ_STATS;
+	return ret;
 }
 
 int rte_vhost_vring_stats_reset(int vid, uint16_t queue_id)
 {
 	struct virtio_net *dev = get_device(vid);
 	struct vhost_virtqueue *vq;
+	int ret = 0;
 
 	if (dev == NULL)
 		return -1;
@@ -2246,14 +2256,21 @@ int rte_vhost_vring_stats_reset(int vid, uint16_t queue_id)
 	vq = dev->virtqueue[queue_id];
 
 	rte_rwlock_write_lock(&vq->access_lock);
+
+	if (unlikely(!vq->access_ok)) {
+		ret = -1;
+		goto out_unlock;
+	}
 	/*
 	 * No need to the reset atomic counters as such, due to the
 	 * above write access_lock preventing them to be updated.
 	 */
 	memset(&vq->stats, 0, sizeof(vq->stats));
+
+out_unlock:
 	rte_rwlock_write_unlock(&vq->access_lock);
 
-	return 0;
+	return ret;
 }
 
 int
-- 
2.41.0

Re: [PATCH v2 0/7] vhost: ensure virtqueue access status is checked

[David Marchand]

On Fri, Oct 20, 2023 at 10:48 AM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
>
> Li Feng initially reported segmentation fault in rte_vhost_vring_call()
> because of not checking the virtqueue metadata can be accessed.
>
> This should be achieved by checking the access_ok status field of
> the virtqueue.
>
> This series also takes the opportunity to fix the other APIs.
> This is split in multiple patches to ease LTS maintainers backports,
> but could be squashed if preferred.

As a followup on this topic, I took some time to annotate this code
around access_ok/access_lock.
For those interested:
https://patchwork.dpdk.org/project/dpdk/list/?series=29950&state=%2A&archive=both


-- 
David Marchand

Re: [PATCH v2 0/7] vhost: ensure virtqueue access status is checked

[David Marchand]

On Fri, Oct 20, 2023 at 10:48 AM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
>
> Li Feng initially reported segmentation fault in rte_vhost_vring_call()
> because of not checking the virtqueue metadata can be accessed.
>
> This should be achieved by checking the access_ok status field of
> the virtqueue.
>
> This series also takes the opportunity to fix the other APIs.
> This is split in multiple patches to ease LTS maintainers backports,
> but could be squashed if preferred.
>
> Changes in v2:
> --------------
> - Rebased to apply on -rc1 (David)
> - Add Fixes tag in patch 1 (David)
> - Fix various typos in commit logs (David)
>
> Maxime Coquelin (7):
>   vhost: fix missing vring call check on virtqueue access
>   vhost: fix missing check on virtqueue access
>   vhost: fix checking virtqueue access when notifying guest
>   vhost: fix check on virtqueue access in async registration
>   vhost: fix check on virtqueue access in in-flight getter
>   vhost: fix missing lock protection in power monitor API
>   vhost: fix checking virtqueue access in stats API
>
>  lib/vhost/vhost.c | 92 +++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 85 insertions(+), 7 deletions(-)

For the series,
Acked-by: David Marchand <david.marchand@redhat.com>


-- 
David Marchand

Re: [PATCH v2 0/7] vhost: ensure virtqueue access status is checked

[Maxime Coquelin]

On 10/20/23 10:47, Maxime Coquelin wrote:
> Li Feng initially reported segmentation fault in rte_vhost_vring_call()
> because of not checking the virtqueue metadata can be accessed.
> 
> This should be achieved by checking the access_ok status field of
> the virtqueue.
> 
> This series also takes the opportunity to fix the other APIs.
> This is split in multiple patches to ease LTS maintainers backports,
> but could be squashed if preferred.
> 
> Changes in v2:
> --------------
> - Rebased to apply on -rc1 (David)
> - Add Fixes tag in patch 1 (David)
> - Fix various typos in commit logs (David)
> 
> Maxime Coquelin (7):
>    vhost: fix missing vring call check on virtqueue access
>    vhost: fix missing check on virtqueue access
>    vhost: fix checking virtqueue access when notifying guest
>    vhost: fix check on virtqueue access in async registration
>    vhost: fix check on virtqueue access in in-flight getter
>    vhost: fix missing lock protection in power monitor API
>    vhost: fix checking virtqueue access in stats API
> 
>   lib/vhost/vhost.c | 92 +++++++++++++++++++++++++++++++++++++++++++----
>   1 file changed, 85 insertions(+), 7 deletions(-)
> 

Applied to next-virtio/for-next-net

Thanks,
Maxime