[PATCH 0/2] remoteDispatchAuthPolkit: Fix lock ordering deadlock if client closes connection during auth
Peter Krempa (2): reproducer remoteDispatchAuthPolkit: Fix lock ordering deadlock if client closes connection during auth src/remote/remote_daemon_dispatch.c | 76 +++++++++++++++-------------- src/rpc/virnetserver.c | 26 +++++++++- src/rpc/virnetserverclient.c | 2 + 3 files changed, 65 insertions(+), 39 deletions(-) -- 2.43.0
To reproduce the issue, start virtqemud with polkit auth enabled while looking at the logs. Run a virsh command which will ask for authentication. After providing the password CTRL+C the virsh instance. Any subsequent connection will get stuck --- src/rpc/virnetserver.c | 26 ++++++++++++++++++++++++-- src/rpc/virnetserverclient.c | 2 ++ 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c index a6c6443c55..f5f50464a2 100644 --- a/src/rpc/virnetserver.c +++ b/src/rpc/virnetserver.c @@ -821,8 +821,30 @@ void virNetServerSetClientAuthenticated(virNetServer *srv, virNetServerClient *client) { - VIR_LOCK_GUARD server_lock = virObjectLockGuard(srv); - VIR_LOCK_GUARD client_lock = virObjectLockGuard(client); + VIR_LOCK_GUARD server_lock; + VIR_LOCK_GUARD client_lock; + + printf("\n\n\n!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!\n"); + printf(" kill virsh now; waiting 10 s\n"); + printf("!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!\n"); + + sleep(10); + + printf("\n\n\n!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!\n"); + printf(" continuing\n"); + printf("!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!\n"); + + server_lock = virObjectLockGuard(srv); + + printf("\n\n\n!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!\n"); + printf(" srv lokced\n"); + printf("!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!\n"); + + client_lock = virObjectLockGuard(client); + + printf("\n\n\n!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!\n"); + printf(" client lokced\n"); + printf("!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!\n"); virNetServerClientSetAuthLocked(client, VIR_NET_SERVER_SERVICE_AUTH_NONE); virNetServerSetClientAuthCompletedLocked(srv, client); diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c index 355aab4b04..bfdf121243 100644 --- a/src/rpc/virnetserverclient.c +++ b/src/rpc/virnetserverclient.c @@ -1013,6 +1013,8 @@ virNetServerClientCloseLocked(virNetServerClient *client) if (client->sock) { g_clear_pointer(&client->sock, virObjectUnref); } + + VIR_DEBUG("client closed"); } -- 2.43.0
Locks in following text: A: virNetServer B: virNetServerClient C: daemonClientPrivate 'virNetServerSetClientAuthenticated' locks A then B 'remoteDispatchAuthPolkit' calls 'virNetServerSetClientAuthenticated' while holding C. If a client closes its connection 'virNetServerProcessClients' with the lock A and B locked will call 'virNetServerClientCloseLocked' which will try to dispose of the 'client' private data by: ref(b); unlock(b); remoteClientFreePrivateCallbacks(); lock(b); unref(b); Unfortunately remoteClientFreePrivateCallbacks() tries lock C. Thus the locks are held in the following order: polkit auth: C -> A connection close: A -> C causing a textbook-example deadlock. To resolve it we can simply drop lock 'C' before calling 'virNetServerSetClientAuthenticated' as the lock is not needed any more. Resolves: https://issues.redhat.com/browse/RHEL-20337 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/remote/remote_daemon_dispatch.c | 76 +++++++++++++++-------------- 1 file changed, 39 insertions(+), 37 deletions(-) diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c index 7daf503b51..aaabd1e56c 100644 --- a/src/remote/remote_daemon_dispatch.c +++ b/src/remote/remote_daemon_dispatch.c @@ -3979,50 +3979,52 @@ remoteDispatchAuthPolkit(virNetServer *server, struct daemonClientPrivate *priv = virNetServerClientGetPrivateData(client); int rv; - VIR_LOCK_GUARD lock = virLockGuardLock(&priv->lock); - - action = virNetServerClientGetReadonly(client) ? - "org.libvirt.unix.monitor" : - "org.libvirt.unix.manage"; - VIR_DEBUG("Start PolicyKit auth %d", virNetServerClientGetFD(client)); - if (virNetServerClientGetAuth(client) != VIR_NET_SERVER_SERVICE_AUTH_POLKIT) { - VIR_ERROR(_("client tried invalid PolicyKit init request")); - goto authfail; - } + VIR_WITH_MUTEX_LOCK_GUARD(&priv->lock) { + action = virNetServerClientGetReadonly(client) ? + "org.libvirt.unix.monitor" : + "org.libvirt.unix.manage"; - if (virNetServerClientGetUNIXIdentity(client, &callerUid, &callerGid, - &callerPid, ×tamp) < 0) { - goto authfail; - } + VIR_DEBUG("Start PolicyKit auth %d", virNetServerClientGetFD(client)); + if (virNetServerClientGetAuth(client) != VIR_NET_SERVER_SERVICE_AUTH_POLKIT) { + VIR_ERROR(_("client tried invalid PolicyKit init request")); + goto authfail; + } - if (timestamp == 0) { - VIR_WARN("Failing polkit auth due to missing client (pid=%lld) start time", - (long long)callerPid); - goto authfail; - } + if (virNetServerClientGetUNIXIdentity(client, &callerUid, &callerGid, + &callerPid, ×tamp) < 0) { + goto authfail; + } - VIR_INFO("Checking PID %lld running as %d", - (long long) callerPid, callerUid); + if (timestamp == 0) { + VIR_WARN("Failing polkit auth due to missing client (pid=%lld) start time", + (long long)callerPid); + goto authfail; + } - rv = virPolkitCheckAuth(action, - callerPid, - timestamp, - callerUid, - NULL, - true); - if (rv == -1) - goto authfail; - else if (rv == -2) - goto authdeny; + VIR_INFO("Checking PID %lld running as %d", + (long long) callerPid, callerUid); - PROBE(RPC_SERVER_CLIENT_AUTH_ALLOW, - "client=%p auth=%d identity=%s", - client, REMOTE_AUTH_POLKIT, ident); - VIR_INFO("Policy allowed action %s from pid %lld, uid %d", - action, (long long) callerPid, callerUid); - ret->complete = 1; + rv = virPolkitCheckAuth(action, + callerPid, + timestamp, + callerUid, + NULL, + true); + if (rv == -1) + goto authfail; + else if (rv == -2) + goto authdeny; + + PROBE(RPC_SERVER_CLIENT_AUTH_ALLOW, + "client=%p auth=%d identity=%s", + client, REMOTE_AUTH_POLKIT, ident); + VIR_INFO("Policy allowed action %s from pid %lld, uid %d", + action, (long long) callerPid, callerUid); + ret->complete = 1; + } + /* this must be called with the private data mutex unlocked */ virNetServerSetClientAuthenticated(server, client); return 0; -- 2.43.0
On Wed, Jan 17, 2024 at 04:13:30PM +0100, Peter Krempa wrote:
Locks in following text: A: virNetServer B: virNetServerClient C: daemonClientPrivate
'virNetServerSetClientAuthenticated' locks A then B
'remoteDispatchAuthPolkit' calls 'virNetServerSetClientAuthenticated' while holding C.
If a client closes its connection 'virNetServerProcessClients' with the lock A and B locked will call 'virNetServerClientCloseLocked' which will try to dispose of the 'client' private data by:
ref(b); unlock(b); remoteClientFreePrivateCallbacks(); lock(b); unref(b);
Unfortunately remoteClientFreePrivateCallbacks() tries lock C.
Thus the locks are held in the following order:
polkit auth: C -> A connection close: A -> C
causing a textbook-example deadlock. To resolve it we can simply drop lock 'C' before calling 'virNetServerSetClientAuthenticated' as the lock is not needed any more.
Resolves: https://issues.redhat.com/browse/RHEL-20337 Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
participants (2)
-
Martin Kletzander -
Peter Krempa