packages/libvirt
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

!327 fix CVE-2024-4418

Browse Source 
From: @jjiacheng 
Reviewed-by: @caozhongwang, @mdsc 
Signed-off-by: @mdsc
This commit is contained in:
openeuler-ci-bot 2024-05-27 13:09:40 +00:00 committed by Gitee
commit 1c6ad1d631
No known key found for this signature in database
GPG Key ID: 173E9B9CA92EEF8F
4 changed files with 463 additions and 1 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

10
libvirt.spec
View File

@ -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 <jiangjiacheng@huawei.com> - 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 <caozhongwang1@huawei.com>
- interface: fix udev_device_get_sysattr_value return value check (CVE-2024-2496)
- remote: check for negative array lengths before allocation (CVE-2024-2494)

94
rpc-ensure-temporary-GSource-is-removed-from-client-.patch Normal file
View File

@ -0,0 +1,94 @@
From 351dc443b1c6718b999cc40250ef0b210bcef118 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= <berrange@redhat.com>
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 <jtomko@redhat.com>
Reported-by: Martin Shirokov <shirokovmartin@gmail.com>
Tested-by: Martin Shirokov <shirokovmartin@gmail.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
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

57
rpc-mark-source-returned-by-virEventGLibAddSocketWat.patch Normal file
View File

@ -0,0 +1,57 @@
From 5ab53ff977e17d20b0bf01010f2c3168781a659e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?J=C3=A1n=20Tomko?= <jtomko@redhat.com>
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 <jtomko@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
---
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

303
util-keep-track-of-full-GSource-object-not-source-ID.patch Normal file
View File

@ -0,0 +1,303 @@
From 4160ede0261dfccd8156721a22fa48d9cd3c448a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= <berrange@redhat.com>
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 <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
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