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

[Cornelia Huck]

On Thu, 9 Mar 2017 12:10:44 +0100
Paolo Bonzini <address@hidden> wrote:

> On 08/03/2017 10:48, Jason Wang wrote:
> > 
> > 
> > On 2017年03月08日 17:10, Paolo Bonzini wrote:
> >>
> >> ----- Original Message -----
> >>> From: "Jason Wang" <address@hidden>
> >>> To: "Paolo Bonzini" <address@hidden>, address@hidden,
> >>> address@hidden
> >>> Cc: address@hidden
> >>> 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.