[libvirt] [PATCH] rpc: fix use-after-free when sending event message

Wang King posted 1 patch 7 years, 1 month ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20170210011225.12056-1-king.wang@huawei.com
src/rpc/virnetserverclient.c | 6 ++++++
1 file changed, 6 insertions(+)
[libvirt] [PATCH] rpc: fix use-after-free when sending event message
Posted by Wang King 7 years, 1 month ago
If there is a process with a client which registers event callbacks,
and it calls libvirt's API which uses the same virConnectPtr in that
callback function. When this process exit abnormally lead to client
disconnect, there is a possibility that the main thread is refer to
virServerClient just after the virServerClient been freed by job
thread of libvirtd.

Following is the backtrace:
 #0  0x00007fda223d66d8 in virClassIsDerivedFrom (klass=0xdeadbeef,parent=0x7fda24c81b40)
 #1  0x00007fda223d6a1e in virObjectIsClass (anyobj=anyobj@entry=0x7fd9e575b400,klass=<optimized out>)
 #2  0x00007fda223d6a44 in virObjectLock (anyobj=anyobj@entry=0x7fd9e575b400)
 #3  0x00007fda22507f71 in virNetServerClientSendMessage (client=client@entry=0x7fd9e575b400, msg=msg@entry=0x7fd9ec30de90)
 #4  0x00007fda230d714d in remoteDispatchObjectEventSend (client=0x7fd9e575b400, program=0x7fda24c844e0, procnr=procnr@entry=348, proc=0x7fda2310e5e0 <xdr_remote_domain_event_callback_tunable_msg>, data=data@entry=0x7ffc3857fdb0)
 #5  0x00007fda230dd71b in remoteRelayDomainEventTunable (conn=<optimized out>, dom=0x7fda27cd7660, params=0x7fda27f3aae0, nparams=1, opaque=0x7fd9e6c99e00)
 #6  0x00007fda224484cb in virDomainEventDispatchDefaultFunc (conn=0x7fda27cd0120, event=0x7fda2736ea00, cb=0x7fda230dd610 <remoteRelayDomainEventTunable>, cbopaque=0x7fd9e6c99e00)
 #7  0x00007fda22446871 in virObjectEventStateDispatchCallbacks (callbacks=<optimized out>, callbacks=<optimized out>, event=0x7fda2736ea00, state=0x7fda24ca3960)
 #8  virObjectEventStateQueueDispatch (callbacks=0x7fda24c65800, queue=0x7ffc3857fe90, state=0x7fda24ca3960)
 #9  virObjectEventStateFlush (state=0x7fda24ca3960)
 #10 virObjectEventTimer (timer=<optimized out>, opaque=0x7fda24ca3960)
 #11 0x00007fda223ae8b9 in virEventPollDispatchTimeouts ()
 #12 virEventPollRunOnce ()
 #13 0x00007fda223ad1d2 in virEventRunDefaultImpl ()
 #14 0x00007fda225046cd in virNetDaemonRun (dmn=dmn@entry=0x7fda24c775c0)
 #15 0x00007fda230d6351 in main (argc=<optimized out>, argv=<optimized out>)

(gdb) p *(virNetServerClientPtr)0x7fd9e575b400
$2 = {parent = {parent = {u = {dummy_align1 = 140573849338048, dummy_align2 = 0x7fd9e65ac0c0, s = {magic = 3864707264, refs = 32729}}, klass = 0x7fda00000078}, lock = {lock = {__data = {__lock = 0,
          __count = 0, __owner = 0, __nusers = 0, __kind = 0, __spins = 0, __list = {__prev = 0x0, __next = 0x0}}, __size = '\000' <repeats 39 times>, __align = 0}}}, wantClose = false,
  delayedClose = false, sock = 0x0, auth = 0, readonly = false, tlsCtxt = 0x0, tls = 0x0, sasl = 0x0, sockTimer = 0, identity = 0x0, nrequests = 0, nrequests_max = 0, rx = 0x0, tx = 0x0, filters = 0x0,
  nextFilterID = 0, dispatchFunc = 0x0, dispatchOpaque = 0x0, privateData = 0x0, privateDataFreeFunc = 0x0, privateDataPreExecRestart = 0x0, privateDataCloseFunc = 0x0, keepalive = 0x0}
---
 src/rpc/virnetserverclient.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c
index 81da82c..562516f 100644
--- a/src/rpc/virnetserverclient.c
+++ b/src/rpc/virnetserverclient.c
@@ -1021,6 +1021,12 @@ void virNetServerClientClose(virNetServerClientPtr client)
         client->sock = NULL;
     }
 
+    if (client->privateData &&
+        client->privateDataFreeFunc) {
+        client->privateDataFreeFunc(client->privateData);
+        client->privateData = NULL;
+    }
+
     virObjectUnlock(client);
 }
 
-- 
2.8.3


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] rpc: fix use-after-free when sending event message
Posted by John Ferlan 7 years, 1 month ago

On 02/09/2017 08:12 PM, Wang King wrote:
> If there is a process with a client which registers event callbacks,
> and it calls libvirt's API which uses the same virConnectPtr in that
> callback function. When this process exit abnormally lead to client
> disconnect, there is a possibility that the main thread is refer to
> virServerClient just after the virServerClient been freed by job
> thread of libvirtd.

Could you give some more context on the code paths you're chasing...
Using libvirt function names and the sequence of events. A few details
on the setup path, then a bit more precision over the failure paths. I
assume this is a corner case, but it's really not clear. Is the issue
reproducible?

>From just reading the commit message, I wonder if perhaps a
virObjectRef() needs to be done somewhere in order to ensure that
something that might be referencing something else doesn't get reaped
too soon, but the code below appears to be different...

> 
> Following is the backtrace:
>  #0  0x00007fda223d66d8 in virClassIsDerivedFrom (klass=0xdeadbeef,parent=0x7fda24c81b40)
>  #1  0x00007fda223d6a1e in virObjectIsClass (anyobj=anyobj@entry=0x7fd9e575b400,klass=<optimized out>)
>  #2  0x00007fda223d6a44 in virObjectLock (anyobj=anyobj@entry=0x7fd9e575b400)
>  #3  0x00007fda22507f71 in virNetServerClientSendMessage (client=client@entry=0x7fd9e575b400, msg=msg@entry=0x7fd9ec30de90)
>  #4  0x00007fda230d714d in remoteDispatchObjectEventSend (client=0x7fd9e575b400, program=0x7fda24c844e0, procnr=procnr@entry=348, proc=0x7fda2310e5e0 <xdr_remote_domain_event_callback_tunable_msg>, data=data@entry=0x7ffc3857fdb0)
>  #5  0x00007fda230dd71b in remoteRelayDomainEventTunable (conn=<optimized out>, dom=0x7fda27cd7660, params=0x7fda27f3aae0, nparams=1, opaque=0x7fd9e6c99e00)
>  #6  0x00007fda224484cb in virDomainEventDispatchDefaultFunc (conn=0x7fda27cd0120, event=0x7fda2736ea00, cb=0x7fda230dd610 <remoteRelayDomainEventTunable>, cbopaque=0x7fd9e6c99e00)
>  #7  0x00007fda22446871 in virObjectEventStateDispatchCallbacks (callbacks=<optimized out>, callbacks=<optimized out>, event=0x7fda2736ea00, state=0x7fda24ca3960)
>  #8  virObjectEventStateQueueDispatch (callbacks=0x7fda24c65800, queue=0x7ffc3857fe90, state=0x7fda24ca3960)
>  #9  virObjectEventStateFlush (state=0x7fda24ca3960)
>  #10 virObjectEventTimer (timer=<optimized out>, opaque=0x7fda24ca3960)
>  #11 0x00007fda223ae8b9 in virEventPollDispatchTimeouts ()
>  #12 virEventPollRunOnce ()
>  #13 0x00007fda223ad1d2 in virEventRunDefaultImpl ()
>  #14 0x00007fda225046cd in virNetDaemonRun (dmn=dmn@entry=0x7fda24c775c0)
>  #15 0x00007fda230d6351 in main (argc=<optimized out>, argv=<optimized out>)
> 
> (gdb) p *(virNetServerClientPtr)0x7fd9e575b400
> $2 = {parent = {parent = {u = {dummy_align1 = 140573849338048, dummy_align2 = 0x7fd9e65ac0c0, s = {magic = 3864707264, refs = 32729}}, klass = 0x7fda00000078}, lock = {lock = {__data = {__lock = 0,
>           __count = 0, __owner = 0, __nusers = 0, __kind = 0, __spins = 0, __list = {__prev = 0x0, __next = 0x0}}, __size = '\000' <repeats 39 times>, __align = 0}}}, wantClose = false,
>   delayedClose = false, sock = 0x0, auth = 0, readonly = false, tlsCtxt = 0x0, tls = 0x0, sasl = 0x0, sockTimer = 0, identity = 0x0, nrequests = 0, nrequests_max = 0, rx = 0x0, tx = 0x0, filters = 0x0,
>   nextFilterID = 0, dispatchFunc = 0x0, dispatchOpaque = 0x0, privateData = 0x0, privateDataFreeFunc = 0x0, privateDataPreExecRestart = 0x0, privateDataCloseFunc = 0x0, keepalive = 0x0}
> ---
>  src/rpc/virnetserverclient.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c
> index 81da82c..562516f 100644
> --- a/src/rpc/virnetserverclient.c
> +++ b/src/rpc/virnetserverclient.c
> @@ -1021,6 +1021,12 @@ void virNetServerClientClose(virNetServerClientPtr client)
>          client->sock = NULL;
>      }
>  
> +    if (client->privateData &&
> +        client->privateDataFreeFunc) {
> +        client->privateDataFreeFunc(client->privateData);
> +        client->privateData = NULL;
> +    }
> +

I am wondering how this really fixes what you claim it does or if
perhaps virNetServerClientDispose needs to add that "client->privateData
= NULL;" when it gets run.

and this is very different from the commit description.

Tks -

John
>      virObjectUnlock(client);
>  }
>  
> 

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