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

[Jason Wang]

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.

> > > An assert
> > > might be better (and I hope we can sort out all of those errors exposed
> > > by the introduction of region caches for 2.9...)
> > I thought we should avoid assert as much as possible in this case. But
> > if you and maintainer want an assert, it's also fine.
> My personal rule-of-thumb:
> - If it is something that can be triggered by the guest, or it is
> something that is easily recovered, set the device to broken.
> - If it is something that indicates that we messed up our internal
> logic, use an assert.
>
> I think arriving here with !caches indicates the second case; but if
> there is a way for a guest to trigger this, setting the device to
> broken would certainly be better.

Yes, broken seems better.

Thanks