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

[Jason Wang]

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

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.

Thanks

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