Re: [PATCH 3/3] softmmu/ioport.c: make MemoryRegionPortioList owner of portio_list MemoryRegions

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 19/4/23 17:16, Mark Cave-Ayland wrote:
> 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 childen via the portio_list owner, yet the MemoryRegionPortioList

"children"

> owns the refcount.
> 
> Update portio_list_destroy() to unparent the MemoryRegion from the portio_list owner and
> then add a finalize() method to 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 <mark.cave-ayland@ilande.co.uk>
> ---
>   softmmu/ioport.c | 34 +++++++++++++++++++++++++++++++---
>   1 file changed, 31 insertions(+), 3 deletions(-)


> @@ -230,6 +230,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.  */
> @@ -246,8 +248,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));

Out of this patch scope, but could this part <...

> +    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);

...> be extracted as qdev_add_child()? It seems to duplicate
code from device_set_realized().

>       if (piolist->flush_coalesced_mmio) {
>           memory_region_set_flush_coalesced(&mrpio->mr);
>       }
> @@ -305,10 +324,19 @@ void portio_list_del(PortioList *piolist)
>       }
>   }

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>