From 72d9196f1ef588c19821d0a4fb563836fdb2a0b7 Mon Sep 17 00:00:00 2001 From: Ben Warren Date: Thu, 2 Mar 2017 13:36:50 -0800 Subject: [PATCH 1/7] Bugfix: Handle error if VM Generation ID device not present This was crashing due to NULL-pointer dereference QMP Test case: ============== (QEMU) query-vm-generation-id {"error": {"class": "GenericError", "desc": "VM Generation ID device not found"}} HMP Test case: ============== virsh # qemu-monitor-command --hmp 3 info vm-generation-id VM Generation ID device not found Signed-off-by: Ben Warren Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin Reviewed-by: Eric Blake --- hmp.c | 4 +++- hw/acpi/vmgenid.c | 1 + 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/hmp.c b/hmp.c index 261843f7a2..edb8970461 100644 --- a/hmp.c +++ b/hmp.c @@ -2608,9 +2608,11 @@ void hmp_hotpluggable_cpus(Monitor *mon, const QDict *qdict) void hmp_info_vm_generation_id(Monitor *mon, const QDict *qdict) { - GuidInfo *info = qmp_query_vm_generation_id(NULL); + Error *err = NULL; + GuidInfo *info = qmp_query_vm_generation_id(&err); if (info) { monitor_printf(mon, "%s\n", info->guid); } + hmp_handle_error(mon, &err); qapi_free_GuidInfo(info); } diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c index 744f2847da..7a3ad17d66 100644 --- a/hw/acpi/vmgenid.c +++ b/hw/acpi/vmgenid.c @@ -248,6 +248,7 @@ GuidInfo *qmp_query_vm_generation_id(Error **errp) Object *obj = find_vmgenid_dev(); if (!obj) { + error_setg(errp, "VM Generation ID device not found"); return NULL; } vms = VMGENID(obj); From 168e4af3c129534ee7d81edc238a9aa9ca0dd474 Mon Sep 17 00:00:00 2001 From: Jason Wang Date: Wed, 15 Mar 2017 19:48:30 +0800 Subject: [PATCH 2/7] virtio: guard against NULL pfn To avoid access stale memory region cache after reset, this patch check the existence of virtqueue pfn for all exported virtqueue access helpers before trying to use them. Cc: Cornelia Huck Cc: Paolo Bonzini Reviewed-by: Cornelia Huck Signed-off-by: Jason Wang Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/virtio/virtio.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index efce4b343a..916457966c 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -318,6 +318,10 @@ int virtio_queue_ready(VirtQueue *vq) * Called within rcu_read_lock(). */ static int virtio_queue_empty_rcu(VirtQueue *vq) { + if (unlikely(!vq->vring.avail)) { + return 1; + } + if (vq->shadow_avail_idx != vq->last_avail_idx) { return 0; } @@ -329,6 +333,10 @@ int virtio_queue_empty(VirtQueue *vq) { bool empty; + if (unlikely(!vq->vring.avail)) { + return 1; + } + if (vq->shadow_avail_idx != vq->last_avail_idx) { return 0; } @@ -431,6 +439,10 @@ void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem, return; } + if (unlikely(!vq->vring.used)) { + return; + } + idx = (idx + vq->used_idx) % vq->vring.num; uelem.id = elem->index; @@ -448,6 +460,10 @@ void virtqueue_flush(VirtQueue *vq, unsigned int count) return; } + if (unlikely(!vq->vring.used)) { + return; + } + /* Make sure buffer is written before we update index. */ smp_wmb(); trace_virtqueue_flush(vq, count); @@ -546,6 +562,16 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes, int64_t len = 0; int rc; + if (unlikely(!vq->vring.desc)) { + if (in_bytes) { + *in_bytes = 0; + } + if (out_bytes) { + *out_bytes = 0; + } + return; + } + rcu_read_lock(); idx = vq->last_avail_idx; total_bufs = in_total = out_total = 0; From e0e2d644096c79a71099b176d08f465f6803a8b1 Mon Sep 17 00:00:00 2001 From: Jason Wang Date: Wed, 15 Mar 2017 19:48:31 +0800 Subject: [PATCH 3/7] virtio: destroy region cache during reset 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 Cc: Paolo Bonzini Reviewed-by: Cornelia Huck Signed-off-by: Jason Wang Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/virtio/virtio.c | 45 ++++++++++++++++++++++++++++++--------------- 1 file changed, 30 insertions(+), 15 deletions(-) diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index 916457966c..a00380fae6 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -185,10 +185,16 @@ static void vring_desc_read(VirtIODevice *vdev, VRingDesc *desc, virtio_tswap16s(vdev, &desc->next); } +static VRingMemoryRegionCaches *vring_get_region_caches(struct VirtQueue *vq) +{ + VRingMemoryRegionCaches *caches = atomic_rcu_read(&vq->vring.caches); + assert(caches != NULL); + return caches; +} /* Called within rcu_read_lock(). */ static inline uint16_t vring_avail_flags(VirtQueue *vq) { - VRingMemoryRegionCaches *caches = atomic_rcu_read(&vq->vring.caches); + VRingMemoryRegionCaches *caches = vring_get_region_caches(vq); hwaddr pa = offsetof(VRingAvail, flags); return virtio_lduw_phys_cached(vq->vdev, &caches->avail, pa); } @@ -196,7 +202,7 @@ static inline uint16_t vring_avail_flags(VirtQueue *vq) /* Called within rcu_read_lock(). */ static inline uint16_t vring_avail_idx(VirtQueue *vq) { - VRingMemoryRegionCaches *caches = atomic_rcu_read(&vq->vring.caches); + VRingMemoryRegionCaches *caches = vring_get_region_caches(vq); hwaddr pa = offsetof(VRingAvail, idx); vq->shadow_avail_idx = virtio_lduw_phys_cached(vq->vdev, &caches->avail, pa); return vq->shadow_avail_idx; @@ -205,7 +211,7 @@ static inline uint16_t vring_avail_idx(VirtQueue *vq) /* Called within rcu_read_lock(). */ static inline uint16_t vring_avail_ring(VirtQueue *vq, int i) { - VRingMemoryRegionCaches *caches = atomic_rcu_read(&vq->vring.caches); + VRingMemoryRegionCaches *caches = vring_get_region_caches(vq); hwaddr pa = offsetof(VRingAvail, ring[i]); return virtio_lduw_phys_cached(vq->vdev, &caches->avail, pa); } @@ -220,7 +226,7 @@ static inline uint16_t vring_get_used_event(VirtQueue *vq) static inline void vring_used_write(VirtQueue *vq, VRingUsedElem *uelem, int i) { - VRingMemoryRegionCaches *caches = atomic_rcu_read(&vq->vring.caches); + VRingMemoryRegionCaches *caches = vring_get_region_caches(vq); hwaddr pa = offsetof(VRingUsed, ring[i]); virtio_tswap32s(vq->vdev, &uelem->id); virtio_tswap32s(vq->vdev, &uelem->len); @@ -231,7 +237,7 @@ static inline void vring_used_write(VirtQueue *vq, VRingUsedElem *uelem, /* Called within rcu_read_lock(). */ static uint16_t vring_used_idx(VirtQueue *vq) { - VRingMemoryRegionCaches *caches = atomic_rcu_read(&vq->vring.caches); + VRingMemoryRegionCaches *caches = vring_get_region_caches(vq); hwaddr pa = offsetof(VRingUsed, idx); return virtio_lduw_phys_cached(vq->vdev, &caches->used, pa); } @@ -239,7 +245,7 @@ static uint16_t vring_used_idx(VirtQueue *vq) /* Called within rcu_read_lock(). */ static inline void vring_used_idx_set(VirtQueue *vq, uint16_t val) { - VRingMemoryRegionCaches *caches = atomic_rcu_read(&vq->vring.caches); + VRingMemoryRegionCaches *caches = vring_get_region_caches(vq); hwaddr pa = offsetof(VRingUsed, idx); virtio_stw_phys_cached(vq->vdev, &caches->used, pa, val); address_space_cache_invalidate(&caches->used, pa, sizeof(val)); @@ -249,7 +255,7 @@ static inline void vring_used_idx_set(VirtQueue *vq, uint16_t val) /* Called within rcu_read_lock(). */ static inline void vring_used_flags_set_bit(VirtQueue *vq, int mask) { - VRingMemoryRegionCaches *caches = atomic_rcu_read(&vq->vring.caches); + VRingMemoryRegionCaches *caches = vring_get_region_caches(vq); VirtIODevice *vdev = vq->vdev; hwaddr pa = offsetof(VRingUsed, flags); uint16_t flags = virtio_lduw_phys_cached(vq->vdev, &caches->used, pa); @@ -261,7 +267,7 @@ static inline void vring_used_flags_set_bit(VirtQueue *vq, int mask) /* Called within rcu_read_lock(). */ static inline void vring_used_flags_unset_bit(VirtQueue *vq, int mask) { - VRingMemoryRegionCaches *caches = atomic_rcu_read(&vq->vring.caches); + VRingMemoryRegionCaches *caches = vring_get_region_caches(vq); VirtIODevice *vdev = vq->vdev; hwaddr pa = offsetof(VRingUsed, flags); uint16_t flags = virtio_lduw_phys_cached(vq->vdev, &caches->used, pa); @@ -279,7 +285,7 @@ static inline void vring_set_avail_event(VirtQueue *vq, uint16_t val) return; } - caches = atomic_rcu_read(&vq->vring.caches); + caches = vring_get_region_caches(vq); pa = offsetof(VRingUsed, ring[vq->vring.num]); virtio_stw_phys_cached(vq->vdev, &caches->used, pa, val); address_space_cache_invalidate(&caches->used, pa, sizeof(val)); @@ -577,7 +583,7 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes, total_bufs = in_total = out_total = 0; max = vq->vring.num; - caches = atomic_rcu_read(&vq->vring.caches); + caches = vring_get_region_caches(vq); if (caches->desc.len < max * sizeof(VRingDesc)) { virtio_error(vdev, "Cannot map descriptor ring"); goto err; @@ -844,7 +850,7 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz) i = head; - caches = atomic_rcu_read(&vq->vring.caches); + caches = vring_get_region_caches(vq); if (caches->desc.len < max * sizeof(VRingDesc)) { virtio_error(vdev, "Cannot map descriptor ring"); goto done; @@ -1143,6 +1149,17 @@ static enum virtio_device_endian virtio_current_cpu_endian(void) } } +static void virtio_virtqueue_reset_region_cache(struct VirtQueue *vq) +{ + VRingMemoryRegionCaches *caches; + + caches = atomic_read(&vq->vring.caches); + atomic_rcu_set(&vq->vring.caches, NULL); + if (caches) { + call_rcu(caches, virtio_free_region_cache, rcu); + } +} + void virtio_reset(void *opaque) { VirtIODevice *vdev = opaque; @@ -1183,6 +1200,7 @@ void virtio_reset(void *opaque) vdev->vq[i].notification = true; vdev->vq[i].vring.num = vdev->vq[i].vring.num_default; vdev->vq[i].inuse = 0; + virtio_virtqueue_reset_region_cache(&vdev->vq[i]); } } @@ -2477,13 +2495,10 @@ static void virtio_device_free_virtqueues(VirtIODevice *vdev) } for (i = 0; i < VIRTIO_QUEUE_MAX; i++) { - VRingMemoryRegionCaches *caches; if (vdev->vq[i].vring.num == 0) { break; } - caches = atomic_read(&vdev->vq[i].vring.caches); - atomic_set(&vdev->vq[i].vring.caches, NULL); - virtio_free_region_cache(caches); + virtio_virtqueue_reset_region_cache(&vdev->vq[i]); } g_free(vdev->vq); } From e45da653223869849877ee8f4b86f511f891d4f8 Mon Sep 17 00:00:00 2001 From: Jason Wang Date: Wed, 15 Mar 2017 19:48:32 +0800 Subject: [PATCH 4/7] virtio: validate address space cache during init We don't check the return value of address_space_cache_init(), this may lead buggy driver use incorrect region caches. Instead of triggering an assert, catch and warn this early in virtio_init_region_cache(). Cc: Cornelia Huck Cc: Paolo Bonzini Reviewed-by: Cornelia Huck Signed-off-by: Jason Wang Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/virtio/virtio.c | 33 +++++++++++++++++++++++++++------ 1 file changed, 27 insertions(+), 6 deletions(-) diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index a00380fae6..82b6060b2a 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -131,6 +131,7 @@ static void virtio_init_region_cache(VirtIODevice *vdev, int n) VRingMemoryRegionCaches *new; hwaddr addr, size; int event_size; + int64_t len; event_size = virtio_vdev_has_feature(vq->vdev, VIRTIO_RING_F_EVENT_IDX) ? 2 : 0; @@ -140,21 +141,41 @@ static void virtio_init_region_cache(VirtIODevice *vdev, int n) } new = g_new0(VRingMemoryRegionCaches, 1); size = virtio_queue_get_desc_size(vdev, n); - address_space_cache_init(&new->desc, vdev->dma_as, - addr, size, false); + len = address_space_cache_init(&new->desc, vdev->dma_as, + addr, size, false); + if (len < size) { + virtio_error(vdev, "Cannot map desc"); + goto err_desc; + } size = virtio_queue_get_used_size(vdev, n) + event_size; - address_space_cache_init(&new->used, vdev->dma_as, - vq->vring.used, size, true); + len = address_space_cache_init(&new->used, vdev->dma_as, + vq->vring.used, size, true); + if (len < size) { + virtio_error(vdev, "Cannot map used"); + goto err_used; + } size = virtio_queue_get_avail_size(vdev, n) + event_size; - address_space_cache_init(&new->avail, vdev->dma_as, - vq->vring.avail, size, false); + len = address_space_cache_init(&new->avail, vdev->dma_as, + vq->vring.avail, size, false); + if (len < size) { + virtio_error(vdev, "Cannot map avail"); + goto err_avail; + } atomic_rcu_set(&vq->vring.caches, new); if (old) { call_rcu(old, virtio_free_region_cache, rcu); } + return; + +err_avail: + address_space_cache_destroy(&new->used); +err_used: + address_space_cache_destroy(&new->desc); +err_desc: + g_free(new); } /* virt queue functions */ From 3716d5902d743e9a395b7d82f48df4fa56ed1ad3 Mon Sep 17 00:00:00 2001 From: Jason Wang Date: Mon, 13 Mar 2017 11:29:58 +0800 Subject: [PATCH 5/7] pci: introduce a bus master container 96a8821d2141 ("virtio: unbreak virtio-pci with IOMMU after caching ring translations") tries to make IOMMU works with virtio memory region cache, but it requires IOMMU to be created before any virtio devices. This is sub optimal, fixing this by introduce a bus master container to make sure address space can be initialized during device registering, and then we can safely set alias and make bus_master_enable_region as its subregion during bus master initialization. Cc: Paolo Bonzini Signed-off-by: Jason Wang Reviewed-by: Paolo Bonzini Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/pci/pci.c | 9 +++++++-- include/hw/pci/pci.h | 1 + 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/hw/pci/pci.c b/hw/pci/pci.c index 273f1e4602..ad46390ec5 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -88,8 +88,8 @@ static void pci_init_bus_master(PCIDevice *pci_dev) OBJECT(pci_dev), "bus master", dma_as->root, 0, memory_region_size(dma_as->root)); memory_region_set_enabled(&pci_dev->bus_master_enable_region, false); - address_space_init(&pci_dev->bus_master_as, - &pci_dev->bus_master_enable_region, pci_dev->name); + memory_region_add_subregion(&pci_dev->bus_master_container_region, 0, + &pci_dev->bus_master_enable_region); } static void pcibus_machine_done(Notifier *notifier, void *data) @@ -995,6 +995,11 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus, pci_dev->devfn = devfn; pci_dev->requester_id_cache = pci_req_id_cache_get(pci_dev); + memory_region_init(&pci_dev->bus_master_container_region, OBJECT(pci_dev), + "bus master container", UINT64_MAX); + address_space_init(&pci_dev->bus_master_as, + &pci_dev->bus_master_container_region, pci_dev->name); + if (qdev_hotplug) { pci_init_bus_master(pci_dev); } diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h index 9349acbfb2..713ede00bf 100644 --- a/include/hw/pci/pci.h +++ b/include/hw/pci/pci.h @@ -284,6 +284,7 @@ struct PCIDevice { char name[64]; PCIIORegion io_regions[PCI_NUM_REGIONS]; AddressSpace bus_master_as; + MemoryRegion bus_master_container_region; MemoryRegion bus_master_enable_region; /* do not access the following fields */ From f0edf2397896aef1799dc9b5818683ea16577030 Mon Sep 17 00:00:00 2001 From: Jason Wang Date: Mon, 13 Mar 2017 11:29:57 +0800 Subject: [PATCH 6/7] Revert "virtio: unbreak virtio-pci with IOMMU after caching ring translations" This reverts commit 96a8821d21411f10d77ea994af369c6e5c35a2cc. Previous patch is a better solution which does not require a strict order between virtio and IOMMU. CC: Paolo Bonzini Signed-off-by: Jason Wang --- hw/virtio/virtio-pci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c index b76f3f62a0..5ce42af9d4 100644 --- a/hw/virtio/virtio-pci.c +++ b/hw/virtio/virtio-pci.c @@ -1153,7 +1153,7 @@ static AddressSpace *virtio_pci_get_dma_as(DeviceState *d) VirtIOPCIProxy *proxy = VIRTIO_PCI(d); PCIDevice *dev = &proxy->pci_dev; - return pci_device_iommu_address_space(dev); + return pci_get_address_space(dev); } static int virtio_pci_add_mem_cap(VirtIOPCIProxy *proxy, From 60a8d8023473dd24957b3a66824f66cd35b80d64 Mon Sep 17 00:00:00 2001 From: Jason Wang Date: Tue, 14 Mar 2017 15:25:05 +0800 Subject: [PATCH 7/7] virtio-pci: reset modern vq meta data We don't reset proxy->vqs[].{num|desc[]|avail[]|used[]}. This means if a driver enable the vq without setting vq address after reset. The old addresses were leaked. Fixing this by resetting modern vq meta data during device reset. Cc: qemu-stable@nongnu.org Signed-off-by: Jason Wang Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/virtio/virtio-pci.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c index 5ce42af9d4..69cc471e56 100644 --- a/hw/virtio/virtio-pci.c +++ b/hw/virtio/virtio-pci.c @@ -1857,6 +1857,10 @@ static void virtio_pci_reset(DeviceState *qdev) for (i = 0; i < VIRTIO_QUEUE_MAX; i++) { proxy->vqs[i].enabled = 0; + proxy->vqs[i].num = 0; + proxy->vqs[i].desc[0] = proxy->vqs[i].desc[1] = 0; + proxy->vqs[i].avail[0] = proxy->vqs[i].avail[1] = 0; + proxy->vqs[i].used[0] = proxy->vqs[i].used[1] = 0; } }