From nobody Sat Feb 7 10:24:12 2026 Delivered-To: importer@patchew.org Received-SPF: none (zohomail.com: 8.43.85.245 is neither permitted nor denied by domain of lists.libvirt.org) client-ip=8.43.85.245; envelope-from=devel-bounces@lists.libvirt.org; helo=lists.libvirt.org; Authentication-Results: mx.zohomail.com; spf=none (zohomail.com: 8.43.85.245 is neither permitted nor denied by domain of lists.libvirt.org) smtp.mailfrom=devel-bounces@lists.libvirt.org; dmarc=fail(p=none dis=none) header.from=redhat.com Return-Path: Received: from lists.libvirt.org (lists.libvirt.org [8.43.85.245]) by mx.zohomail.com with SMTPS id 1714654528747327.4124412689398; Thu, 2 May 2024 05:55:28 -0700 (PDT) Received: by lists.libvirt.org (Postfix, from userid 996) id 66FA11B87; Thu, 2 May 2024 08:55:27 -0400 (EDT) Received: from lists.libvirt.org (localhost [IPv6:::1]) by lists.libvirt.org (Postfix) with ESMTP id 446241DFC; Thu, 2 May 2024 08:54:38 -0400 (EDT) Received: by lists.libvirt.org (Postfix, from userid 996) id 3469E1D8F; Thu, 2 May 2024 08:54:35 -0400 (EDT) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by lists.libvirt.org (Postfix) with ESMTPS id 9108B1DE2 for ; Thu, 2 May 2024 08:54:34 -0400 (EDT) Received: from mimecast-mx02.redhat.com (mx-ext.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-584-uUHNba5TMviadLpAHhQoXw-1; Thu, 02 May 2024 08:54:32 -0400 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.rdu2.redhat.com [10.11.54.8]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id A37493C02B47 for ; Thu, 2 May 2024 12:54:32 +0000 (UTC) Received: from toolbox.redhat.com (unknown [10.42.28.138]) by smtp.corp.redhat.com (Postfix) with ESMTP id 37E3BC13FA1; Thu, 2 May 2024 12:54:32 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on lists.libvirt.org X-Spam-Level: X-Spam-Status: No, score=-0.7 required=5.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL,SPF_HELO_NONE autolearn=unavailable autolearn_force=no version=3.4.4 X-MC-Unique: uUHNba5TMviadLpAHhQoXw-1 From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= To: devel@lists.libvirt.org Cc: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= Subject: [PATCH] rpc: ensure temporary GSource is removed from client event loop Date: Thu, 2 May 2024 13:54:31 +0100 Message-ID: <20240502125431.3210288-1-berrange@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.8 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Message-ID-Hash: 43IXZZEWKIWEVNYPT4R37K6QWN4MAKSC X-Message-ID-Hash: 43IXZZEWKIWEVNYPT4R37K6QWN4MAKSC X-MailFrom: berrange@redhat.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; header-match-config-1; header-match-config-2; header-match-config-3; header-match-devel.lists.libvirt.org-0; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; suspicious-header X-Mailman-Version: 3.2.2 Precedence: list List-Id: Development discussions about the libvirt library & tools Archived-At: List-Archive: List-Help: List-Post: List-Subscribe: List-Unsubscribe: Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable X-ZM-MESSAGEID: 1714654529058100001 Users are seeing periodic segfaults from libvirt client apps, especially thread heavy ones like virt-manager. A typical stack trace would end up in the virNetClientIOEventFD method, with illegal access to stale stack data. eg =3D=3D238721=3D=3DERROR: AddressSanitizer: stack-use-after-return on addres= s 0x75cd18709788 at pc 0x75cd3111f907 bp 0x75cd181ff550 sp 0x75cd181ff548 WRITE of size 4 at 0x75cd18709788 thread T11 #0 0x75cd3111f906 in virNetClientIOEventFD /usr/src/debug/libvirt/libvi= rt-10.2.0/build/../src/rpc/virnetclient.c:1634:15 #1 0x75cd3210d198 (/usr/lib/libglib-2.0.so.0+0x5a198) (BuildId: 0a2311= dfbbc6c215dc36f4b6bdd2b4b6fbae55a2) #2 0x75cd3216c3be (/usr/lib/libglib-2.0.so.0+0xb93be) (BuildId: 0a2311= dfbbc6c215dc36f4b6bdd2b4b6fbae55a2) #3 0x75cd3210ddc6 in g_main_loop_run (/usr/lib/libglib-2.0.so.0+0x5adc6= ) (BuildId: 0a2311dfbbc6c215dc36f4b6bdd2b4b6fbae55a2) #4 0x75cd3111a47c in virNetClientIOEventLoop /usr/src/debug/libvirt/lib= virt-10.2.0/build/../src/rpc/virnetclient.c:1722:9 #5 0x75cd3111a47c in virNetClientIO /usr/src/debug/libvirt/libvirt-10.2= .0/build/../src/rpc/virnetclient.c:2002:10 #6 0x75cd3111a47c in virNetClientSendInternal /usr/src/debug/libvirt/li= bvirt-10.2.0/build/../src/rpc/virnetclient.c:2170:11 #7 0x75cd311198a8 in virNetClientSendWithReply /usr/src/debug/libvirt/l= ibvirt-10.2.0/build/../src/rpc/virnetclient.c:2198:11 #8 0x75cd31111653 in virNetClientProgramCall /usr/src/debug/libvirt/lib= virt-10.2.0/build/../src/rpc/virnetclientprogram.c:318:9 #9 0x75cd31241c8f in callFull /usr/src/debug/libvirt/libvirt-10.2.0/bui= ld/../src/remote/remote_driver.c:6054:10 #10 0x75cd31241c8f in call /usr/src/debug/libvirt/libvirt-10.2.0/build/= ../src/remote/remote_driver.c:6076:12 #11 0x75cd31241c8f in remoteNetworkGetXMLDesc /usr/src/debug/libvirt/li= bvirt-10.2.0/build/src/remote/remote_client_bodies.h:5959:9 #12 0x75cd31410ff7 in virNetworkGetXMLDesc /usr/src/debug/libvirt/libvi= rt-10.2.0/build/../src/libvirt-network.c:952:15 The root cause is a bad assumption in the virNetClientIOEventLoop method. This method is run by whichever thread currently owns the buck, and is responsible for handling I/O. Inside a for(;;) loop, this method creates a temporary GSource, adds it to the event loop and runs g_main_loop_run(). When I/O is ready, the GSource callback (virNetClientIOEventFD) will fire and call g_main_loop_quit(), and return G_SOURCE_REMOVE which results in the temporary GSource being destroyed. A g_autoptr() will then remove the last reference. What was overlooked, is that a second thread can come along and while it can't enter virNetClientIOEventLoop, it will register an idle source that uses virNetClientIOWakeup to interrupt the original thread's 'g_main_loop_run' call. When this happens the virNetClientIOEventFD callback never runs, and so the temporary GSource is not destroyed. The g_autoptr() will remove a reference, but by virtue of still being attached to the event context, there is an extra reference held causing GSource to be leaked. The next time 'g_main_loop_run' is called, the original GSource will trigger its callback, and access data that was allocated on the stack by the previous thread, and likely SEGV. To solve this, the thread calling 'g_main_loop_run' must call g_source_destroy, immediately upon return, to guarantee that the temporary GSource is removed. CVE-2024-4418 Reported-by: Martin Shirokov Signed-off-by: Daniel P. Berrang=C3=A9 Reviewed-by: J=C3=A1n Tomko --- src/rpc/virnetclient.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c index 68098b1c8d..147b0d661a 100644 --- a/src/rpc/virnetclient.c +++ b/src/rpc/virnetclient.c @@ -1657,7 +1657,7 @@ static int virNetClientIOEventLoop(virNetClient *clie= nt, #endif /* !WIN32 */ int timeout =3D -1; virNetMessage *msg =3D NULL; - g_autoptr(GSource) G_GNUC_UNUSED source =3D NULL; + g_autoptr(GSource) source =3D NULL; GIOCondition ev =3D 0; struct virNetClientIOEventData data =3D { .client =3D client, @@ -1721,6 +1721,18 @@ static int virNetClientIOEventLoop(virNetClient *cli= ent, =20 g_main_loop_run(client->eventLoop); =20 + /* + * If virNetClientIOEventFD ran, this GSource will already be + * destroyed due to G_SOURCE_REMOVE. It is harmless to re-destroy + * it, since we still own a reference. + * + * If virNetClientIOWakeup ran, it will have interrupted the + * g_main_loop_run call, before virNetClientIOEventFD could + * run, and thus the GSource is still registered, and we need + * to destroy it since it is referencing stack memory for 'data' + */ + g_source_destroy(source); + #ifndef WIN32 ignore_value(pthread_sigmask(SIG_SETMASK, &oldmask, NULL)); #endif /* !WIN32 */ --=20 2.43.0 _______________________________________________ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-leave@lists.libvirt.org