On Thu, 9 Mar 2017 12:10:44 +0100
Paolo Bonzini <pbonz...@redhat.com> wrote:
> 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.
Yes, virtio_queue_empty{_rcu} should check for !desc.
> 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.
virtqueue_get_avail_bytes is an exported interface, so I think it
should check as well.
virtqueue_fill and virtqueue_flush as well, I think. Better safe than
sorry.
