When `owner` == `mr`, `object_unparent` will crash:

object_unparent(mr) ->
object_property_del_child(mr, mr) ->
object_finalize_child_property(mr, name, mr) ->
object_unref(mr) ->
object_finalize(mr) ->
object_property_del_all(mr) ->
object_finalize_child_property(mr, name, mr) ->
object_unref(mr) ->
fail on g_assert(obj->ref > 0)

However, passing a different `owner` to `memory_region_init` is not
enough. `memory_region_ref` has an optimization where it takes a ref
only on the owner. It specifically warns against calling unparent on
the memory region. So we initialize the memory region first and then
patch in the owner with itself.

Signed-off-by: Joelle van Dyne <j@getutm.app>
---
 hw/display/virtio-gpu-virgl.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index 18404be5892..70e0aff0ca3 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -123,7 +123,8 @@ virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g,
     vmr->g = g;
 
     mr = &vmr->mr;
-    memory_region_init_ram_ptr(mr, OBJECT(mr), "blob", size, data);
+    memory_region_init_ram_ptr(mr, OBJECT(g), "blob", size, data);
+    mr->owner = OBJECT(mr);
     memory_region_add_subregion(&b->hostmem, offset, mr);
     memory_region_set_enabled(mr, true);
 
-- 
2.50.1 (Apple Git-155)

On 2025/12/24 3:40, Joelle van Dyne wrote:
> When `owner` == `mr`, `object_unparent` will crash:
> 
> object_unparent(mr) ->
> object_property_del_child(mr, mr) ->
> object_finalize_child_property(mr, name, mr) ->
> object_unref(mr) ->
> object_finalize(mr) ->
> object_property_del_all(mr) ->
> object_finalize_child_property(mr, name, mr) ->
> object_unref(mr) ->
> fail on g_assert(obj->ref > 0)
> 
> However, passing a different `owner` to `memory_region_init` is not
> enough. `memory_region_ref` has an optimization where it takes a ref
> only on the owner. It specifically warns against calling unparent on
> the memory region. So we initialize the memory region first and then
> patch in the owner with itself.

Patching outside system/memory.c can be fragile.

I think an object is being a child of itself, which doesn't make sense. 
This can be avoided by passing NULL as name. The object will be an 
orphan so it will have to be freed with object_unref() instead of 
object_unparent().

Regards,
Akihiko Odaki

On Tue, Dec 23, 2025 at 9:32 PM Akihiko Odaki
<odaki@rsg.ci.i.u-tokyo.ac.jp> wrote:
>
> On 2025/12/24 3:40, Joelle van Dyne wrote:
> > When `owner` == `mr`, `object_unparent` will crash:
> >
> > object_unparent(mr) ->
> > object_property_del_child(mr, mr) ->
> > object_finalize_child_property(mr, name, mr) ->
> > object_unref(mr) ->
> > object_finalize(mr) ->
> > object_property_del_all(mr) ->
> > object_finalize_child_property(mr, name, mr) ->
> > object_unref(mr) ->
> > fail on g_assert(obj->ref > 0)
> >
> > However, passing a different `owner` to `memory_region_init` is not
> > enough. `memory_region_ref` has an optimization where it takes a ref
> > only on the owner. It specifically warns against calling unparent on
> > the memory region. So we initialize the memory region first and then
> > patch in the owner with itself.
>
> Patching outside system/memory.c can be fragile.
>
> I think an object is being a child of itself, which doesn't make sense.
> This can be avoided by passing NULL as name. The object will be an
> orphan so it will have to be freed with object_unref() instead of
> object_unparent().
I didn't want to break anything unintentionally and wanted to be safe
by making the change as close to the original logic as possible
(having introduced a UAF in v1 after making a one line change). Maybe
Antonio or Robert can give more insight on the intention of using self
as owner and if making it an orphan is acceptable?

>
> Regards,
> Akihiko Odaki

On 2025/12/24 16:46, Joelle van Dyne wrote:
> On Tue, Dec 23, 2025 at 9:32 PM Akihiko Odaki
> <odaki@rsg.ci.i.u-tokyo.ac.jp> wrote:
>>
>> On 2025/12/24 3:40, Joelle van Dyne wrote:
>>> When `owner` == `mr`, `object_unparent` will crash:
>>>
>>> object_unparent(mr) ->
>>> object_property_del_child(mr, mr) ->
>>> object_finalize_child_property(mr, name, mr) ->
>>> object_unref(mr) ->
>>> object_finalize(mr) ->
>>> object_property_del_all(mr) ->
>>> object_finalize_child_property(mr, name, mr) ->
>>> object_unref(mr) ->
>>> fail on g_assert(obj->ref > 0)
>>>
>>> However, passing a different `owner` to `memory_region_init` is not
>>> enough. `memory_region_ref` has an optimization where it takes a ref
>>> only on the owner. It specifically warns against calling unparent on
>>> the memory region. So we initialize the memory region first and then
>>> patch in the owner with itself.
>>
>> Patching outside system/memory.c can be fragile.
>>
>> I think an object is being a child of itself, which doesn't make sense.
>> This can be avoided by passing NULL as name. The object will be an
>> orphan so it will have to be freed with object_unref() instead of
>> object_unparent().
> I didn't want to break anything unintentionally and wanted to be safe
> by making the change as close to the original logic as possible
> (having introduced a UAF in v1 after making a one line change). Maybe
> Antonio or Robert can give more insight on the intention of using self
> as owner and if making it an orphan is acceptable?

The intention here is that the reference counting of memory regions 
independent of the device. If you make the device owner, the reference 
counter of the device will be used for memory region (see
memory_region_ref()). But this memory region can go away while the 
device stays, so its reference counting needs to be done independently.

By making the memory region itself owner, the reference counter of the 
memory region itself will be used, just like normal objects.

So the owner needs to be the memory region itself. But it is not 
necessary to have the parent/child relationship, and passing NULL as 
name only removes the relationship.

Regards,
Akihiko Odaki