Re: [PATCH v2 4/5] vhost: check vring address before calling unmap

[Dima Stepanov]

On Mon, May 11, 2020 at 11:05:58AM +0800, Jason Wang wrote:
> 
> On 2020/4/30 下午9:36, Dima Stepanov wrote:
> >Since disconnect can happen at any time during initialization not all
> >vring buffers (for instance used vring) can be intialized successfully.
> >If the buffer was not initialized then vhost_memory_unmap call will lead
> >to SIGSEGV. Add checks for the vring address value before calling unmap.
> >Also add assert() in the vhost_memory_unmap() routine.
> >
> >Signed-off-by: Dima Stepanov <address@hidden>
> >---
> >  hw/virtio/vhost.c | 27 +++++++++++++++++++++------
> >  1 file changed, 21 insertions(+), 6 deletions(-)
> >
> >diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> >index ddbdc53..3ee50c4 100644
> >--- a/hw/virtio/vhost.c
> >+++ b/hw/virtio/vhost.c
> >@@ -314,6 +314,8 @@ static void vhost_memory_unmap(struct vhost_dev *dev, 
> >void *buffer,
> >                                 hwaddr len, int is_write,
> >                                 hwaddr access_len)
> >  {
> >+    assert(buffer);
> >+
> >      if (!vhost_dev_has_iommu(dev)) {
> >          cpu_physical_memory_unmap(buffer, len, is_write, access_len);
> >      }
> >@@ -1132,12 +1134,25 @@ static void vhost_virtqueue_stop(struct vhost_dev 
> >*dev,
> >                                                  vhost_vq_index);
> >      }
> >-    vhost_memory_unmap(dev, vq->used, virtio_queue_get_used_size(vdev, idx),
> >-                       1, virtio_queue_get_used_size(vdev, idx));
> >-    vhost_memory_unmap(dev, vq->avail, virtio_queue_get_avail_size(vdev, 
> >idx),
> >-                       0, virtio_queue_get_avail_size(vdev, idx));
> >-    vhost_memory_unmap(dev, vq->desc, virtio_queue_get_desc_size(vdev, idx),
> >-                       0, virtio_queue_get_desc_size(vdev, idx));
> >+    /*
> >+     * Since the vhost-user disconnect can happen during initialization
> >+     * check if vring was initialized, before making unmap.
> >+     */
> >+    if (vq->used) {
> >+        vhost_memory_unmap(dev, vq->used,
> >+                           virtio_queue_get_used_size(vdev, idx),
> >+                           1, virtio_queue_get_used_size(vdev, idx));
> >+    }
> >+    if (vq->avail) {
> >+        vhost_memory_unmap(dev, vq->avail,
> >+                           virtio_queue_get_avail_size(vdev, idx),
> >+                           0, virtio_queue_get_avail_size(vdev, idx));
> >+    }
> >+    if (vq->desc) {
> >+        vhost_memory_unmap(dev, vq->desc,
> >+                           virtio_queue_get_desc_size(vdev, idx),
> >+                           0, virtio_queue_get_desc_size(vdev, idx));
> >+    }
> 
> 
> Any reason not checking hdev->started instead? vhost_dev_start() will set it
> to true if virtqueues were correctly mapped.
> 
> Thanks
Well i see it a little bit different:
 - vhost_dev_start() sets hdev->started to true before starting
   virtqueues
 - vhost_virtqueue_start() maps all the memory
If we hit the vhost disconnect at the start of the
vhost_virtqueue_start(), for instance for this call:
  r = dev->vhost_ops->vhost_set_vring_base(dev, &state);
Then we will call vhost_user_blk_disconnect:
  vhost_user_blk_disconnect()->
    vhost_user_blk_stop()->
      vhost_dev_stop()->
        vhost_virtqueue_stop()
As a result we will come in this routine with the hdev->started still
set to true, but if used/avail/desc fields still uninitialized and set
to 0.

> 
> 
> >  }
> >  static void vhost_eventfd_add(MemoryListener *listener,
>