From 53e51d85ef1fdd295c8f09792b8e7490c148f4b3 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Thu, 16 Jun 2011 18:45:36 +0200 Subject: [PATCH 1/9] Fix automatically assigned network names for netdev If a network client doesn't have a name, we make one up, with assign_name(). assign_name() creates a name MODEL.NUM, where MODEL is the client's model, and NUM is the number of MODELs that already exist. Bug: it misses clients that are not on a VLAN, i.e. netdevs and the NICs using them: $ qemu-system-x86_64 -nodefaults -vnc :0 -S -monitor stdio -netdev user,id=hostnet0 -net nic,netdev=hostnet0 -netdev user,id=hostnet1 -net nic,netdev=hostnet1 QEMU 0.14.50 monitor - type 'help' for more information (qemu) info network Devices not on any VLAN: hostnet0: net=10.0.2.0, restricted=n peer=e1000.0 hostnet1: net=10.0.2.0, restricted=n peer=e1000.0 e1000.0: model=e1000,macaddr=52:54:00:12:34:56 peer=hostnet0 e1000.0: model=e1000,macaddr=52:54:00:12:34:57 peer=hostnet1 Fix that. Signed-off-by: Markus Armbruster Signed-off-by: Michael S. Tsirkin --- net.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/net.c b/net.c index 66123ad409..55c73c592d 100644 --- a/net.c +++ b/net.c @@ -150,12 +150,11 @@ void qemu_macaddr_default_if_unset(MACAddr *macaddr) static char *assign_name(VLANClientState *vc1, const char *model) { VLANState *vlan; + VLANClientState *vc; char buf[256]; int id = 0; QTAILQ_FOREACH(vlan, &vlans, next) { - VLANClientState *vc; - QTAILQ_FOREACH(vc, &vlan->clients, next) { if (vc != vc1 && strcmp(vc->model, model) == 0) { id++; @@ -163,6 +162,12 @@ static char *assign_name(VLANClientState *vc1, const char *model) } } + QTAILQ_FOREACH(vc, &non_vlan_clients, next) { + if (vc != vc1 && strcmp(vc->model, model) == 0) { + id++; + } + } + snprintf(buf, sizeof(buf), "%s.%d", model, id); return qemu_strdup(buf); From 85dde9a90b9d26273ef531d344b2cdfee9a6683d Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Thu, 16 Jun 2011 18:45:37 +0200 Subject: [PATCH 2/9] Fix netdev name lookup in -device, device_add, netdev_del qemu_find_netdev() looks up members of non_vlan_clients by name. It happily returns the first match. Trouble is the names need not be unique. non_vlan_clients contains host parts (netdevs) and guest parts (NICs). Netdevs have unique names: a netdev's name is a (mandatory) qemu_netdev_opts ID, and these are unique. NIC names are not unique. If a NIC has a qdev ID (which is unique), that's its name. Else, we make up a name. The made-up names are unique, but they can clash with qdev IDs. Even if NICs had unique names, they could still clash with netdev names. Callers of qemu_find_netdev(): * net_init_nic() wants a netdev. It happens to work because it runs before NICs get added to non_vlan_clients. * do_netdev_del() wants a netdev. If it gets a NIC, it complains and fails. Bug: a netdev with the same name that comes later in non_vlan_clients can't be deleted: $ qemu-system-x86_64 -nodefaults -vnc :0 -S -monitor stdio -netdev user,id=hostnet0 -device virtio-net-pci,netdev=hostnet0,id=virtio1 [...] (qemu) netdev_add user,id=virtio1 (qemu) info network Devices not on any VLAN: hostnet0: net=10.0.2.0, restricted=n peer=virtio1 virtio1: model=virtio-net-pci,macaddr=52:54:00:12:34:56 peer=hostnet0 virtio1: net=10.0.2.0, restricted=n (qemu) netdev_del virtio1 Device 'virtio1' not found * parse_netdev() wants a netdev. If it gets a NIC, it gets confused. With the test setup above: (qemu) device_add virtio-net-pci,netdev=virtio1 Property 'virtio-net-pci.netdev' can't take value 'virtio1', it's in use You can even connect two NICs to each other: $ qemu-system-x86_64 -nodefaults -vnc :0 -S -monitor stdio -device virtio-net-pci,id=virtio1 -device e1000,netdev=virtio1 [...] Devices not on any VLAN: virtio1: model=virtio-net-pci,macaddr=52:54:00:12:34:56 peer=e1000.0 e1000.0: model=e1000,macaddr=52:54:00:12:34:57 peer=virtio1 (qemu) q Segmentation fault (core dumped) * do_set_link() works fine for both netdevs and NICs. Whether it really makes sense for netdevs is debatable, but that's outside this patch's scope. Change qemu_find_netdev() to return only netdevs. This fixes the netdev_del and device_add/-device bugs demonstrated above. To avoid changing set_link, make do_set_link() search non_vlan_clients by hand instead of calling qemu_find_netdev(). Signed-off-by: Markus Armbruster Signed-off-by: Michael S. Tsirkin --- net.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/net.c b/net.c index 55c73c592d..e329c693ea 100644 --- a/net.c +++ b/net.c @@ -658,6 +658,8 @@ VLANClientState *qemu_find_netdev(const char *id) VLANClientState *vc; QTAILQ_FOREACH(vc, &non_vlan_clients, next) { + if (vc->info->type == NET_CLIENT_TYPE_NIC) + continue; if (!strcmp(vc->name, id)) { return vc; } @@ -1217,7 +1219,7 @@ int do_netdev_del(Monitor *mon, const QDict *qdict, QObject **ret_data) VLANClientState *vc; vc = qemu_find_netdev(id); - if (!vc || vc->info->type == NET_CLIENT_TYPE_NIC) { + if (!vc) { qerror_report(QERR_DEVICE_NOT_FOUND, id); return -1; } @@ -1262,7 +1264,11 @@ int do_set_link(Monitor *mon, const QDict *qdict, QObject **ret_data) } } } - vc = qemu_find_netdev(name); + QTAILQ_FOREACH(vc, &non_vlan_clients, next) { + if (!strcmp(vc->name, name)) { + goto done; + } + } done: if (!vc) { From 42e4126b793d15ec40f3a84017e1d8afecda1b6d Mon Sep 17 00:00:00 2001 From: Jan Kiszka Date: Fri, 22 Jul 2011 11:05:01 +0200 Subject: [PATCH 3/9] pci: Common overflow prevention Introduce pci_config_read/write_common helpers to prevent passing accesses down the callback chain that go beyond the config space limits. Adjust length assertions as they are no longer correct (cutting may generate valid 3 byte accesses). Signed-off-by: Jan Kiszka Signed-off-by: Michael S. Tsirkin --- hw/pci.c | 6 ++---- hw/pci_host.c | 24 ++++++++++++++++++++---- hw/pci_host.h | 6 ++++++ hw/pcie_host.c | 12 ++++++------ 4 files changed, 34 insertions(+), 14 deletions(-) diff --git a/hw/pci.c b/hw/pci.c index b904a4ecb6..ef94739718 100644 --- a/hw/pci.c +++ b/hw/pci.c @@ -1108,8 +1108,7 @@ uint32_t pci_default_read_config(PCIDevice *d, uint32_t address, int len) { uint32_t val = 0; - assert(len == 1 || len == 2 || len == 4); - len = MIN(len, pci_config_size(d) - address); + memcpy(&val, d->config + address, len); return le32_to_cpu(val); } @@ -1117,9 +1116,8 @@ uint32_t pci_default_read_config(PCIDevice *d, void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l) { int i, was_irq_disabled = pci_irq_disabled(d); - uint32_t config_size = pci_config_size(d); - for (i = 0; i < l && addr + i < config_size; val >>= 8, ++i) { + for (i = 0; i < l; val >>= 8, ++i) { uint8_t wmask = d->wmask[addr + i]; uint8_t w1cmask = d->w1cmask[addr + i]; assert(!(wmask & w1cmask)); diff --git a/hw/pci_host.c b/hw/pci_host.c index 728e2d4ce5..2e8a29f1e3 100644 --- a/hw/pci_host.c +++ b/hw/pci_host.c @@ -47,17 +47,33 @@ static inline PCIDevice *pci_dev_find_by_addr(PCIBus *bus, uint32_t addr) return pci_find_device(bus, bus_num, devfn); } +void pci_host_config_write_common(PCIDevice *pci_dev, uint32_t addr, + uint32_t limit, uint32_t val, uint32_t len) +{ + assert(len <= 4); + pci_dev->config_write(pci_dev, addr, val, MIN(len, limit - addr)); +} + +uint32_t pci_host_config_read_common(PCIDevice *pci_dev, uint32_t addr, + uint32_t limit, uint32_t len) +{ + assert(len <= 4); + return pci_dev->config_read(pci_dev, addr, MIN(len, limit - addr)); +} + void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len) { PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr); uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1); - if (!pci_dev) + if (!pci_dev) { return; + } PCI_DPRINTF("%s: %s: addr=%02" PRIx32 " val=%08" PRIx32 " len=%d\n", __func__, pci_dev->name, config_addr, val, len); - pci_dev->config_write(pci_dev, config_addr, val, len); + pci_host_config_write_common(pci_dev, config_addr, PCI_CONFIG_SPACE_SIZE, + val, len); } uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len) @@ -66,12 +82,12 @@ uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len) uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1); uint32_t val; - assert(len == 1 || len == 2 || len == 4); if (!pci_dev) { return ~0x0; } - val = pci_dev->config_read(pci_dev, config_addr, len); + val = pci_host_config_read_common(pci_dev, config_addr, + PCI_CONFIG_SPACE_SIZE, len); PCI_DPRINTF("%s: %s: addr=%02"PRIx32" val=%08"PRIx32" len=%d\n", __func__, pci_dev->name, config_addr, val, len); diff --git a/hw/pci_host.h b/hw/pci_host.h index 0a585951e0..c8390eec56 100644 --- a/hw/pci_host.h +++ b/hw/pci_host.h @@ -39,6 +39,12 @@ struct PCIHostState { PCIBus *bus; }; +/* common internal helpers for PCI/PCIe hosts, cut off overflows */ +void pci_host_config_write_common(PCIDevice *pci_dev, uint32_t addr, + uint32_t limit, uint32_t val, uint32_t len); +uint32_t pci_host_config_read_common(PCIDevice *pci_dev, uint32_t addr, + uint32_t limit, uint32_t len); + void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len); uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len); diff --git a/hw/pcie_host.c b/hw/pcie_host.c index b7498656f2..f0b3d13aae 100644 --- a/hw/pcie_host.c +++ b/hw/pcie_host.c @@ -57,22 +57,22 @@ static void pcie_mmcfg_data_write(PCIBus *s, { PCIDevice *pci_dev = pcie_dev_find_by_mmcfg_addr(s, mmcfg_addr); - if (!pci_dev) + if (!pci_dev) { return; - - pci_dev->config_write(pci_dev, - PCIE_MMCFG_CONFOFFSET(mmcfg_addr), val, len); + } + pci_host_config_write_common(pci_dev, PCIE_MMCFG_CONFOFFSET(mmcfg_addr), + pci_config_size(pci_dev), val, len); } static uint32_t pcie_mmcfg_data_read(PCIBus *s, uint32_t addr, int len) { PCIDevice *pci_dev = pcie_dev_find_by_mmcfg_addr(s, addr); - assert(len == 1 || len == 2 || len == 4); if (!pci_dev) { return ~0x0; } - return pci_dev->config_read(pci_dev, PCIE_MMCFG_CONFOFFSET(addr), len); + return pci_host_config_read_common(pci_dev, PCIE_MMCFG_CONFOFFSET(addr), + pci_config_size(pci_dev), len); } static void pcie_mmcfg_data_writeb(void *opaque, From 1129714ff43bd947740d587956a655210e8b93ed Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Wed, 27 Jul 2011 11:08:20 +0300 Subject: [PATCH 4/9] virtio-pci: use generic logic for command access In practice, guests don't generate config requests that cross a word boundary, so the logic to detect command word access is correct because PCI_COMMAND is 0x4. But depending on this is tricky, further, it will break with guests that do try to generate a misaligned access as we pass it to devices without splitting. Better to use the generic range_covers_byte for this. Signed-off-by: Michael S. Tsirkin --- hw/virtio-pci.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c index d685243728..4f770fe185 100644 --- a/hw/virtio-pci.c +++ b/hw/virtio-pci.c @@ -27,6 +27,7 @@ #include "kvm.h" #include "blockdev.h" #include "virtio-pci.h" +#include "range.h" /* from Linux's linux/virtio_pci.h */ @@ -516,17 +517,16 @@ static void virtio_write_config(PCIDevice *pci_dev, uint32_t address, { VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev); - if (PCI_COMMAND == address) { - if (!(val & PCI_COMMAND_MASTER)) { - if (!(proxy->flags & VIRTIO_PCI_FLAG_BUS_MASTER_BUG)) { - virtio_pci_stop_ioeventfd(proxy); - virtio_set_status(proxy->vdev, - proxy->vdev->status & ~VIRTIO_CONFIG_S_DRIVER_OK); - } - } + pci_default_write_config(pci_dev, address, val, len); + + if (range_covers_byte(address, len, PCI_COMMAND) && + !(pci_dev->config[PCI_COMMAND] & PCI_COMMAND_MASTER) && + !(proxy->flags & VIRTIO_PCI_FLAG_BUS_MASTER_BUG)) { + virtio_pci_stop_ioeventfd(proxy); + virtio_set_status(proxy->vdev, + proxy->vdev->status & ~VIRTIO_CONFIG_S_DRIVER_OK); } - pci_default_write_config(pci_dev, address, val, len); msix_write_config(pci_dev, address, val, len); } From d92551f28eff7cb6572ed3147399e51f5f5dfc22 Mon Sep 17 00:00:00 2001 From: Amit Shah Date: Wed, 27 Jul 2011 14:00:30 +0530 Subject: [PATCH 5/9] virtio-blk: Fix memleak on exit Calling virtio_cleanup() will free up memory allocated in virtio_common_init(). Signed-off-by: Amit Shah Signed-off-by: Michael S. Tsirkin --- hw/virtio-blk.c | 1 + 1 file changed, 1 insertion(+) diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c index 6471ac85ab..836dbc3c12 100644 --- a/hw/virtio-blk.c +++ b/hw/virtio-blk.c @@ -594,4 +594,5 @@ void virtio_blk_exit(VirtIODevice *vdev) { VirtIOBlock *s = to_virtio_blk(vdev); unregister_savevm(s->qdev, "virtio-blk", s); + virtio_cleanup(vdev); } From b52dfd71f33b902e612b12f6cc89f3b61e4d3e22 Mon Sep 17 00:00:00 2001 From: Amit Shah Date: Wed, 27 Jul 2011 14:00:31 +0530 Subject: [PATCH 6/9] virtio-net: don't use vdev after virtio_cleanup virtio_cleanup() will be changed by the following patch to remove the VirtIONet struct that gets allocated via virtio_common_init(). Ensure we don't dereference the structure after calling the cleanup function. Signed-off-by: Amit Shah Signed-off-by: Michael S. Tsirkin --- hw/virtio-net.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/virtio-net.c b/hw/virtio-net.c index 6997e02dcf..09c665babe 100644 --- a/hw/virtio-net.c +++ b/hw/virtio-net.c @@ -1073,6 +1073,6 @@ void virtio_net_exit(VirtIODevice *vdev) qemu_bh_delete(n->tx_bh); } - virtio_cleanup(&n->vdev); qemu_del_vlan_client(&n->nic->nc); + virtio_cleanup(&n->vdev); } From 845f85fa1597c72609bd10a37b9586b445c13d49 Mon Sep 17 00:00:00 2001 From: Amit Shah Date: Wed, 27 Jul 2011 14:00:32 +0530 Subject: [PATCH 7/9] virtio: Plug memleak by freeing vdev virtio_common_init() allocates RAM for the vdev struct (and any additional memory, depending on the size passed to the function). This memory wasn't being freed until now. Signed-off-by: Amit Shah Signed-off-by: Michael S. Tsirkin --- hw/virtio.c | 1 + 1 file changed, 1 insertion(+) diff --git a/hw/virtio.c b/hw/virtio.c index a8f4940da2..93dfb1e359 100644 --- a/hw/virtio.c +++ b/hw/virtio.c @@ -834,6 +834,7 @@ void virtio_cleanup(VirtIODevice *vdev) if (vdev->config) qemu_free(vdev->config); qemu_free(vdev->vq); + qemu_free(vdev); } static void virtio_vmstate_change(void *opaque, int running, int reason) From 43e86c8f5b6d9f6279e20dede4e1f7829bdc43b7 Mon Sep 17 00:00:00 2001 From: Isaku Yamahata Date: Fri, 29 Jul 2011 10:01:43 +0900 Subject: [PATCH 8/9] pcie_host: verify mmcfg address range For a conventional pci device behind a pcie-to-pci bridge, pci_host handlers get confused by an out of bounds access in the range [256, 4K). Check for such an access and make it have no effect. Signed-off-by: Isaku Yamahata Signed-off-by: Michael S. Tsirkin --- hw/pcie_host.c | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/hw/pcie_host.c b/hw/pcie_host.c index f0b3d13aae..f9fea3d918 100644 --- a/hw/pcie_host.c +++ b/hw/pcie_host.c @@ -56,23 +56,39 @@ static void pcie_mmcfg_data_write(PCIBus *s, uint32_t mmcfg_addr, uint32_t val, int len) { PCIDevice *pci_dev = pcie_dev_find_by_mmcfg_addr(s, mmcfg_addr); + uint32_t addr; + uint32_t limit; if (!pci_dev) { return; } - pci_host_config_write_common(pci_dev, PCIE_MMCFG_CONFOFFSET(mmcfg_addr), - pci_config_size(pci_dev), val, len); + addr = PCIE_MMCFG_CONFOFFSET(mmcfg_addr); + limit = pci_config_size(pci_dev); + if (limit <= addr) { + /* conventional pci device can be behind pcie-to-pci bridge. + 256 <= addr < 4K has no effects. */ + return; + } + pci_host_config_write_common(pci_dev, addr, limit, val, len); } -static uint32_t pcie_mmcfg_data_read(PCIBus *s, uint32_t addr, int len) +static uint32_t pcie_mmcfg_data_read(PCIBus *s, uint32_t mmcfg_addr, int len) { - PCIDevice *pci_dev = pcie_dev_find_by_mmcfg_addr(s, addr); + PCIDevice *pci_dev = pcie_dev_find_by_mmcfg_addr(s, mmcfg_addr); + uint32_t addr; + uint32_t limit; if (!pci_dev) { return ~0x0; } - return pci_host_config_read_common(pci_dev, PCIE_MMCFG_CONFOFFSET(addr), - pci_config_size(pci_dev), len); + addr = PCIE_MMCFG_CONFOFFSET(mmcfg_addr); + limit = pci_config_size(pci_dev); + if (limit <= addr) { + /* conventional pci device can be behind pcie-to-pci bridge. + 256 <= addr < 4K has no effects. */ + return ~0x0; + } + return pci_host_config_read_common(pci_dev, addr, limit, len); } static void pcie_mmcfg_data_writeb(void *opaque, From cb4b4fde82b064472c13fb9d983ca36a70e560aa Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Wed, 3 Aug 2011 15:24:41 +0300 Subject: [PATCH 9/9] vhost: remove an incorrect assert The 'to' can go negative when the first region gets removed (it gets incremented by to 0 immediately afterward), which makes the assertion fail. Nothing breaks if to < 0 here so just remove the assert. Tested-by: David Ahern Signed-off-by: Michael S. Tsirkin --- hw/vhost.c | 1 - 1 file changed, 1 deletion(-) diff --git a/hw/vhost.c b/hw/vhost.c index c3d88214fe..19e72555c4 100644 --- a/hw/vhost.c +++ b/hw/vhost.c @@ -120,7 +120,6 @@ static void vhost_dev_unassign_memory(struct vhost_dev *dev, if (start_addr <= reg->guest_phys_addr && memlast >= reglast) { --dev->mem->nregions; --to; - assert(to >= 0); ++overlap_middle; continue; }