[libvirt] [PATCH 0/2] Fix possible use-after-free when sending event message
Rework patches/logic most recently posted here: https://www.redhat.com/archives/libvir-list/2017-March/msg00072.html John Ferlan (2): daemon: Rework remoteClientFreeFunc cleanup loops into C macro remote: Fix possible use-after-free when sending event message daemon/remote.c | 158 +++++++++++++++++++------------------------------------- 1 file changed, 52 insertions(+), 106 deletions(-) -- 2.9.3
Rather than 'n' repetitive code segments, let's create a single macro which will make the code easier to read. Signed-off-by: John Ferlan <jferlan@redhat.com> --- daemon/remote.c | 120 +++++++++++++++----------------------------------------- 1 file changed, 31 insertions(+), 89 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index 1c9708c..6fdafce 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -1623,6 +1623,24 @@ void remoteRelayConnectionClosedEvent(virConnectPtr conn ATTRIBUTE_UNUSED, int r &msg); } +#define CLEAN_EVENT_CALLBACK(conn, eventCallbacks, neventCallbacks, name) \ + do { \ + size_t i; \ + for (i = 0; i < neventCallbacks; i++) { \ + int callbackID = eventCallbacks[i]->callbackID; \ + if (callbackID < 0) { \ + VIR_WARN("unexpected incomplete %s callback %zu", name, i); \ + continue; \ + } \ + VIR_DEBUG("Deregistering remote %s event relay %d", \ + name, callbackID); \ + eventCallbacks[i]->callbackID = -1; \ + if (virConnectDomainEventDeregisterAny(conn, callbackID) < 0) \ + VIR_WARN("unexpected %s event deregister failure", name); \ + } \ + VIR_FREE(eventCallbacks); \ + } while (0); + /* * You must hold lock for at least the client * We don't free stuff here, merely disconnect the client's @@ -1637,98 +1655,21 @@ void remoteClientFreeFunc(void *data) /* Deregister event delivery callback */ if (priv->conn) { virIdentityPtr sysident = virIdentityGetSystem(); - size_t i; virIdentitySetCurrent(sysident); - for (i = 0; i < priv->ndomainEventCallbacks; i++) { - int callbackID = priv->domainEventCallbacks[i]->callbackID; - if (callbackID < 0) { - VIR_WARN("unexpected incomplete domain callback %zu", i); - continue; - } - VIR_DEBUG("Deregistering remote domain event relay %d", - callbackID); - priv->domainEventCallbacks[i]->callbackID = -1; - if (virConnectDomainEventDeregisterAny(priv->conn, callbackID) < 0) - VIR_WARN("unexpected domain event deregister failure"); - } - VIR_FREE(priv->domainEventCallbacks); - - for (i = 0; i < priv->nnetworkEventCallbacks; i++) { - int callbackID = priv->networkEventCallbacks[i]->callbackID; - if (callbackID < 0) { - VIR_WARN("unexpected incomplete network callback %zu", i); - continue; - } - VIR_DEBUG("Deregistering remote network event relay %d", - callbackID); - priv->networkEventCallbacks[i]->callbackID = -1; - if (virConnectNetworkEventDeregisterAny(priv->conn, - callbackID) < 0) - VIR_WARN("unexpected network event deregister failure"); - } - VIR_FREE(priv->networkEventCallbacks); - - for (i = 0; i < priv->nstorageEventCallbacks; i++) { - int callbackID = priv->storageEventCallbacks[i]->callbackID; - if (callbackID < 0) { - VIR_WARN("unexpected incomplete storage pool callback %zu", i); - continue; - } - VIR_DEBUG("Deregistering remote storage pool event relay %d", - callbackID); - priv->storageEventCallbacks[i]->callbackID = -1; - if (virConnectStoragePoolEventDeregisterAny(priv->conn, - callbackID) < 0) - VIR_WARN("unexpected storage pool event deregister failure"); - } - VIR_FREE(priv->storageEventCallbacks); - - for (i = 0; i < priv->nnodeDeviceEventCallbacks; i++) { - int callbackID = priv->nodeDeviceEventCallbacks[i]->callbackID; - if (callbackID < 0) { - VIR_WARN("unexpected incomplete node device callback %zu", i); - continue; - } - VIR_DEBUG("Deregistering remote node device event relay %d", - callbackID); - priv->nodeDeviceEventCallbacks[i]->callbackID = -1; - if (virConnectNodeDeviceEventDeregisterAny(priv->conn, - callbackID) < 0) - VIR_WARN("unexpected node device event deregister failure"); - } - VIR_FREE(priv->nodeDeviceEventCallbacks); - - for (i = 0; i < priv->nsecretEventCallbacks; i++) { - int callbackID = priv->secretEventCallbacks[i]->callbackID; - if (callbackID < 0) { - VIR_WARN("unexpected incomplete secret callback %zu", i); - continue; - } - VIR_DEBUG("Deregistering remote secret event relay %d", - callbackID); - priv->secretEventCallbacks[i]->callbackID = -1; - if (virConnectSecretEventDeregisterAny(priv->conn, - callbackID) < 0) - VIR_WARN("unexpected secret event deregister failure"); - } - VIR_FREE(priv->secretEventCallbacks); - - for (i = 0; i < priv->nqemuEventCallbacks; i++) { - int callbackID = priv->qemuEventCallbacks[i]->callbackID; - if (callbackID < 0) { - VIR_WARN("unexpected incomplete qemu monitor callback %zu", i); - continue; - } - VIR_DEBUG("Deregistering remote qemu monitor event relay %d", - callbackID); - priv->qemuEventCallbacks[i]->callbackID = -1; - if (virConnectDomainQemuMonitorEventDeregister(priv->conn, - callbackID) < 0) - VIR_WARN("unexpected qemu monitor event deregister failure"); - } - VIR_FREE(priv->qemuEventCallbacks); + CLEAN_EVENT_CALLBACK(priv->conn, priv->domainEventCallbacks, + priv->ndomainEventCallbacks, "domain"); + CLEAN_EVENT_CALLBACK(priv->conn, priv->networkEventCallbacks, + priv->nnetworkEventCallbacks, "network"); + CLEAN_EVENT_CALLBACK(priv->conn, priv->storageEventCallbacks, + priv->nstorageEventCallbacks, "storage"); + CLEAN_EVENT_CALLBACK(priv->conn, priv->nodeDeviceEventCallbacks, + priv->nnodeDeviceEventCallbacks, "node device"); + CLEAN_EVENT_CALLBACK(priv->conn, priv->secretEventCallbacks, + priv->nsecretEventCallbacks, "secret"); + CLEAN_EVENT_CALLBACK(priv->conn, priv->qemuEventCallbacks, + priv->nqemuEventCallbacks, "qemu monitor"); if (priv->closeRegistered) { if (virConnectUnregisterCloseCallback(priv->conn, @@ -1744,6 +1685,7 @@ void remoteClientFreeFunc(void *data) VIR_FREE(priv); } +#undef CLEAN_EVENT_CALLBACK static void remoteClientCloseFunc(virNetServerClientPtr client) -- 2.9.3
On 03/25/2017 08:51 AM, John Ferlan wrote:
Rather than 'n' repetitive code segments, let's create a single macro which will make the code easier to read.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- daemon/remote.c | 120 +++++++++++++++----------------------------------------- 1 file changed, 31 insertions(+), 89 deletions(-)
diff --git a/daemon/remote.c b/daemon/remote.c index 1c9708c..6fdafce 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -1623,6 +1623,24 @@ void remoteRelayConnectionClosedEvent(virConnectPtr conn ATTRIBUTE_UNUSED, int r &msg); }
+#define CLEAN_EVENT_CALLBACK(conn, eventCallbacks, neventCallbacks, name) \ + do { \ + size_t i; \ + for (i = 0; i < neventCallbacks; i++) { \ + int callbackID = eventCallbacks[i]->callbackID; \ + if (callbackID < 0) { \ + VIR_WARN("unexpected incomplete %s callback %zu", name, i); \ + continue; \ + } \ + VIR_DEBUG("Deregistering remote %s event relay %d", \ + name, callbackID); \ + eventCallbacks[i]->callbackID = -1; \ + if (virConnectDomainEventDeregisterAny(conn, callbackID) < 0) \
This call is different for each loop, and you've made them all identical. You need to make the deregister function an argument to the macro. Otherwise it seems like a nice de-duplication of code. ACK if you fix that.
Based upon an idea and some research by Wang King <king.wang@huawei.com> and xinhua.Cao <caoxinhua@huawei.com>. 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@redhat.com> --- daemon/remote.c | 38 +++++++++++++++++++++----------------- 1 file changed, 21 insertions(+), 17 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index 6fdafce..6bb6239 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); } @@ -3845,7 +3849,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; @@ -3872,7 +3876,7 @@ remoteDispatchConnectDomainEventRegister(virNetServerPtr server ATTRIBUTE_UNUSED rv = 0; cleanup: - VIR_FREE(callback); + remoteEventCallbackFree(callback); if (rv < 0) virNetMessageSaveError(rerr); virMutexUnlock(&priv->lock); @@ -4080,7 +4084,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; @@ -4107,7 +4111,7 @@ remoteDispatchConnectDomainEventRegisterAny(virNetServerPtr server ATTRIBUTE_UNU rv = 0; cleanup: - VIR_FREE(callback); + remoteEventCallbackFree(callback); if (rv < 0) virNetMessageSaveError(rerr); virMutexUnlock(&priv->lock); @@ -4156,7 +4160,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; @@ -4183,7 +4187,7 @@ remoteDispatchConnectDomainEventCallbackRegisterAny(virNetServerPtr server ATTRI rv = 0; cleanup: - VIR_FREE(callback); + remoteEventCallbackFree(callback); if (rv < 0) virNetMessageSaveError(rerr); virObjectUnref(dom); @@ -5666,7 +5670,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; @@ -5693,7 +5697,7 @@ remoteDispatchConnectNetworkEventRegisterAny(virNetServerPtr server ATTRIBUTE_UN rv = 0; cleanup: - VIR_FREE(callback); + remoteEventCallbackFree(callback); if (rv < 0) virNetMessageSaveError(rerr); virObjectUnref(net); @@ -5788,7 +5792,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; @@ -5815,7 +5819,7 @@ remoteDispatchConnectStoragePoolEventRegisterAny(virNetServerPtr server ATTRIBUT rv = 0; cleanup: - VIR_FREE(callback); + remoteEventCallbackFree(callback); if (rv < 0) virNetMessageSaveError(rerr); virObjectUnref(pool); @@ -5909,7 +5913,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; @@ -5936,7 +5940,7 @@ remoteDispatchConnectNodeDeviceEventRegisterAny(virNetServerPtr server ATTRIBUTE rv = 0; cleanup: - VIR_FREE(callback); + remoteEventCallbackFree(callback); if (rv < 0) virNetMessageSaveError(rerr); virObjectUnref(dev); @@ -6030,7 +6034,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; @@ -6057,7 +6061,7 @@ remoteDispatchConnectSecretEventRegisterAny(virNetServerPtr server ATTRIBUTE_UNU rv = 0; cleanup: - VIR_FREE(callback); + remoteEventCallbackFree(callback); if (rv < 0) virNetMessageSaveError(rerr); virObjectUnref(secret); @@ -6146,7 +6150,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, @@ -6173,7 +6177,7 @@ qemuDispatchConnectDomainMonitorEventRegister(virNetServerPtr server ATTRIBUTE_U rv = 0; cleanup: - VIR_FREE(callback); + remoteEventCallbackFree(callback); if (rv < 0) virNetMessageSaveError(rerr); virObjectUnref(dom); -- 2.9.3
participants (2)
-
John Ferlan -
Laine Stump