[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH V2 2/3] virtio: destroy region cache during rese
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [PATCH V2 2/3] virtio: destroy region cache during reset |
Date: |
Mon, 13 Mar 2017 11:20:02 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 |
On 13/03/2017 11:05, Cornelia Huck wrote:
> On Mon, 13 Mar 2017 14:29:42 +0800
> Jason Wang <address@hidden> 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
>> see them.
>>
>> Cc: Cornelia Huck <address@hidden>
>> Cc: Paolo Bonzini <address@hidden>
>> Signed-off-by: Jason Wang <address@hidden>
>> ---
>> Changes from v1:
>> - switch to use rcu in virtio_virtqueue_region_cache()
>> - use unlikely() when needed
>> ---
>> hw/virtio/virtio.c | 60
>> +++++++++++++++++++++++++++++++++++++++++++++++-------
>> 1 file changed, 53 insertions(+), 7 deletions(-)
>>
>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>> index 76cc81b..f086452 100644
>> --- a/hw/virtio/virtio.c
>> +++ b/hw/virtio/virtio.c
>> @@ -190,6 +190,10 @@ static inline uint16_t vring_avail_flags(VirtQueue *vq)
>> {
>> VRingMemoryRegionCaches *caches = atomic_rcu_read(&vq->vring.caches);
>> hwaddr pa = offsetof(VRingAvail, flags);
>> + if (unlikely(!caches)) {
>> + virtio_error(vq->vdev, "Cannot map avail flags");
>> + return 0;
>
> I'm still not 100% convinced of those checks; but they don't do any
> harm.
Same here... We would be hiding a bug.
>> + }
>> return virtio_lduw_phys_cached(vq->vdev, &caches->avail, pa);
>> }
>>
>
> (...)
>
>> +static void virtio_virtqueue_reset_region_cache(struct VirtQueue *vq)
>> +{
>> + VRingMemoryRegionCaches *caches;
>> +
>> + caches = atomic_read(&vq->vring.caches);
>> + atomic_set(&vq->vring.caches, NULL);
>
> Needs atomic_rcu_set(), I think.
Not necessarily, see kernel rcu_assign_pointer vs. RCU_INIT_POINTER.
But it's probably easier to use it.
Paolo
>> + if (caches) {
>> + call_rcu(caches, virtio_free_region_cache, rcu);
>> + }
>> +}
>> +
>> void virtio_reset(void *opaque)
>> {
>> VirtIODevice *vdev = opaque;
>