[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Paolo Bonzini
Subject: Re: [Qemu-devel] [PATCH] virtio: destroy region cache during reset
Date: Thu, 9 Mar 2017 12:10:44 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0

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.

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.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]