On 08/03/2017 10:48, Jason Wang wrote: > > > 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.
> > 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(). Ok, this is good! I think we should check vq->vring.desc in virtio_queue_empty and virtio_queue_empty_rcu, and that should do it. It will cover virtqueue_pop too. virtqueue_get_avail_bytes is always preceded or followed by virtio_queue_empty, virtio_queue_ready or virtqueue_pop, but perhaps we should add a check there too. Paolo Paolo
