[PATCH v2 2/2] remote: Fix possible use-after-free when sending event message

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Based upon an idea and some research by Wang King <king.wang@xxxxxxxxxx>
and xinhua.Cao <caoxinhua@xxxxxxxxxx>.

Since we're assigning the 'client' to our callback event lookaside list,
it's imperative that we grab a reference to the object; otherwise, when
the object is unref'd during virNetServerProcessClients when it's determined
that the virNetServerClientIsClosed and the memory is free'd before perhaps
the object event state callbacks are run.  When a virObjectLock() is run,
before sending the message the following trace occurs;

    #0  0x00007fda223d66d8 in virClassIsDerivedFrom
        (klass=0xdeadbeef, parent=0x7fda24c81b40)
         at util/virobject.c:169
    #1  0x00007fda223d6a1e in virObjectIsClass
        (anyobj=anyobj@entry=0x7fd9e575b400, klass=<optimized out>)
         at util/virobject.c:365
    #2  0x00007fda223d6a44 in virObjectLock
        (anyobj=0x7fd9e575b400)
        at util/virobject.c:317
    #3  0x00007fda22507f71 in virNetServerClientSendMessage
        (client=client@entry=0x7fd9e575b400, msg=msg@entry=0x7fd9ec30de90)
        at rpc/virnetserverclient.c:1422
    #4  0x00007fda230d714d in remoteDispatchObjectEventSend
        (client=0x7fd9e575b400, program=0x7fda24c844e0, procnr=348,
         proc=0x7fda2310e5e0 <xdr_remote_domain_event_callback_tunable_msg>,
         data=0x7ffc3857fdb0)
        at remote.c:3803
    #5  0x00007fda230dd71b in remoteRelayDomainEventTunable
        (conn=<optimized out>, dom=0x7fda27cd7660, params=0x7fda27f3aae0,
         nparams=1,opaque=0x7fd9e6c99e00)
        at remote.c:1033
    #6  0x00007fda224484cb in virDomainEventDispatchDefaultFunc
        (conn=0x7fda27cd0120, event=0x7fda2736ea00, cb=0x7fda230dd610
         <remoteRelayDomainEventTunable>, cbopaque=0x7fd9e6c99e00)
        at conf/domain_event.c:1910
    #7  0x00007fda22446871 in virObjectEventStateDispatchCallbacks
        (callbacks=<optimized out>, callbacks=<optimized out>,
         event=0x7fda2736ea00,state=0x7fda24ca3960)
        at conf/object_event.c:722
    #8  virObjectEventStateQueueDispatch
        (callbacks=0x7fda24c65800, queue=0x7ffc3857fe90, state=0x7fda24ca3960)
        at conf/object_event.c:736
    #9  virObjectEventStateFlush (state=0x7fda24ca3960)
        at conf/object_event.c:814
    #10 virObjectEventTimer (timer=<optimized out>, opaque=0x7fda24ca3960)
        at conf/object_event.c:560
    #11 0x00007fda223ae8b9 in virEventPollDispatchTimeouts ()
        at util/vireventpoll.c:458
    #12 virEventPollRunOnce ()
        at util/vireventpoll.c:654
    #13 0x00007fda223ad1d2 in virEventRunDefaultImpl ()
        at util/virevent.c:314
    #14 0x00007fda225046cd in virNetDaemonRun (dmn=0x7fda24c775c0)
        at rpc/virnetdaemon.c:818
    #15 0x00007fda230d6351 in main (argc=<optimized out>, argv=<optimized out>)
        at libvirtd.c:1623

Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
---
 daemon/remote.c | 38 +++++++++++++++++++++-----------------
 1 file changed, 21 insertions(+), 17 deletions(-)

diff --git a/daemon/remote.c b/daemon/remote.c
index 5cdc53e..25a29cf 100644
--- a/daemon/remote.c
+++ b/daemon/remote.c
@@ -124,7 +124,11 @@ remoteDispatchObjectEventSend(virNetServerClientPtr client,
 static void
 remoteEventCallbackFree(void *opaque)
 {
-    VIR_FREE(opaque);
+    daemonClientEventCallbackPtr callback = opaque;
+    if (!callback)
+        return;
+    virObjectUnref(callback->client);
+    VIR_FREE(callback);
 }
 
 
@@ -3896,7 +3900,7 @@ remoteDispatchConnectDomainEventRegister(virNetServerPtr server ATTRIBUTE_UNUSED
      */
     if (VIR_ALLOC(callback) < 0)
         goto cleanup;
-    callback->client = client;
+    callback->client = virObjectRef(client);
     callback->eventID = VIR_DOMAIN_EVENT_ID_LIFECYCLE;
     callback->callbackID = -1;
     callback->legacy = true;
@@ -3923,7 +3927,7 @@ remoteDispatchConnectDomainEventRegister(virNetServerPtr server ATTRIBUTE_UNUSED
     rv = 0;
 
  cleanup:
-    VIR_FREE(callback);
+    remoteEventCallbackFree(callback);
     if (rv < 0)
         virNetMessageSaveError(rerr);
     virMutexUnlock(&priv->lock);
@@ -4131,7 +4135,7 @@ remoteDispatchConnectDomainEventRegisterAny(virNetServerPtr server ATTRIBUTE_UNU
      * success, we use 'ref' to save a copy of the pointer.  */
     if (VIR_ALLOC(callback) < 0)
         goto cleanup;
-    callback->client = client;
+    callback->client = virObjectRef(client);
     callback->eventID = args->eventID;
     callback->callbackID = -1;
     callback->legacy = true;
@@ -4158,7 +4162,7 @@ remoteDispatchConnectDomainEventRegisterAny(virNetServerPtr server ATTRIBUTE_UNU
     rv = 0;
 
  cleanup:
-    VIR_FREE(callback);
+    remoteEventCallbackFree(callback);
     if (rv < 0)
         virNetMessageSaveError(rerr);
     virMutexUnlock(&priv->lock);
@@ -4207,7 +4211,7 @@ remoteDispatchConnectDomainEventCallbackRegisterAny(virNetServerPtr server ATTRI
      * success, we use 'ref' to save a copy of the pointer.  */
     if (VIR_ALLOC(callback) < 0)
         goto cleanup;
-    callback->client = client;
+    callback->client = virObjectRef(client);
     callback->eventID = args->eventID;
     callback->callbackID = -1;
     ref = callback;
@@ -4234,7 +4238,7 @@ remoteDispatchConnectDomainEventCallbackRegisterAny(virNetServerPtr server ATTRI
     rv = 0;
 
  cleanup:
-    VIR_FREE(callback);
+    remoteEventCallbackFree(callback);
     if (rv < 0)
         virNetMessageSaveError(rerr);
     virObjectUnref(dom);
@@ -5717,7 +5721,7 @@ remoteDispatchConnectNetworkEventRegisterAny(virNetServerPtr server ATTRIBUTE_UN
      * success, we use 'ref' to save a copy of the pointer.  */
     if (VIR_ALLOC(callback) < 0)
         goto cleanup;
-    callback->client = client;
+    callback->client = virObjectRef(client);
     callback->eventID = args->eventID;
     callback->callbackID = -1;
     ref = callback;
@@ -5744,7 +5748,7 @@ remoteDispatchConnectNetworkEventRegisterAny(virNetServerPtr server ATTRIBUTE_UN
     rv = 0;
 
  cleanup:
-    VIR_FREE(callback);
+    remoteEventCallbackFree(callback);
     if (rv < 0)
         virNetMessageSaveError(rerr);
     virObjectUnref(net);
@@ -5839,7 +5843,7 @@ remoteDispatchConnectStoragePoolEventRegisterAny(virNetServerPtr server ATTRIBUT
      * success, we use 'ref' to save a copy of the pointer.  */
     if (VIR_ALLOC(callback) < 0)
         goto cleanup;
-    callback->client = client;
+    callback->client = virObjectRef(client);
     callback->eventID = args->eventID;
     callback->callbackID = -1;
     ref = callback;
@@ -5866,7 +5870,7 @@ remoteDispatchConnectStoragePoolEventRegisterAny(virNetServerPtr server ATTRIBUT
     rv = 0;
 
  cleanup:
-    VIR_FREE(callback);
+    remoteEventCallbackFree(callback);
     if (rv < 0)
         virNetMessageSaveError(rerr);
     virObjectUnref(pool);
@@ -5960,7 +5964,7 @@ remoteDispatchConnectNodeDeviceEventRegisterAny(virNetServerPtr server ATTRIBUTE
      * success, we use 'ref' to save a copy of the pointer.  */
     if (VIR_ALLOC(callback) < 0)
         goto cleanup;
-    callback->client = client;
+    callback->client = virObjectRef(client);
     callback->eventID = args->eventID;
     callback->callbackID = -1;
     ref = callback;
@@ -5987,7 +5991,7 @@ remoteDispatchConnectNodeDeviceEventRegisterAny(virNetServerPtr server ATTRIBUTE
     rv = 0;
 
  cleanup:
-    VIR_FREE(callback);
+    remoteEventCallbackFree(callback);
     if (rv < 0)
         virNetMessageSaveError(rerr);
     virObjectUnref(dev);
@@ -6081,7 +6085,7 @@ remoteDispatchConnectSecretEventRegisterAny(virNetServerPtr server ATTRIBUTE_UNU
      * success, we use 'ref' to save a copy of the pointer.  */
     if (VIR_ALLOC(callback) < 0)
         goto cleanup;
-    callback->client = client;
+    callback->client = virObjectRef(client);
     callback->eventID = args->eventID;
     callback->callbackID = -1;
     ref = callback;
@@ -6108,7 +6112,7 @@ remoteDispatchConnectSecretEventRegisterAny(virNetServerPtr server ATTRIBUTE_UNU
     rv = 0;
 
  cleanup:
-    VIR_FREE(callback);
+    remoteEventCallbackFree(callback);
     if (rv < 0)
         virNetMessageSaveError(rerr);
     virObjectUnref(secret);
@@ -6197,7 +6201,7 @@ qemuDispatchConnectDomainMonitorEventRegister(virNetServerPtr server ATTRIBUTE_U
      * success, we use 'ref' to save a copy of the pointer.  */
     if (VIR_ALLOC(callback) < 0)
         goto cleanup;
-    callback->client = client;
+    callback->client = virObjectRef(client);
     callback->callbackID = -1;
     ref = callback;
     if (VIR_APPEND_ELEMENT(priv->qemuEventCallbacks,
@@ -6224,7 +6228,7 @@ qemuDispatchConnectDomainMonitorEventRegister(virNetServerPtr server ATTRIBUTE_U
     rv = 0;
 
  cleanup:
-    VIR_FREE(callback);
+    remoteEventCallbackFree(callback);
     if (rv < 0)
         virNetMessageSaveError(rerr);
     virObjectUnref(dom);
-- 
2.9.3

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux