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

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

[Fam Zheng]

Since 69382d8b (qdev: Fix object reference leak in case device.realize()
fails), object_property_set_bool could release the object. The error
path wants the type name, so hold an reference before realizing it.

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

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index ee4a083..5783442 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -354,12 +354,14 @@ void qdev_init_nofail(DeviceState *dev)
 
     assert(!dev->realized);
 
+    object_ref(OBJECT(dev));
     object_property_set_bool(OBJECT(dev), true, "realized", &err);
     if (err) {
         error_reportf_err(err, "Initialization of device %s failed: ",
                           object_get_typename(OBJECT(dev)));
         exit(1);
     }
+    object_unref(OBJECT(dev));
 }
 
 void qdev_machine_creation_done(void)
-- 
2.7.4

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

[John Snow]

On 08/01/2016 11:41 PM, Fam Zheng wrote:
> Since 69382d8b (qdev: Fix object reference leak in case device.realize()
> fails), object_property_set_bool could release the object. The error
> path wants the type name, so hold an reference before realizing it.
>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  hw/core/qdev.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index ee4a083..5783442 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -354,12 +354,14 @@ void qdev_init_nofail(DeviceState *dev)
>
>      assert(!dev->realized);
>
> +    object_ref(OBJECT(dev));
>      object_property_set_bool(OBJECT(dev), true, "realized", &err);
>      if (err) {
>          error_reportf_err(err, "Initialization of device %s failed: ",
>                            object_get_typename(OBJECT(dev)));
>          exit(1);
>      }
> +    object_unref(OBJECT(dev));
>  }
>
>  void qdev_machine_creation_done(void)
>

Thanks :)

(For the list: this fixes qcow2 iotest 051. This is for-2.7.)

Reviewed-by: John Snow <jsnow@redhat.com>

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

[Paolo Bonzini]

On 02/08/2016 06:00, John Snow wrote:
> 
> 
> On 08/01/2016 11:41 PM, Fam Zheng wrote:
>> Since 69382d8b (qdev: Fix object reference leak in case device.realize()
>> fails), object_property_set_bool could release the object. The error
>> path wants the type name, so hold an reference before realizing it.
>>
>> Cc: Igor Mammedov <imammedo@redhat.com>
>> Signed-off-by: Fam Zheng <famz@redhat.com>
>> ---
>>  hw/core/qdev.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>> index ee4a083..5783442 100644
>> --- a/hw/core/qdev.c
>> +++ b/hw/core/qdev.c
>> @@ -354,12 +354,14 @@ void qdev_init_nofail(DeviceState *dev)
>>
>>      assert(!dev->realized);
>>
>> +    object_ref(OBJECT(dev));
>>      object_property_set_bool(OBJECT(dev), true, "realized", &err);
>>      if (err) {
>>          error_reportf_err(err, "Initialization of device %s failed: ",
>>                            object_get_typename(OBJECT(dev)));
>>          exit(1);
>>      }
>> +    object_unref(OBJECT(dev));
>>  }
>>
>>  void qdev_machine_creation_done(void)
>>
> 
> Thanks :)
> 
> (For the list: this fixes qcow2 iotest 051. This is for-2.7.)
> 
> Reviewed-by: John Snow <jsnow@redhat.com>

Queued, thanks.

Paolo

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

[Igor Mammedov]

On Tue, 2 Aug 2016 00:00:27 -0400
John Snow <jsnow@redhat.com> wrote:

> On 08/01/2016 11:41 PM, Fam Zheng wrote:
> > Since 69382d8b (qdev: Fix object reference leak in case device.realize()
> > fails), object_property_set_bool could release the object. The error
> > path wants the type name, so hold an reference before realizing it.
> >
> > Cc: Igor Mammedov <imammedo@redhat.com>
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  hw/core/qdev.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > index ee4a083..5783442 100644
> > --- a/hw/core/qdev.c
> > +++ b/hw/core/qdev.c
> > @@ -354,12 +354,14 @@ void qdev_init_nofail(DeviceState *dev)
> >
> >      assert(!dev->realized);
> >
> > +    object_ref(OBJECT(dev));
> >      object_property_set_bool(OBJECT(dev), true, "realized", &err);
> >      if (err) {
> >          error_reportf_err(err, "Initialization of device %s failed: ",
> >                            object_get_typename(OBJECT(dev)));
> >          exit(1);
> >      }
> > +    object_unref(OBJECT(dev));
> >  }
> >
> >  void qdev_machine_creation_done(void)
> >  
> 
> Thanks :)
> 
> (For the list: this fixes qcow2 iotest 051. This is for-2.7.)
I don't see any error at 'make check' time,
could you provide reproducer CLI?

> 
> Reviewed-by: John Snow <jsnow@redhat.com>

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

[Fam Zheng]

On Tue, 08/02 10:14, Igor Mammedov wrote:
> On Tue, 2 Aug 2016 00:00:27 -0400
> John Snow <jsnow@redhat.com> wrote:
> 
> > On 08/01/2016 11:41 PM, Fam Zheng wrote:
> > > Since 69382d8b (qdev: Fix object reference leak in case device.realize()
> > > fails), object_property_set_bool could release the object. The error
> > > path wants the type name, so hold an reference before realizing it.
> > >
> > > Cc: Igor Mammedov <imammedo@redhat.com>
> > > Signed-off-by: Fam Zheng <famz@redhat.com>
> > > ---
> > >  hw/core/qdev.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > > index ee4a083..5783442 100644
> > > --- a/hw/core/qdev.c
> > > +++ b/hw/core/qdev.c
> > > @@ -354,12 +354,14 @@ void qdev_init_nofail(DeviceState *dev)
> > >
> > >      assert(!dev->realized);
> > >
> > > +    object_ref(OBJECT(dev));
> > >      object_property_set_bool(OBJECT(dev), true, "realized", &err);
> > >      if (err) {
> > >          error_reportf_err(err, "Initialization of device %s failed: ",
> > >                            object_get_typename(OBJECT(dev)));
> > >          exit(1);
> > >      }
> > > +    object_unref(OBJECT(dev));
> > >  }
> > >
> > >  void qdev_machine_creation_done(void)
> > >  
> > 
> > Thanks :)
> > 
> > (For the list: this fixes qcow2 iotest 051. This is for-2.7.)
> I don't see any error at 'make check' time,
> could you provide reproducer CLI?

It crashes for me with something as simple as:

 $ qemu-system-x86_64 -drive if=ide

Fam

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

[Igor Mammedov]

On Tue,  2 Aug 2016 11:41:41 +0800
Fam Zheng <famz@redhat.com> wrote:

> Since 69382d8b (qdev: Fix object reference leak in case device.realize()
> fails), object_property_set_bool could release the object. The error
> path wants the type name, so hold an reference before realizing it.
> 
> Cc: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  hw/core/qdev.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index ee4a083..5783442 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -354,12 +354,14 @@ void qdev_init_nofail(DeviceState *dev)
>  
>      assert(!dev->realized);
>  
> +    object_ref(OBJECT(dev));
>      object_property_set_bool(OBJECT(dev), true, "realized", &err);
>      if (err) {
>          error_reportf_err(err, "Initialization of device %s failed: ",
>                            object_get_typename(OBJECT(dev)));
>          exit(1);
>      }
> +    object_unref(OBJECT(dev));
>  }
>  
>  void qdev_machine_creation_done(void)

I'm not sure that this is the right fix, commit 69382d8b only affects
reference created by realize() itself.
Probably reference counting wrong somewhere else,
for typical device call sequence is following:

 qdev_create() {
    object_new() -> ref == 1
    qdev_set_parent_bus() -> ref == 2
    object_unref() -> ref == 1
 } -> ref == 1
 
 do property settings and other stuff ...

 
 qdev_init_nofail() { called with ref == 1
    object_property_set_bool(true, "realized")
    if error:
          ref == 1
    else:
          ref == 2 (+1 for implicitly assigned parent)
 }

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

[Fam Zheng]

On Tue, 08/02 09:55, Igor Mammedov wrote:
>  qdev_init_nofail() { called with ref == 1

Yes it does.

>     object_property_set_bool(true, "realized")
>     if error:
>           ref == 1
            ^

This is not the case for qdev, the object is actually released by
object_property_set_bool if fail.

The problem seems to be that qdev_create doesn't set OBJECT(dev)->parent,
because it eventually calls object_property_add_link instead of
object_property_add_child.

>     else:
>           ref == 2 (+1 for implicitly assigned parent)
>  }

Fam

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

[Paolo Bonzini]

On 02/08/2016 09:55, Igor Mammedov wrote:
> On Tue,  2 Aug 2016 11:41:41 +0800
> Fam Zheng <famz@redhat.com> wrote:
> 
>> Since 69382d8b (qdev: Fix object reference leak in case device.realize()
>> fails), object_property_set_bool could release the object. The error
>> path wants the type name, so hold an reference before realizing it.
>>
>> Cc: Igor Mammedov <imammedo@redhat.com>
>> Signed-off-by: Fam Zheng <famz@redhat.com>
>> ---
>>  hw/core/qdev.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>> index ee4a083..5783442 100644
>> --- a/hw/core/qdev.c
>> +++ b/hw/core/qdev.c
>> @@ -354,12 +354,14 @@ void qdev_init_nofail(DeviceState *dev)
>>  
>>      assert(!dev->realized);
>>  
>> +    object_ref(OBJECT(dev));
>>      object_property_set_bool(OBJECT(dev), true, "realized", &err);
>>      if (err) {
>>          error_reportf_err(err, "Initialization of device %s failed: ",
>>                            object_get_typename(OBJECT(dev)));
>>          exit(1);
>>      }
>> +    object_unref(OBJECT(dev));
>>  }
>>  
>>  void qdev_machine_creation_done(void)
> 
> I'm not sure that this is the right fix, commit 69382d8b only affects
> reference created by realize() itself.
> Probably reference counting wrong somewhere else,
> for typical device call sequence is following:
> 
>  qdev_create() {
>     object_new() -> ref == 1
>     qdev_set_parent_bus() -> ref == 2
>     object_unref() -> ref == 1
>  } -> ref == 1
>  
>  do property settings and other stuff ...
> 
>  
>  qdev_init_nofail() { called with ref == 1
>     object_property_set_bool(true, "realized")
>     if error:
>           ref == 1

If there is an error and the device was unattached, you get here:

    if (unattached_parent) {
        object_unparent(OBJECT(dev));
        unattached_count--;
    }

and object_unparent undoes qdev_set_parent_bus so that the refcount
drops to 0.

Paolo

>     else:
>           ref == 2 (+1 for implicitly assigned parent)
>  }
> 
> 

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

[Igor Mammedov]

On Tue, 2 Aug 2016 15:05:28 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 02/08/2016 09:55, Igor Mammedov wrote:
> > On Tue,  2 Aug 2016 11:41:41 +0800
> > Fam Zheng <famz@redhat.com> wrote:
> >   
> >> Since 69382d8b (qdev: Fix object reference leak in case device.realize()
> >> fails), object_property_set_bool could release the object. The error
> >> path wants the type name, so hold an reference before realizing it.
> >>
> >> Cc: Igor Mammedov <imammedo@redhat.com>
> >> Signed-off-by: Fam Zheng <famz@redhat.com>
> >> ---
> >>  hw/core/qdev.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> >> index ee4a083..5783442 100644
> >> --- a/hw/core/qdev.c
> >> +++ b/hw/core/qdev.c
> >> @@ -354,12 +354,14 @@ void qdev_init_nofail(DeviceState *dev)
> >>  
> >>      assert(!dev->realized);
> >>  
> >> +    object_ref(OBJECT(dev));
> >>      object_property_set_bool(OBJECT(dev), true, "realized", &err);
> >>      if (err) {
> >>          error_reportf_err(err, "Initialization of device %s failed: ",
> >>                            object_get_typename(OBJECT(dev)));
> >>          exit(1);
> >>      }
> >> +    object_unref(OBJECT(dev));
> >>  }
> >>  
> >>  void qdev_machine_creation_done(void)  
> > 
> > I'm not sure that this is the right fix, commit 69382d8b only affects
> > reference created by realize() itself.
> > Probably reference counting wrong somewhere else,
> > for typical device call sequence is following:
> > 
> >  qdev_create() {
> >     object_new() -> ref == 1
> >     qdev_set_parent_bus() -> ref == 2
> >     object_unref() -> ref == 1
> >  } -> ref == 1
> >  
> >  do property settings and other stuff ...
> > 
> >  
> >  qdev_init_nofail() { called with ref == 1
> >     object_property_set_bool(true, "realized")
> >     if error:
> >           ref == 1  
> 
> If there is an error and the device was unattached, you get here:
> 
>     if (unattached_parent) {
>         object_unparent(OBJECT(dev));
>         unattached_count--;
>     }
> 
> and object_unparent undoes qdev_set_parent_bus so that the refcount
> drops to 0.
I've sent v2 of patch that fixes issue with error handling
in slightly another way
(A little bit more generic and consistent than just wrapping realize with ref/unref)

> 
> Paolo
> 
> >     else:
> >           ref == 2 (+1 for implicitly assigned parent)
> >  }
> > 
> >