[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Jason Wang
Subject: Re: [Qemu-devel] [PATCH V2 2/3] virtio: destroy region cache during reset
Date: Tue, 14 Mar 2017 10:13:37 +0800
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0

On 2017年03月13日 18:05, Cornelia Huck wrote:
On Mon, 13 Mar 2017 14:29:42 +0800
Jason Wang <address@hidden> 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 <address@hidden>
Cc: Paolo Bonzini <address@hidden>
Signed-off-by: Jason Wang <address@hidden>
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

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.


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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]