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

[Jason Wang]

On 2017年03月08日 17:10, Paolo Bonzini wrote:

----- Original Message -----
From: "Jason Wang" <jasow...@redhat.com>
To: "Paolo Bonzini" <pbonz...@redhat.com>, m...@redhat.com, 
qemu-devel@nongnu.org
Cc: pet...@redhat.com
Sent: Wednesday, March 8, 2017 7:22:06 AM
Subject: Re: [Qemu-devel] [PATCH] virtio: destroy region cache during reset



On 2017年03月08日 11:21, Jason Wang wrote:
On 2017年03月07日 18:55, Paolo Bonzini wrote:
On 07/03/2017 09:47, Jason Wang 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.
I'm still not sure as to how this can happen.  Reset does clear
desc/used/avail, which should then be checked before accessing the
caches.
But the code does not check them in fact? (E.g the attached qtest
patch can still pass check-qtest).

Thanks
Ok, the reproducer seems wrong. And I think what you mean is something
like the check done in virtio_queue_ready(). But looks like not all
virtqueue check for this. One example is virtio_net_handle_ctrl(), and
there may be even more. So you want to fix them all?
Why would virtio_net_handle_ctrl be called when desc == 0?  The checks
are all in common virtio code.

static void virtio_queue_notify_vq(VirtQueue *vq)
{
     if (vq->vring.desc && vq->handle_output) {
         VirtIODevice *vdev = vq->vdev;

         if (unlikely(vdev->broken)) {
             return;
         }

         trace_virtio_queue_notify(vdev, vq - vdev->vq, vq);
         vq->handle_output(vdev, vq);
     }
}
                                                                                
                                                                                
                1440,29       55%
Paolo

Right, I miss this.

But I find two possible leaks by auditing the callers of virtqueue_pop():

virtio_input_send() and virtio_balloon_set_status() which will call 
virtio_balloon_receive_stats().

Thanks