Re: [Qemu-devel] Use-after-free during unrealize in system_reset

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Jun 09, 2014 at 09:51:56AM +0200, Paolo Bonzini wrote:
> Il 08/06/2014 16:52, Michael S. Tsirkin ha scritto:
> >On Sun, Jun 08, 2014 at 04:40:56PM +0200, Paolo Bonzini wrote:
> >>Il 08/06/2014 12:46, Michael S. Tsirkin ha scritto:
> >>>Tested-by: Michael S. Tsirkin <mst@redhat.com>
> >>
> >>You probably tested the reversal, actually. :)
> >>
> >>Actually, there is a reason for it.  "Unassembling" the device
> >>(unparent) should come after "powering it down" (unrealize).
> >>
> >>However, the bus is missing a recursive unrealization of the devices
> >>below it prior to calling bc->unrealize:
> >>
> >>diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> >>index e65a5aa..4282491 100644
> >>--- a/hw/core/qdev.c
> >>+++ b/hw/core/qdev.c
> >>@@ -567,32 +567,35 @@ static void bus_set_realized(Object *obj, bool value,
> >>Error **errp)
> >> {
> >>     BusState *bus = BUS(obj);
> >>     BusClass *bc = BUS_GET_CLASS(bus);
> >>+    BusChild *kid;
> >>     Error *local_err = NULL;
> >>
> >>     if (value && !bus->realized) {
> >>         if (bc->realize) {
> >>             bc->realize(bus, &local_err);
> >>-
> >>-            if (local_err != NULL) {
> >>-                goto error;
> >>-            }
> >>-
> >>         }
> >>+
> >>+        /* TODO: recursive realization */
> >>     } else if (!value && bus->realized) {
> >>-        if (bc->unrealize) {
> >>+        QTAILQ_FOREACH(kid, &bus->children, sibling) {
> >>+            DeviceState *dev = kid->child;
> >>+            object_property_set_bool(OBJECT(dev), false, "realized",
> >>+                                     &local_err);
> >>+            if (local_err != NULL) {
> >>+                break;
> >>+            }
> >>+        }
> >>+        if (bc->unrealize && local_err == NULL) {
> >>             bc->unrealize(bus, &local_err);
> >>-
> >>-            if (local_err != NULL) {
> >>-                goto error;
> >>-            }
> >>         }
> >>     }
> >>
> >>+    if (local_err != NULL) {
> >>+        error_propagate(errp, local_err);
> >>+        return;
> >>+    }
> >>+
> >>     bus->realized = value;
> >>-    return;
> >>-
> >>-error:
> >>-    error_propagate(errp, local_err);
> >> }
> >>
> >> void qbus_create_inplace(void *bus, size_t size, const char *typename,
> >>
> >>
> >>This seems to fix the bug too.
> >>
> >>Paolo
> >
> >Want to submit as a proper patch?
> 
> I can, but I suppose Andreas will watch this when he's back and comment.
> 
> Paolo

I don't think we want to keep such serious memory corruptors out of
tree. We can always revert the fix if necessary, for now
it's impossible for people to test hotplug properly,
this blocks progress.

-- 
MST