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

[Cornelia Huck]

On Thu, 9 Mar 2017 10:19:47 +0800
Jason Wang <address@hidden> wrote:

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

Hm, how? Without your patch, the queue addresses are reset to 0 in that
case but the cache is not cleaned up.

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

Yes, the cleanup-on-reset is definetly needed.

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

And that as well, agreed.

I'm still not sure why the checks for !caches help, though...