From fe8f1c8b8650c8ab5e7d27338f4538582651bd14 Mon Sep 17 00:00:00 2001 From: John Ferlan Date: Mon, 27 Mar 2017 12:47:37 -0400 Subject: [PATCH] remote: Fix possible use-after-free when sending event message Based upon an idea and some research by Wang King and xinhua.Cao . 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=) 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 , data=0x7ffc3857fdb0) at remote.c:3803 #5 0x00007fda230dd71b in remoteRelayDomainEventTunable (conn=, dom=0x7fda27cd7660, params=0x7fda27f3aae0, nparams=1,opaque=0x7fd9e6c99e00) at remote.c:1033 #6 0x00007fda224484cb in virDomainEventDispatchDefaultFunc (conn=0x7fda27cd0120, event=0x7fda2736ea00, cb=0x7fda230dd610 , cbopaque=0x7fd9e6c99e00) at conf/domain_event.c:1910 #7 0x00007fda22446871 in virObjectEventStateDispatchCallbacks (callbacks=, callbacks=, 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=, 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=, argv=) at libvirtd.c:1623 Signed-off-by: John Ferlan --- daemon/remote.c | 38 +++++++++++++++++++++----------------- 1 file changed, 21 insertions(+), 17 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index 1e13f51e8b..0dbb250ff0 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); } @@ -3894,7 +3898,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; @@ -3921,7 +3925,7 @@ remoteDispatchConnectDomainEventRegister(virNetServerPtr server ATTRIBUTE_UNUSED rv = 0; cleanup: - VIR_FREE(callback); + remoteEventCallbackFree(callback); if (rv < 0) virNetMessageSaveError(rerr); virMutexUnlock(&priv->lock); @@ -4129,7 +4133,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; @@ -4156,7 +4160,7 @@ remoteDispatchConnectDomainEventRegisterAny(virNetServerPtr server ATTRIBUTE_UNU rv = 0; cleanup: - VIR_FREE(callback); + remoteEventCallbackFree(callback); if (rv < 0) virNetMessageSaveError(rerr); virMutexUnlock(&priv->lock); @@ -4205,7 +4209,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; @@ -4232,7 +4236,7 @@ remoteDispatchConnectDomainEventCallbackRegisterAny(virNetServerPtr server ATTRI rv = 0; cleanup: - VIR_FREE(callback); + remoteEventCallbackFree(callback); if (rv < 0) virNetMessageSaveError(rerr); virObjectUnref(dom); @@ -5715,7 +5719,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; @@ -5742,7 +5746,7 @@ remoteDispatchConnectNetworkEventRegisterAny(virNetServerPtr server ATTRIBUTE_UN rv = 0; cleanup: - VIR_FREE(callback); + remoteEventCallbackFree(callback); if (rv < 0) virNetMessageSaveError(rerr); virObjectUnref(net); @@ -5837,7 +5841,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; @@ -5864,7 +5868,7 @@ remoteDispatchConnectStoragePoolEventRegisterAny(virNetServerPtr server ATTRIBUT rv = 0; cleanup: - VIR_FREE(callback); + remoteEventCallbackFree(callback); if (rv < 0) virNetMessageSaveError(rerr); virObjectUnref(pool); @@ -5958,7 +5962,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; @@ -5985,7 +5989,7 @@ remoteDispatchConnectNodeDeviceEventRegisterAny(virNetServerPtr server ATTRIBUTE rv = 0; cleanup: - VIR_FREE(callback); + remoteEventCallbackFree(callback); if (rv < 0) virNetMessageSaveError(rerr); virObjectUnref(dev); @@ -6079,7 +6083,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; @@ -6106,7 +6110,7 @@ remoteDispatchConnectSecretEventRegisterAny(virNetServerPtr server ATTRIBUTE_UNU rv = 0; cleanup: - VIR_FREE(callback); + remoteEventCallbackFree(callback); if (rv < 0) virNetMessageSaveError(rerr); virObjectUnref(secret); @@ -6195,7 +6199,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, @@ -6222,7 +6226,7 @@ qemuDispatchConnectDomainMonitorEventRegister(virNetServerPtr server ATTRIBUTE_U rv = 0; cleanup: - VIR_FREE(callback); + remoteEventCallbackFree(callback); if (rv < 0) virNetMessageSaveError(rerr); virObjectUnref(dom); -- GitLab