[Qemu-devel] [PATCH V3 0/3] Several fixes for virito region caches

[Qemu-devel] [PATCH V3 0/3] Several fixes for virito region caches

[Jason Wang]

Hi:

This series contains several fixes for virito region caches, please
review.

Thanks.

Jason Wang (3):
  virtio: guard against NULL pfn
  virtio: destroy region cache during reset
  virtio: validate address space cache during init

 hw/virtio/virtio.c | 105 ++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 83 insertions(+), 22 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH V3 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>
---
Changes from V2:
- return 1 instead of 0 for virtio_queue_empty_*(), and return as
  early as possible
---
 hw/virtio/virtio.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index efce4b3..9164579 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -318,6 +318,10 @@ int virtio_queue_ready(VirtQueue *vq)
  * Called within rcu_read_lock().  */
 static int virtio_queue_empty_rcu(VirtQueue *vq)
 {
+    if (unlikely(!vq->vring.avail)) {
+        return 1;
+    }
+
     if (vq->shadow_avail_idx != vq->last_avail_idx) {
         return 0;
     }
@@ -329,6 +333,10 @@ int virtio_queue_empty(VirtQueue *vq)
 {
     bool empty;
 
+    if (unlikely(!vq->vring.avail)) {
+        return 1;
+    }
+
     if (vq->shadow_avail_idx != vq->last_avail_idx) {
         return 0;
     }
@@ -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,16 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
     int64_t len = 0;
     int rc;
 
+    if (unlikely(!vq->vring.desc)) {
+        if (in_bytes) {
+            *in_bytes = 0;
+        }
+        if (out_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 V3 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 V2:
- introduce a helper and assert caches != NULL
Changes from v1:
- switch to use rcu in virtio_virtqueue_region_cache()
- use unlikely() when needed
---
 hw/virtio/virtio.c | 46 ++++++++++++++++++++++++++++++----------------
 1 file changed, 30 insertions(+), 16 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 9164579..f607a09 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -185,10 +185,16 @@ static void vring_desc_read(VirtIODevice *vdev, VRingDesc *desc,
     virtio_tswap16s(vdev, &desc->next);
 }
 
+static VRingMemoryRegionCaches *vring_get_region_caches(struct VirtQueue *vq)
+{
+    VRingMemoryRegionCaches *caches = atomic_rcu_read(&vq->vring.caches);
+    assert(caches != NULL);
+    return caches;
+}
 /* Called within rcu_read_lock().  */
 static inline uint16_t vring_avail_flags(VirtQueue *vq)
 {
-    VRingMemoryRegionCaches *caches = atomic_rcu_read(&vq->vring.caches);
+    VRingMemoryRegionCaches *caches = vring_get_region_caches(vq);
     hwaddr pa = offsetof(VRingAvail, flags);
     return virtio_lduw_phys_cached(vq->vdev, &caches->avail, pa);
 }
@@ -196,7 +202,7 @@ static inline uint16_t vring_avail_flags(VirtQueue *vq)
 /* Called within rcu_read_lock().  */
 static inline uint16_t vring_avail_idx(VirtQueue *vq)
 {
-    VRingMemoryRegionCaches *caches = atomic_rcu_read(&vq->vring.caches);
+    VRingMemoryRegionCaches *caches = vring_get_region_caches(vq);
     hwaddr pa = offsetof(VRingAvail, idx);
     vq->shadow_avail_idx = virtio_lduw_phys_cached(vq->vdev, &caches->avail, pa);
     return vq->shadow_avail_idx;
@@ -205,7 +211,7 @@ static inline uint16_t vring_avail_idx(VirtQueue *vq)
 /* Called within rcu_read_lock().  */
 static inline uint16_t vring_avail_ring(VirtQueue *vq, int i)
 {
-    VRingMemoryRegionCaches *caches = atomic_rcu_read(&vq->vring.caches);
+    VRingMemoryRegionCaches *caches = vring_get_region_caches(vq);
     hwaddr pa = offsetof(VRingAvail, ring[i]);
     return virtio_lduw_phys_cached(vq->vdev, &caches->avail, pa);
 }
@@ -220,7 +226,7 @@ static inline uint16_t vring_get_used_event(VirtQueue *vq)
 static inline void vring_used_write(VirtQueue *vq, VRingUsedElem *uelem,
                                     int i)
 {
-    VRingMemoryRegionCaches *caches = atomic_rcu_read(&vq->vring.caches);
+    VRingMemoryRegionCaches *caches = vring_get_region_caches(vq);
     hwaddr pa = offsetof(VRingUsed, ring[i]);
     virtio_tswap32s(vq->vdev, &uelem->id);
     virtio_tswap32s(vq->vdev, &uelem->len);
@@ -231,7 +237,7 @@ static inline void vring_used_write(VirtQueue *vq, VRingUsedElem *uelem,
 /* Called within rcu_read_lock().  */
 static uint16_t vring_used_idx(VirtQueue *vq)
 {
-    VRingMemoryRegionCaches *caches = atomic_rcu_read(&vq->vring.caches);
+    VRingMemoryRegionCaches *caches = vring_get_region_caches(vq);
     hwaddr pa = offsetof(VRingUsed, idx);
     return virtio_lduw_phys_cached(vq->vdev, &caches->used, pa);
 }
@@ -239,7 +245,7 @@ static uint16_t vring_used_idx(VirtQueue *vq)
 /* Called within rcu_read_lock().  */
 static inline void vring_used_idx_set(VirtQueue *vq, uint16_t val)
 {
-    VRingMemoryRegionCaches *caches = atomic_rcu_read(&vq->vring.caches);
+    VRingMemoryRegionCaches *caches = vring_get_region_caches(vq);
     hwaddr pa = offsetof(VRingUsed, idx);
     virtio_stw_phys_cached(vq->vdev, &caches->used, pa, val);
     address_space_cache_invalidate(&caches->used, pa, sizeof(val));
@@ -249,11 +255,10 @@ static inline void vring_used_idx_set(VirtQueue *vq, uint16_t val)
 /* Called within rcu_read_lock().  */
 static inline void vring_used_flags_set_bit(VirtQueue *vq, int mask)
 {
-    VRingMemoryRegionCaches *caches = atomic_rcu_read(&vq->vring.caches);
+    VRingMemoryRegionCaches *caches = vring_get_region_caches(vq);
     VirtIODevice *vdev = vq->vdev;
     hwaddr pa = offsetof(VRingUsed, flags);
     uint16_t 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));
 }
@@ -261,7 +266,7 @@ static inline void vring_used_flags_set_bit(VirtQueue *vq, int mask)
 /* Called within rcu_read_lock().  */
 static inline void vring_used_flags_unset_bit(VirtQueue *vq, int mask)
 {
-    VRingMemoryRegionCaches *caches = atomic_rcu_read(&vq->vring.caches);
+    VRingMemoryRegionCaches *caches = vring_get_region_caches(vq);
     VirtIODevice *vdev = vq->vdev;
     hwaddr pa = offsetof(VRingUsed, flags);
     uint16_t flags = virtio_lduw_phys_cached(vq->vdev, &caches->used, pa);
@@ -279,7 +284,7 @@ static inline void vring_set_avail_event(VirtQueue *vq, uint16_t val)
         return;
     }
 
-    caches = atomic_rcu_read(&vq->vring.caches);
+    caches = vring_get_region_caches(vq);
     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));
@@ -577,7 +582,7 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
     total_bufs = in_total = out_total = 0;
 
     max = vq->vring.num;
-    caches = atomic_rcu_read(&vq->vring.caches);
+    caches = vring_get_region_caches(vq);
     if (caches->desc.len < max * sizeof(VRingDesc)) {
         virtio_error(vdev, "Cannot map descriptor ring");
         goto err;
@@ -844,7 +849,7 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz)
 
     i = head;
 
-    caches = atomic_rcu_read(&vq->vring.caches);
+    caches = vring_get_region_caches(vq);
     if (caches->desc.len < max * sizeof(VRingDesc)) {
         virtio_error(vdev, "Cannot map descriptor ring");
         goto done;
@@ -1143,6 +1148,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_rcu_set(&vq->vring.caches, NULL);
+    if (caches) {
+        call_rcu(caches, virtio_free_region_cache, rcu);
+    }
+}
+
 void virtio_reset(void *opaque)
 {
     VirtIODevice *vdev = opaque;
@@ -1183,6 +1199,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]);
     }
 }
 
@@ -2477,13 +2494,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

[Qemu-devel] [PATCH V3 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>
Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.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 f607a09..7714688 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 V3 0/3] Several fixes for virito region caches

[Zhang Chen]

On 03/14/2017 11:01 AM, Jason Wang wrote:
> Hi:
>
> This series contains several fixes for virito region caches, please
> review.

Title and comments typo:

S/virito/virtio

Thanks
Zhang Chen

>
> Thanks.
>
> Jason Wang (3):
>    virtio: guard against NULL pfn
>    virtio: destroy region cache during reset
>    virtio: validate address space cache during init
>
>   hw/virtio/virtio.c | 105 ++++++++++++++++++++++++++++++++++++++++++-----------
>   1 file changed, 83 insertions(+), 22 deletions(-)
>

-- 
Thanks
Zhang Chen

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

[Cornelia Huck]

On Tue, 14 Mar 2017 11:01:40 +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>
> ---
> Changes from V2:
> - return 1 instead of 0 for virtio_queue_empty_*(), and return as
>   early as possible
> ---
>  hw/virtio/virtio.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)

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

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

[Cornelia Huck]

On Tue, 14 Mar 2017 11:01:41 +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 V2:
> - introduce a helper and assert caches != NULL
> Changes from v1:
> - switch to use rcu in virtio_virtqueue_region_cache()
> - use unlikely() when needed
> ---
>  hw/virtio/virtio.c | 46 ++++++++++++++++++++++++++++++----------------
>  1 file changed, 30 insertions(+), 16 deletions(-)
> 

> @@ -249,11 +255,10 @@ static inline void vring_used_idx_set(VirtQueue *vq, uint16_t val)
>  /* Called within rcu_read_lock().  */
>  static inline void vring_used_flags_set_bit(VirtQueue *vq, int mask)
>  {
> -    VRingMemoryRegionCaches *caches = atomic_rcu_read(&vq->vring.caches);
> +    VRingMemoryRegionCaches *caches = vring_get_region_caches(vq);
>      VirtIODevice *vdev = vq->vdev;
>      hwaddr pa = offsetof(VRingUsed, flags);
>      uint16_t flags = virtio_lduw_phys_cached(vq->vdev, &caches->used, pa);
> -

Unrelated whitespace change.

>      virtio_stw_phys_cached(vdev, &caches->used, pa, flags | mask);
>      address_space_cache_invalidate(&caches->used, pa, sizeof(flags));
>  }

Other than that:

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

Re: [Qemu-devel] [PATCH V3 0/3] Several fixes for virito region caches

[Jason Wang]

On 2017年03月14日 16:35, Zhang Chen wrote:
>
>
> On 03/14/2017 11:01 AM, Jason Wang wrote:
>> Hi:
>>
>> This series contains several fixes for virito region caches, please
>> review.
>
> Title and comments typo:
>
> S/virito/virtio
>
> Thanks
> Zhang Chen

Right.

Thanks

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

[Jason Wang]

On 2017年03月14日 17:29, Cornelia Huck wrote:
> On Tue, 14 Mar 2017 11:01:41 +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 V2:
>> - introduce a helper and assert caches != NULL
>> Changes from v1:
>> - switch to use rcu in virtio_virtqueue_region_cache()
>> - use unlikely() when needed
>> ---
>>   hw/virtio/virtio.c | 46 ++++++++++++++++++++++++++++++----------------
>>   1 file changed, 30 insertions(+), 16 deletions(-)
>>
>> @@ -249,11 +255,10 @@ static inline void vring_used_idx_set(VirtQueue *vq, uint16_t val)
>>   /* Called within rcu_read_lock().  */
>>   static inline void vring_used_flags_set_bit(VirtQueue *vq, int mask)
>>   {
>> -    VRingMemoryRegionCaches *caches = atomic_rcu_read(&vq->vring.caches);
>> +    VRingMemoryRegionCaches *caches = vring_get_region_caches(vq);
>>       VirtIODevice *vdev = vq->vdev;
>>       hwaddr pa = offsetof(VRingUsed, flags);
>>       uint16_t flags = virtio_lduw_phys_cached(vq->vdev, &caches->used, pa);
>> -
> Unrelated whitespace change.

Right. If no more comments from any others. I think Michael can probably 
fix this during merge.

Thanks

>
>>       virtio_stw_phys_cached(vdev, &caches->used, pa, flags | mask);
>>       address_space_cache_invalidate(&caches->used, pa, sizeof(flags));
>>   }
> Other than that:
>
> Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
>
>

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

[Michael S. Tsirkin]

On Wed, Mar 15, 2017 at 10:14:48AM +0800, Jason Wang wrote:
> 
> 
> On 2017年03月14日 17:29, Cornelia Huck wrote:
> > On Tue, 14 Mar 2017 11:01:41 +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 V2:
> > > - introduce a helper and assert caches != NULL
> > > Changes from v1:
> > > - switch to use rcu in virtio_virtqueue_region_cache()
> > > - use unlikely() when needed
> > > ---
> > >   hw/virtio/virtio.c | 46 ++++++++++++++++++++++++++++++----------------
> > >   1 file changed, 30 insertions(+), 16 deletions(-)
> > > 
> > > @@ -249,11 +255,10 @@ static inline void vring_used_idx_set(VirtQueue *vq, uint16_t val)
> > >   /* Called within rcu_read_lock().  */
> > >   static inline void vring_used_flags_set_bit(VirtQueue *vq, int mask)
> > >   {
> > > -    VRingMemoryRegionCaches *caches = atomic_rcu_read(&vq->vring.caches);
> > > +    VRingMemoryRegionCaches *caches = vring_get_region_caches(vq);
> > >       VirtIODevice *vdev = vq->vdev;
> > >       hwaddr pa = offsetof(VRingUsed, flags);
> > >       uint16_t flags = virtio_lduw_phys_cached(vq->vdev, &caches->used, pa);
> > > -
> > Unrelated whitespace change.
> 
> Right. If no more comments from any others. I think Michael can probably fix
> this during merge.
> 
> Thanks

I'd rather get patches that apply cleanly. Minor changes mean you
can keep reviewed-by tags.

> > 
> > >       virtio_stw_phys_cached(vdev, &caches->used, pa, flags | mask);
> > >       address_space_cache_invalidate(&caches->used, pa, sizeof(flags));
> > >   }
> > Other than that:
> > 
> > Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> > 
> >