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

[Jason Wang]

On 2020/5/11 下午5:11, Dima Stepanov wrote:
> 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.


I may miss something, but consider both vhost_dev_start() and 
vhost_user_blk_disconnect() were serialized in main loop. Can this 
really happen?

Thanks


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