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

[Michal Privoznik]

On 03/27/2017 06:47 PM, John Ferlan wrote:
> Based upon an idea and some research by Wang King <king.wang at huawei.com>
> and xinhua.Cao <caoxinhua at 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 at 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 at entry=0x7fd9e575b400, msg=msg at 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 at redhat.com>
> ---
>  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);

I don't really see how this could work in the first place. I mean,
@callback is allocated in the chunk above. Cool. Then, in between these
two chunks it's passed (under code name @ref) to
virConnectDomainEventRegisterAny() ...

Aaand I get it now. Just realized that VIR_APPEND_ELEMENT() sets
@callback to NULL. So we are safe here. D'oh.

ACK

Michal