diff mbox series

[v3] vhost: avoid potential null pointer access

Message ID 20230912074217.2480397-1-fengli@smartx.com (mailing list archive)
State Superseded, archived
Delegated to: Maxime Coquelin
Headers
Series [v3] vhost: avoid potential null pointer access |

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/github-robot: build success github build: passed
ci/intel-Functional success Functional PASS
ci/loongarch-compilation success Compilation OK
ci/loongarch-unit-testing success Unit Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-compile-amd64-testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-unit-amd64-testing success Testing PASS
ci/iol-unit-arm64-testing success Testing PASS
ci/iol-compile-arm64-testing success Testing PASS

Commit Message

Li Feng Sept. 12, 2023, 7:42 a.m. UTC 
  If the user calls rte_vhost_vring_call() on a ring that has been
invalidated, we will encounter SEGV.

We should check the pointer firstly before accessing it.

Signed-off-by: Li Feng <fengli@smartx.com>
---
v2 -> v3:
- Also fix the rte_vhost_vring_call_nonblock.

v1 -> v2:
- Fix rebase error.



 lib/vhost/vhost.c | 14 ++++++++------
 lib/vhost/vhost.h | 12 ++++++++++--
 2 files changed, 18 insertions(+), 8 deletions(-)

--
2.41.0

Comments

Maxime Coquelin Sept. 25, 2023, 8:15 a.m. UTC | #1 
On 9/12/23 09:42, Li Feng wrote:
> If the user calls rte_vhost_vring_call() on a ring that has been
> invalidated, we will encounter SEGV.
> 
> We should check the pointer firstly before accessing it.
> 
> Signed-off-by: Li Feng <fengli@smartx.com>
> ---
> v2 -> v3:
> - Also fix the rte_vhost_vring_call_nonblock.
> 
> v1 -> v2:
> - Fix rebase error.
> 
> 
> 
>   lib/vhost/vhost.c | 14 ++++++++------
>   lib/vhost/vhost.h | 12 ++++++++++--
>   2 files changed, 18 insertions(+), 8 deletions(-)


Thanks for posting the fix, the segmentation fault may indeed happen 
when injecting IRQ from the app directly using the Vhost API. It cannot
happen when vhost_vring_call() is calle directly from
rte_enqueue_burst/rte_dequeue_burst though.

so I think below patch would be better:

diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
index eb6309b681..733e0ab289 100644
--- a/lib/vhost/vhost.c
+++ b/lib/vhost/vhost.c
@@ -1341,6 +1341,9 @@ rte_vhost_vring_call(int vid, uint16_t vring_idx)

         rte_rwlock_read_lock(&vq->access_lock);

+       if (unlikely(!vq->access_ok))
+               return -1;
+
         if (vq_is_packed(dev))
                 vhost_vring_call_packed(dev, vq);
         else
@@ -1371,6 +1374,9 @@ 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))
+               return -1;
+
         if (vq_is_packed(dev))
                 vhost_vring_call_packed(dev, vq);
         else


Do you confirm that fixes your issue?

Thanks,
Maxime
Maxime Coquelin Sept. 25, 2023, 8:26 a.m. UTC | #2 
On 9/25/23 10:15, Maxime Coquelin wrote:
> 
> 
> On 9/12/23 09:42, Li Feng wrote:
>> If the user calls rte_vhost_vring_call() on a ring that has been
>> invalidated, we will encounter SEGV.
>>
>> We should check the pointer firstly before accessing it.
>>
>> Signed-off-by: Li Feng <fengli@smartx.com>
>> ---
>> v2 -> v3:
>> - Also fix the rte_vhost_vring_call_nonblock.
>>
>> v1 -> v2:
>> - Fix rebase error.
>>
>>
>>
>>   lib/vhost/vhost.c | 14 ++++++++------
>>   lib/vhost/vhost.h | 12 ++++++++++--
>>   2 files changed, 18 insertions(+), 8 deletions(-)
> 
> 
> Thanks for posting the fix, the segmentation fault may indeed happen 
> when injecting IRQ from the app directly using the Vhost API. It cannot
> happen when vhost_vring_call() is calle directly from
> rte_enqueue_burst/rte_dequeue_burst though.
> 
> so I think below patch would be better:
> 
> diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
> index eb6309b681..733e0ab289 100644
> --- a/lib/vhost/vhost.c
> +++ b/lib/vhost/vhost.c
> @@ -1341,6 +1341,9 @@ rte_vhost_vring_call(int vid, uint16_t vring_idx)
> 
>          rte_rwlock_read_lock(&vq->access_lock);
> 
> +       if (unlikely(!vq->access_ok))
> +               return -1;
> +
>          if (vq_is_packed(dev))
>                  vhost_vring_call_packed(dev, vq);
>          else
> @@ -1371,6 +1374,9 @@ 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))
> +               return -1;
> +
>          if (vq_is_packed(dev))
>                  vhost_vring_call_packed(dev, vq);
>          else
> 
> 
> Do you confirm that fixes your issue?

As pointed out by David off-list, there are other places where we need
to add this check. I will prepare a patch fixing them all.

Thanks,
Maxime

> Thanks,
> Maxime
Morten Brørup Sept. 25, 2023, 10:37 a.m. UTC | #3 
> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
> Sent: Monday, 25 September 2023 10.15
> 
> On 9/12/23 09:42, Li Feng wrote:
> > If the user calls rte_vhost_vring_call() on a ring that has been
> > invalidated, we will encounter SEGV.
> >
> > We should check the pointer firstly before accessing it.
> >
> > Signed-off-by: Li Feng <fengli@smartx.com>
> > ---
> > v2 -> v3:
> > - Also fix the rte_vhost_vring_call_nonblock.
> >
> > v1 -> v2:
> > - Fix rebase error.
> >
> >
> >
> >   lib/vhost/vhost.c | 14 ++++++++------
> >   lib/vhost/vhost.h | 12 ++++++++++--
> >   2 files changed, 18 insertions(+), 8 deletions(-)
> 
> 
> Thanks for posting the fix, the segmentation fault may indeed happen
> when injecting IRQ from the app directly using the Vhost API. It cannot
> happen when vhost_vring_call() is calle directly from
> rte_enqueue_burst/rte_dequeue_burst though.
> 
> so I think below patch would be better:
> 
> diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
> index eb6309b681..733e0ab289 100644
> --- a/lib/vhost/vhost.c
> +++ b/lib/vhost/vhost.c
> @@ -1341,6 +1341,9 @@ rte_vhost_vring_call(int vid, uint16_t vring_idx)
> 
>          rte_rwlock_read_lock(&vq->access_lock);
> 
> +       if (unlikely(!vq->access_ok))
> +               return -1;

Don't you need to release the lock before returning here?

> +
>          if (vq_is_packed(dev))
>                  vhost_vring_call_packed(dev, vq);
>          else
> @@ -1371,6 +1374,9 @@ 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))
> +               return -1;

Don't you need to release the lock before returning here?

> +
>          if (vq_is_packed(dev))
>                  vhost_vring_call_packed(dev, vq);
>          else
> 
> 
> Do you confirm that fixes your issue?
> 
> Thanks,
> Maxime
Maxime Coquelin Sept. 25, 2023, 10:59 a.m. UTC | #4 
On 9/25/23 12:37, Morten Brørup wrote:
>> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
>> Sent: Monday, 25 September 2023 10.15
>>
>> On 9/12/23 09:42, Li Feng wrote:
>>> If the user calls rte_vhost_vring_call() on a ring that has been
>>> invalidated, we will encounter SEGV.
>>>
>>> We should check the pointer firstly before accessing it.
>>>
>>> Signed-off-by: Li Feng <fengli@smartx.com>
>>> ---
>>> v2 -> v3:
>>> - Also fix the rte_vhost_vring_call_nonblock.
>>>
>>> v1 -> v2:
>>> - Fix rebase error.
>>>
>>>
>>>
>>>    lib/vhost/vhost.c | 14 ++++++++------
>>>    lib/vhost/vhost.h | 12 ++++++++++--
>>>    2 files changed, 18 insertions(+), 8 deletions(-)
>>
>>
>> Thanks for posting the fix, the segmentation fault may indeed happen
>> when injecting IRQ from the app directly using the Vhost API. It cannot
>> happen when vhost_vring_call() is calle directly from
>> rte_enqueue_burst/rte_dequeue_burst though.
>>
>> so I think below patch would be better:
>>
>> diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
>> index eb6309b681..733e0ab289 100644
>> --- a/lib/vhost/vhost.c
>> +++ b/lib/vhost/vhost.c
>> @@ -1341,6 +1341,9 @@ rte_vhost_vring_call(int vid, uint16_t vring_idx)
>>
>>           rte_rwlock_read_lock(&vq->access_lock);
>>
>> +       if (unlikely(!vq->access_ok))
>> +               return -1;
> 
> Don't you need to release the lock before returning here?

Of course yes, I actually caught it after sending my reply, it is 
already fixed locally.

But thanks for the review, much appreciated!
Maxime

>> +
>>           if (vq_is_packed(dev))
>>                   vhost_vring_call_packed(dev, vq);
>>           else
>> @@ -1371,6 +1374,9 @@ 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))
>> +               return -1;
> 
> Don't you need to release the lock before returning here?
> 
>> +
>>           if (vq_is_packed(dev))
>>                   vhost_vring_call_packed(dev, vq);
>>           else
>>
>>
>> Do you confirm that fixes your issue?
>>
>> Thanks,
>> Maxime
>
diff mbox series

Patch

diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
index eb6309b681..46f3391167 100644
--- a/lib/vhost/vhost.c
+++ b/lib/vhost/vhost.c
@@ -1327,6 +1327,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,13 +1343,13 @@  rte_vhost_vring_call(int vid, uint16_t vring_idx)
 	rte_rwlock_read_lock(&vq->access_lock);

 	if (vq_is_packed(dev))
-		vhost_vring_call_packed(dev, vq);
+		ret = vhost_vring_call_packed(dev, vq);
 	else
-		vhost_vring_call_split(dev, vq);
+		ret = vhost_vring_call_split(dev, vq);

 	rte_rwlock_read_unlock(&vq->access_lock);

-	return 0;
+	return ret;
 }

 int
@@ -1356,6 +1357,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,13 +1374,13 @@  rte_vhost_vring_call_nonblock(int vid, uint16_t vring_idx)
 		return -EAGAIN;

 	if (vq_is_packed(dev))
-		vhost_vring_call_packed(dev, vq);
+		ret = vhost_vring_call_packed(dev, vq);
 	else
-		vhost_vring_call_split(dev, vq);
+		ret = vhost_vring_call_split(dev, vq);

 	rte_rwlock_read_unlock(&vq->access_lock);

-	return 0;
+	return ret;
 }

 uint16_t
diff --git a/lib/vhost/vhost.h b/lib/vhost/vhost.h
index 9723429b1c..4c09c2ef0e 100644
--- a/lib/vhost/vhost.h
+++ b/lib/vhost/vhost.h
@@ -930,12 +930,15 @@  vhost_vring_inject_irq(struct virtio_net *dev, struct vhost_virtqueue *vq)
 		dev->notify_ops->guest_notified(dev->vid);
 }

-static __rte_always_inline void
+static __rte_always_inline int
 vhost_vring_call_split(struct virtio_net *dev, struct vhost_virtqueue *vq)
 {
 	/* Flush used->idx update before we read avail->flags. */
 	rte_atomic_thread_fence(__ATOMIC_SEQ_CST);

+	if (!vq->avail || !vq->used)
+		return -1;
+
 	/* Don't kick guest if we don't reach index specified by guest. */
 	if (dev->features & (1ULL << VIRTIO_RING_F_EVENT_IDX)) {
 		uint16_t old = vq->signalled_used;
@@ -957,9 +960,10 @@  vhost_vring_call_split(struct virtio_net *dev, struct vhost_virtqueue *vq)
 		if (!(vq->avail->flags & VRING_AVAIL_F_NO_INTERRUPT))
 			vhost_vring_inject_irq(dev, vq);
 	}
+	return 0;
 }

-static __rte_always_inline void
+static __rte_always_inline int
 vhost_vring_call_packed(struct virtio_net *dev, struct vhost_virtqueue *vq)
 {
 	uint16_t old, new, off, off_wrap;
@@ -968,6 +972,9 @@  vhost_vring_call_packed(struct virtio_net *dev, struct vhost_virtqueue *vq)
 	/* Flush used desc update. */
 	rte_atomic_thread_fence(__ATOMIC_SEQ_CST);

+	if (!vq->driver_event)
+		return -1;
+
 	if (!(dev->features & (1ULL << VIRTIO_RING_F_EVENT_IDX))) {
 		if (vq->driver_event->flags !=
 				VRING_EVENT_F_DISABLE)
@@ -1008,6 +1015,7 @@  vhost_vring_call_packed(struct virtio_net *dev, struct vhost_virtqueue *vq)
 kick:
 	if (kick)
 		vhost_vring_inject_irq(dev, vq);
+	return 0;
 }

 static __rte_always_inline void