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

[PATCH 0/7] vhost: ensure vitqueue 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.

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 ins stats API

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

-- 
2.41.0

[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

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

[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 0/7] vhost: ensure vitqueue access status is checked

[David Marchand]

Hello Maxime,

On Mon, Sep 25, 2023 at 6:36 PM 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.
>
> 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

Nit: fix*

>   vhost: fix missing lock protection in power monitor API
>   vhost: fix checking virtqueue access ins stats API

This series needs some rebasing, and I had a small comment on a first
patch Fixes: tag, but otherwise it lgtm.


-- 
David Marchand