Patchwork qdev: fix use-after-free in the error path of qdev_init_nofail

login
register
mail settings
Submitter Anthony Liguori
Date June 27, 2012, 12:41 p.m.
Message ID <1340800884-4571-1-git-send-email-aliguori@us.ibm.com>
Download mbox | patch
Permalink /patch/167645/
State New
Headers show

Comments

Anthony Liguori - June 27, 2012, 12:41 p.m.
From Markus:

Before:

    $ qemu-system-x86_64 -display none -drive if=ide
    qemu-system-x86_64: Device needs media, but drive is empty
    qemu-system-x86_64: Initialization of device ide-hd failed
    [Exit 1 ]

After:

    $ qemu-system-x86_64 -display none -drive if=ide
    qemu-system-x86_64: Device needs media, but drive is empty
    Segmentation fault (core dumped)
    [Exit 139 (SIGSEGV)]

This error always existed as qdev_init() frees the object.  But QOM
goes a bit further and purposefully sets the class pointer to NULL to
help find use-after-free.  It worked :-)

Cc: Andreas Faerber <afaerber@suse.de>
Reported-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/qdev.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)
Andreas Färber - June 27, 2012, 1:14 p.m.
Am 27.06.2012 14:41, schrieb Anthony Liguori:
> From Markus:
> 
> Before:
> 
>     $ qemu-system-x86_64 -display none -drive if=ide
>     qemu-system-x86_64: Device needs media, but drive is empty
>     qemu-system-x86_64: Initialization of device ide-hd failed
>     [Exit 1 ]
> 
> After:
> 
>     $ qemu-system-x86_64 -display none -drive if=ide
>     qemu-system-x86_64: Device needs media, but drive is empty
>     Segmentation fault (core dumped)
>     [Exit 139 (SIGSEGV)]
> 
> This error always existed as qdev_init() frees the object.  But QOM
> goes a bit further and purposefully sets the class pointer to NULL to
> help find use-after-free.  It worked :-)
> 
> Cc: Andreas Faerber <afaerber@suse.de>
> Reported-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

Reviewed-by: Andreas Färber <afaerber@suse.de>

This together with the semantics discussions we're having makes me think
we should attack "QOM'ifying" qdev sooner than later. I.e., reviewing
what naming, chaining, etc. we can already change to align the
TYPE_DEVICE-derived types with the generic QOM infrastructure.

Andreas
Anthony Liguori - June 27, 2012, 1:44 p.m.
On 06/27/2012 08:14 AM, Andreas Färber wrote:
> Am 27.06.2012 14:41, schrieb Anthony Liguori:
>>  From Markus:
>>
>> Before:
>>
>>      $ qemu-system-x86_64 -display none -drive if=ide
>>      qemu-system-x86_64: Device needs media, but drive is empty
>>      qemu-system-x86_64: Initialization of device ide-hd failed
>>      [Exit 1 ]
>>
>> After:
>>
>>      $ qemu-system-x86_64 -display none -drive if=ide
>>      qemu-system-x86_64: Device needs media, but drive is empty
>>      Segmentation fault (core dumped)
>>      [Exit 139 (SIGSEGV)]
>>
>> This error always existed as qdev_init() frees the object.  But QOM
>> goes a bit further and purposefully sets the class pointer to NULL to
>> help find use-after-free.  It worked :-)
>>
>> Cc: Andreas Faerber<afaerber@suse.de>
>> Reported-by: Markus Armbruster<armbru@redhat.com>
>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>
> Reviewed-by: Andreas Färber<afaerber@suse.de>
>
> This together with the semantics discussions we're having makes me think
> we should attack "QOM'ifying" qdev sooner than later. I.e., reviewing
> what naming, chaining, etc. we can already change to align the
> TYPE_DEVICE-derived types with the generic QOM infrastructure.

We really ought to find all uses of qdev_init_nofail() or qdev_try_init() 
(including the sysbus et al derivatives) and add explicit qdev_free() calls in 
the error handling case such that we can remove the qdev_free() calls that are 
done automatically in the init function.

Destructing an object automagically in a virtual method is pretty darn evil and 
really promotes these sort of problems.

Regards,

Anthony Liguori

>
> Andreas
>
Markus Armbruster - June 27, 2012, 2:03 p.m.
Anthony Liguori <aliguori@us.ibm.com> writes:

>>From Markus:
>
> Before:
>
>     $ qemu-system-x86_64 -display none -drive if=ide
>     qemu-system-x86_64: Device needs media, but drive is empty
>     qemu-system-x86_64: Initialization of device ide-hd failed
>     [Exit 1 ]
>
> After:
>
>     $ qemu-system-x86_64 -display none -drive if=ide
>     qemu-system-x86_64: Device needs media, but drive is empty
>     Segmentation fault (core dumped)
>     [Exit 139 (SIGSEGV)]
>
> This error always existed as qdev_init() frees the object.  But QOM
> goes a bit further and purposefully sets the class pointer to NULL to
> help find use-after-free.  It worked :-)
>
> Cc: Andreas Faerber <afaerber@suse.de>
> Reported-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

Tested-by: Markus Armbruster <armbru@redhat.com>
Andreas Färber - June 27, 2012, 2:07 p.m.
Am 27.06.2012 15:44, schrieb Anthony Liguori:
> On 06/27/2012 08:14 AM, Andreas Färber wrote:
>> This together with the semantics discussions we're having makes me think
>> we should attack "QOM'ifying" qdev sooner than later. I.e., reviewing
>> what naming, chaining, etc. we can already change to align the
>> TYPE_DEVICE-derived types with the generic QOM infrastructure.
> 
> We really ought to find all uses of qdev_init_nofail() or
> qdev_try_init() (including the sysbus et al derivatives) and add
> explicit qdev_free() calls in the error handling case such that we can
> remove the qdev_free() calls that are done automatically in the init
> function.

I strongly disagree: We should instead rip out qdev_free() and use
object_delete(). "free" vs. "delete" is still a qdev'ism, no need to
make things worse.

No disagreement on adding explicit QOM-style calls.

Regards,
Andreas

> 
> Destructing an object automagically in a virtual method is pretty darn
> evil and really promotes these sort of problems.
> 
> Regards,
> 
> Anthony Liguori

Patch

diff --git a/hw/qdev.c b/hw/qdev.c
index a6c4c02..af54467 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -258,9 +258,10 @@  int qdev_simple_unplug_cb(DeviceState *dev)
    way is somewhat unclean, and best avoided.  */
 void qdev_init_nofail(DeviceState *dev)
 {
+    const char *typename = object_get_typename(OBJECT(dev));
+
     if (qdev_init(dev) < 0) {
-        error_report("Initialization of device %s failed",
-                     object_get_typename(OBJECT(dev)));
+        error_report("Initialization of device %s failed", typename);
         exit(1);
     }
 }