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

[Cornelia Huck]

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
harm.

> +    }
>      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.

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