Compare commits

...

4 Commits

Author SHA1 Message Date
Daniel P. Berrangé fa34176617 Disable -Werror on mingw builds
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
2024-06-05 13:44:49 +01:00
Daniel P. Berrangé c5bb34b10f Fix crash in event loop (CVE-2024-4418), event loop stall, GSource leak & udev leak
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
2024-06-05 13:31:00 +01:00
Daniel P. Berrangé ee6c35ff11 Fix multiple crashes listing interfaces
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
2024-03-12 16:25:51 +00:00
jonathanspw dfeabf11e5 fix regression for default input bus in libvirt >= 9.3 2024-01-03 13:42:36 -06:00
8 changed files with 456 additions and 2 deletions
@@ -0,0 +1,40 @@
From 76cdc7adf55723ff8da146bd3c15c64d0afd5d93 Mon Sep 17 00:00:00 2001
From: Martin Kletzander <mkletzan@redhat.com>
Date: Tue, 27 Feb 2024 16:20:12 +0100
Subject: [PATCH] Fix off-by-one error in udevListInterfacesByStatus
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Ever since this function was introduced in 2012 it could've tried
filling in an extra interface name. That was made worse in 2019 when
the caller functions started accepting NULL arrays of size 0.
This is assigned CVE-2024-1441.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Reported-by: Alexander Kuznetsov <kuznetsovam@altlinux.org>
Fixes: 5a33366f5c0b18c93d161bd144f9f079de4ac8ca
Fixes: d6064e2759a24e0802f363e3a810dc5a7d7ebb15
Reviewed-by: Ján Tomko <jtomko@redhat.com>
(cherry picked from commit c664015fe3a7bf59db26686e9ed69af011c6ebb8)
---
src/interface/interface_backend_udev.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/interface/interface_backend_udev.c b/src/interface/interface_backend_udev.c
index fb6799ed94..4091483060 100644
--- a/src/interface/interface_backend_udev.c
+++ b/src/interface/interface_backend_udev.c
@@ -222,7 +222,7 @@ udevListInterfacesByStatus(virConnectPtr conn,
g_autoptr(virInterfaceDef) def = NULL;
/* Ensure we won't exceed the size of our array */
- if (count > names_len)
+ if (count >= names_len)
break;
path = udev_list_entry_get_name(dev_entry);
--
2.43.0
@@ -0,0 +1,41 @@
From 3499354e12a1c1832bf4030693a64e03ceb79d05 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= <berrange@redhat.com>
Date: Wed, 5 Jun 2024 11:16:21 +0100
Subject: [PATCH] interface: fix udev reference leak with invalid flags
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
The udevInterfaceGetXMLDesc method takes a reference on the udev
driver as its first action. If the virCheckFlags() condition
fails, however, this reference is never released.
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
src/interface/interface_backend_udev.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/interface/interface_backend_udev.c b/src/interface/interface_backend_udev.c
index fdf11a8318..e1a50389c9 100644
--- a/src/interface/interface_backend_udev.c
+++ b/src/interface/interface_backend_udev.c
@@ -1027,12 +1027,14 @@ static char *
udevInterfaceGetXMLDesc(virInterfacePtr ifinfo,
unsigned int flags)
{
- struct udev *udev = udev_ref(driver->udev);
+ struct udev *udev = NULL;
g_autoptr(virInterfaceDef) ifacedef = NULL;
char *xmlstr = NULL;
virCheckFlags(VIR_INTERFACE_XML_INACTIVE, NULL);
+ udev = udev_ref(driver->udev);
+
/* Recursively build up the interface XML based on the requested
* interface name
*/
--
2.45.1
@@ -0,0 +1,90 @@
From c120b31f826cd51127d28f8beaa61ac0d5f03048 Mon Sep 17 00:00:00 2001
From: Dmitry Frolov <frolov@swemel.ru>
Date: Tue, 12 Sep 2023 15:56:47 +0300
Subject: [PATCH] interface: fix udev_device_get_sysattr_value return value
check
Reviewing the code I found that return value of function
udev_device_get_sysattr_value() is dereferenced without a check.
udev_device_get_sysattr_value() may return NULL by number of reasons.
v2: VIR_DEBUG added, replaced STREQ(NULLSTR()) with STREQ_NULLABLE()
v3: More checks added, to skip earlier. More verbose VIR_DEBUG.
Signed-off-by: Dmitry Frolov <frolov@swemel.ru>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
(cherry picked from commit 2ca94317ac642a70921947150ced8acc674ccdc8)
---
src/interface/interface_backend_udev.c | 26 +++++++++++++++++++-------
1 file changed, 19 insertions(+), 7 deletions(-)
diff --git a/src/interface/interface_backend_udev.c b/src/interface/interface_backend_udev.c
index a0485ddd21..fb6799ed94 100644
--- a/src/interface/interface_backend_udev.c
+++ b/src/interface/interface_backend_udev.c
@@ -23,6 +23,7 @@
#include <dirent.h>
#include <libudev.h>
+#include "virlog.h"
#include "virerror.h"
#include "virfile.h"
#include "datatypes.h"
@@ -40,6 +41,8 @@
#define VIR_FROM_THIS VIR_FROM_INTERFACE
+VIR_LOG_INIT("interface.interface_backend_udev");
+
struct udev_iface_driver {
struct udev *udev;
/* pid file FD, ensures two copies of the driver can't use the same root */
@@ -354,11 +357,20 @@ udevConnectListAllInterfaces(virConnectPtr conn,
const char *macaddr;
g_autoptr(virInterfaceDef) def = NULL;
- path = udev_list_entry_get_name(dev_entry);
- dev = udev_device_new_from_syspath(udev, path);
- name = udev_device_get_sysname(dev);
+ if (!(path = udev_list_entry_get_name(dev_entry))) {
+ VIR_DEBUG("Skipping interface, path == NULL");
+ continue;
+ }
+ if (!(dev = udev_device_new_from_syspath(udev, path))) {
+ VIR_DEBUG("Skipping interface '%s', dev == NULL", path);
+ continue;
+ }
+ if (!(name = udev_device_get_sysname(dev))) {
+ VIR_DEBUG("Skipping interface '%s', name == NULL", path);
+ continue;
+ }
macaddr = udev_device_get_sysattr_value(dev, "address");
- status = STREQ(udev_device_get_sysattr_value(dev, "operstate"), "up");
+ status = STREQ_NULLABLE(udev_device_get_sysattr_value(dev, "operstate"), "up");
def = udevGetMinimalDefForDevice(dev);
if (!virConnectListAllInterfacesCheckACL(conn, def)) {
@@ -964,9 +976,9 @@ udevGetIfaceDef(struct udev *udev, const char *name)
/* MTU */
mtu_str = udev_device_get_sysattr_value(dev, "mtu");
- if (virStrToLong_ui(mtu_str, NULL, 10, &mtu) < 0) {
+ if (!mtu_str || virStrToLong_ui(mtu_str, NULL, 10, &mtu) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Could not parse MTU value '%1$s'"), mtu_str);
+ _("Could not parse MTU value '%1$s'"), NULLSTR(mtu_str));
goto error;
}
ifacedef->mtu = mtu;
@@ -1089,7 +1101,7 @@ udevInterfaceIsActive(virInterfacePtr ifinfo)
goto cleanup;
/* Check if it's active or not */
- status = STREQ(udev_device_get_sysattr_value(dev, "operstate"), "up");
+ status = STREQ_NULLABLE(udev_device_get_sysattr_value(dev, "operstate"), "up");
udev_device_unref(dev);
--
2.43.0
@@ -0,0 +1,49 @@
From 98f1cf88fa7e0f992d93f376418fbfb3996a9690 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= <berrange@redhat.com>
Date: Fri, 17 May 2024 14:55:24 +0100
Subject: [PATCH] rpc: avoid leak of GSource in use for interrupting main loop
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
We never release the reference on the GSource created for
interrupting the main loop, nor do we remove it from the
main context if our thread is woken up prior to the wakeup
callback firing.
This can result in a leak of GSource objects, along with an
ever growing list of GSources attached to the main context,
which will gradually slow down execution of the loop, as
several operations are O(N) for the number of attached GSource
objects.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
src/rpc/virnetclient.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c
index 147b0d661a..6d424eb599 100644
--- a/src/rpc/virnetclient.c
+++ b/src/rpc/virnetclient.c
@@ -1946,7 +1946,7 @@ static int virNetClientIO(virNetClient *client,
/* Check to see if another thread is dispatching */
if (client->haveTheBuck) {
/* Force other thread to wakeup from poll */
- GSource *wakeup = g_idle_source_new();
+ g_autoptr(GSource) wakeup = g_idle_source_new();
g_source_set_callback(wakeup, virNetClientIOWakeup, client->eventLoop, NULL);
g_source_attach(wakeup, client->eventCtx);
@@ -1968,6 +1968,7 @@ static int virNetClientIO(virNetClient *client,
return -1;
}
+ g_source_destroy(wakeup);
VIR_DEBUG("Woken up from sleep head=%p call=%p",
client->waitDispatch, thiscall);
/* Three reasons we can be woken up
--
2.45.1
@@ -0,0 +1,99 @@
From 8074d64dc2eca846d6a61efe1a9b7428a0ce1dd1 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] 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
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
==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 68098b1c8d..147b0d661a 100644
--- a/src/rpc/virnetclient.c
+++ b/src/rpc/virnetclient.c
@@ -1657,7 +1657,7 @@ static int virNetClientIOEventLoop(virNetClient *client,
#endif /* !WIN32 */
int timeout = -1;
virNetMessage *msg = NULL;
- g_autoptr(GSource) G_GNUC_UNUSED source = NULL;
+ g_autoptr(GSource) source = NULL;
GIOCondition ev = 0;
struct virNetClientIOEventData data = {
.client = client,
@@ -1721,6 +1721,18 @@ static int virNetClientIOEventLoop(virNetClient *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.45.1
@@ -0,0 +1,76 @@
From 7cb03e6a28e465c49f0cabe8fe2e7d21edb5aadf Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= <berrange@redhat.com>
Date: Mon, 18 Dec 2023 12:17:18 +0000
Subject: [PATCH] rpc: fix race in waking up client event loop
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
The first thread to issue a client RPC request will own the event
loop execution, sitting in the virNetClientIOEventLoop function.
It releases the client lock while running:
virNetClientUnlock()
g_main_loop_run()
virNetClientLock()
If a second thread arrives with an RPC request, it will queue it
for the first thread to process. To inform the first thread that
there's a new request it calls g_main_loop_quit() to break it out
of the main loop.
This works if the first thread is in g_main_loop_run() at that
time. There is a small window of opportunity, however, where
the first thread has released the client lock, but not yet got
into g_main_loop_run(). If that happens, the wakeup from the
second thread is lost.
This patch deals with that by changing the way the wakeup is
performed. Instead of directly calling g_main_loop_quit(), the
second thread creates an idle source to run the quit function
from within the first thread. This guarantees that the first
thread will see the wakeup.
Tested by: Fima Shevrin <efim.shevrin@virtuozzo.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Denis V. Lunev <den@openvz.org>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
src/rpc/virnetclient.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c
index 4ab8af68c5..68098b1c8d 100644
--- a/src/rpc/virnetclient.c
+++ b/src/rpc/virnetclient.c
@@ -1848,6 +1848,15 @@ static void virNetClientIOUpdateCallback(virNetClient *client,
}
+static gboolean virNetClientIOWakeup(gpointer opaque)
+{
+ GMainLoop *loop = opaque;
+
+ g_main_loop_quit(loop);
+
+ return G_SOURCE_REMOVE;
+}
+
/*
* This function sends a message to remote server and awaits a reply
*
@@ -1925,7 +1934,9 @@ static int virNetClientIO(virNetClient *client,
/* Check to see if another thread is dispatching */
if (client->haveTheBuck) {
/* Force other thread to wakeup from poll */
- g_main_loop_quit(client->eventLoop);
+ GSource *wakeup = g_idle_source_new();
+ g_source_set_callback(wakeup, virNetClientIOWakeup, client->eventLoop, NULL);
+ g_source_attach(wakeup, client->eventCtx);
/* If we are non-blocking, detach the thread and keep the call in the
* queue. */
--
2.43.0
@@ -0,0 +1,31 @@
From c9056e682a8a67dc29e39eb01392fcf8ee978c31 Mon Sep 17 00:00:00 2001
From: Jonathan Wright <jonathan@almalinux.org>
Date: Wed, 3 Jan 2024 09:26:59 -0600
Subject: [PATCH] conf: Restore setting default bus for input devices
Prior to v9.3.0-rc1~30 we used to set default bus for <input/>
devices, during XML parsing. In the commit this code was moved to
a post parse callback. But somehow the line that sets the bus in
one specific case disappeared. Bring it back.
Resolves: https://gitlab.com/libvirt/libvirt/-/issues/577
Fixes: c4bc4d3b82fbe22e03c986ca896090f481df5c10
Signed-off-by: Jonathan Wright <jonathan@almalinux.org>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
---
src/conf/domain_postparse.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/conf/domain_postparse.c b/src/conf/domain_postparse.c
index e79913b73f..ee27023f3e 100644
--- a/src/conf/domain_postparse.c
+++ b/src/conf/domain_postparse.c
@@ -657,6 +657,7 @@ virDomainInputDefPostParse(virDomainInputDef *input,
if ((input->type == VIR_DOMAIN_INPUT_TYPE_MOUSE ||
input->type == VIR_DOMAIN_INPUT_TYPE_KBD) &&
(ARCH_IS_X86(def->os.arch) || def->os.arch == VIR_ARCH_NONE)) {
+ input->bus = VIR_DOMAIN_INPUT_BUS_PS2;
} else if (ARCH_IS_S390(def->os.arch) ||
input->type == VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH) {
input->bus = VIR_DOMAIN_INPUT_BUS_VIRTIO;
--
+30 -2
View File
@@ -229,7 +229,7 @@
Summary: Library providing a simple virtualization API
Name: libvirt
Version: 9.7.0
Release: 1%{?dist}
Release: 4%{?dist}
License: GPL-2.0-or-later AND LGPL-2.1-only AND LGPL-2.1-or-later AND OFL-1.1
URL: https://libvirt.org/
@@ -238,6 +238,19 @@ URL: https://libvirt.org/
%endif
Source: https://download.libvirt.org/%{?mainturl}libvirt-%{version}.tar.xz
# Fix regression in default input bus
# https://gitlab.com/libvirt/libvirt/-/issues/577
Patch1: libvirt-regression-input-default-bus.patch
# Crash listing interfaces with missing link status
# https://bugzilla.redhat.com/show_bug.cgi?id=2266014
Patch2: 0001-interface-fix-udev_device_get_sysattr_value-return-v.patch
# CVE listing interfaces with missized array
Patch3: 0001-Fix-off-by-one-error-in-udevListInterfacesByStatus.patch
Patch4: 0001-rpc-ensure-temporary-GSource-is-removed-from-client-.patch
Patch5: 0001-rpc-fix-race-in-waking-up-client-event-loop.patch
Patch6: 0001-rpc-avoid-leak-of-GSource-in-use-for-interrupting-ma.patch
Patch7: 0001-interface-fix-udev-reference-leak-with-invalid-flags.patch
Requires: libvirt-daemon = %{version}-%{release}
Requires: libvirt-daemon-config-network = %{version}-%{release}
Requires: libvirt-daemon-config-nwfilter = %{version}-%{release}
@@ -1364,7 +1377,8 @@ export SOURCE_DATE_EPOCH=$(stat --printf='%Y' %{_specdir}/libvirt.spec)
-Dtests=disabled \
-Dudev=disabled \
-Dwireshark_dissector=disabled \
-Dyajl=disabled
-Dyajl=disabled \
%{?enable_werror}
%mingw_ninja
%endif
@@ -2470,6 +2484,20 @@ exit 0
%changelog
* Wed Jun 5 2024 Daniel P. Berrangé <berrange@redhat.com> - 9.7.0-4
- Fix crash in event loop (CVE-2024-4418)
- Fix I/O stall when multiple threads issue RPC calls
- Fix leak of GSource handle
- Fix leak of udev reference (rhbz #2266017)
- Disable -Werror on mingw builds
* Tue Mar 12 2024 Daniel P. Berrangé <berrange@redhat.com> - 9.7.0-3
- Fix crash listing interfaces with missing link status attribute (rhbz #2266014)
- Fix crash listing interfaces with missized array (CVE-2024-1441)
* Wed Jan 03 2024 Jonathan Wright <jonathan@almalinux.org> - 9.7.0-2
- conf: fix regression for default input bus
* Fri Sep 1 2023 Daniel P. Berrangé <berrange@redhat.com> - 9.7.0-1
- Update to version 9.7.0