Peter Maydell <[email protected]> writes: > In the dino PCI host bridge device, we call pci_register_root_bus() > in the device's instance_init. This is a problem for two reasons > * the PCI bridge is then available to the rest of the simulation > (e.g. via pci_qdev_find_device()), even though it hasn't > yet been realized > * we do not attempt to unregister in an instance_deinit, > which means that if you go through an instance_init -> deinit > lifecycle the freed memory for the host-bridge device is > left on the pci_host_bridges list > > ASAN reports the resulting use-after-free: > > ==1771223==ERROR: AddressSanitizer: heap-use-after-free on address > 0x527000018f80 at pc 0x5b4b9d3369b5 bp 0x7ffd01929980 sp 0x7ffd01929978 > WRITE of size 8 at 0x527000018f80 thread T0 > #0 0x5b4b9d3369b4 in pci_host_bus_register > /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/hppa-asan/../../hw/pci/pci.c:608:5 > #1 0x5b4b9d321566 in pci_root_bus_internal_init > /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/hppa-asan/../../hw/pci/pci.c:677:5 > #2 0x5b4b9d3215e0 in pci_root_bus_new > /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/hppa-asan/../../hw/pci/pci.c:706:5 > #3 0x5b4b9d321fe5 in pci_register_root_bus > /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/hppa-asan/../../hw/pci/pci.c:751:11 > #4 0x5b4b9d390521 in dino_pcihost_init > /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/hppa-asan/../../hw/pci-host/dino.c:473:16 > > 0x527000018f80 is located 1664 bytes inside of 12384-byte region > [0x527000018900,0x52700001b960) > freed by thread T0 here: > #0 0x5b4b9cab185a in free > (/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/hppa-asan/qemu-system-hppa+0x17ad85a) > (BuildId: ca496bb2e4fc750ebd289b448bad8d99c0ecd140) > #1 0x5b4b9e3ee723 in object_finalize > /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/hppa-asan/../../qom/object.c:734:9 > #2 0x5b4b9e3e69db in object_unref > /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/hppa-asan/../../qom/object.c:1232:9 > #3 0x5b4b9ea6173c in qmp_device_list_properties > /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/hppa-asan/../../qom/qom-qmp-cmds.c:237:5 > #4 0x5b4b9ec4e0f3 in qmp_marshal_device_list_properties > /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/hppa-asan/qapi/qapi-commands-qdev.c:65:14 > > previously allocated by thread T0 here: > #0 0x5b4b9cab1af3 in malloc > (/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/hppa-asan/qemu-system-hppa+0x17adaf3) > (BuildId: ca496bb2e4fc750ebd289b448bad8d99c0ecd140) > #1 0x799d8270eb09 in g_malloc > (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x62b09) (BuildId: > 1eb6131419edb83b2178b682829a6913cf682d75) > #2 0x5b4b9e3e75fc in object_new_with_type > /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/hppa-asan/../../qom/object.c:767:15 > #3 0x5b4b9e3e7409 in object_new_with_class > /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/hppa-asan/../../qom/object.c:782:12 > #4 0x5b4b9ea609a5 in qmp_device_list_properties > /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/hppa-asan/../../qom/qom-qmp-cmds.c:206:11 > > where we allocated one instance of the dino device, put it on the > list, freed it, and then trying to allocate a second instance touches > the freed memory on the pci_host_bridges list. > > Fix this by deferring all the setup of memory regions and registering > the PCI bridge to the device's realize method. This brings it into > line with almost all other PCI host bridges, which call > pci_register_root_bus() in realize.
Would it be worth adding a kdoc comment to the declaration of pci_register_root_bus to explain this? > > Cc: [email protected] > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/3118 > Fixes: 63901b6cc4d8b4 ("dino: move PCI bus initialisation to > dino_pcihost_init()") > Signed-off-by: Peter Maydell <[email protected]> > --- > hw/pci-host/dino.c | 90 +++++++++++++++++++++------------------------- > 1 file changed, 41 insertions(+), 49 deletions(-) > > diff --git a/hw/pci-host/dino.c b/hw/pci-host/dino.c > index 11b353be2ea..924053499c1 100644 > --- a/hw/pci-host/dino.c > +++ b/hw/pci-host/dino.c > @@ -413,6 +413,47 @@ static void dino_pcihost_reset(DeviceState *dev) > static void dino_pcihost_realize(DeviceState *dev, Error **errp) > { > DinoState *s = DINO_PCI_HOST_BRIDGE(dev); > + PCIHostState *phb = PCI_HOST_BRIDGE(dev); > + > + /* Dino PCI access from main memory. */ > + memory_region_init_io(&s->this_mem, OBJECT(s), &dino_chip_ops, > + s, "dino", 4096); > + > + /* Dino PCI config. */ > + memory_region_init_io(&phb->conf_mem, OBJECT(phb), > + &dino_config_addr_ops, DEVICE(s), > + "pci-conf-idx", 4); > + memory_region_init_io(&phb->data_mem, OBJECT(phb), > + &dino_config_data_ops, DEVICE(s), > + "pci-conf-data", 4); > + memory_region_add_subregion(&s->this_mem, DINO_PCI_CONFIG_ADDR, > + &phb->conf_mem); > + memory_region_add_subregion(&s->this_mem, DINO_CONFIG_DATA, > + &phb->data_mem); > + > + /* Dino PCI bus memory. */ > + memory_region_init(&s->pci_mem, OBJECT(s), "pci-memory", 4 * GiB); > + > + phb->bus = pci_register_root_bus(DEVICE(s), "pci", > + dino_set_irq, dino_pci_map_irq, s, > + &s->pci_mem, get_system_io(), > + PCI_DEVFN(0, 0), 32, TYPE_PCI_BUS); > + > + /* Set up windows into PCI bus memory. */ > + for (int i = 1; i < 31; i++) { > + uint32_t addr = 0xf0000000 + i * DINO_MEM_CHUNK_SIZE; > + char *name = g_strdup_printf("PCI Outbound Window %d", i); > + memory_region_init_alias(&s->pci_mem_alias[i], OBJECT(s), > + name, &s->pci_mem, addr, > + DINO_MEM_CHUNK_SIZE); > + g_free(name); minor nit: this could be an autofree but I get you are just moving code here. Reviewed-by: Alex Bennée <[email protected]> -- Alex Bennée Virtualisation Tech Lead @ Linaro
