Re: [Qemu-devel] [PATCH] virtio: destroy region cache during reset

[Jason Wang]

On 2017年03月08日 18:12, Cornelia Huck wrote:
> On Wed, 8 Mar 2017 17:51:22 +0800
> Jason Wang <address@hidden> wrote:
>
> > On 2017年03月08日 17:19, Cornelia Huck wrote:
> > > On Wed, 8 Mar 2017 11:18:27 +0800
> > > Jason Wang <address@hidden> wrote:
> > >
> > > > On 2017年03月07日 18:16, Cornelia Huck wrote:
> > > > > On Tue,  7 Mar 2017 16:47:58 +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
> > > > > > use them. While at it, also validate address_space_cache_init() during
> > > > > > virtio_init_region_cache() to make sure we have a correct region
> > > > > > cache.
> > > > > >
> > > > > > Signed-off-by: Jason Wang <address@hidden>
> > > > > > ---
> > > > > >     hw/virtio/virtio.c | 88 
> > > > > > ++++++++++++++++++++++++++++++++++++++++++++++--------
> > > > > >     1 file changed, 76 insertions(+), 12 deletions(-)
> > > > > > @@ -190,6 +211,10 @@ static inline uint16_t vring_avail_flags(VirtQueue *vq)
> > > > > >     {
> > > > > >         VRingMemoryRegionCaches *caches = atomic_rcu_read(&vq->vring.caches);
> > > > > >         hwaddr pa = offsetof(VRingAvail, flags);
> > > > > > +    if (!caches) {
> > > > > > +        virtio_error(vq->vdev, "Cannot map avail flags");
> > > > > I'm not sure that virtio_error is the right thing here; ending up in
> > > > > this function with !caches indicates an error in our logic.
> > > > Probably not, this can be triggered by buggy guest.
> > > I would think that even a buggy guest should not be able to trigger
> > > accesses to vring structures that have not yet been set up. What am I
> > > missing?
> > I think this may happen if a driver start the device without setting the
> > vring address. In this case, region caches cache the mapping of previous
> > driver. But maybe I was wrong.
> So the sequence would be:
>
> - Driver #1 sets up the device, then abandons it without cleaning up
> via reset

If driver #1 reset the device in this case, the caches would be NULL.

> - Driver #2 uses the device without doing a reset or proper setup

Without this patch, even if driver #2 do a reset, it can still use the 
old map if it don't set queue pfn.

> ?
>
> I can't quite see why caches would be NULL in that case...
>
> Having reset clean up the caches as well (IOW, the other part of your
> patch) should make sure that we always have a consistent view of
> descriptors and their caches,

And prevent it from being leaked to untrusted drivers.

Thanks

> I think. The checks for desc != NULL and
> friends should catch other driver buggyness.