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

[Jason Wang <jasowang@redhat.com>]

On 2017年03月08日 17:19, Cornelia Huck wrote:
> On Wed, 8 Mar 2017 11:18:27 +0800
> Jason Wang <jasowang@redhat.com> wrote:
>
>> On 2017年03月07日 18:16, Cornelia Huck wrote:
>>> On Tue,  7 Mar 2017 16:47:58 +0800
>>> Jason Wang <jasowang@redhat.com> 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. Fix this by
>>>> destroy the region cache during reset and validate it before trying to
>>>> use them. While at it, also validate address_space_cache_init() during
>>>> virtio_init_region_cache() to make sure we have a correct region
>>>> cache.
>>>>
>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>>> ---
>>>>    hw/virtio/virtio.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++--------
>>>>    1 file changed, 76 insertions(+), 12 deletions(-)
>>>> @@ -190,6 +211,10 @@ static inline uint16_t vring_avail_flags(VirtQueue *vq)
>>>>    {
>>>>        VRingMemoryRegionCaches *caches = atomic_rcu_read(&vq->vring.caches);
>>>>        hwaddr pa = offsetof(VRingAvail, flags);
>>>> +    if (!caches) {
>>>> +        virtio_error(vq->vdev, "Cannot map avail flags");
>>> I'm not sure that virtio_error is the right thing here; ending up in
>>> this function with !caches indicates an error in our logic.
>> Probably not, this can be triggered by buggy guest.
> I would think that even a buggy guest should not be able to trigger
> accesses to vring structures that have not yet been set up. What am I
> missing?

I think this may happen if a driver start the device without setting the 
vring address. In this case, region caches cache the mapping of previous 
driver. But maybe I was wrong.

>
>>> An assert
>>> might be better (and I hope we can sort out all of those errors exposed
>>> by the introduction of region caches for 2.9...)
>> I thought we should avoid assert as much as possible in this case. But
>> if you and maintainer want an assert, it's also fine.
> My personal rule-of-thumb:
> - If it is something that can be triggered by the guest, or it is
> something that is easily recovered, set the device to broken.
> - If it is something that indicates that we messed up our internal
> logic, use an assert.
>
> I think arriving here with !caches indicates the second case; but if
> there is a way for a guest to trigger this, setting the device to
> broken would certainly be better.

Yes, broken seems better.

Thanks