On 13/03/2017 11:05, Cornelia Huck wrote: > On Mon, 13 Mar 2017 14:29:42 +0800 > Jason Wang <jasow...@redhat.com> 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 <cornelia.h...@de.ibm.com> >> Cc: Paolo Bonzini <pbonz...@redhat.com> >> Signed-off-by: Jason Wang <jasow...@redhat.com> >> --- >> 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.
Same here... We would be hiding a bug. >> + } >> 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. Not necessarily, see kernel rcu_assign_pointer vs. RCU_INIT_POINTER. But it's probably easier to use it. Paolo >> + if (caches) { >> + call_rcu(caches, virtio_free_region_cache, rcu); >> + } >> +} >> + >> void virtio_reset(void *opaque) >> { >> VirtIODevice *vdev = opaque; >