[Qemu-devel] [PATCH v2] qdev: Fix use after free in qdev_init_nofail error path

[Qemu-devel] [PATCH v2] qdev: Fix use after free in qdev_init_nofail error path

[Igor Mammedov]

Since 69382d8b (qdev: Fix object reference leak in case device.realize()
fails), object_property_set_bool releases the device object
in case realize's failed and device hasn't explicitly assigned
parent. It happens due to object_unparent() on error handling
path releases not only implicitly set parent but also calls
device_unparent() which releases the remaining reference hold
buy parent bus.

qdev_try_create() created device has only 1 reference hold by
parent bus which makes impossible to unparent/re-parent object
without destroying it.

Fix it by making caller of qdev_try_create()/qdev_init_nofail()
own a reference from object_new() until qdev_init_nofail() is
successfully executed so that uparenting won't destroy device
until caller owns pointer returned by qdev_try_create().

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/core/qdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index ee4a083..0139d67 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -151,7 +151,6 @@ DeviceState *qdev_try_create(BusState *bus, const char *type)
     }
 
     qdev_set_parent_bus(dev, bus);
-    object_unref(OBJECT(dev));
     return dev;
 }
 
@@ -360,6 +359,7 @@ void qdev_init_nofail(DeviceState *dev)
                           object_get_typename(OBJECT(dev)));
         exit(1);
     }
+    object_unref(OBJECT(dev));
 }
 
 void qdev_machine_creation_done(void)
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2] qdev: Fix use after free in qdev_init_nofail error path

[Paolo Bonzini]

On 02/08/2016 12:49, Igor Mammedov wrote:
> Since 69382d8b (qdev: Fix object reference leak in case device.realize()
> fails), object_property_set_bool releases the device object
> in case realize's failed and device hasn't explicitly assigned
> parent. It happens due to object_unparent() on error handling
> path releases not only implicitly set parent but also calls
> device_unparent() which releases the remaining reference hold
> buy parent bus.
> 
> qdev_try_create() created device has only 1 reference hold by
> parent bus which makes impossible to unparent/re-parent object
> without destroying it.
> 
> Fix it by making caller of qdev_try_create()/qdev_init_nofail()
> own a reference from object_new() until qdev_init_nofail() is
> successfully executed so that uparenting won't destroy device
> until caller owns pointer returned by qdev_try_create().

Your patch is indeed nicer in theory, but in practice I'm not sure that
there are no occurrences of object_new + qdev_init_nofail, or
qdev_try_create + object_property_set_bool.  These would be broken, so
I'm going with Fam's.

Paolo

> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/core/qdev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index ee4a083..0139d67 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -151,7 +151,6 @@ DeviceState *qdev_try_create(BusState *bus, const char *type)
>      }
>  
>      qdev_set_parent_bus(dev, bus);
> -    object_unref(OBJECT(dev));
>      return dev;
>  }
>  
> @@ -360,6 +359,7 @@ void qdev_init_nofail(DeviceState *dev)
>                            object_get_typename(OBJECT(dev)));
>          exit(1);
>      }
> +    object_unref(OBJECT(dev));
>  }
>  
>  void qdev_machine_creation_done(void)
>