From 690705ca0b0f1ed24a34ccd14c9866fbe47c69a6 Mon Sep 17 00:00:00 2001 From: Mark Cave-Ayland Date: Wed, 19 Apr 2023 16:16:52 +0100 Subject: [PATCH] softmmu/ioport.c: make MemoryRegionPortioList owner of portio_list MemoryRegions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently when portio_list MemoryRegions are freed using portio_list_destroy() the RCU thread segfaults generating a backtrace similar to that below: #0 0x5555599a34b6 in phys_section_destroy ../softmmu/physmem.c:996 #1 0x5555599a37a3 in phys_sections_free ../softmmu/physmem.c:1011 #2 0x5555599b24aa in address_space_dispatch_free ../softmmu/physmem.c:2430 #3 0x55555996a283 in flatview_destroy ../softmmu/memory.c:292 #4 0x55555a2cb9fb in call_rcu_thread ../util/rcu.c:284 #5 0x55555a29b71d in qemu_thread_start ../util/qemu-thread-posix.c:541 #6 0x7ffff4a0cea6 in start_thread nptl/pthread_create.c:477 #7 0x7ffff492ca2e in __clone (/lib/x86_64-linux-gnu/libc.so.6+0xfca2e) The problem here is that portio_list_destroy() unparents the portio_list MemoryRegions causing them to be freed immediately, however the flatview still has a reference to the MemoryRegion and so causes a use-after-free segfault when the RCU thread next updates the flatview. Solve the lifetime issue by making MemoryRegionPortioList the owner of the portio_list MemoryRegions, and then reparenting them to the portio_list owner. This ensures that they can be accessed as QOM children via the portio_list owner, yet the MemoryRegionPortioList owns the refcount. Update portio_list_destroy() to unparent the MemoryRegion from the portio_list owner (while keeping mrpio->mr live until finalization of the MemoryRegionPortioList), so that the portio_list MemoryRegions remain allocated until flatview_destroy() removes the final refcount upon the next flatview update. Signed-off-by: Mark Cave-Ayland Reviewed-by: Philippe Mathieu-Daudé Message-Id: <20230419151652.362717-4-mark.cave-ayland@ilande.co.uk> Signed-off-by: Paolo Bonzini --- softmmu/ioport.c | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/softmmu/ioport.c b/softmmu/ioport.c index 33720fe5664a..b66e0a5a8ebc 100644 --- a/softmmu/ioport.c +++ b/softmmu/ioport.c @@ -229,6 +229,8 @@ static void portio_list_add_1(PortioList *piolist, unsigned off_low, unsigned off_high) { MemoryRegionPortioList *mrpio; + Object *owner; + char *name; unsigned i; /* Copy the sub-list and null-terminate it. */ @@ -245,8 +247,25 @@ static void portio_list_add_1(PortioList *piolist, mrpio->ports[i].base = start + off_low; } - memory_region_init_io(&mrpio->mr, piolist->owner, &portio_ops, mrpio, + /* + * The MemoryRegion owner is the MemoryRegionPortioList since that manages + * the lifecycle via the refcount + */ + memory_region_init_io(&mrpio->mr, OBJECT(mrpio), &portio_ops, mrpio, piolist->name, off_high - off_low); + + /* Reparent the MemoryRegion to the piolist owner */ + object_ref(&mrpio->mr); + object_unparent(OBJECT(&mrpio->mr)); + if (!piolist->owner) { + owner = container_get(qdev_get_machine(), "/unattached"); + } else { + owner = piolist->owner; + } + name = g_strdup_printf("%s[*]", piolist->name); + object_property_add_child(owner, name, OBJECT(&mrpio->mr)); + g_free(name); + if (piolist->flush_coalesced_mmio) { memory_region_set_flush_coalesced(&mrpio->mr); } @@ -308,6 +327,7 @@ static void memory_region_portio_list_finalize(Object *obj) { MemoryRegionPortioList *mrpio = MEMORY_REGION_PORTIO_LIST(obj); + object_unref(&mrpio->mr); g_free(mrpio->ports); }