diff --git a/libvirt.spec b/libvirt.spec index 393d50e7e721e845ea10dae3df94868812a1a5af..5d238ff6a682195d3ef4a12be748b2d2bb9f06fd 100644 --- a/libvirt.spec +++ b/libvirt.spec @@ -101,7 +101,7 @@ Summary: Library providing a simple virtualization API Name: libvirt Version: 6.2.0 -Release: 62 +Release: 63 License: LGPLv2+ URL: https://libvirt.org/ @@ -507,6 +507,9 @@ Patch0394: vdpa-support-vdpa-device-migrate.patch Patch0395: virsh-Fix-off-by-one-error-in-udevListInterfacesBySt.patch Patch0396: remote-check-for-negative-array-lengths-before-alloc.patch Patch0397: interface-fix-udev_device_get_sysattr_value-return-v.patch +Patch0398: util-keep-track-of-full-GSource-object-not-source-ID.patch +Patch0399: rpc-mark-source-returned-by-virEventGLibAddSocketWat.patch +Patch0400: rpc-ensure-temporary-GSource-is-removed-from-client-.patch Requires: libvirt-daemon = %{version}-%{release} Requires: libvirt-daemon-config-network = %{version}-%{release} @@ -2243,6 +2246,11 @@ exit 0 %changelog +* Fri May 24 2024 jiangjiacheng - 6.2.0-63 +- util: keep track of full GSource object not source ID number +- rpc: mark source returned by virEventGLibAddSocketWatch as unused +- rpc: ensure temporary GSource is removed from client event loop (CVE-2024-4418) + * Wed Apr 10 2024 caozhongwang - interface: fix udev_device_get_sysattr_value return value check (CVE-2024-2496) - remote: check for negative array lengths before allocation (CVE-2024-2494) diff --git a/rpc-ensure-temporary-GSource-is-removed-from-client-.patch b/rpc-ensure-temporary-GSource-is-removed-from-client-.patch new file mode 100644 index 0000000000000000000000000000000000000000..6604cd12f056c8f110dcf1c102a395b50e14db8a --- /dev/null +++ b/rpc-ensure-temporary-GSource-is-removed-from-client-.patch @@ -0,0 +1,94 @@ +From 351dc443b1c6718b999cc40250ef0b210bcef118 Mon Sep 17 00:00:00 2001 +From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= +Date: Tue, 30 Apr 2024 11:51:15 +0100 +Subject: [PATCH 3/3] rpc: ensure temporary GSource is removed from client + event loop +MIME-Version: 1.0 +Content-Type: text/plain; charset=UTF-8 +Content-Transfer-Encoding: 8bit + +==238721==ERROR: AddressSanitizer: stack-use-after-return on address 0x75cd18709788 at pc 0x75cd3111f907 bp 0x75cd181ff550 sp 0x75cd181ff548 +WRITE of size 4 at 0x75cd18709788 thread T11 + #0 0x75cd3111f906 in virNetClientIOEventFD /usr/src/debug/libvirt/libvirt-10.2.0/build/../src/rpc/virnetclient.c:1634:15 + #1 0x75cd3210d198 (/usr/lib/libglib-2.0.so.0+0x5a198) (BuildId: 0a2311dfbbc6c215dc36f4b6bdd2b4b6fbae55a2) + #2 0x75cd3216c3be (/usr/lib/libglib-2.0.so.0+0xb93be) (BuildId: 0a2311dfbbc6c215dc36f4b6bdd2b4b6fbae55a2) + #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/libvirt-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/libvirt-10.2.0/build/../src/rpc/virnetclient.c:2170:11 + #7 0x75cd311198a8 in virNetClientSendWithReply /usr/src/debug/libvirt/libvirt-10.2.0/build/../src/rpc/virnetclient.c:2198:11 + #8 0x75cd31111653 in virNetClientProgramCall /usr/src/debug/libvirt/libvirt-10.2.0/build/../src/rpc/virnetclientprogram.c:318:9 + #9 0x75cd31241c8f in callFull /usr/src/debug/libvirt/libvirt-10.2.0/build/../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/libvirt-10.2.0/build/src/remote/remote_client_bodies.h:5959:9 + #12 0x75cd31410ff7 in virNetworkGetXMLDesc /usr/src/debug/libvirt/libvirt-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 +Reviewed-by: Ján Tomko +Reported-by: Martin Shirokov +Tested-by: Martin Shirokov +Signed-off-by: Daniel P. Berrangé +--- + 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 f4bb537d50..969c624ae5 100644 +--- a/src/rpc/virnetclient.c ++++ b/src/rpc/virnetclient.c +@@ -1619,7 +1619,7 @@ static int virNetClientIOEventLoop(virNetClientPtr client, + #endif /* !WIN32 */ + int timeout = -1; + virNetMessagePtr msg = NULL; +- g_autoptr(GSource) G_GNUC_UNUSED source = NULL; ++ g_autoptr(GSource) source = NULL; + GIOCondition ev = 0; + struct virNetClientIOEventData data = { + .client = client, +@@ -1683,6 +1683,18 @@ static int virNetClientIOEventLoop(virNetClientPtr client, + + g_main_loop_run(client->eventLoop); + ++ /* ++ * 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 */ +-- +2.33.0 + diff --git a/rpc-mark-source-returned-by-virEventGLibAddSocketWat.patch b/rpc-mark-source-returned-by-virEventGLibAddSocketWat.patch new file mode 100644 index 0000000000000000000000000000000000000000..7c418710d064f0281d0fdd20069498716d442368 --- /dev/null +++ b/rpc-mark-source-returned-by-virEventGLibAddSocketWat.patch @@ -0,0 +1,57 @@ +From 5ab53ff977e17d20b0bf01010f2c3168781a659e Mon Sep 17 00:00:00 2001 +From: =?UTF-8?q?J=C3=A1n=20Tomko?= +Date: Fri, 3 Sep 2021 16:36:54 +0200 +Subject: [PATCH 2/3] rpc: mark source returned by virEventGLibAddSocketWatch + as unused +MIME-Version: 1.0 +Content-Type: text/plain; charset=UTF-8 +Content-Transfer-Encoding: 8bit + +Two users of virEventGLibAddSocketWatch care about the GSource +it returns. + +The other three free it by assigning it to an autofreed variable. + +Mark them with G_GNUC_UNUSED to make this obvious to the reader +and the compiler. + +Signed-off-by: Ján Tomko +Reviewed-by: Michal Privoznik +--- + src/rpc/virnetclient.c | 6 +++--- + 1 file changed, 3 insertions(+), 3 deletions(-) + +diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c +index 0a413f0153..f4bb537d50 100644 +--- a/src/rpc/virnetclient.c ++++ b/src/rpc/virnetclient.c +@@ -826,7 +826,7 @@ virNetClientIOEventTLS(int fd, + static gboolean + virNetClientTLSHandshake(virNetClientPtr client) + { +- g_autoptr(GSource) source = NULL; ++ g_autoptr(GSource) G_GNUC_UNUSED source = NULL; + GIOCondition ev; + int ret; + +@@ -883,7 +883,7 @@ int virNetClientSetTLSSession(virNetClientPtr client, + int ret; + char buf[1]; + int len; +- g_autoptr(GSource) source = NULL; ++ g_autoptr(GSource) G_GNUC_UNUSED source = NULL; + + #ifndef WIN32 + sigset_t oldmask, blockedsigs; +@@ -1619,7 +1619,7 @@ static int virNetClientIOEventLoop(virNetClientPtr client, + #endif /* !WIN32 */ + int timeout = -1; + virNetMessagePtr msg = NULL; +- g_autoptr(GSource) source = NULL; ++ g_autoptr(GSource) G_GNUC_UNUSED source = NULL; + GIOCondition ev = 0; + struct virNetClientIOEventData data = { + .client = client, +-- +2.33.0 + diff --git a/util-keep-track-of-full-GSource-object-not-source-ID.patch b/util-keep-track-of-full-GSource-object-not-source-ID.patch new file mode 100644 index 0000000000000000000000000000000000000000..b8f0adcf909c3539800d6bc6cdeb19552a9e9764 --- /dev/null +++ b/util-keep-track-of-full-GSource-object-not-source-ID.patch @@ -0,0 +1,303 @@ +From 4160ede0261dfccd8156721a22fa48d9cd3c448a Mon Sep 17 00:00:00 2001 +From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= +Date: Tue, 28 Jul 2020 15:54:13 +0100 +Subject: [PATCH 1/3] util: keep track of full GSource object not source ID + number +MIME-Version: 1.0 +Content-Type: text/plain; charset=UTF-8 +Content-Transfer-Encoding: 8bit + +The source ID number is an alternative way to identify a source that has +been added to a GMainContext. Internally when a source ID is given, glib +will lookup the corresponding GSource and use that. The use of a source +ID is racy in some cases though, because it is invalid to continue to +use an ID number after the GSource has been removed. It is thus safer +to use the GSource object directly and have full control over the ref +counting and thus cleanup. + +Reviewed-by: Michal Privoznik +Signed-off-by: Daniel P. Berrangé +--- + src/rpc/virnetclient.c | 27 +++++++------ + src/util/vireventglib.c | 73 +++++++++++++++++++++++------------- + src/util/vireventglibwatch.c | 19 ++++++---- + src/util/vireventglibwatch.h | 13 ++++--- + 4 files changed, 79 insertions(+), 53 deletions(-) + +diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c +index 1c5bef86a1..0a413f0153 100644 +--- a/src/rpc/virnetclient.c ++++ b/src/rpc/virnetclient.c +@@ -826,6 +826,7 @@ virNetClientIOEventTLS(int fd, + static gboolean + virNetClientTLSHandshake(virNetClientPtr client) + { ++ g_autoptr(GSource) source = NULL; + GIOCondition ev; + int ret; + +@@ -840,10 +841,10 @@ virNetClientTLSHandshake(virNetClientPtr client) + else + ev = G_IO_OUT; + +- virEventGLibAddSocketWatch(virNetSocketGetFD(client->sock), +- ev, +- client->eventCtx, +- virNetClientIOEventTLS, client, NULL); ++ source = virEventGLibAddSocketWatch(virNetSocketGetFD(client->sock), ++ ev, ++ client->eventCtx, ++ virNetClientIOEventTLS, client, NULL); + + return TRUE; + } +@@ -882,6 +883,7 @@ int virNetClientSetTLSSession(virNetClientPtr client, + int ret; + char buf[1]; + int len; ++ g_autoptr(GSource) source = NULL; + + #ifndef WIN32 + sigset_t oldmask, blockedsigs; +@@ -934,10 +936,10 @@ int virNetClientSetTLSSession(virNetClientPtr client, + * etc. If we make the grade, it will send us a '\1' byte. + */ + +- virEventGLibAddSocketWatch(virNetSocketGetFD(client->sock), +- G_IO_IN, +- client->eventCtx, +- virNetClientIOEventTLSConfirm, client, NULL); ++ source = virEventGLibAddSocketWatch(virNetSocketGetFD(client->sock), ++ G_IO_IN, ++ client->eventCtx, ++ virNetClientIOEventTLSConfirm, client, NULL); + + #ifndef WIN32 + /* Block SIGWINCH from interrupting poll in curses programs */ +@@ -1617,6 +1619,7 @@ static int virNetClientIOEventLoop(virNetClientPtr client, + #endif /* !WIN32 */ + int timeout = -1; + virNetMessagePtr msg = NULL; ++ g_autoptr(GSource) source = NULL; + GIOCondition ev = 0; + struct virNetClientIOEventData data = { + .client = client, +@@ -1651,10 +1654,10 @@ static int virNetClientIOEventLoop(virNetClientPtr client, + if (client->nstreams) + ev |= G_IO_IN; + +- virEventGLibAddSocketWatch(virNetSocketGetFD(client->sock), +- ev, +- client->eventCtx, +- virNetClientIOEventFD, &data, NULL); ++ source = virEventGLibAddSocketWatch(virNetSocketGetFD(client->sock), ++ ev, ++ client->eventCtx, ++ virNetClientIOEventFD, &data, NULL); + + /* Release lock while poll'ing so other threads + * can stuff themselves on the queue */ +diff --git a/src/util/vireventglib.c b/src/util/vireventglib.c +index 803332a6f8..6e334b3398 100644 +--- a/src/util/vireventglib.c ++++ b/src/util/vireventglib.c +@@ -45,7 +45,7 @@ struct virEventGLibHandle + int fd; + int events; + int removed; +- guint source; ++ GSource *source; + virEventHandleCallback cb; + void *opaque; + virFreeCallback ff; +@@ -56,7 +56,7 @@ struct virEventGLibTimeout + int timer; + int interval; + int removed; +- guint source; ++ GSource *source; + virEventTimeoutCallback cb; + void *opaque; + virFreeCallback ff; +@@ -210,23 +210,25 @@ virEventGLibHandleUpdate(int watch, + if (events == data->events) + goto cleanup; + +- if (data->source != 0) { +- VIR_DEBUG("Removed old handle watch=%d", data->source); +- g_source_remove(data->source); ++ if (data->source != NULL) { ++ VIR_DEBUG("Removed old handle source=%p", data->source); ++ g_source_destroy(data->source); ++ g_source_unref(data->source); + } + + data->source = virEventGLibAddSocketWatch( + data->fd, cond, NULL, virEventGLibHandleDispatch, data, NULL); + + data->events = events; +- VIR_DEBUG("Added new handle watch=%d", data->source); ++ VIR_DEBUG("Added new handle source=%p", data->source); + } else { +- if (data->source == 0) ++ if (data->source == NULL) + goto cleanup; + +- VIR_DEBUG("Removed old handle watch=%d", data->source); +- g_source_remove(data->source); +- data->source = 0; ++ VIR_DEBUG("Removed old handle source=%p", data->source); ++ g_source_destroy(data->source); ++ g_source_unref(data->source); ++ data->source = NULL; + data->events = 0; + } + +@@ -272,9 +274,10 @@ virEventGLibHandleRemove(int watch) + VIR_DEBUG("Remove handle data=%p watch=%d fd=%d", + data, watch, data->fd); + +- if (data->source != 0) { +- g_source_remove(data->source); +- data->source = 0; ++ if (data->source != NULL) { ++ g_source_destroy(data->source); ++ g_source_unref(data->source); ++ data->source = NULL; + data->events = 0; + } + +@@ -309,6 +312,22 @@ virEventGLibTimeoutDispatch(void *opaque) + return TRUE; + } + ++ ++static GSource * ++virEventGLibTimeoutCreate(int interval, ++ struct virEventGLibTimeout *data) ++{ ++ GSource *source = g_timeout_source_new(interval); ++ ++ g_source_set_callback(source, ++ virEventGLibTimeoutDispatch, ++ data, NULL); ++ g_source_attach(source, NULL); ++ ++ return source; ++} ++ ++ + static int + virEventGLibTimeoutAdd(int interval, + virEventTimeoutCallback cb, +@@ -327,9 +346,7 @@ virEventGLibTimeoutAdd(int interval, + data->opaque = opaque; + data->ff = ff; + if (interval >= 0) +- data->source = g_timeout_add(interval, +- virEventGLibTimeoutDispatch, +- data); ++ data->source = virEventGLibTimeoutCreate(interval, data); + + g_ptr_array_add(timeouts, data); + +@@ -390,19 +407,20 @@ virEventGLibTimeoutUpdate(int timer, + VIR_DEBUG("Update timeout data=%p timer=%d interval=%d ms", data, timer, interval); + + if (interval >= 0) { +- if (data->source != 0) +- g_source_remove(data->source); ++ if (data->source != NULL) { ++ g_source_destroy(data->source); ++ g_source_unref(data->source); ++ } + + data->interval = interval; +- data->source = g_timeout_add(data->interval, +- virEventGLibTimeoutDispatch, +- data); ++ data->source = virEventGLibTimeoutCreate(interval, data); + } else { +- if (data->source == 0) ++ if (data->source == NULL) + goto cleanup; + +- g_source_remove(data->source); +- data->source = 0; ++ g_source_destroy(data->source); ++ g_source_unref(data->source); ++ data->source = NULL; + } + + cleanup: +@@ -448,9 +466,10 @@ virEventGLibTimeoutRemove(int timer) + VIR_DEBUG("Remove timeout data=%p timer=%d", + data, timer); + +- if (data->source != 0) { +- g_source_remove(data->source); +- data->source = 0; ++ if (data->source != NULL) { ++ g_source_destroy(data->source); ++ g_source_unref(data->source); ++ data->source = NULL; + } + + /* since the actual timeout deletion is done asynchronously, a timeoutUpdate call may +diff --git a/src/util/vireventglibwatch.c b/src/util/vireventglibwatch.c +index 178707f6b7..b7f3a8786a 100644 +--- a/src/util/vireventglibwatch.c ++++ b/src/util/vireventglibwatch.c +@@ -233,17 +233,20 @@ GSource *virEventGLibCreateSocketWatch(int fd, + #endif /* WIN32 */ + + +-guint virEventGLibAddSocketWatch(int fd, +- GIOCondition condition, +- GMainContext *context, +- virEventGLibSocketFunc func, +- gpointer opaque, +- GDestroyNotify notify) ++GSource * ++virEventGLibAddSocketWatch(int fd, ++ GIOCondition condition, ++ GMainContext *context, ++ virEventGLibSocketFunc func, ++ gpointer opaque, ++ GDestroyNotify notify) + { +- g_autoptr(GSource) source = NULL; ++ GSource *source = NULL; + + source = virEventGLibCreateSocketWatch(fd, condition); + g_source_set_callback(source, (GSourceFunc)func, opaque, notify); + +- return g_source_attach(source, context); ++ g_source_attach(source, context); ++ ++ return source; + } +diff --git a/src/util/vireventglibwatch.h b/src/util/vireventglibwatch.h +index 2f7e61cfba..f57be1f503 100644 +--- a/src/util/vireventglibwatch.h ++++ b/src/util/vireventglibwatch.h +@@ -40,9 +40,10 @@ typedef gboolean (*virEventGLibSocketFunc)(int fd, + GIOCondition condition, + gpointer data); + +-guint virEventGLibAddSocketWatch(int fd, +- GIOCondition condition, +- GMainContext *context, +- virEventGLibSocketFunc func, +- gpointer opaque, +- GDestroyNotify notify); ++GSource *virEventGLibAddSocketWatch(int fd, ++ GIOCondition condition, ++ GMainContext *context, ++ virEventGLibSocketFunc func, ++ gpointer opaque, ++ GDestroyNotify notify) ++ G_GNUC_WARN_UNUSED_RESULT; +-- +2.33.0 +