diff -Nru libvirt-6.0.0/debian/changelog libvirt-6.0.0/debian/changelog --- libvirt-6.0.0/debian/changelog 2024-03-30 20:05:56.000000000 +0000 +++ libvirt-6.0.0/debian/changelog 2024-04-16 17:20:13.000000000 +0000 @@ -1,4 +1,24 @@ -libvirt (6.0.0-0ubuntu8.18) focal; urgency=medium +libvirt (6.0.0-0ubuntu8.20) focal; urgency=medium + + * d/p/u/lp2059272-2-qemu-Wait-qemuProcessReconnect-threads-in-cleanup.patch: + Remove patch. It is not possible to wait for qemuProcessReconnect() + in cleanup: it talks to QEMU monitor, which blocks on replies from + event loop, but it's already stopped at cleanup, delaying shutdown. + + * d/p/u/lp2059272-2-qemu-Do-not-save-XML-in-shutdown-on-init.patch: + Instead of waiting at cleanup for threads which might be blocked + thus would _not even reach_ the function that causes the problem, + just skip that function if it is _actually reached_ while daemon + shutdown is in progress. That is in the init path and would just + run again anyway the next time libvirtd is started (LP: #2059272) + + * NOTE: This package contains the changes from 6.0.0-0ubuntu8.18 and + 6.0.0-0ubuntu8.17 in focal-proposed (with symbolic changelog entry) + superseded by 6.0.0-0ubuntu8.19 in focal-security. + + -- Mauricio Faria de Oliveira Tue, 16 Apr 2024 14:20:13 -0300 + +libvirt (6.0.0-0ubuntu8.20~ubuntu8.18) focal; urgency=medium * d/p/u/lp2059272-1-qemu-Fix-potential-crash-during-driver-cleanup.patch: On QEMU driver cleanup, release (stop) the worker thread pool _first_, @@ -13,13 +33,32 @@ -- Mauricio Faria de Oliveira Sat, 30 Mar 2024 17:05:56 -0300 -libvirt (6.0.0-0ubuntu8.17) focal; urgency=medium +libvirt (6.0.0-0ubuntu8.20~ubuntu8.17) focal; urgency=medium * d/p/u/lp-1989078-*.patch: allow arm64 to lock its OVMF/AAVMF resources (LP: #1989078) -- Christian Ehrhardt Mon, 09 Jan 2023 08:48:16 +0100 +libvirt (6.0.0-0ubuntu8.19) focal-security; urgency=medium + + * SECURITY UPDATE: off-by-one in udevListInterfacesByStatus() + - debian/patches/CVE-2024-1441.patch: properly check count in + src/interface/interface_backend_udev.c. + - CVE-2024-1441 + * SECURITY UPDATE: crash in RPC library + - debian/patches/CVE-2024-2494.patch: check values in + src/remote/remote_daemon_dispatch.c, src/rpc/gendispatch.pl. + - CVE-2024-2494 + * SECURITY UPDATE: null pointer deref in udevConnectListAllInterfaces() + - debian/patches/CVE-2024-2496.patch: fix udev_device_get_sysattr_value + return value check in src/interface/interface_backend_udev.c. + - CVE-2024-2496 + * NOTE: This package does _not_ contain the changes from + 6.0.0-0ubuntu8.18 in focal-proposed. + + -- Marc Deslauriers Fri, 12 Apr 2024 13:50:27 -0400 + libvirt (6.0.0-0ubuntu8.16) focal-security; urgency=medium * SECURITY UPDATE: crash via double-free memory issue diff -Nru libvirt-6.0.0/debian/patches/CVE-2024-1441.patch libvirt-6.0.0/debian/patches/CVE-2024-1441.patch --- libvirt-6.0.0/debian/patches/CVE-2024-1441.patch 1970-01-01 00:00:00.000000000 +0000 +++ libvirt-6.0.0/debian/patches/CVE-2024-1441.patch 2024-04-12 17:49:17.000000000 +0000 @@ -0,0 +1,59 @@ +From c664015fe3a7bf59db26686e9ed69af011c6ebb8 Mon Sep 17 00:00:00 2001 +From: Martin Kletzander +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 +Reported-by: Alexander Kuznetsov +Fixes: 5a33366f5c0b18c93d161bd144f9f079de4ac8ca +Fixes: d6064e2759a24e0802f363e3a810dc5a7d7ebb15 +Reviewed-by: Ján Tomko +--- + NEWS.rst | 15 +++++++++++++++ + src/interface/interface_backend_udev.c | 2 +- + 2 files changed, 16 insertions(+), 1 deletion(-) + +#--- a/NEWS.rst +#+++ b/NEWS.rst +#@@ -312,6 +312,21 @@ v9.2.0 (2023-04-01) +# v9.1.0 (2023-03-01) +# =================== +# +#+ * ``CVE-2024-1441``: Fix off-by-one error leading to a crash +#+ +#+ In **libvirt-1.0.0** there were couple of interface listing APIs +#+ introduced which had an off-by-one error. That error could lead to a +#+ very rare crash if an array was passed to those functions which did +#+ not fit all the interfaces. +#+ +#+ In **libvirt-5.10** a check for non-NULL arrays has been adjusted to +#+ allow for NULL arrays with size 0 instead of rejecting all NULL +#+ arrays. However that made the above issue significantly worse since +#+ that off-by-one error now did not write beyond an array, but +#+ dereferenced said NULL pointer making the crash certain in a +#+ specific scenario in which a NULL array of size 0 was passed to the +#+ aforementioned functions. +#+ +# * **Removed features** +# +# * vbox: removed support for version 5.2 and 6.0 APIs +--- a/src/interface/interface_backend_udev.c ++++ b/src/interface/interface_backend_udev.c +@@ -220,7 +220,7 @@ udevListInterfacesByStatus(virConnectPtr + virInterfaceDefPtr def; + + /* 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); diff -Nru libvirt-6.0.0/debian/patches/CVE-2024-2494.patch libvirt-6.0.0/debian/patches/CVE-2024-2494.patch --- libvirt-6.0.0/debian/patches/CVE-2024-2494.patch 1970-01-01 00:00:00.000000000 +0000 +++ libvirt-6.0.0/debian/patches/CVE-2024-2494.patch 2024-04-12 17:49:48.000000000 +0000 @@ -0,0 +1,209 @@ +From 8a3f8d957507c1f8223fdcf25a3ff885b15557f2 Mon Sep 17 00:00:00 2001 +From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= +Date: Fri, 15 Mar 2024 10:47:50 +0000 +Subject: [PATCH] remote: check for negative array lengths before allocation +MIME-Version: 1.0 +Content-Type: text/plain; charset=UTF-8 +Content-Transfer-Encoding: 8bit + +While the C API entry points will validate non-negative lengths +for various parameters, the RPC server de-serialization code +will need to allocate memory for arrays before entering the C +API. These allocations will thus happen before the non-negative +length check is performed. + +Passing a negative length to the g_new0 function will usually +result in a crash due to the negative length being treated as +a huge positive number. + +This was found and diagnosed by ALT Linux Team with AFLplusplus. + +CVE-2024-2494 +Reviewed-by: Michal Privoznik +Found-by: Alexandr Shashkin +Co-developed-by: Alexander Kuznetsov +Signed-off-by: Daniel P. Berrangé +--- + src/remote/remote_daemon_dispatch.c | 65 +++++++++++++++++++++++++++++ + src/rpc/gendispatch.pl | 5 +++ + 2 files changed, 70 insertions(+) + +--- a/src/remote/remote_daemon_dispatch.c ++++ b/src/remote/remote_daemon_dispatch.c +@@ -2307,6 +2307,10 @@ remoteDispatchDomainGetSchedulerParamete + if (!conn) + goto cleanup; + ++ if (args->nparams < 0) { ++ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams must be non-negative")); ++ goto cleanup; ++ } + if (args->nparams > REMOTE_DOMAIN_SCHEDULER_PARAMETERS_MAX) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large")); + goto cleanup; +@@ -2355,6 +2359,10 @@ remoteDispatchDomainGetSchedulerParamete + if (!conn) + goto cleanup; + ++ if (args->nparams < 0) { ++ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams must be non-negative")); ++ goto cleanup; ++ } + if (args->nparams > REMOTE_DOMAIN_SCHEDULER_PARAMETERS_MAX) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large")); + goto cleanup; +@@ -2516,6 +2524,10 @@ remoteDispatchDomainBlockStatsFlags(virN + goto cleanup; + flags = args->flags; + ++ if (args->nparams < 0) { ++ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams must be non-negative")); ++ goto cleanup; ++ } + if (args->nparams > REMOTE_DOMAIN_BLOCK_STATS_PARAMETERS_MAX) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large")); + goto cleanup; +@@ -2750,6 +2762,14 @@ remoteDispatchDomainGetVcpuPinInfo(virNe + if (!(dom = get_nonnull_domain(conn, args->dom))) + goto cleanup; + ++ if (args->ncpumaps < 0) { ++ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("ncpumaps must be non-negative")); ++ goto cleanup; ++ } ++ if (args->maplen < 0) { ++ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("maplen must be non-negative")); ++ goto cleanup; ++ } + if (args->ncpumaps > REMOTE_VCPUINFO_MAX) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("ncpumaps > REMOTE_VCPUINFO_MAX")); + goto cleanup; +@@ -2845,6 +2865,11 @@ remoteDispatchDomainGetEmulatorPinInfo(v + if (!(dom = get_nonnull_domain(conn, args->dom))) + goto cleanup; + ++ if (args->maplen < 0) { ++ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("maplen must be non-negative")); ++ goto cleanup; ++ } ++ + /* Allocate buffers to take the results */ + if (args->maplen > 0 && + VIR_ALLOC_N(cpumaps, args->maplen) < 0) +@@ -2893,6 +2918,14 @@ remoteDispatchDomainGetVcpus(virNetServe + if (!(dom = get_nonnull_domain(conn, args->dom))) + goto cleanup; + ++ if (args->maxinfo < 0) { ++ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("maxinfo must be non-negative")); ++ goto cleanup; ++ } ++ if (args->maplen < 0) { ++ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("maxinfo must be non-negative")); ++ goto cleanup; ++ } + if (args->maxinfo > REMOTE_VCPUINFO_MAX) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("maxinfo > REMOTE_VCPUINFO_MAX")); + goto cleanup; +@@ -3140,6 +3173,10 @@ remoteDispatchDomainGetMemoryParameters( + + flags = args->flags; + ++ if (args->nparams < 0) { ++ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams must be non-negative")); ++ goto cleanup; ++ } + if (args->nparams > REMOTE_DOMAIN_MEMORY_PARAMETERS_MAX) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large")); + goto cleanup; +@@ -3200,6 +3237,10 @@ remoteDispatchDomainGetNumaParameters(vi + + flags = args->flags; + ++ if (args->nparams < 0) { ++ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams must be non-negative")); ++ goto cleanup; ++ } + if (args->nparams > REMOTE_DOMAIN_NUMA_PARAMETERS_MAX) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large")); + goto cleanup; +@@ -3260,6 +3301,10 @@ remoteDispatchDomainGetBlkioParameters(v + + flags = args->flags; + ++ if (args->nparams < 0) { ++ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams must be non-negative")); ++ goto cleanup; ++ } + if (args->nparams > REMOTE_DOMAIN_BLKIO_PARAMETERS_MAX) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large")); + goto cleanup; +@@ -3321,6 +3366,10 @@ remoteDispatchNodeGetCPUStats(virNetServ + + flags = args->flags; + ++ if (args->nparams < 0) { ++ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams must be non-negative")); ++ goto cleanup; ++ } + if (args->nparams > REMOTE_NODE_CPU_STATS_MAX) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large")); + goto cleanup; +@@ -3389,6 +3438,10 @@ remoteDispatchNodeGetMemoryStats(virNetS + + flags = args->flags; + ++ if (args->nparams < 0) { ++ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams must be non-negative")); ++ goto cleanup; ++ } + if (args->nparams > REMOTE_NODE_MEMORY_STATS_MAX) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large")); + goto cleanup; +@@ -3570,6 +3623,10 @@ remoteDispatchDomainGetBlockIoTune(virNe + if (!conn) + goto cleanup; + ++ if (args->nparams < 0) { ++ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams must be non-negative")); ++ goto cleanup; ++ } + if (args->nparams > REMOTE_DOMAIN_BLOCK_IO_TUNE_PARAMETERS_MAX) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large")); + goto cleanup; +@@ -5128,6 +5185,10 @@ remoteDispatchDomainGetInterfaceParamete + + flags = args->flags; + ++ if (args->nparams < 0) { ++ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams must be non-negative")); ++ goto cleanup; ++ } + if (args->nparams > REMOTE_DOMAIN_INTERFACE_PARAMETERS_MAX) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large")); + goto cleanup; +@@ -5350,6 +5411,10 @@ remoteDispatchNodeGetMemoryParameters(vi + + flags = args->flags; + ++ if (args->nparams < 0) { ++ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams must be non-negative")); ++ goto cleanup; ++ } + if (args->nparams > REMOTE_NODE_MEMORY_PARAMETERS_MAX) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large")); + goto cleanup; +--- a/src/rpc/gendispatch.pl ++++ b/src/rpc/gendispatch.pl +@@ -1067,6 +1067,11 @@ elsif ($mode eq "server") { + print "\n"; + + if ($single_ret_as_list) { ++ print " if (args->$single_ret_list_max_var < 0) {\n"; ++ print " virReportError(VIR_ERR_RPC,\n"; ++ print " \"%s\", _(\"max$single_ret_list_name must be non-negative\"));\n"; ++ print " goto cleanup;\n"; ++ print " }\n"; + print " if (args->$single_ret_list_max_var > $single_ret_list_max_define) {\n"; + print " virReportError(VIR_ERR_RPC,\n"; + print " \"%s\", _(\"max$single_ret_list_name > $single_ret_list_max_define\"));\n"; diff -Nru libvirt-6.0.0/debian/patches/CVE-2024-2496.patch libvirt-6.0.0/debian/patches/CVE-2024-2496.patch --- libvirt-6.0.0/debian/patches/CVE-2024-2496.patch 1970-01-01 00:00:00.000000000 +0000 +++ libvirt-6.0.0/debian/patches/CVE-2024-2496.patch 2024-04-12 17:49:54.000000000 +0000 @@ -0,0 +1,86 @@ +Backport of: + +From 2ca94317ac642a70921947150ced8acc674ccdc8 Mon Sep 17 00:00:00 2001 +From: Dmitry Frolov +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 +Reviewed-by: Martin Kletzander +--- + src/interface/interface_backend_udev.c | 26 +++++++++++++++++++------- + 1 file changed, 19 insertions(+), 7 deletions(-) + +--- a/src/interface/interface_backend_udev.c ++++ b/src/interface/interface_backend_udev.c +@@ -23,6 +23,7 @@ + #include + #include + ++#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 */ +@@ -370,11 +373,20 @@ udevConnectListAllInterfaces(virConnectP + const char *macaddr; + virInterfaceDefPtr def; + +- 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)) { +@@ -999,9 +1011,9 @@ udevGetIfaceDef(struct udev *udev, const + + /* 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 '%s'"), mtu_str); ++ _("Could not parse MTU value '%1$s'"), NULLSTR(mtu_str)); + goto error; + } + ifacedef->mtu = mtu; +@@ -1128,7 +1140,7 @@ udevInterfaceIsActive(virInterfacePtr if + 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); + diff -Nru libvirt-6.0.0/debian/patches/series libvirt-6.0.0/debian/patches/series --- libvirt-6.0.0/debian/patches/series 2024-03-30 20:05:56.000000000 +0000 +++ libvirt-6.0.0/debian/patches/series 2024-04-16 17:03:03.000000000 +0000 @@ -186,7 +186,10 @@ CVE-2021-4147-6pre1.patch CVE-2021-4147-6.patch CVE-2022-0897.patch +CVE-2024-1441.patch +CVE-2024-2494.patch +CVE-2024-2496.patch ubuntu/lp-1989078-apparmor-Fix-QEMU-access-for-UEFI-variable-files.patch ubuntu/lp-1989078-apparmor-Allow-locking-AAVMF-firmware.patch ubuntu/lp2059272-1-qemu-Fix-potential-crash-during-driver-cleanup.patch -ubuntu/lp2059272-2-qemu-Wait-qemuProcessReconnect-threads-in-cleanup.patch +ubuntu/lp2059272-2-qemu-Do-not-save-XML-in-shutdown-on-init.patch diff -Nru libvirt-6.0.0/debian/patches/ubuntu/lp2059272-2-qemu-Do-not-save-XML-in-shutdown-on-init.patch libvirt-6.0.0/debian/patches/ubuntu/lp2059272-2-qemu-Do-not-save-XML-in-shutdown-on-init.patch --- libvirt-6.0.0/debian/patches/ubuntu/lp2059272-2-qemu-Do-not-save-XML-in-shutdown-on-init.patch 1970-01-01 00:00:00.000000000 +0000 +++ libvirt-6.0.0/debian/patches/ubuntu/lp2059272-2-qemu-Do-not-save-XML-in-shutdown-on-init.patch 2024-04-16 17:03:03.000000000 +0000 @@ -0,0 +1,290 @@ +Bug-Ubuntu: http://bugs.launchpad.net/bugs/2059272 +Forwarded: not-needed + +From: Mauricio Faria de Oliveira +Date: Fri, 05 Apr 2024 14:36:20 -0300 +Subject: [PATCH] qemu: Do not save domain status XML in + qemuProcessReconnect() if shutdown is requested + +Problem: + +If libvirt is shutdown during initialization (that is unlikely, but may happen), +the QEMU driver's qemuProcessReconnect() threads (used to reconnect to the QEMU +process/monitor of running domains and update the domain status XML accordingly) +may still be running after qemuStateCleanup() freed & zeroed QEMU driver memory. + +This makes the QEMU driver's function pointer/callback to format XML to be NULL, +thus qemuProcessReconnect() -> virDomainObjSave() -> virDomainObjFormat() writes +an incomplete domain status XML file, lacking the QEMU driver specific section, +for example, without the path to the QEMU monitor socket (""). + +The next time libvirtd initializes the QEMU driver, which looks for the domain +status XML files in order to identify the running domains so as to manage them, +there is a parsing error for such XML file, which aborts the identification of +that domain, thus libvirt is not even aware of it, and cannot manage it (e.g., +the domain is not present in `virsh list` nor recognized in `virsh` commands). + +- main() + - daemonStateInit() + - new thread: daemonRunStateInit() + - virStateInitialize() + - qemuStateInitialize() + - driver.xmlopt = virQEMUDriverCreateXMLConf() + - qemuProcessReconnectAll() + - new threads: qemuProcessReconnect() + - virDomainObjSave() + - virDomainObjFormat() + - driver.xmlopt.privateData.format() + - daemonSetupSignals() + - virNetDaemonRun() + - dmn->quit = false + - while (!dmn->quit) + - virEventRunDefaultImpl() + - virStateCleanup() + - qemuStateCleanup() + - unref/free/zero(driver.xmlopt) + +SIG{INT,QUIT,TERM} +- daemonShutdownHandler() + - dmn->quit = true + +Solution in Jammy/upstream: + +It is not sufficient to release (stop) the worker thread pool in order to wait +for the qemuProcessReconnect() threads as they are created independently of the +worker thread pool, with individual/direct calls to virThreadCreate(). + +In Jammy and later there is the .stateShutdownWait() callback (called before the +.stateCleanup() callback, ie, qemuStateCleanup()), which indirectly synchronizes +with qemuProcessReconnect() threads due to virObjectLock() on domain objects. + +However, that callback is not available in Focal, and it is a big change that is +not SRU material (10 patches with significant changes). + +Solution in Focal: + +In theory, the entire qemuProcessReconnect() function should synchronize with +the event loop as 1. shutdown stops it then proceeds to qemuStateCleanup(), and + 2. its calls to QEMU monitor depend on the event loop to get the monitor reply. +(And the QEMU driver memory might be used after release in qemuStateCleanup().) + +In practice, only the call to update XML should synchronize with the event loop, +because it is the actual "commit" of the result of the QEMU monitor calls above. +(This doesn't address the usage of QEMU driver memory after release, admittedly, +but there are no bugs for that yet; and impact is at shutdown time 'only'.) + +Note that, if any of the QEMU monitor calls above block waiting command replies +because the event loop has stopped, the update XML call is not reached. That is +actually OK (since it prevents an incomplete XML) and is what happens currently. + +*But*, if it *is* reached, then it *must* either run with QEMU driver in place +all the way through (so it can write a complete XML file), or *not run* at all. + +So, we can just check whether libvirtd shutdown is set and skip the XML update. + +It is OK to skip the XML update, which runs at libvirtd initialization, because +if a libvirtd shutdown is happening (i.e., an early shutdown) the resulting XML +*is not going to be used anyway* this time / this libvirtd run, and it +*is going to run again anyway* next time / next libvirtd initialization. + +Implementation: + +The libvirtd _daemon_ runs the QEMU _driver_. + +The shutdown handler sets the 'quit' flag in the _daemon_ (stops the event loop) +and this is not accessible by the _driver_, originally. + +However, the daemon passes itself to the driver as an opaque pointer on init, so +the driver can tell it not to shutdown due to timeout, if there are running VMs. + +So, let's use that existing pointer, plus a new function at the daemon level, to +check whether the daemon's 'quit' flag is set (ie, the daemon is shutting down), +and not update the XML in that case. + +The daemon object lock is convenient, as it is used by both the shutdown handler +and the event loop, so if we lock it and 'quit' is not set, the shutdown is not +going to start until we unlock it. Thus, we update the XML with that lock held. + +We also make sure the daemon reference count is correctly (inc/dec)remented in +the qemuProcessReconnect() threads, so that it is not freed/zeroed at shutdown +before we lock it, since we access its memory to take the daemon object lock. + +Details for Makefile.inc.am: + +The plumbing of `virNetDaemonQuitRequested()` from daemon-level to driver-level +needs the RPC include headers and static linking archives/objects, that in turn +need the probes linked (since RPC code has probes). + +(The probes link with `libvirt_driver_qemu_la` not `libvirt_driver_qemu_IMPL_la` +as RPC does because a _test_ already links the probes *and* IMPLementation, thus +would build-fail per duplicate definitions of the probes if IMPL had those too.) + +Index: libvirt-6.0.0/src/qemu/Makefile.inc.am +=================================================================== +--- libvirt-6.0.0.orig/src/qemu/Makefile.inc.am ++++ libvirt-6.0.0/src/qemu/Makefile.inc.am +@@ -99,6 +99,8 @@ libvirt_driver_qemu_impl_la_CFLAGS = \ + -I$(builddir)/access \ + -I$(srcdir)/conf \ + -I$(srcdir)/secret \ ++ -I$(srcdir)/rpc \ ++ -I$(builddir)/rpc \ + $(AM_CFLAGS) \ + $(NULL) + libvirt_driver_qemu_impl_la_LDFLAGS = $(AM_LDFLAGS) +@@ -108,10 +110,13 @@ libvirt_driver_qemu_impl_la_LIBADD = \ + $(LIBNL_LIBS) \ + $(SELINUX_LIBS) \ + $(LIBXML_LIBS) \ ++ libvirt-net-rpc.la \ ++ libvirt-net-rpc-server.la \ + $(NULL) + libvirt_driver_qemu_impl_la_SOURCES = $(QEMU_DRIVER_SOURCES) + + if WITH_DTRACE_PROBES ++libvirt_driver_qemu_la_LIBADD += libvirt_probes.lo + libvirt_driver_qemu_la_LIBADD += libvirt_qemu_probes.lo + nodist_libvirt_driver_qemu_la_SOURCES = libvirt_qemu_probes.h + BUILT_SOURCES += libvirt_qemu_probes.h +Index: libvirt-6.0.0/src/qemu/qemu_driver.c +=================================================================== +--- libvirt-6.0.0.orig/src/qemu/qemu_driver.c ++++ libvirt-6.0.0/src/qemu/qemu_driver.c +@@ -153,6 +153,14 @@ static int qemuOpenFileAs(uid_t fallback + + static virQEMUDriverPtr qemu_driver; + ++/* Store a pointer to the daemon running the QEMU driver. ++ * ++ * Note: that is already in 'qemu_driver->inhibitOpaque', ++ * but it is released and cleared during daemon shutdown. ++ * ++ * This remains in place during shutdown with references. */ ++void *qemu_driver_dmn; ++ + /* Looks up the domain object from snapshot and unlocks the + * driver. The returned domain object is locked and ref'd and the + * caller must call virDomainObjEndAPI() on it. */ +@@ -658,6 +666,7 @@ qemuStateInitialize(bool privileged, + + qemu_driver->inhibitCallback = callback; + qemu_driver->inhibitOpaque = opaque; ++ qemu_driver_dmn = opaque; + + qemu_driver->privileged = privileged; + qemu_driver->hostarch = virArchFromHost(); +Index: libvirt-6.0.0/src/qemu/qemu_process.c +=================================================================== +--- libvirt-6.0.0.orig/src/qemu/qemu_process.c ++++ libvirt-6.0.0/src/qemu/qemu_process.c +@@ -96,6 +96,28 @@ + + VIR_LOG_INIT("qemu.qemu_process"); + ++/* Use the pointer to the daemon running the QEMU driver. ++ * ++ * It is (un)referenced in qemuProcessReconnect{Helper}() ++ * so it has references while qemuProcessReconnect() runs. ++ * ++ * Thus, it can be safely used there even during shutdown. */ ++extern void *qemu_driver_dmn; ++ ++/* Function virNetDaemonQuitRequested() exports whether ++ * a daemon has been requested to shut down (dmn->quit). ++ * ++ * It must be called with the daemon's object lock held ++ * as 'dmn->quit' is set asynchronously under that lock ++ * by signal handlers and it is checked under that lock ++ * for the event loop to continue (or not, if it is set). ++ * ++ * So, it is guaranteed that, if the function return is ++ * false under the daemon lock, the event loop will not ++ * continue or break to shutdown while the lock is held. ++ */ ++#include "rpc/virnetdaemon.h" ++ + /** + * qemuProcessRemoveDomainStatus + * +@@ -8119,8 +8141,27 @@ qemuProcessReconnect(void *opaque) + } + + /* update domain state XML with possibly updated state in virDomainObj */ +- if (virDomainObjSave(obj, driver->xmlopt, cfg->stateDir) < 0) ++ ++ /* ++ * But *not* if shutdown is detected during this initialization ++ * as the QEMU driver XML formatter may have been freed already, ++ * which effectively removes the QEMU information from the file, ++ * causing an error to parse this domain on next initialization. ++ * ++ * The same update will just happen again on next initialization, ++ * and it isn't useful on this initialization as we are shutting ++ * down anyway; so just skip it, do it next time. (LP: #2059272) ++ */ ++ virObjectLock(qemu_driver_dmn); ++ if (virNetDaemonQuitRequested(qemu_driver_dmn)) { ++ VIR_INFO("Leaving the update of '%s' domain status XML for the next " ++ "initialization (shutdown detected on this initialization).", ++ obj->def->name); ++ } else if (virDomainObjSave(obj, driver->xmlopt, cfg->stateDir) < 0) { ++ virObjectUnlock(qemu_driver_dmn); + goto error; ++ } ++ virObjectUnlock(qemu_driver_dmn); + + /* Run an hook to allow admins to do some magic */ + if (virHookPresent(VIR_HOOK_DRIVER_QEMU)) { +@@ -8152,6 +8193,7 @@ qemuProcessReconnect(void *opaque) + if (!virDomainObjIsActive(obj)) + qemuDomainRemoveInactiveJob(driver, obj); + } ++ virObjectUnref(qemu_driver_dmn); + virDomainObjEndAPI(&obj); + virNWFilterUnlockFilterUpdates(); + virIdentitySetCurrent(NULL); +@@ -8210,6 +8252,7 @@ qemuProcessReconnectHelper(virDomainObjP + * that handles the reconnect */ + virObjectLock(obj); + virObjectRef(obj); ++ virObjectRef(qemu_driver_dmn); + + if (virThreadCreate(&thread, false, qemuProcessReconnect, data) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", +@@ -8224,6 +8267,7 @@ qemuProcessReconnectHelper(virDomainObjP + QEMU_ASYNC_JOB_NONE, 0); + qemuDomainRemoveInactiveJobLocked(src->driver, obj); + ++ virObjectUnref(qemu_driver_dmn); + virDomainObjEndAPI(&obj); + virNWFilterUnlockFilterUpdates(); + g_clear_object(&data->identity); +Index: libvirt-6.0.0/src/rpc/virnetdaemon.c +=================================================================== +--- libvirt-6.0.0.orig/src/rpc/virnetdaemon.c ++++ libvirt-6.0.0/src/rpc/virnetdaemon.c +@@ -859,6 +859,12 @@ virNetDaemonQuit(virNetDaemonPtr dmn) + virObjectUnlock(dmn); + } + ++bool ++virNetDaemonQuitRequested(virNetDaemonPtr dmn) ++{ ++ return dmn->quit; ++} ++ + static int + daemonServerClose(void *payload, + const void *key G_GNUC_UNUSED, +Index: libvirt-6.0.0/src/rpc/virnetdaemon.h +=================================================================== +--- libvirt-6.0.0.orig/src/rpc/virnetdaemon.h ++++ libvirt-6.0.0/src/rpc/virnetdaemon.h +@@ -68,6 +68,7 @@ void virNetDaemonUpdateServices(virNetDa + void virNetDaemonRun(virNetDaemonPtr dmn); + + void virNetDaemonQuit(virNetDaemonPtr dmn); ++bool virNetDaemonQuitRequested(virNetDaemonPtr dmn); + + void virNetDaemonClose(virNetDaemonPtr dmn); + diff -Nru libvirt-6.0.0/debian/patches/ubuntu/lp2059272-2-qemu-Wait-qemuProcessReconnect-threads-in-cleanup.patch libvirt-6.0.0/debian/patches/ubuntu/lp2059272-2-qemu-Wait-qemuProcessReconnect-threads-in-cleanup.patch --- libvirt-6.0.0/debian/patches/ubuntu/lp2059272-2-qemu-Wait-qemuProcessReconnect-threads-in-cleanup.patch 2024-03-30 20:05:56.000000000 +0000 +++ libvirt-6.0.0/debian/patches/ubuntu/lp2059272-2-qemu-Wait-qemuProcessReconnect-threads-in-cleanup.patch 1970-01-01 00:00:00.000000000 +0000 @@ -1,221 +0,0 @@ -Bug-Ubuntu: http://bugs.launchpad.net/bugs/2059272 -Forwarded: not-needed - -From: Mauricio Faria de Oliveira -Date: Thu, 28 Mar 2024 14:36:20 -0300 -Subject: [PATCH] qemu: wait for qemuProcessReconnect() threads in - qemuStateCleanup() - -If libvirt is shutdown during initialization (which is unlikely, -but it might happen), the QEMU driver's qemuProcessReconnect() -threads (used to reconnect to running domains' QEMU processes) -may be still running, and access QEMU driver's memory released -in qemuStateCleanup() (ie, use-after-free). - -It is not sufficient to release (stop) the worker thread pool -in order to wait for the threads running qemuProcessReconnect(), -as they are created independently of the worker thread pool, -with individual/direct calls to virThreadCreate(). - -In Jammy and later there is the .stateShutdownWait() callback -(called before .stateCleanup()), which indirectly synchronizes -with them due to virObjectLock/Unlock() on the domain objects. - -However, that callback is not available in Focal, and it is a -big change to be SRUed (10 patches with significant changes). - -So, this patch adopts a simple approach, based on an atomic -counter for the number of threads running qemuProcessReconnect(), -and wait for it to become zero in qemuStateCleanup() (i.e., all -created threads finished running): - - - The counter is incremented _before_ creating the thread (and also - before qemuStateCleanup() may be called if shutdown handler fires). - - - The counter is decremented in case of _error_ creating the thread, - and in case the thread function _returns_ (only one return point). - -The wait timeout is set in LIBVIRT_QEMU_STATE_CLEANUP_WAIT_TIMEOUT: --1 = wait indefinitely; 0 = do not wait; N = wait up to N seconds. - -The default is to wait up to 30 seconds, an educated trade-off: - - - Usually, the _right_ thing is to wait indefinitely until all - such threads finish, so that released memory is not used. - - But that is a behavior change with a big impact to regular - operations in case of regressions (i.e., libvirt stop/restart - would not finish). - - So, providing a bounded timeout is good on that front. - - And also based on the fact that newer versions do it similarly - (forceful shutdown timer, once the signal to shutdown arrives). - - And in practice such threads do not take long, so even though - the timeout is bounded, all threads should finish within it. - -Index: libvirt-6.0.0/src/qemu/qemu_conf.h -=================================================================== ---- libvirt-6.0.0.orig/src/qemu/qemu_conf.h -+++ libvirt-6.0.0/src/qemu/qemu_conf.h -@@ -311,6 +311,9 @@ struct _virQEMUDriver { - - /* Immutable pointer, self-locking APIs */ - virHashAtomicPtr migrationErrors; -+ -+ /* Atomic inc/dec only */ -+ int qemuProcessReconnectThreads; - }; - - virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged); -Index: libvirt-6.0.0/src/qemu/qemu_driver.c -=================================================================== ---- libvirt-6.0.0.orig/src/qemu/qemu_driver.c -+++ libvirt-6.0.0/src/qemu/qemu_driver.c -@@ -104,6 +104,7 @@ - #include "virdomainsnapshotobjlist.h" - #include "virenum.h" - #include "virdomaincheckpointobjlist.h" -+#include "viratomic.h" - - #define VIR_FROM_THIS VIR_FROM_QEMU - -@@ -153,6 +154,16 @@ static int qemuOpenFileAs(uid_t fallback - - static virQEMUDriverPtr qemu_driver; - -+/** -+ * __qemuDriverIsNull() -+ * -+ * Check whether `qemu_driver` is NULL, without exporting its symbol. -+ */ -+int -+__qemuDriverIsNull(void) { -+ return (qemu_driver == NULL); -+} -+ - /* Looks up the domain object from snapshot and unlocks the - * driver. The returned domain object is locked and ref'd and the - * caller must call virDomainObjEndAPI() on it. */ -@@ -1109,6 +1120,57 @@ qemuStateStop(void) - } - - /** -+ * qemuStateCleanupWait() -+ * -+ * Wait for qemuProcessReconnect() threads to finish (LP: #2059272). -+ * -+ * The timeout can be set with LIBVIRT_QEMU_STATE_CLEANUP_WAIT_TIMEOUT -+ * (30 seconds by default): -+ * -1: wait indefinitely -+ * 0: do not wait -+ * N: wait up to N seconds -+ */ -+static void -+qemuStateCleanupWait(void) -+{ -+ int threads, timeout = 30, seconds = 0; -+ char *timeout_env; -+ -+ /* Set timeout */ -+ if ((timeout_env = getenv("LIBVIRT_QEMU_STATE_CLEANUP_WAIT_TIMEOUT")) && -+ virStrToLong_i(timeout_env, NULL, 10, &timeout) < 0) -+ VIR_ERROR("Failed to parse LIBVIRT_QEMU_STATE_CLEANUP_WAIT_TIMEOUT"); -+ -+ VIR_DEBUG("timeout %i, timeout_env '%s'", timeout, timeout_env); -+ -+ /* Maybe wait */ -+ while ((threads = virAtomicIntGet(&qemu_driver->qemuProcessReconnectThreads)) -+ && (threads > 0) && (timeout < 0 || seconds < timeout)) { -+ -+ VIR_DEBUG("threads %i, seconds %i", threads, seconds); -+ -+ if (seconds == 0) -+ VIR_WARN("Waiting for qemuProcessReconnect() threads (%i) to end. " -+ "Configure with LIBVIRT_QEMU_STATE_CLEANUP_WAIT_TIMEOUT " -+ "(-1 = wait; 0 = do not wait; N = wait up to N seconds; " -+ "current = %i)", threads, timeout); -+ -+ seconds++; -+ g_usleep(1 * G_USEC_PER_SEC); -+ } -+ -+ /* Last check */ -+ if (threads > 0) -+ VIR_WARN("Leaving qemuProcessReconnect() threads (%i) per timeout (%i)", -+ threads, timeout); -+ else if (threads < 0) -+ VIR_ERROR("Negative qemuProcessReconnect() threads (%i); timeout (%i)", -+ threads, timeout); -+ else -+ VIR_DEBUG("All qemuProcessReconnect() threads finished"); -+} -+ -+/** - * qemuStateCleanup: - * - * Release resources allocated by QEMU driver (no domain is shut off though) -@@ -1119,6 +1181,8 @@ qemuStateCleanup(void) - if (!qemu_driver) - return -1; - -+ qemuStateCleanupWait(); -+ - virThreadPoolFree(qemu_driver->workerPool); - virObjectUnref(qemu_driver->migrationErrors); - virObjectUnref(qemu_driver->closeCallbacks); -Index: libvirt-6.0.0/src/qemu/qemu_process.c -=================================================================== ---- libvirt-6.0.0.orig/src/qemu/qemu_process.c -+++ libvirt-6.0.0/src/qemu/qemu_process.c -@@ -58,6 +58,7 @@ - #include "qemu_extdevice.h" - #include "qemu_firmware.h" - #include "qemu_backup.h" -+#include "qemu_driver.h" - - #include "cpu/cpu.h" - #include "cpu/cpu_x86.h" -@@ -8152,6 +8153,15 @@ qemuProcessReconnect(void *opaque) - if (!virDomainObjIsActive(obj)) - qemuDomainRemoveInactiveJob(driver, obj); - } -+ -+ if (!__qemuDriverIsNull()) { -+ VIR_DEBUG("Decrementing qemuProcessReconnect() threads."); -+ virAtomicIntDecAndTest(&driver->qemuProcessReconnectThreads); -+ } else { -+ VIR_DEBUG("Not decrementing qemuProcessReconnect() threads " -+ "as the QEMU driver is already deallocated/freed."); -+ } -+ - virDomainObjEndAPI(&obj); - virNWFilterUnlockFilterUpdates(); - virIdentitySetCurrent(NULL); -@@ -8211,6 +8221,9 @@ qemuProcessReconnectHelper(virDomainObjP - virObjectLock(obj); - virObjectRef(obj); - -+ VIR_DEBUG("Incrementing qemuProcessReconnect() threads."); -+ virAtomicIntInc(&src->driver->qemuProcessReconnectThreads); -+ - if (virThreadCreate(&thread, false, qemuProcessReconnect, data) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Could not create thread. QEMU initialization " -@@ -8224,6 +8237,9 @@ qemuProcessReconnectHelper(virDomainObjP - QEMU_ASYNC_JOB_NONE, 0); - qemuDomainRemoveInactiveJobLocked(src->driver, obj); - -+ VIR_DEBUG("Decrementing qemuProcessReconnect() threads."); -+ virAtomicIntDecAndTest(&src->driver->qemuProcessReconnectThreads); -+ - virDomainObjEndAPI(&obj); - virNWFilterUnlockFilterUpdates(); - g_clear_object(&data->identity); -Index: libvirt-6.0.0/src/qemu/qemu_driver.h -=================================================================== ---- libvirt-6.0.0.orig/src/qemu/qemu_driver.h -+++ libvirt-6.0.0/src/qemu/qemu_driver.h -@@ -22,3 +22,5 @@ - #pragma once - - int qemuRegister(void); -+ -+int __qemuDriverIsNull(void);