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

[Cornelia Huck]

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
- Driver #2 uses the device without doing a reset or proper setup

?

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, I think. The checks for desc != NULL and
friends should catch other driver buggyness.