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

[PATCH 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: 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 c03bb9c6eb..e9c775fa26 100644
--- a/lib/vhost/vhost.c
+++ b/lib/vhost/vhost.c
@@ -1328,6 +1328,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)
@@ -1342,14 +1343,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
@@ -1357,6 +1364,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)
@@ -1372,14 +1380,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 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 e9c775fa26..83b71ffb23 100644
--- a/lib/vhost/vhost.c
+++ b/lib/vhost/vhost.c
@@ -1416,7 +1416,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;
@@ -1508,9 +1511,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;
@@ -1601,7 +1610,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 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 83b71ffb23..f07674334d 100644
--- a/lib/vhost/vhost.c
+++ b/lib/vhost/vhost.c
@@ -1540,6 +1540,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;
+
 	if (dev->backend_ops->inject_irq(dev, vq)) {
 		if (dev->flags & VIRTIO_DEV_STATS_ENABLED)
 			__atomic_fetch_add(&vq->stats.guest_notifications_error,
@@ -1552,6 +1555,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 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 f07674334d..7f5d25255a 100644
--- a/lib/vhost/vhost.c
+++ b/lib/vhost/vhost.c
@@ -1853,7 +1853,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;
@@ -1905,6 +1913,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) {
@@ -1916,6 +1929,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 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 7f5d25255a..51383410bf 100644
--- a/lib/vhost/vhost.c
+++ b/lib/vhost/vhost.c
@@ -2069,9 +2069,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 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 51383410bf..28eedf11d9 100644
--- a/lib/vhost/vhost.c
+++ b/lib/vhost/vhost.c
@@ -2117,6 +2117,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;
@@ -2127,6 +2128,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;
@@ -2146,7 +2154,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 7/7] vhost: fix checking virtqueue access ins 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 28eedf11d9..7b0bdda520 100644
--- a/lib/vhost/vhost.c
+++ b/lib/vhost/vhost.c
@@ -2195,6 +2195,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;
@@ -2211,6 +2212,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
@@ -2220,15 +2227,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;
@@ -2242,14 +2252,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 1/7] vhost: fix missing vring call check on virtqueue access

[David Marchand]

On Mon, Sep 25, 2023 at 6:36 PM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
>
> 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.

Even if adding the lock was not enough, I would flag 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>


-- 
David Marchand

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

[Maxime Coquelin]

On 10/19/23 09:24, David Marchand wrote:
> On Mon, Sep 25, 2023 at 6:36 PM Maxime Coquelin
> <maxime.coquelin@redhat.com> wrote:
>>
>> 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.
> 
> Even if adding the lock was not enough, I would flag Fixes:
> 6c299bb7322f ("vhost: introduce vring call API")

Makes sense, adding it to v2.

Thanks,
Maxime

>>
>> 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>
> 
>