[Qemu-devel] [PATCH V2 1/3] virtio: guard against NULL pfn

[Qemu-devel] [PATCH V2 1/3] virtio: guard against NULL pfn

[Jason Wang]

To avoid access stale memory region cache after reset, this patch
check the existence of virtqueue pfn for all exported virtqueue access
helpers before trying to use them.

Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/virtio/virtio.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index efce4b3..76cc81b 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -322,6 +322,10 @@ static int virtio_queue_empty_rcu(VirtQueue *vq)
         return 0;
     }
 
+    if (unlikely(!vq->vring.avail)) {
+        return 0;
+    }
+
     return vring_avail_idx(vq) == vq->last_avail_idx;
 }
 
@@ -333,6 +337,10 @@ int virtio_queue_empty(VirtQueue *vq)
         return 0;
     }
 
+    if (unlikely(!vq->vring.avail)) {
+        return 0;
+    }
+
     rcu_read_lock();
     empty = vring_avail_idx(vq) == vq->last_avail_idx;
     rcu_read_unlock();
@@ -431,6 +439,10 @@ void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem,
         return;
     }
 
+    if (unlikely(!vq->vring.used)) {
+        return;
+    }
+
     idx = (idx + vq->used_idx) % vq->vring.num;
 
     uelem.id = elem->index;
@@ -448,6 +460,10 @@ void virtqueue_flush(VirtQueue *vq, unsigned int count)
         return;
     }
 
+    if (unlikely(!vq->vring.used)) {
+        return;
+    }
+
     /* Make sure buffer is written before we update index. */
     smp_wmb();
     trace_virtqueue_flush(vq, count);
@@ -546,6 +562,11 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
     int64_t len = 0;
     int rc;
 
+    if (unlikely(!vq->vring.desc)) {
+        *in_bytes = *out_bytes = 0;
+        return;
+    }
+
     rcu_read_lock();
     idx = vq->last_avail_idx;
     total_bufs = in_total = out_total = 0;
-- 
2.7.4

[Qemu-devel] [PATCH V2 2/3] virtio: destroy region cache during reset

[Jason Wang]

We don't destroy region cache during reset which can make the maps
of previous driver leaked to a buggy or malicious driver that don't
set vring address before starting to use the device. Fix this by
destroy the region cache during reset and validate it before trying to
see them.

Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
Changes from v1:
- switch to use rcu in virtio_virtqueue_region_cache()
- use unlikely() when needed
---
 hw/virtio/virtio.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 53 insertions(+), 7 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 76cc81b..f086452 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -190,6 +190,10 @@ static inline uint16_t vring_avail_flags(VirtQueue *vq)
 {
     VRingMemoryRegionCaches *caches = atomic_rcu_read(&vq->vring.caches);
     hwaddr pa = offsetof(VRingAvail, flags);
+    if (unlikely(!caches)) {
+        virtio_error(vq->vdev, "Cannot map avail flags");
+        return 0;
+    }
     return virtio_lduw_phys_cached(vq->vdev, &caches->avail, pa);
 }
 
@@ -198,6 +202,10 @@ static inline uint16_t vring_avail_idx(VirtQueue *vq)
 {
     VRingMemoryRegionCaches *caches = atomic_rcu_read(&vq->vring.caches);
     hwaddr pa = offsetof(VRingAvail, idx);
+    if (unlikely(!caches)) {
+        virtio_error(vq->vdev, "Cannot map avail idx");
+        return vq->shadow_avail_idx;
+    }
     vq->shadow_avail_idx = virtio_lduw_phys_cached(vq->vdev, &caches->avail, pa);
     return vq->shadow_avail_idx;
 }
@@ -207,6 +215,10 @@ static inline uint16_t vring_avail_ring(VirtQueue *vq, int i)
 {
     VRingMemoryRegionCaches *caches = atomic_rcu_read(&vq->vring.caches);
     hwaddr pa = offsetof(VRingAvail, ring[i]);
+    if (unlikely(!caches)) {
+        virtio_error(vq->vdev, "Cannot map avail ring");
+        return 0;
+    }
     return virtio_lduw_phys_cached(vq->vdev, &caches->avail, pa);
 }
 
@@ -222,6 +234,10 @@ static inline void vring_used_write(VirtQueue *vq, VRingUsedElem *uelem,
 {
     VRingMemoryRegionCaches *caches = atomic_rcu_read(&vq->vring.caches);
     hwaddr pa = offsetof(VRingUsed, ring[i]);
+    if (unlikely(!caches)) {
+        virtio_error(vq->vdev, "Cannot map used ring");
+        return;
+    }
     virtio_tswap32s(vq->vdev, &uelem->id);
     virtio_tswap32s(vq->vdev, &uelem->len);
     address_space_write_cached(&caches->used, pa, uelem, sizeof(VRingUsedElem));
@@ -233,6 +249,10 @@ static uint16_t vring_used_idx(VirtQueue *vq)
 {
     VRingMemoryRegionCaches *caches = atomic_rcu_read(&vq->vring.caches);
     hwaddr pa = offsetof(VRingUsed, idx);
+    if (unlikely(!caches)) {
+        virtio_error(vq->vdev, "Cannot map used ring");
+        return 0;
+    }
     return virtio_lduw_phys_cached(vq->vdev, &caches->used, pa);
 }
 
@@ -241,6 +261,10 @@ static inline void vring_used_idx_set(VirtQueue *vq, uint16_t val)
 {
     VRingMemoryRegionCaches *caches = atomic_rcu_read(&vq->vring.caches);
     hwaddr pa = offsetof(VRingUsed, idx);
+    if (unlikely(!caches)) {
+        virtio_error(vq->vdev, "Cannot map used idx");
+        return;
+    }
     virtio_stw_phys_cached(vq->vdev, &caches->used, pa, val);
     address_space_cache_invalidate(&caches->used, pa, sizeof(val));
     vq->used_idx = val;
@@ -252,8 +276,13 @@ static inline void vring_used_flags_set_bit(VirtQueue *vq, int mask)
     VRingMemoryRegionCaches *caches = atomic_rcu_read(&vq->vring.caches);
     VirtIODevice *vdev = vq->vdev;
     hwaddr pa = offsetof(VRingUsed, flags);
-    uint16_t flags = virtio_lduw_phys_cached(vq->vdev, &caches->used, pa);
+    uint16_t flags;
 
+    if (unlikely(!caches)) {
+        virtio_error(vq->vdev, "Cannot map used flags");
+        return;
+    }
+    flags = virtio_lduw_phys_cached(vq->vdev, &caches->used, pa);
     virtio_stw_phys_cached(vdev, &caches->used, pa, flags | mask);
     address_space_cache_invalidate(&caches->used, pa, sizeof(flags));
 }
@@ -266,6 +295,10 @@ static inline void vring_used_flags_unset_bit(VirtQueue *vq, int mask)
     hwaddr pa = offsetof(VRingUsed, flags);
     uint16_t flags = virtio_lduw_phys_cached(vq->vdev, &caches->used, pa);
 
+    if (unlikely(!caches)) {
+        virtio_error(vq->vdev, "Cannot map used flags");
+        return;
+    }
     virtio_stw_phys_cached(vdev, &caches->used, pa, flags & ~mask);
     address_space_cache_invalidate(&caches->used, pa, sizeof(flags));
 }
@@ -280,6 +313,10 @@ static inline void vring_set_avail_event(VirtQueue *vq, uint16_t val)
     }
 
     caches = atomic_rcu_read(&vq->vring.caches);
+    if (unlikely(!caches)) {
+        virtio_error(vq->vdev, "Cannot map avail event");
+        return;
+    }
     pa = offsetof(VRingUsed, ring[vq->vring.num]);
     virtio_stw_phys_cached(vq->vdev, &caches->used, pa, val);
     address_space_cache_invalidate(&caches->used, pa, sizeof(val));
@@ -573,7 +610,7 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
 
     max = vq->vring.num;
     caches = atomic_rcu_read(&vq->vring.caches);
-    if (caches->desc.len < max * sizeof(VRingDesc)) {
+    if (unlikely(!caches) || caches->desc.len < max * sizeof(VRingDesc)) {
         virtio_error(vdev, "Cannot map descriptor ring");
         goto err;
     }
@@ -840,7 +877,7 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz)
     i = head;
 
     caches = atomic_rcu_read(&vq->vring.caches);
-    if (caches->desc.len < max * sizeof(VRingDesc)) {
+    if (unlikely(!caches) || caches->desc.len < max * sizeof(VRingDesc)) {
         virtio_error(vdev, "Cannot map descriptor ring");
         goto done;
     }
@@ -1138,6 +1175,17 @@ static enum virtio_device_endian virtio_current_cpu_endian(void)
     }
 }
 
+static void virtio_virtqueue_reset_region_cache(struct VirtQueue *vq)
+{
+    VRingMemoryRegionCaches *caches;
+
+    caches = atomic_read(&vq->vring.caches);
+    atomic_set(&vq->vring.caches, NULL);
+    if (caches) {
+        call_rcu(caches, virtio_free_region_cache, rcu);
+    }
+}
+
 void virtio_reset(void *opaque)
 {
     VirtIODevice *vdev = opaque;
@@ -1178,6 +1226,7 @@ void virtio_reset(void *opaque)
         vdev->vq[i].notification = true;
         vdev->vq[i].vring.num = vdev->vq[i].vring.num_default;
         vdev->vq[i].inuse = 0;
+        virtio_virtqueue_reset_region_cache(&vdev->vq[i]);
     }
 }
 
@@ -2472,13 +2521,10 @@ static void virtio_device_free_virtqueues(VirtIODevice *vdev)
     }
 
     for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
-        VRingMemoryRegionCaches *caches;
         if (vdev->vq[i].vring.num == 0) {
             break;
         }
-        caches = atomic_read(&vdev->vq[i].vring.caches);
-        atomic_set(&vdev->vq[i].vring.caches, NULL);
-        virtio_free_region_cache(caches);
+        virtio_virtqueue_reset_region_cache(&vdev->vq[i]);
     }
     g_free(vdev->vq);
 }
-- 
2.7.4

Re: [Qemu-devel] [PATCH V2 2/3] virtio: destroy region cache during reset

[Cornelia Huck]

On Mon, 13 Mar 2017 14:29:42 +0800
Jason Wang <jasowang@redhat.com> wrote:

> We don't destroy region cache during reset which can make the maps
> of previous driver leaked to a buggy or malicious driver that don't
> set vring address before starting to use the device. Fix this by
> destroy the region cache during reset and validate it before trying to
> see them.
> 
> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
> Changes from v1:
> - switch to use rcu in virtio_virtqueue_region_cache()
> - use unlikely() when needed
> ---
>  hw/virtio/virtio.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 53 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 76cc81b..f086452 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -190,6 +190,10 @@ static inline uint16_t vring_avail_flags(VirtQueue *vq)
>  {
>      VRingMemoryRegionCaches *caches = atomic_rcu_read(&vq->vring.caches);
>      hwaddr pa = offsetof(VRingAvail, flags);
> +    if (unlikely(!caches)) {
> +        virtio_error(vq->vdev, "Cannot map avail flags");
> +        return 0;

I'm still not 100% convinced of those checks; but they don't do any
harm.

> +    }
>      return virtio_lduw_phys_cached(vq->vdev, &caches->avail, pa);
>  }
> 

(...)

> +static void virtio_virtqueue_reset_region_cache(struct VirtQueue *vq)
> +{
> +    VRingMemoryRegionCaches *caches;
> +
> +    caches = atomic_read(&vq->vring.caches);
> +    atomic_set(&vq->vring.caches, NULL);

Needs atomic_rcu_set(), I think.

> +    if (caches) {
> +        call_rcu(caches, virtio_free_region_cache, rcu);
> +    }
> +}
> +
>  void virtio_reset(void *opaque)
>  {
>      VirtIODevice *vdev = opaque;

Re: [Qemu-devel] [PATCH V2 2/3] virtio: destroy region cache during reset

[Paolo Bonzini]

On 13/03/2017 11:05, Cornelia Huck wrote:
> On Mon, 13 Mar 2017 14:29:42 +0800
> Jason Wang <jasowang@redhat.com> wrote:
> 
>> We don't destroy region cache during reset which can make the maps
>> of previous driver leaked to a buggy or malicious driver that don't
>> set vring address before starting to use the device. Fix this by
>> destroy the region cache during reset and validate it before trying to
>> see them.
>>
>> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>> Changes from v1:
>> - switch to use rcu in virtio_virtqueue_region_cache()
>> - use unlikely() when needed
>> ---
>>  hw/virtio/virtio.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++-------
>>  1 file changed, 53 insertions(+), 7 deletions(-)
>>
>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>> index 76cc81b..f086452 100644
>> --- a/hw/virtio/virtio.c
>> +++ b/hw/virtio/virtio.c
>> @@ -190,6 +190,10 @@ static inline uint16_t vring_avail_flags(VirtQueue *vq)
>>  {
>>      VRingMemoryRegionCaches *caches = atomic_rcu_read(&vq->vring.caches);
>>      hwaddr pa = offsetof(VRingAvail, flags);
>> +    if (unlikely(!caches)) {
>> +        virtio_error(vq->vdev, "Cannot map avail flags");
>> +        return 0;
> 
> I'm still not 100% convinced of those checks; but they don't do any
> harm.

Same here... We would be hiding a bug.

>> +    }
>>      return virtio_lduw_phys_cached(vq->vdev, &caches->avail, pa);
>>  }
>>
> 
> (...)
> 
>> +static void virtio_virtqueue_reset_region_cache(struct VirtQueue *vq)
>> +{
>> +    VRingMemoryRegionCaches *caches;
>> +
>> +    caches = atomic_read(&vq->vring.caches);
>> +    atomic_set(&vq->vring.caches, NULL);
> 
> Needs atomic_rcu_set(), I think.

Not necessarily, see kernel rcu_assign_pointer vs. RCU_INIT_POINTER.
But it's probably easier to use it.

Paolo

>> +    if (caches) {
>> +        call_rcu(caches, virtio_free_region_cache, rcu);
>> +    }
>> +}
>> +
>>  void virtio_reset(void *opaque)
>>  {
>>      VirtIODevice *vdev = opaque;
> 

Re: [Qemu-devel] [PATCH V2 2/3] virtio: destroy region cache during reset

[Jason Wang]

On 2017年03月13日 18:20, Paolo Bonzini wrote:
> On 13/03/2017 11:05, Cornelia Huck wrote:
>> On Mon, 13 Mar 2017 14:29:42 +0800
>> Jason Wang<jasowang@redhat.com>  wrote:
>>
>>> We don't destroy region cache during reset which can make the maps
>>> of previous driver leaked to a buggy or malicious driver that don't
>>> set vring address before starting to use the device. Fix this by
>>> destroy the region cache during reset and validate it before trying to
>>> see them.
>>>
>>> Cc: Cornelia Huck<cornelia.huck@de.ibm.com>
>>> Cc: Paolo Bonzini<pbonzini@redhat.com>
>>> Signed-off-by: Jason Wang<jasowang@redhat.com>
>>> ---
>>> Changes from v1:
>>> - switch to use rcu in virtio_virtqueue_region_cache()
>>> - use unlikely() when needed
>>> ---
>>>   hw/virtio/virtio.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++-------
>>>   1 file changed, 53 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>>> index 76cc81b..f086452 100644
>>> --- a/hw/virtio/virtio.c
>>> +++ b/hw/virtio/virtio.c
>>> @@ -190,6 +190,10 @@ static inline uint16_t vring_avail_flags(VirtQueue *vq)
>>>   {
>>>       VRingMemoryRegionCaches *caches = atomic_rcu_read(&vq->vring.caches);
>>>       hwaddr pa = offsetof(VRingAvail, flags);
>>> +    if (unlikely(!caches)) {
>>> +        virtio_error(vq->vdev, "Cannot map avail flags");
>>> +        return 0;
>> I'm still not 100% convinced of those checks; but they don't do any
>> harm.
> Same here... We would be hiding a bug.
>

Yes, I will use assert here.

Thanks

Re: [Qemu-devel] [PATCH V2 2/3] virtio: destroy region cache during reset

[Jason Wang]

On 2017年03月13日 18:05, Cornelia Huck wrote:
> On Mon, 13 Mar 2017 14:29:42 +0800
> Jason Wang <jasowang@redhat.com> wrote:
>
>> We don't destroy region cache during reset which can make the maps
>> of previous driver leaked to a buggy or malicious driver that don't
>> set vring address before starting to use the device. Fix this by
>> destroy the region cache during reset and validate it before trying to
>> see them.
>>
>> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>> Changes from v1:
>> - switch to use rcu in virtio_virtqueue_region_cache()
>> - use unlikely() when needed
>> ---
>>   hw/virtio/virtio.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++-------
>>   1 file changed, 53 insertions(+), 7 deletions(-)
>>
>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>> index 76cc81b..f086452 100644
>> --- a/hw/virtio/virtio.c
>> +++ b/hw/virtio/virtio.c
>> @@ -190,6 +190,10 @@ static inline uint16_t vring_avail_flags(VirtQueue *vq)
>>   {
>>       VRingMemoryRegionCaches *caches = atomic_rcu_read(&vq->vring.caches);
>>       hwaddr pa = offsetof(VRingAvail, flags);
>> +    if (unlikely(!caches)) {
>> +        virtio_error(vq->vdev, "Cannot map avail flags");
>> +        return 0;
> I'm still not 100% convinced of those checks; but they don't do any
> harm.

Right, consider we've already had patch 1, I think it should be fine to 
use assert here.

>
>> +    }
>>       return virtio_lduw_phys_cached(vq->vdev, &caches->avail, pa);
>>   }
>>
> (...)
>
>> +static void virtio_virtqueue_reset_region_cache(struct VirtQueue *vq)
>> +{
>> +    VRingMemoryRegionCaches *caches;
>> +
>> +    caches = atomic_read(&vq->vring.caches);
>> +    atomic_set(&vq->vring.caches, NULL);
> Needs atomic_rcu_set(), I think.

Any atomic write should be fine here, but I agree atomic_rcu_set() is 
better. Will use it in next version.

Thanks

>
>> +    if (caches) {
>> +        call_rcu(caches, virtio_free_region_cache, rcu);
>> +    }
>> +}
>> +
>>   void virtio_reset(void *opaque)
>>   {
>>       VirtIODevice *vdev = opaque;
>

[Qemu-devel] [PATCH V2 3/3] virtio: validate address space cache during init

[Jason Wang]

We don't check the return value of address_space_cache_init(), this
may lead buggy driver use incorrect region caches. Instead of
triggering an assert, catch and warn this early in
virtio_init_region_cache().

Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/virtio/virtio.c | 33 +++++++++++++++++++++++++++------
 1 file changed, 27 insertions(+), 6 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index f086452..dc5bec7 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -131,6 +131,7 @@ static void virtio_init_region_cache(VirtIODevice *vdev, int n)
     VRingMemoryRegionCaches *new;
     hwaddr addr, size;
     int event_size;
+    int64_t len;
 
     event_size = virtio_vdev_has_feature(vq->vdev, VIRTIO_RING_F_EVENT_IDX) ? 2 : 0;
 
@@ -140,21 +141,41 @@ static void virtio_init_region_cache(VirtIODevice *vdev, int n)
     }
     new = g_new0(VRingMemoryRegionCaches, 1);
     size = virtio_queue_get_desc_size(vdev, n);
-    address_space_cache_init(&new->desc, vdev->dma_as,
-                             addr, size, false);
+    len = address_space_cache_init(&new->desc, vdev->dma_as,
+                                   addr, size, false);
+    if (len < size) {
+        virtio_error(vdev, "Cannot map desc");
+        goto err_desc;
+    }
 
     size = virtio_queue_get_used_size(vdev, n) + event_size;
-    address_space_cache_init(&new->used, vdev->dma_as,
-                             vq->vring.used, size, true);
+    len = address_space_cache_init(&new->used, vdev->dma_as,
+                                   vq->vring.used, size, true);
+    if (len < size) {
+        virtio_error(vdev, "Cannot map used");
+        goto err_used;
+    }
 
     size = virtio_queue_get_avail_size(vdev, n) + event_size;
-    address_space_cache_init(&new->avail, vdev->dma_as,
-                             vq->vring.avail, size, false);
+    len = address_space_cache_init(&new->avail, vdev->dma_as,
+                                   vq->vring.avail, size, false);
+    if (len < size) {
+        virtio_error(vdev, "Cannot map avail");
+        goto err_avail;
+    }
 
     atomic_rcu_set(&vq->vring.caches, new);
     if (old) {
         call_rcu(old, virtio_free_region_cache, rcu);
     }
+    return;
+
+err_avail:
+    address_space_cache_destroy(&new->used);
+err_used:
+    address_space_cache_destroy(&new->desc);
+err_desc:
+    g_free(new);
 }
 
 /* virt queue functions */
-- 
2.7.4

Re: [Qemu-devel] [PATCH V2 3/3] virtio: validate address space cache during init

[Cornelia Huck]

On Mon, 13 Mar 2017 14:29:43 +0800
Jason Wang <jasowang@redhat.com> wrote:

> We don't check the return value of address_space_cache_init(), this
> may lead buggy driver use incorrect region caches. Instead of
> triggering an assert, catch and warn this early in
> virtio_init_region_cache().
> 
> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  hw/virtio/virtio.c | 33 +++++++++++++++++++++++++++------
>  1 file changed, 27 insertions(+), 6 deletions(-)

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

Re: [Qemu-devel] [PATCH V2 1/3] virtio: guard against NULL pfn

[Cornelia Huck]

On Mon, 13 Mar 2017 14:29:41 +0800
Jason Wang <jasowang@redhat.com> wrote:

> To avoid access stale memory region cache after reset, this patch
> check the existence of virtqueue pfn for all exported virtqueue access
> helpers before trying to use them.
> 
> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  hw/virtio/virtio.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index efce4b3..76cc81b 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -322,6 +322,10 @@ static int virtio_queue_empty_rcu(VirtQueue *vq)
>          return 0;
>      }
> 
> +    if (unlikely(!vq->vring.avail)) {
> +        return 0;

Shouldn't that rather return !0 (denoting a non-existing queue as
empty)?

> +    }
> +
>      return vring_avail_idx(vq) == vq->last_avail_idx;
>  }
> 
> @@ -333,6 +337,10 @@ int virtio_queue_empty(VirtQueue *vq)
>          return 0;
>      }
> 
> +    if (unlikely(!vq->vring.avail)) {
> +        return 0;

Likewise.

> +    }
> +
>      rcu_read_lock();
>      empty = vring_avail_idx(vq) == vq->last_avail_idx;
>      rcu_read_unlock();
> @@ -431,6 +439,10 @@ void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem,
>          return;
>      }
> 
> +    if (unlikely(!vq->vring.used)) {
> +        return;
> +    }
> +
>      idx = (idx + vq->used_idx) % vq->vring.num;
> 
>      uelem.id = elem->index;
> @@ -448,6 +460,10 @@ void virtqueue_flush(VirtQueue *vq, unsigned int count)
>          return;
>      }
> 
> +    if (unlikely(!vq->vring.used)) {
> +        return;
> +    }
> +
>      /* Make sure buffer is written before we update index. */
>      smp_wmb();
>      trace_virtqueue_flush(vq, count);
> @@ -546,6 +562,11 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
>      int64_t len = 0;
>      int rc;
> 
> +    if (unlikely(!vq->vring.desc)) {
> +        *in_bytes = *out_bytes = 0;

I think you need to check for in_bytes and out_bytes being !NULL first.

> +        return;
> +    }
> +
>      rcu_read_lock();
>      idx = vq->last_avail_idx;
>      total_bufs = in_total = out_total = 0;

Re: [Qemu-devel] [PATCH V2 1/3] virtio: guard against NULL pfn

[Paolo Bonzini]

On 13/03/2017 10:55, Cornelia Huck wrote:
> On Mon, 13 Mar 2017 14:29:41 +0800
> Jason Wang <jasowang@redhat.com> wrote:
> 
>> To avoid access stale memory region cache after reset, this patch
>> check the existence of virtqueue pfn for all exported virtqueue access
>> helpers before trying to use them.
>>
>> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>>  hw/virtio/virtio.c | 21 +++++++++++++++++++++
>>  1 file changed, 21 insertions(+)
>>
>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>> index efce4b3..76cc81b 100644
>> --- a/hw/virtio/virtio.c
>> +++ b/hw/virtio/virtio.c
>> @@ -322,6 +322,10 @@ static int virtio_queue_empty_rcu(VirtQueue *vq)
>>          return 0;
>>      }
>>
>> +    if (unlikely(!vq->vring.avail)) {
>> +        return 0;
> 
> Shouldn't that rather return !0 (denoting a non-existing queue as
> empty)?

Yes, and the check should also go first (before the function can return 0).

Paolo

>> +    }
>> +
>>      return vring_avail_idx(vq) == vq->last_avail_idx;
>>  }
>>
>> @@ -333,6 +337,10 @@ int virtio_queue_empty(VirtQueue *vq)
>>          return 0;
>>      }
>>
>> +    if (unlikely(!vq->vring.avail)) {
>> +        return 0;
> 
> Likewise.
> 
>> +    }
>> +
>>      rcu_read_lock();
>>      empty = vring_avail_idx(vq) == vq->last_avail_idx;
>>      rcu_read_unlock();
>> @@ -431,6 +439,10 @@ void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem,
>>          return;
>>      }
>>
>> +    if (unlikely(!vq->vring.used)) {
>> +        return;
>> +    }
>> +
>>      idx = (idx + vq->used_idx) % vq->vring.num;
>>
>>      uelem.id = elem->index;
>> @@ -448,6 +460,10 @@ void virtqueue_flush(VirtQueue *vq, unsigned int count)
>>          return;
>>      }
>>
>> +    if (unlikely(!vq->vring.used)) {
>> +        return;
>> +    }
>> +
>>      /* Make sure buffer is written before we update index. */
>>      smp_wmb();
>>      trace_virtqueue_flush(vq, count);
>> @@ -546,6 +562,11 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
>>      int64_t len = 0;
>>      int rc;
>>
>> +    if (unlikely(!vq->vring.desc)) {
>> +        *in_bytes = *out_bytes = 0;
> 
> I think you need to check for in_bytes and out_bytes being !NULL first.
> 
>> +        return;
>> +    }
>> +
>>      rcu_read_lock();
>>      idx = vq->last_avail_idx;
>>      total_bufs = in_total = out_total = 0;
> 

Re: [Qemu-devel] [PATCH V2 1/3] virtio: guard against NULL pfn

[Jason Wang]

On 2017年03月13日 18:18, Paolo Bonzini wrote:
>
> On 13/03/2017 10:55, Cornelia Huck wrote:
>> On Mon, 13 Mar 2017 14:29:41 +0800
>> Jason Wang <jasowang@redhat.com> wrote:
>>
>>> To avoid access stale memory region cache after reset, this patch
>>> check the existence of virtqueue pfn for all exported virtqueue access
>>> helpers before trying to use them.
>>>
>>> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>> ---
>>>   hw/virtio/virtio.c | 21 +++++++++++++++++++++
>>>   1 file changed, 21 insertions(+)
>>>
>>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>>> index efce4b3..76cc81b 100644
>>> --- a/hw/virtio/virtio.c
>>> +++ b/hw/virtio/virtio.c
>>> @@ -322,6 +322,10 @@ static int virtio_queue_empty_rcu(VirtQueue *vq)
>>>           return 0;
>>>       }
>>>
>>> +    if (unlikely(!vq->vring.avail)) {
>>> +        return 0;
>> Shouldn't that rather return !0 (denoting a non-existing queue as
>> empty)?
> Yes, and the check should also go first (before the function can return 0).
>
> Paolo

Yes, will fix in V3.

Thanks

Re: [Qemu-devel] [PATCH V2 1/3] virtio: guard against NULL pfn

[Jason Wang]

On 2017年03月13日 17:55, Cornelia Huck wrote:
> On Mon, 13 Mar 2017 14:29:41 +0800
> Jason Wang <jasowang@redhat.com> wrote:
>
>> To avoid access stale memory region cache after reset, this patch
>> check the existence of virtqueue pfn for all exported virtqueue access
>> helpers before trying to use them.
>>
>> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>>   hw/virtio/virtio.c | 21 +++++++++++++++++++++
>>   1 file changed, 21 insertions(+)
>>
>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>> index efce4b3..76cc81b 100644
>> --- a/hw/virtio/virtio.c
>> +++ b/hw/virtio/virtio.c
>> @@ -322,6 +322,10 @@ static int virtio_queue_empty_rcu(VirtQueue *vq)
>>           return 0;
>>       }
>>
>> +    if (unlikely(!vq->vring.avail)) {
>> +        return 0;
> Shouldn't that rather return !0 (denoting a non-existing queue as
> empty)?

Yes.

>
>> +    }
>> +
>>       return vring_avail_idx(vq) == vq->last_avail_idx;
>>   }
>>
>> @@ -333,6 +337,10 @@ int virtio_queue_empty(VirtQueue *vq)
>>           return 0;
>>       }
>>
>> +    if (unlikely(!vq->vring.avail)) {
>> +        return 0;
> Likewise.
>
>> +    }
>> +
>>       rcu_read_lock();
>>       empty = vring_avail_idx(vq) == vq->last_avail_idx;
>>       rcu_read_unlock();
>> @@ -431,6 +439,10 @@ void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem,
>>           return;
>>       }
>>
>> +    if (unlikely(!vq->vring.used)) {
>> +        return;
>> +    }
>> +
>>       idx = (idx + vq->used_idx) % vq->vring.num;
>>
>>       uelem.id = elem->index;
>> @@ -448,6 +460,10 @@ void virtqueue_flush(VirtQueue *vq, unsigned int count)
>>           return;
>>       }
>>
>> +    if (unlikely(!vq->vring.used)) {
>> +        return;
>> +    }
>> +
>>       /* Make sure buffer is written before we update index. */
>>       smp_wmb();
>>       trace_virtqueue_flush(vq, count);
>> @@ -546,6 +562,11 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
>>       int64_t len = 0;
>>       int rc;
>>
>> +    if (unlikely(!vq->vring.desc)) {
>> +        *in_bytes = *out_bytes = 0;
> I think you need to check for in_bytes and out_bytes being !NULL first.

Right, will fix this in v2.

Thanks

>
>> +        return;
>> +    }
>> +
>>       rcu_read_lock();
>>       idx = vq->last_avail_idx;
>>       total_bufs = in_total = out_total = 0;
>