[Qemu-devel] [PATCH] virtio: destroy region cache during reset

[Qemu-devel] [PATCH] 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
use them. While at it, also validate address_space_cache_init() during
virtio_init_region_cache() to make sure we have a correct region
cache.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/virtio/virtio.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 76 insertions(+), 12 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 09f4cf4..90324f6 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 */
@@ -190,6 +211,10 @@ static inline uint16_t vring_avail_flags(VirtQueue *vq)
 {
     VRingMemoryRegionCaches *caches = atomic_rcu_read(&vq->vring.caches);
     hwaddr pa = offsetof(VRingAvail, flags);
+    if (!caches) {
+        virtio_error(vq->vdev, "Cannot map avail flags");
+        return 0;
+    }
     return virtio_lduw_phys_cached(vq->vdev, &caches->avail, pa);
 }
 
@@ -198,6 +223,10 @@ static inline uint16_t vring_avail_idx(VirtQueue *vq)
 {
     VRingMemoryRegionCaches *caches = atomic_rcu_read(&vq->vring.caches);
     hwaddr pa = offsetof(VRingAvail, idx);
+    if (!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 +236,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 (!caches) {
+        virtio_error(vq->vdev, "Cannot map avail ring");
+        return 0;
+    }
     return virtio_lduw_phys_cached(vq->vdev, &caches->avail, pa);
 }
 
@@ -222,6 +255,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 (!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 +270,10 @@ static uint16_t vring_used_idx(VirtQueue *vq)
 {
     VRingMemoryRegionCaches *caches = atomic_rcu_read(&vq->vring.caches);
     hwaddr pa = offsetof(VRingUsed, idx);
+    if (!caches) {
+        virtio_error(vq->vdev, "Cannot map used ring");
+        return 0;
+    }
     return virtio_lduw_phys_cached(vq->vdev, &caches->used, pa);
 }
 
@@ -241,6 +282,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 (!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;
@@ -254,6 +299,10 @@ static inline void vring_used_flags_set_bit(VirtQueue *vq, int mask)
     hwaddr pa = offsetof(VRingUsed, flags);
     uint16_t flags = virtio_lduw_phys_cached(vq->vdev, &caches->used, pa);
 
+    if (!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));
 }
@@ -266,6 +315,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 (!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 +333,10 @@ static inline void vring_set_avail_event(VirtQueue *vq, uint16_t val)
     }
 
     caches = atomic_rcu_read(&vq->vring.caches);
+    if (!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));
@@ -552,7 +609,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 (!caches || caches->desc.len < max * sizeof(VRingDesc)) {
         virtio_error(vdev, "Cannot map descriptor ring");
         goto err;
     }
@@ -819,7 +876,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 (!caches || caches->desc.len < max * sizeof(VRingDesc)) {
         virtio_error(vdev, "Cannot map descriptor ring");
         goto done;
     }
@@ -1117,6 +1174,15 @@ 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);
+    virtio_free_region_cache(caches);
+}
+
 void virtio_reset(void *opaque)
 {
     VirtIODevice *vdev = opaque;
@@ -1157,6 +1223,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]);
     }
 }
 
@@ -2451,13 +2518,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] virtio: destroy region cache during reset

[Cornelia Huck]

On Tue,  7 Mar 2017 16:47:58 +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
> use them. While at it, also validate address_space_cache_init() during
> virtio_init_region_cache() to make sure we have a correct region
> cache.
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  hw/virtio/virtio.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 76 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 09f4cf4..90324f6 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);
>  }

I think it would be more readable if you moved adding this check (which
is a good idea) into a separate patch.

> 
>  /* virt queue functions */
> @@ -190,6 +211,10 @@ static inline uint16_t vring_avail_flags(VirtQueue *vq)
>  {
>      VRingMemoryRegionCaches *caches = atomic_rcu_read(&vq->vring.caches);
>      hwaddr pa = offsetof(VRingAvail, flags);
> +    if (!caches) {
> +        virtio_error(vq->vdev, "Cannot map avail flags");

I'm not sure that virtio_error is the right thing here; ending up in
this function with !caches indicates an error in our logic. An assert
might be better (and I hope we can sort out all of those errors exposed
by the introduction of region caches for 2.9...)

> +        return 0;
> +    }
>      return virtio_lduw_phys_cached(vq->vdev, &caches->avail, pa);
>  }
> 
> @@ -198,6 +223,10 @@ static inline uint16_t vring_avail_idx(VirtQueue *vq)
>  {
>      VRingMemoryRegionCaches *caches = atomic_rcu_read(&vq->vring.caches);
>      hwaddr pa = offsetof(VRingAvail, idx);
> +    if (!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 +236,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 (!caches) {
> +        virtio_error(vq->vdev, "Cannot map avail ring");
> +        return 0;
> +    }
>      return virtio_lduw_phys_cached(vq->vdev, &caches->avail, pa);
>  }
> 
> @@ -222,6 +255,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 (!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 +270,10 @@ static uint16_t vring_used_idx(VirtQueue *vq)
>  {
>      VRingMemoryRegionCaches *caches = atomic_rcu_read(&vq->vring.caches);
>      hwaddr pa = offsetof(VRingUsed, idx);
> +    if (!caches) {
> +        virtio_error(vq->vdev, "Cannot map used ring");
> +        return 0;
> +    }
>      return virtio_lduw_phys_cached(vq->vdev, &caches->used, pa);
>  }
> 
> @@ -241,6 +282,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 (!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;
> @@ -254,6 +299,10 @@ static inline void vring_used_flags_set_bit(VirtQueue *vq, int mask)
>      hwaddr pa = offsetof(VRingUsed, flags);
>      uint16_t flags = virtio_lduw_phys_cached(vq->vdev, &caches->used, pa);
> 
> +    if (!caches) {
> +        virtio_error(vq->vdev, "Cannot map used flags");

Regardless of whether using virtio_error here is fine: caches was
already dereferenced above...

> +        return;
> +    }
>      virtio_stw_phys_cached(vdev, &caches->used, pa, flags | mask);
>      address_space_cache_invalidate(&caches->used, pa, sizeof(flags));
>  }
> @@ -266,6 +315,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 (!caches) {
> +        virtio_error(vq->vdev, "Cannot map used flags");

dito

> +        return;
> +    }
>      virtio_stw_phys_cached(vdev, &caches->used, pa, flags & ~mask);
>      address_space_cache_invalidate(&caches->used, pa, sizeof(flags));
>  }
> @@ -280,6 +333,10 @@ static inline void vring_set_avail_event(VirtQueue *vq, uint16_t val)
>      }
> 
>      caches = atomic_rcu_read(&vq->vring.caches);
> +    if (!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));
> @@ -552,7 +609,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 (!caches || caches->desc.len < max * sizeof(VRingDesc)) {
>          virtio_error(vdev, "Cannot map descriptor ring");
>          goto err;
>      }
> @@ -819,7 +876,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 (!caches || caches->desc.len < max * sizeof(VRingDesc)) {
>          virtio_error(vdev, "Cannot map descriptor ring");
>          goto done;
>      }
> @@ -1117,6 +1174,15 @@ 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);
> +    virtio_free_region_cache(caches);

Shouldn't this use rcu to free it? Unconditionally setting caches to
NULL feels wrong...

> +}
> +
>  void virtio_reset(void *opaque)
>  {
>      VirtIODevice *vdev = opaque;
> @@ -1157,6 +1223,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]);

...especially as you call it in a reset context here.

>      }
>  }
> 
> @@ -2451,13 +2518,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]);

OTOH, immediate destruction may still be called for during device
finalization.

>      }
>      g_free(vdev->vq);
>  }

Re: [Qemu-devel] [PATCH] virtio: destroy region cache during reset

[Jason Wang]

On 2017年03月07日 18:16, Cornelia Huck wrote:
> On Tue,  7 Mar 2017 16:47:58 +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
>> use them. While at it, also validate address_space_cache_init() during
>> virtio_init_region_cache() to make sure we have a correct region
>> cache.
>>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>>   hw/virtio/virtio.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++--------
>>   1 file changed, 76 insertions(+), 12 deletions(-)
>>
>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>> index 09f4cf4..90324f6 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);
>>   }
> I think it would be more readable if you moved adding this check (which
> is a good idea) into a separate patch.

Ok.

>>   /* virt queue functions */
>> @@ -190,6 +211,10 @@ static inline uint16_t vring_avail_flags(VirtQueue *vq)
>>   {
>>       VRingMemoryRegionCaches *caches = atomic_rcu_read(&vq->vring.caches);
>>       hwaddr pa = offsetof(VRingAvail, flags);
>> +    if (!caches) {
>> +        virtio_error(vq->vdev, "Cannot map avail flags");
> I'm not sure that virtio_error is the right thing here; ending up in
> this function with !caches indicates an error in our logic.

Probably not, this can be triggered by buggy guest.

> An assert
> might be better (and I hope we can sort out all of those errors exposed
> by the introduction of region caches for 2.9...)

I thought we should avoid assert as much as possible in this case. But 
if you and maintainer want an assert, it's also fine.

>
>> +        return 0;
>> +    }
>>       return virtio_lduw_phys_cached(vq->vdev, &caches->avail, pa);
>>   }
>>
>> @@ -198,6 +223,10 @@ static inline uint16_t vring_avail_idx(VirtQueue *vq)
>>   {
>>       VRingMemoryRegionCaches *caches = atomic_rcu_read(&vq->vring.caches);
>>       hwaddr pa = offsetof(VRingAvail, idx);
>> +    if (!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 +236,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 (!caches) {
>> +        virtio_error(vq->vdev, "Cannot map avail ring");
>> +        return 0;
>> +    }
>>       return virtio_lduw_phys_cached(vq->vdev, &caches->avail, pa);
>>   }
>>
>> @@ -222,6 +255,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 (!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 +270,10 @@ static uint16_t vring_used_idx(VirtQueue *vq)
>>   {
>>       VRingMemoryRegionCaches *caches = atomic_rcu_read(&vq->vring.caches);
>>       hwaddr pa = offsetof(VRingUsed, idx);
>> +    if (!caches) {
>> +        virtio_error(vq->vdev, "Cannot map used ring");
>> +        return 0;
>> +    }
>>       return virtio_lduw_phys_cached(vq->vdev, &caches->used, pa);
>>   }
>>
>> @@ -241,6 +282,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 (!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;
>> @@ -254,6 +299,10 @@ static inline void vring_used_flags_set_bit(VirtQueue *vq, int mask)
>>       hwaddr pa = offsetof(VRingUsed, flags);
>>       uint16_t flags = virtio_lduw_phys_cached(vq->vdev, &caches->used, pa);
>>
>> +    if (!caches) {
>> +        virtio_error(vq->vdev, "Cannot map used flags");
> Regardless of whether using virtio_error here is fine: caches was
> already dereferenced above...
>
>> +        return;
>> +    }
>>       virtio_stw_phys_cached(vdev, &caches->used, pa, flags | mask);
>>       address_space_cache_invalidate(&caches->used, pa, sizeof(flags));
>>   }
>> @@ -266,6 +315,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 (!caches) {
>> +        virtio_error(vq->vdev, "Cannot map used flags");
> dito
>
>> +        return;
>> +    }
>>       virtio_stw_phys_cached(vdev, &caches->used, pa, flags & ~mask);
>>       address_space_cache_invalidate(&caches->used, pa, sizeof(flags));
>>   }
>> @@ -280,6 +333,10 @@ static inline void vring_set_avail_event(VirtQueue *vq, uint16_t val)
>>       }
>>
>>       caches = atomic_rcu_read(&vq->vring.caches);
>> +    if (!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));
>> @@ -552,7 +609,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 (!caches || caches->desc.len < max * sizeof(VRingDesc)) {
>>           virtio_error(vdev, "Cannot map descriptor ring");
>>           goto err;
>>       }
>> @@ -819,7 +876,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 (!caches || caches->desc.len < max * sizeof(VRingDesc)) {
>>           virtio_error(vdev, "Cannot map descriptor ring");
>>           goto done;
>>       }
>> @@ -1117,6 +1174,15 @@ 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);
>> +    virtio_free_region_cache(caches);
> Shouldn't this use rcu to free it? Unconditionally setting caches to
> NULL feels wrong...

Right, will switch to use rcu.

>> +}
>> +
>>   void virtio_reset(void *opaque)
>>   {
>>       VirtIODevice *vdev = opaque;
>> @@ -1157,6 +1223,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]);
> ...especially as you call it in a reset context here.
>
>>       }
>>   }
>>
>> @@ -2451,13 +2518,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]);
> OTOH, immediate destruction may still be called for during device
> finalization.
>

Right but to avoid code duplication, use rcu unconditionally should be 
no harm here.

Thanks

>>       }
>>       g_free(vdev->vq);
>>   }

Re: [Qemu-devel] [PATCH] virtio: destroy region cache during reset

[Cornelia Huck]

On Wed, 8 Mar 2017 11:18:27 +0800
Jason Wang <jasowang@redhat.com> wrote:

> On 2017年03月07日 18:16, Cornelia Huck wrote:
> > On Tue,  7 Mar 2017 16:47:58 +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
> >> use them. While at it, also validate address_space_cache_init() during
> >> virtio_init_region_cache() to make sure we have a correct region
> >> cache.
> >>
> >> Signed-off-by: Jason Wang <jasowang@redhat.com>
> >> ---
> >>   hw/virtio/virtio.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++--------
> >>   1 file changed, 76 insertions(+), 12 deletions(-)

> >> @@ -190,6 +211,10 @@ static inline uint16_t vring_avail_flags(VirtQueue *vq)
> >>   {
> >>       VRingMemoryRegionCaches *caches = atomic_rcu_read(&vq->vring.caches);
> >>       hwaddr pa = offsetof(VRingAvail, flags);
> >> +    if (!caches) {
> >> +        virtio_error(vq->vdev, "Cannot map avail flags");
> > I'm not sure that virtio_error is the right thing here; ending up in
> > this function with !caches indicates an error in our logic.
> 
> Probably not, this can be triggered by buggy guest.

I would think that even a buggy guest should not be able to trigger
accesses to vring structures that have not yet been set up. What am I
missing?

> 
> > An assert
> > might be better (and I hope we can sort out all of those errors exposed
> > by the introduction of region caches for 2.9...)
> 
> I thought we should avoid assert as much as possible in this case. But 
> if you and maintainer want an assert, it's also fine.

My personal rule-of-thumb:
- If it is something that can be triggered by the guest, or it is
something that is easily recovered, set the device to broken.
- If it is something that indicates that we messed up our internal
logic, use an assert.

I think arriving here with !caches indicates the second case; but if
there is a way for a guest to trigger this, setting the device to
broken would certainly be better.

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

> >> @@ -1117,6 +1174,15 @@ 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);
> >> +    virtio_free_region_cache(caches);
> > Shouldn't this use rcu to free it? Unconditionally setting caches to
> > NULL feels wrong...
> 
> Right, will switch to use rcu.
> 
> >> +}
> >> +
> >>   void virtio_reset(void *opaque)
> >>   {
> >>       VirtIODevice *vdev = opaque;
> >> @@ -1157,6 +1223,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]);
> > ...especially as you call it in a reset context here.
> >
> >>       }
> >>   }
> >>
> >> @@ -2451,13 +2518,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]);
> > OTOH, immediate destruction may still be called for during device
> > finalization.
> >
> 
> Right but to avoid code duplication, use rcu unconditionally should be 
> no harm here.

Yes, this should be fine.

Re: [Qemu-devel] [PATCH] virtio: destroy region cache during reset

[Jason Wang]

On 2017年03月08日 17:19, Cornelia Huck wrote:
> On Wed, 8 Mar 2017 11:18:27 +0800
> Jason Wang <jasowang@redhat.com> wrote:
>
>> On 2017年03月07日 18:16, Cornelia Huck wrote:
>>> On Tue,  7 Mar 2017 16:47:58 +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
>>>> use them. While at it, also validate address_space_cache_init() during
>>>> virtio_init_region_cache() to make sure we have a correct region
>>>> cache.
>>>>
>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>>> ---
>>>>    hw/virtio/virtio.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++--------
>>>>    1 file changed, 76 insertions(+), 12 deletions(-)
>>>> @@ -190,6 +211,10 @@ static inline uint16_t vring_avail_flags(VirtQueue *vq)
>>>>    {
>>>>        VRingMemoryRegionCaches *caches = atomic_rcu_read(&vq->vring.caches);
>>>>        hwaddr pa = offsetof(VRingAvail, flags);
>>>> +    if (!caches) {
>>>> +        virtio_error(vq->vdev, "Cannot map avail flags");
>>> I'm not sure that virtio_error is the right thing here; ending up in
>>> this function with !caches indicates an error in our logic.
>> Probably not, this can be triggered by buggy guest.
> I would think that even a buggy guest should not be able to trigger
> accesses to vring structures that have not yet been set up. What am I
> missing?

I think this may happen if a driver start the device without setting the 
vring address. In this case, region caches cache the mapping of previous 
driver. But maybe I was wrong.

>
>>> An assert
>>> might be better (and I hope we can sort out all of those errors exposed
>>> by the introduction of region caches for 2.9...)
>> I thought we should avoid assert as much as possible in this case. But
>> if you and maintainer want an assert, it's also fine.
> My personal rule-of-thumb:
> - If it is something that can be triggered by the guest, or it is
> something that is easily recovered, set the device to broken.
> - If it is something that indicates that we messed up our internal
> logic, use an assert.
>
> I think arriving here with !caches indicates the second case; but if
> there is a way for a guest to trigger this, setting the device to
> broken would certainly be better.

Yes, broken seems better.

Thanks

Re: [Qemu-devel] [PATCH] virtio: destroy region cache during reset

[Cornelia Huck]

On Wed, 8 Mar 2017 17:51:22 +0800
Jason Wang <jasowang@redhat.com> wrote:

> On 2017年03月08日 17:19, Cornelia Huck wrote:
> > On Wed, 8 Mar 2017 11:18:27 +0800
> > Jason Wang <jasowang@redhat.com> wrote:
> >
> >> On 2017年03月07日 18:16, Cornelia Huck wrote:
> >>> On Tue,  7 Mar 2017 16:47:58 +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
> >>>> use them. While at it, also validate address_space_cache_init() during
> >>>> virtio_init_region_cache() to make sure we have a correct region
> >>>> cache.
> >>>>
> >>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
> >>>> ---
> >>>>    hw/virtio/virtio.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++--------
> >>>>    1 file changed, 76 insertions(+), 12 deletions(-)
> >>>> @@ -190,6 +211,10 @@ static inline uint16_t vring_avail_flags(VirtQueue *vq)
> >>>>    {
> >>>>        VRingMemoryRegionCaches *caches = atomic_rcu_read(&vq->vring.caches);
> >>>>        hwaddr pa = offsetof(VRingAvail, flags);
> >>>> +    if (!caches) {
> >>>> +        virtio_error(vq->vdev, "Cannot map avail flags");
> >>> I'm not sure that virtio_error is the right thing here; ending up in
> >>> this function with !caches indicates an error in our logic.
> >> Probably not, this can be triggered by buggy guest.
> > I would think that even a buggy guest should not be able to trigger
> > accesses to vring structures that have not yet been set up. What am I
> > missing?
> 
> I think this may happen if a driver start the device without setting the 
> vring address. In this case, region caches cache the mapping of previous 
> driver. But maybe I was wrong.

So the sequence would be:

- Driver #1 sets up the device, then abandons it without cleaning up
via reset
- Driver #2 uses the device without doing a reset or proper setup

?

I can't quite see why caches would be NULL in that case...

Having reset clean up the caches as well (IOW, the other part of your
patch) should make sure that we always have a consistent view of
descriptors and their caches, I think. The checks for desc != NULL and
friends should catch other driver buggyness.

Re: [Qemu-devel] [PATCH] virtio: destroy region cache during reset

[Jason Wang]

On 2017年03月08日 18:12, Cornelia Huck wrote:
> On Wed, 8 Mar 2017 17:51:22 +0800
> Jason Wang <jasowang@redhat.com> wrote:
>
>> On 2017年03月08日 17:19, Cornelia Huck wrote:
>>> On Wed, 8 Mar 2017 11:18:27 +0800
>>> Jason Wang <jasowang@redhat.com> wrote:
>>>
>>>> On 2017年03月07日 18:16, Cornelia Huck wrote:
>>>>> On Tue,  7 Mar 2017 16:47:58 +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
>>>>>> use them. While at it, also validate address_space_cache_init() during
>>>>>> virtio_init_region_cache() to make sure we have a correct region
>>>>>> cache.
>>>>>>
>>>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>>>>> ---
>>>>>>     hw/virtio/virtio.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++--------
>>>>>>     1 file changed, 76 insertions(+), 12 deletions(-)
>>>>>> @@ -190,6 +211,10 @@ static inline uint16_t vring_avail_flags(VirtQueue *vq)
>>>>>>     {
>>>>>>         VRingMemoryRegionCaches *caches = atomic_rcu_read(&vq->vring.caches);
>>>>>>         hwaddr pa = offsetof(VRingAvail, flags);
>>>>>> +    if (!caches) {
>>>>>> +        virtio_error(vq->vdev, "Cannot map avail flags");
>>>>> I'm not sure that virtio_error is the right thing here; ending up in
>>>>> this function with !caches indicates an error in our logic.
>>>> Probably not, this can be triggered by buggy guest.
>>> I would think that even a buggy guest should not be able to trigger
>>> accesses to vring structures that have not yet been set up. What am I
>>> missing?
>> I think this may happen if a driver start the device without setting the
>> vring address. In this case, region caches cache the mapping of previous
>> driver. But maybe I was wrong.
> So the sequence would be:
>
> - Driver #1 sets up the device, then abandons it without cleaning up
> via reset

If driver #1 reset the device in this case, the caches would be NULL.

> - Driver #2 uses the device without doing a reset or proper setup

Without this patch, even if driver #2 do a reset, it can still use the 
old map if it don't set queue pfn.

>
> ?
>
> I can't quite see why caches would be NULL in that case...
>
> Having reset clean up the caches as well (IOW, the other part of your
> patch) should make sure that we always have a consistent view of
> descriptors and their caches,

And prevent it from being leaked to untrusted drivers.

Thanks

> I think. The checks for desc != NULL and
> friends should catch other driver buggyness.
>
>

Re: [Qemu-devel] [PATCH] virtio: destroy region cache during reset

[Cornelia Huck]

On Thu, 9 Mar 2017 10:19:47 +0800
Jason Wang <jasowang@redhat.com> wrote:

> On 2017年03月08日 18:12, Cornelia Huck wrote:
> > On Wed, 8 Mar 2017 17:51:22 +0800
> > Jason Wang <jasowang@redhat.com> wrote:
> >
> >> On 2017年03月08日 17:19, Cornelia Huck wrote:
> >>> On Wed, 8 Mar 2017 11:18:27 +0800
> >>> Jason Wang <jasowang@redhat.com> wrote:
> >>>
> >>>> On 2017年03月07日 18:16, Cornelia Huck wrote:
> >>>>> On Tue,  7 Mar 2017 16:47:58 +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
> >>>>>> use them. While at it, also validate address_space_cache_init() during
> >>>>>> virtio_init_region_cache() to make sure we have a correct region
> >>>>>> cache.
> >>>>>>
> >>>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
> >>>>>> ---
> >>>>>>     hw/virtio/virtio.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++--------
> >>>>>>     1 file changed, 76 insertions(+), 12 deletions(-)
> >>>>>> @@ -190,6 +211,10 @@ static inline uint16_t vring_avail_flags(VirtQueue *vq)
> >>>>>>     {
> >>>>>>         VRingMemoryRegionCaches *caches = atomic_rcu_read(&vq->vring.caches);
> >>>>>>         hwaddr pa = offsetof(VRingAvail, flags);
> >>>>>> +    if (!caches) {
> >>>>>> +        virtio_error(vq->vdev, "Cannot map avail flags");
> >>>>> I'm not sure that virtio_error is the right thing here; ending up in
> >>>>> this function with !caches indicates an error in our logic.
> >>>> Probably not, this can be triggered by buggy guest.
> >>> I would think that even a buggy guest should not be able to trigger
> >>> accesses to vring structures that have not yet been set up. What am I
> >>> missing?
> >> I think this may happen if a driver start the device without setting the
> >> vring address. In this case, region caches cache the mapping of previous
> >> driver. But maybe I was wrong.
> > So the sequence would be:
> >
> > - Driver #1 sets up the device, then abandons it without cleaning up
> > via reset
> 
> If driver #1 reset the device in this case, the caches would be NULL.

Hm, how? Without your patch, the queue addresses are reset to 0 in that
case but the cache is not cleaned up.

> 
> > - Driver #2 uses the device without doing a reset or proper setup
> 
> Without this patch, even if driver #2 do a reset, it can still use the 
> old map if it don't set queue pfn.

Yes, the cleanup-on-reset is definetly needed.

> 
> >
> > ?
> >
> > I can't quite see why caches would be NULL in that case...
> >
> > Having reset clean up the caches as well (IOW, the other part of your
> > patch) should make sure that we always have a consistent view of
> > descriptors and their caches,
> 
> And prevent it from being leaked to untrusted drivers.

And that as well, agreed.

I'm still not sure why the checks for !caches help, though...

Re: [Qemu-devel] [PATCH] virtio: destroy region cache during reset

[Paolo Bonzini]

On 09/03/2017 12:07, Cornelia Huck wrote:
>>> - Driver #2 uses the device without doing a reset or proper setup
>> Without this patch, even if driver #2 do a reset, it can still use the 
>> old map if it don't set queue pfn.
> 
> Yes, the cleanup-on-reset is definetly needed.

It is good to have for defensiveness, but it would still cause a
segfault so we should also add the checks on vq->vring.desc throughout.

Paolo

Re: [Qemu-devel] [PATCH] virtio: destroy region cache during reset

[Cornelia Huck]

On Thu, 9 Mar 2017 12:12:00 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 09/03/2017 12:07, Cornelia Huck wrote:
> >>> - Driver #2 uses the device without doing a reset or proper setup
> >> Without this patch, even if driver #2 do a reset, it can still use the 
> >> old map if it don't set queue pfn.
> > 
> > Yes, the cleanup-on-reset is definetly needed.
> 
> It is good to have for defensiveness, but it would still cause a
> segfault so we should also add the checks on vq->vring.desc throughout.

Agreed.

Re: [Qemu-devel] [PATCH] virtio: destroy region cache during reset

[Jason Wang]

On 2017年03月09日 19:07, Cornelia Huck wrote:
> On Thu, 9 Mar 2017 10:19:47 +0800
> Jason Wang<jasowang@redhat.com>  wrote:
>
>> On 2017年03月08日 18:12, Cornelia Huck wrote:
>>> On Wed, 8 Mar 2017 17:51:22 +0800
>>> Jason Wang<jasowang@redhat.com>  wrote:
>>>
>>>> On 2017年03月08日 17:19, Cornelia Huck wrote:
>>>>> On Wed, 8 Mar 2017 11:18:27 +0800
>>>>> Jason Wang<jasowang@redhat.com>  wrote:
>>>>>
>>>>>> On 2017年03月07日 18:16, Cornelia Huck wrote:
>>>>>>> On Tue,  7 Mar 2017 16:47:58 +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
>>>>>>>> use them. While at it, also validate address_space_cache_init() during
>>>>>>>> virtio_init_region_cache() to make sure we have a correct region
>>>>>>>> cache.
>>>>>>>>
>>>>>>>> Signed-off-by: Jason Wang<jasowang@redhat.com>
>>>>>>>> ---
>>>>>>>>      hw/virtio/virtio.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++--------
>>>>>>>>      1 file changed, 76 insertions(+), 12 deletions(-)
>>>>>>>> @@ -190,6 +211,10 @@ static inline uint16_t vring_avail_flags(VirtQueue *vq)
>>>>>>>>      {
>>>>>>>>          VRingMemoryRegionCaches *caches = atomic_rcu_read(&vq->vring.caches);
>>>>>>>>          hwaddr pa = offsetof(VRingAvail, flags);
>>>>>>>> +    if (!caches) {
>>>>>>>> +        virtio_error(vq->vdev, "Cannot map avail flags");
>>>>>>> I'm not sure that virtio_error is the right thing here; ending up in
>>>>>>> this function with !caches indicates an error in our logic.
>>>>>> Probably not, this can be triggered by buggy guest.
>>>>> I would think that even a buggy guest should not be able to trigger
>>>>> accesses to vring structures that have not yet been set up. What am I
>>>>> missing?
>>>> I think this may happen if a driver start the device without setting the
>>>> vring address. In this case, region caches cache the mapping of previous
>>>> driver. But maybe I was wrong.
>>> So the sequence would be:
>>>
>>> - Driver #1 sets up the device, then abandons it without cleaning up
>>> via reset
>> If driver #1 reset the device in this case, the caches would be NULL.
> Hm, how? Without your patch, the queue addresses are reset to 0 in that
> case but the cache is not cleaned up.
>

Yes, but with this patch, caches will be set to NULL during reset. So 
the following vq access without seting queue pfn may hit NULL.

Thanks

Re: [Qemu-devel] [PATCH] virtio: destroy region cache during reset

[Paolo Bonzini]

On 07/03/2017 09:47, Jason Wang 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.

I'm still not sure as to how this can happen.  Reset does clear
desc/used/avail, which should then be checked before accessing the caches.

Paolo

> Fix this by
> destroy the region cache during reset and validate it before trying to
> use them. While at it, also validate address_space_cache_init() during
> virtio_init_region_cache() to make sure we have a correct region
> cache.

Re: [Qemu-devel] [PATCH] virtio: destroy region cache during reset

[Jason Wang]

[-- Attachment #1: Type: text/plain, Size: 825 bytes --]



On 2017年03月07日 18:55, Paolo Bonzini wrote:
>
> On 07/03/2017 09:47, Jason Wang 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.
> I'm still not sure as to how this can happen.  Reset does clear
> desc/used/avail, which should then be checked before accessing the caches.

But the code does not check them in fact? (E.g the attached qtest patch 
can still pass check-qtest).

Thanks

>
> Paolo
>
>> Fix this by
>> destroy the region cache during reset and validate it before trying to
>> use them. While at it, also validate address_space_cache_init() during
>> virtio_init_region_cache() to make sure we have a correct region
>> cache.


[-- Attachment #2: 0001-virtio-net-qtest-for-region-cache.patch --]
[-- Type: text/x-patch, Size: 5409 bytes --]

>From 8f588662d439c74e4da3924464167d2be330fd66 Mon Sep 17 00:00:00 2001
From: Jason Wang <jasowang@redhat.com>
Date: Wed, 8 Mar 2017 11:19:37 +0800
Subject: [PATCH] virtio-net: qtest for region cache

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 tests/libqos/virtio-pci.c | 21 ++++++++++++++++++---
 tests/libqos/virtio.c     |  8 ++++++++
 tests/libqos/virtio.h     |  6 ++++++
 tests/virtio-net-test.c   |  3 +++
 4 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/tests/libqos/virtio-pci.c b/tests/libqos/virtio-pci.c
index 7ac15c0..6273635 100644
--- a/tests/libqos/virtio-pci.c
+++ b/tests/libqos/virtio-pci.c
@@ -222,8 +222,9 @@ static void qvirtio_pci_set_queue_address(QVirtioDevice *d, uint32_t pfn)
     qpci_io_writel(dev->pdev, dev->bar, VIRTIO_PCI_QUEUE_PFN, pfn);
 }
 
-static QVirtQueue *qvirtio_pci_virtqueue_setup(QVirtioDevice *d,
-                                        QGuestAllocator *alloc, uint16_t index)
+static QVirtQueue *__qvirtio_pci_virtqueue_setup(QVirtioDevice *d,
+                                                 QGuestAllocator *alloc, uint16_t index,
+                                                 bool set_pfn)
 {
     uint32_t feat;
     uint64_t addr;
@@ -254,11 +255,24 @@ static QVirtQueue *qvirtio_pci_virtqueue_setup(QVirtioDevice *d,
     addr = guest_alloc(alloc, qvring_size(vqpci->vq.size,
                                           VIRTIO_PCI_VRING_ALIGN));
     qvring_init(alloc, &vqpci->vq, addr);
-    qvirtio_pci_set_queue_address(d, vqpci->vq.desc / VIRTIO_PCI_VRING_ALIGN);
+    if (set_pfn)
+        qvirtio_pci_set_queue_address(d, vqpci->vq.desc / VIRTIO_PCI_VRING_ALIGN);
 
     return &vqpci->vq;
 }
 
+static QVirtQueue *qvirtio_pci_virtqueue_setup(QVirtioDevice *d,
+                                        QGuestAllocator *alloc, uint16_t index)
+{
+    return __qvirtio_pci_virtqueue_setup(d, alloc, index, true);
+}
+
+static QVirtQueue *qvirtio_pci_virtqueue_setup2(QVirtioDevice *d,
+                                        QGuestAllocator *alloc, uint16_t index)
+{
+    return __qvirtio_pci_virtqueue_setup(d, alloc, index, false);
+}
+
 static void qvirtio_pci_virtqueue_cleanup(QVirtQueue *vq,
                                           QGuestAllocator *alloc)
 {
@@ -290,6 +304,7 @@ const QVirtioBus qvirtio_pci = {
     .get_queue_size = qvirtio_pci_get_queue_size,
     .set_queue_address = qvirtio_pci_set_queue_address,
     .virtqueue_setup = qvirtio_pci_virtqueue_setup,
+    .virtqueue_setup2 = qvirtio_pci_virtqueue_setup2,
     .virtqueue_cleanup = qvirtio_pci_virtqueue_cleanup,
     .virtqueue_kick = qvirtio_pci_virtqueue_kick,
 };
diff --git a/tests/libqos/virtio.c b/tests/libqos/virtio.c
index ec30cb9..def910f 100644
--- a/tests/libqos/virtio.c
+++ b/tests/libqos/virtio.c
@@ -49,6 +49,12 @@ QVirtQueue *qvirtqueue_setup(QVirtioDevice *d,
     return d->bus->virtqueue_setup(d, alloc, index);
 }
 
+QVirtQueue *qvirtqueue_setup2(QVirtioDevice *d,
+                             QGuestAllocator *alloc, uint16_t index)
+{
+    return d->bus->virtqueue_setup(d, alloc, index);
+}
+
 void qvirtqueue_cleanup(const QVirtioBus *bus, QVirtQueue *vq,
                         QGuestAllocator *alloc)
 {
@@ -77,8 +83,10 @@ void qvirtio_set_driver(QVirtioDevice *d)
 void qvirtio_set_driver_ok(QVirtioDevice *d)
 {
     d->bus->set_status(d, d->bus->get_status(d) | VIRTIO_CONFIG_S_DRIVER_OK);
+    #if 0
     g_assert_cmphex(d->bus->get_status(d), ==, VIRTIO_CONFIG_S_DRIVER_OK |
                     VIRTIO_CONFIG_S_DRIVER | VIRTIO_CONFIG_S_ACKNOWLEDGE);
+    #endif
 }
 
 void qvirtio_wait_queue_isr(QVirtioDevice *d,
diff --git a/tests/libqos/virtio.h b/tests/libqos/virtio.h
index 3397a08..2c194b9 100644
--- a/tests/libqos/virtio.h
+++ b/tests/libqos/virtio.h
@@ -82,6 +82,10 @@ struct QVirtioBus {
     QVirtQueue *(*virtqueue_setup)(QVirtioDevice *d, QGuestAllocator *alloc,
                                                                 uint16_t index);
 
+  /* Setup the virtqueue specified by index */
+    QVirtQueue *(*virtqueue_setup2)(QVirtioDevice *d, QGuestAllocator *alloc,
+                                                                uint16_t index);
+
     /* Free virtqueue resources */
     void (*virtqueue_cleanup)(QVirtQueue *vq, QGuestAllocator *alloc);
 
@@ -123,6 +127,8 @@ uint8_t qvirtio_wait_status_byte_no_isr(QVirtioDevice *d,
 void qvirtio_wait_config_isr(QVirtioDevice *d, gint64 timeout_us);
 QVirtQueue *qvirtqueue_setup(QVirtioDevice *d,
                              QGuestAllocator *alloc, uint16_t index);
+QVirtQueue *qvirtqueue_setup2(QVirtioDevice *d,
+			      QGuestAllocator *alloc, uint16_t index);
 void qvirtqueue_cleanup(const QVirtioBus *bus, QVirtQueue *vq,
                         QGuestAllocator *alloc);
 
diff --git a/tests/virtio-net-test.c b/tests/virtio-net-test.c
index 8f94360..df54b19 100644
--- a/tests/virtio-net-test.c
+++ b/tests/virtio-net-test.c
@@ -224,6 +224,9 @@ static void pci_basic(gconstpointer data)
 
     rx = (QVirtQueuePCI *)qvirtqueue_setup(&dev->vdev, qs->alloc, 0);
     tx = (QVirtQueuePCI *)qvirtqueue_setup(&dev->vdev, qs->alloc, 1);
+    qvirtio_reset(&dev->vdev);
+    rx = (QVirtQueuePCI *)qvirtqueue_setup2(&dev->vdev, qs->alloc, 0);
+    tx = (QVirtQueuePCI *)qvirtqueue_setup2(&dev->vdev, qs->alloc, 1);
 
     driver_init(&dev->vdev);
     func(&dev->vdev, qs->alloc, &rx->vq, &tx->vq, sv[0]);
-- 
2.7.4

Re: [Qemu-devel] [PATCH] virtio: destroy region cache during reset

[Jason Wang]

On 2017年03月08日 11:21, Jason Wang wrote:
>
> On 2017年03月07日 18:55, Paolo Bonzini wrote:
>>
>> On 07/03/2017 09:47, Jason Wang 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.
>> I'm still not sure as to how this can happen.  Reset does clear
>> desc/used/avail, which should then be checked before accessing the 
>> caches.
>
> But the code does not check them in fact? (E.g the attached qtest 
> patch can still pass check-qtest).
>
> Thanks 

Ok, the reproducer seems wrong. And I think what you mean is something 
like the check done in virtio_queue_ready(). But looks like not all 
virtqueue check for this. One example is virtio_net_handle_ctrl(), and 
there may be even more. So you want to fix them all?

Thanks

Re: [Qemu-devel] [PATCH] virtio: destroy region cache during reset

[Paolo Bonzini]

----- Original Message -----
> From: "Jason Wang" <jasowang@redhat.com>
> To: "Paolo Bonzini" <pbonzini@redhat.com>, mst@redhat.com, qemu-devel@nongnu.org
> Cc: peterx@redhat.com
> Sent: Wednesday, March 8, 2017 7:22:06 AM
> Subject: Re: [Qemu-devel] [PATCH] virtio: destroy region cache during reset
> 
> 
> 
> On 2017年03月08日 11:21, Jason Wang wrote:
> >
> > On 2017年03月07日 18:55, Paolo Bonzini wrote:
> >>
> >> On 07/03/2017 09:47, Jason Wang 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.
> >> I'm still not sure as to how this can happen.  Reset does clear
> >> desc/used/avail, which should then be checked before accessing the
> >> caches.
> >
> > But the code does not check them in fact? (E.g the attached qtest
> > patch can still pass check-qtest).
> >
> > Thanks
> 
> Ok, the reproducer seems wrong. And I think what you mean is something
> like the check done in virtio_queue_ready(). But looks like not all
> virtqueue check for this. One example is virtio_net_handle_ctrl(), and
> there may be even more. So you want to fix them all?

Why would virtio_net_handle_ctrl be called when desc == 0?  The checks
are all in common virtio code.

static void virtio_queue_notify_vq(VirtQueue *vq)
{
    if (vq->vring.desc && vq->handle_output) {
        VirtIODevice *vdev = vq->vdev;

        if (unlikely(vdev->broken)) {
            return;
        }

        trace_virtio_queue_notify(vdev, vq - vdev->vq, vq);
        vq->handle_output(vdev, vq);
    }
}
                                                                                                                                                                               1440,29       55%
Paolo

Re: [Qemu-devel] [PATCH] virtio: destroy region cache during reset

[Jason Wang]

On 2017年03月08日 17:10, Paolo Bonzini wrote:
>
> ----- Original Message -----
>> From: "Jason Wang" <jasowang@redhat.com>
>> To: "Paolo Bonzini" <pbonzini@redhat.com>, mst@redhat.com, qemu-devel@nongnu.org
>> Cc: peterx@redhat.com
>> Sent: Wednesday, March 8, 2017 7:22:06 AM
>> Subject: Re: [Qemu-devel] [PATCH] virtio: destroy region cache during reset
>>
>>
>>
>> On 2017年03月08日 11:21, Jason Wang wrote:
>>> On 2017年03月07日 18:55, Paolo Bonzini wrote:
>>>> On 07/03/2017 09:47, Jason Wang 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.
>>>> I'm still not sure as to how this can happen.  Reset does clear
>>>> desc/used/avail, which should then be checked before accessing the
>>>> caches.
>>> But the code does not check them in fact? (E.g the attached qtest
>>> patch can still pass check-qtest).
>>>
>>> Thanks
>> Ok, the reproducer seems wrong. And I think what you mean is something
>> like the check done in virtio_queue_ready(). But looks like not all
>> virtqueue check for this. One example is virtio_net_handle_ctrl(), and
>> there may be even more. So you want to fix them all?
> Why would virtio_net_handle_ctrl be called when desc == 0?  The checks
> are all in common virtio code.
>
> static void virtio_queue_notify_vq(VirtQueue *vq)
> {
>      if (vq->vring.desc && vq->handle_output) {
>          VirtIODevice *vdev = vq->vdev;
>
>          if (unlikely(vdev->broken)) {
>              return;
>          }
>
>          trace_virtio_queue_notify(vdev, vq - vdev->vq, vq);
>          vq->handle_output(vdev, vq);
>      }
> }
>                                                                                                                                                                                 1440,29       55%
> Paolo

Right, I miss this.

But I find two possible leaks by auditing the callers of virtqueue_pop():

virtio_input_send() and virtio_balloon_set_status() which will call 
virtio_balloon_receive_stats().

Thanks

Re: [Qemu-devel] [PATCH] virtio: destroy region cache during reset

[Paolo Bonzini]

On 08/03/2017 10:48, Jason Wang wrote:
> 
> 
> On 2017年03月08日 17:10, Paolo Bonzini wrote:
>>
>> ----- Original Message -----
>>> From: "Jason Wang" <jasowang@redhat.com>
>>> To: "Paolo Bonzini" <pbonzini@redhat.com>, mst@redhat.com,
>>> qemu-devel@nongnu.org
>>> Cc: peterx@redhat.com
>>> Sent: Wednesday, March 8, 2017 7:22:06 AM
>>> Subject: Re: [Qemu-devel] [PATCH] virtio: destroy region cache during
>>> reset
>>>
>>>
>>>
>>> On 2017年03月08日 11:21, Jason Wang wrote:
>>>> On 2017年03月07日 18:55, Paolo Bonzini wrote:
>>>>> On 07/03/2017 09:47, Jason Wang 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.
>>>>> I'm still not sure as to how this can happen.  Reset does clear
>>>>> desc/used/avail, which should then be checked before accessing the
>>>>> caches.
>>>> But the code does not check them in fact? (E.g the attached qtest
>>>> patch can still pass check-qtest).
>>>>
>>>> Thanks
>>> Ok, the reproducer seems wrong. And I think what you mean is something
>>> like the check done in virtio_queue_ready(). But looks like not all
>>> virtqueue check for this. One example is virtio_net_handle_ctrl(), and
>>> there may be even more. So you want to fix them all?
>> Why would virtio_net_handle_ctrl be called when desc == 0?  The checks
>> are all in common virtio code.

> 
> Right, I miss this.
> 
> But I find two possible leaks by auditing the callers of virtqueue_pop():
> 
> virtio_input_send() and virtio_balloon_set_status() which will call
> virtio_balloon_receive_stats().

Ok, this is good!  I think we should check vq->vring.desc in
virtio_queue_empty and virtio_queue_empty_rcu, and that should do it.
It will cover virtqueue_pop too.

virtqueue_get_avail_bytes is always preceded or followed by
virtio_queue_empty, virtio_queue_ready or virtqueue_pop, but perhaps we
should add a check there too.

Paolo

Paolo

Re: [Qemu-devel] [PATCH] virtio: destroy region cache during reset

[Cornelia Huck]

On Thu, 9 Mar 2017 12:10:44 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 08/03/2017 10:48, Jason Wang wrote:
> > 
> > 
> > On 2017年03月08日 17:10, Paolo Bonzini wrote:
> >>
> >> ----- Original Message -----
> >>> From: "Jason Wang" <jasowang@redhat.com>
> >>> To: "Paolo Bonzini" <pbonzini@redhat.com>, mst@redhat.com,
> >>> qemu-devel@nongnu.org
> >>> Cc: peterx@redhat.com
> >>> Sent: Wednesday, March 8, 2017 7:22:06 AM
> >>> Subject: Re: [Qemu-devel] [PATCH] virtio: destroy region cache during
> >>> reset
> >>>
> >>>
> >>>
> >>> On 2017年03月08日 11:21, Jason Wang wrote:
> >>>> On 2017年03月07日 18:55, Paolo Bonzini wrote:
> >>>>> On 07/03/2017 09:47, Jason Wang 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.
> >>>>> I'm still not sure as to how this can happen.  Reset does clear
> >>>>> desc/used/avail, which should then be checked before accessing the
> >>>>> caches.
> >>>> But the code does not check them in fact? (E.g the attached qtest
> >>>> patch can still pass check-qtest).
> >>>>
> >>>> Thanks
> >>> Ok, the reproducer seems wrong. And I think what you mean is something
> >>> like the check done in virtio_queue_ready(). But looks like not all
> >>> virtqueue check for this. One example is virtio_net_handle_ctrl(), and
> >>> there may be even more. So you want to fix them all?
> >> Why would virtio_net_handle_ctrl be called when desc == 0?  The checks
> >> are all in common virtio code.
> 
> > 
> > Right, I miss this.
> > 
> > But I find two possible leaks by auditing the callers of virtqueue_pop():
> > 
> > virtio_input_send() and virtio_balloon_set_status() which will call
> > virtio_balloon_receive_stats().
> 
> Ok, this is good!  I think we should check vq->vring.desc in
> virtio_queue_empty and virtio_queue_empty_rcu, and that should do it.
> It will cover virtqueue_pop too.

Yes, virtio_queue_empty{_rcu} should check for !desc.

> virtqueue_get_avail_bytes is always preceded or followed by
> virtio_queue_empty, virtio_queue_ready or virtqueue_pop, but perhaps we
> should add a check there too.

virtqueue_get_avail_bytes is an exported interface, so I think it
should check as well.

virtqueue_fill and virtqueue_flush as well, I think. Better safe than
sorry.

Re: [Qemu-devel] [PATCH] virtio: destroy region cache during reset

[Cornelia Huck]

On Wed, 8 Mar 2017 14:22:06 +0800
Jason Wang <jasowang@redhat.com> wrote:

> On 2017年03月08日 11:21, Jason Wang wrote:
> >
> > On 2017年03月07日 18:55, Paolo Bonzini wrote:
> >>
> >> On 07/03/2017 09:47, Jason Wang 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.
> >> I'm still not sure as to how this can happen.  Reset does clear
> >> desc/used/avail, which should then be checked before accessing the 
> >> caches.
> >
> > But the code does not check them in fact? (E.g the attached qtest 
> > patch can still pass check-qtest).
> >
> > Thanks 
> 
> Ok, the reproducer seems wrong. And I think what you mean is something 
> like the check done in virtio_queue_ready(). But looks like not all 
> virtqueue check for this. One example is virtio_net_handle_ctrl(), and 

Shouldn't the check for desc in virtio_queue_notify_vq() already take
care of that?

> there may be even more. So you want to fix them all?

Obviously not speaking for Paolo, but I think the virtio core should
have be audited for missing guards.

Re: [Qemu-devel] [PATCH] virtio: destroy region cache during reset

[Jason Wang]

On 2017年03月08日 17:30, Cornelia Huck wrote:
> On Wed, 8 Mar 2017 14:22:06 +0800
> Jason Wang <jasowang@redhat.com> wrote:
>
>> On 2017年03月08日 11:21, Jason Wang wrote:
>>> On 2017年03月07日 18:55, Paolo Bonzini wrote:
>>>> On 07/03/2017 09:47, Jason Wang 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.
>>>> I'm still not sure as to how this can happen.  Reset does clear
>>>> desc/used/avail, which should then be checked before accessing the
>>>> caches.
>>> But the code does not check them in fact? (E.g the attached qtest
>>> patch can still pass check-qtest).
>>>
>>> Thanks
>> Ok, the reproducer seems wrong. And I think what you mean is something
>> like the check done in virtio_queue_ready(). But looks like not all
>> virtqueue check for this. One example is virtio_net_handle_ctrl(), and
> Shouldn't the check for desc in virtio_queue_notify_vq() already take
> care of that?

Yes, I miss this.

>
>> there may be even more. So you want to fix them all?
> Obviously not speaking for Paolo, but I think the virtio core should
> have be audited for missing guards.
>

Probably, otherwise we need a careful auditing of all callers of caches.

Thanks

Re: [Qemu-devel] [PATCH] virtio: destroy region cache during reset

[Cornelia Huck]

On Wed, 8 Mar 2017 17:53:23 +0800
Jason Wang <jasowang@redhat.com> wrote:

> On 2017年03月08日 17:30, Cornelia Huck wrote:
> > On Wed, 8 Mar 2017 14:22:06 +0800
> > Jason Wang <jasowang@redhat.com> wrote:

> >> there may be even more. So you want to fix them all?
> > Obviously not speaking for Paolo, but I think the virtio core should
> > have be audited for missing guards.
> >
> 
> Probably, otherwise we need a careful auditing of all callers of caches.

Yes, especially as all direct calls to the caches are localized in the
core anyway.