From 1727bf7cc86418302a3f59536b8637ee9816f842 Mon Sep 17 00:00:00 2001 From: eabdullin Date: Wed, 24 Jul 2024 12:11:14 +0300 Subject: [PATCH] - vmx: Do not require all ID data for VMWare Distributed Switch - vmx: Do not require DVS Port ID - rpc: ensure temporary GSource is removed from client event loop - qemu: Fix migration with disabled vmx-* CPU features --- ...tion-with-disabled-vmx--CPU-features.patch | 111 ++++++++++++ ...GSource-is-removed-from-client-event.patch | 96 ++++++++++ ...bvirt-vmx-Do-not-require-DVS-Port-ID.patch | 83 +++++++++ ...D-data-for-VMWare-Distributed-Switch.patch | 166 ++++++++++++++++++ SPECS/libvirt.spec | 22 ++- 5 files changed, 475 insertions(+), 3 deletions(-) create mode 100644 SOURCES/libvirt-qemu-Fix-migration-with-disabled-vmx--CPU-features.patch create mode 100644 SOURCES/libvirt-rpc-ensure-temporary-GSource-is-removed-from-client-event.patch create mode 100644 SOURCES/libvirt-vmx-Do-not-require-DVS-Port-ID.patch create mode 100644 SOURCES/libvirt-vmx-Do-not-require-all-ID-data-for-VMWare-Distributed-Switch.patch diff --git a/SOURCES/libvirt-qemu-Fix-migration-with-disabled-vmx--CPU-features.patch b/SOURCES/libvirt-qemu-Fix-migration-with-disabled-vmx--CPU-features.patch new file mode 100644 index 0000000..2a19a05 --- /dev/null +++ b/SOURCES/libvirt-qemu-Fix-migration-with-disabled-vmx--CPU-features.patch @@ -0,0 +1,111 @@ +From e622970c8785ec1f7e142d72f792d89f870e07d0 Mon Sep 17 00:00:00 2001 +From: Jiri Denemark +Date: Wed, 12 Jun 2024 16:44:28 +0200 +Subject: [PATCH] qemu: Fix migration with disabled vmx-* CPU features +MIME-Version: 1.0 +Content-Type: text/plain; charset=UTF-8 +Content-Transfer-Encoding: 8bit + +When starting a domain on a host which lacks a vmx-* CPU feature which +is expected to be enabled by the CPU model specified in the domain XML, +libvirt properly marks such feature as disabled in the active domain +XML. But migrating the domain to a similar host which lacks the same +vmx-* feature will fail with libvirt reporting the feature as missing. +This is because of a bug in the hack ensuring backward compatibility +libvirt running on the destination thinks the missing feature is +expected to be enabled. + +https://issues.redhat.com/browse/RHEL-40899 + +Fixes: v10.1.0-85-g5fbfa5ab8a +Signed-off-by: Jiri Denemark +Reviewed-by: Ján Tomko +--- + src/cpu/cpu_x86.c | 50 ++++++++++++++++++++++++++++------------------- + 1 file changed, 30 insertions(+), 20 deletions(-) + +diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c +index 7a70eed9e7f..fcbce0ec467 100644 +--- a/src/cpu/cpu_x86.c ++++ b/src/cpu/cpu_x86.c +@@ -3023,10 +3023,7 @@ virCPUx86UpdateLive(virCPUDef *cpu, + if (!(map = virCPUx86GetMap())) + return -1; + +- if (!(model = x86ModelFromCPU(cpu, map, -1))) +- return -1; +- +- if (hostPassthrough && ++ if (!(model = x86ModelFromCPU(cpu, map, -1)) || + !(modelDisabled = x86ModelFromCPU(cpu, map, VIR_CPU_FEATURE_DISABLE))) + return -1; + +@@ -3039,39 +3036,52 @@ virCPUx86UpdateLive(virCPUDef *cpu, + for (i = 0; i < map->nfeatures; i++) { + virCPUx86Feature *feature = map->features[i]; + virCPUFeaturePolicy expected = VIR_CPU_FEATURE_LAST; ++ bool explicit = false; ++ bool ignore = false; + +- if (x86DataIsSubset(&model->data, &feature->data)) ++ if (x86DataIsSubset(&model->data, &feature->data)) { ++ explicit = true; + expected = VIR_CPU_FEATURE_REQUIRE; +- else if (!hostPassthrough || +- x86DataIsSubset(&modelDisabled->data, &feature->data)) ++ } else if (x86DataIsSubset(&modelDisabled->data, &feature->data)) { ++ explicit = true; ++ expected = VIR_CPU_FEATURE_DISABLE; ++ } else if (!hostPassthrough) { ++ /* implicitly disabled */ + expected = VIR_CPU_FEATURE_DISABLE; ++ } ++ ++ /* Features enabled or disabled by the hypervisor are ignored by ++ * check='full' in case they were added to the model later and not ++ * explicitly mentioned in the CPU definition. This matches how libvirt ++ * behaved before the features were added. ++ */ ++ if (!explicit && ++ g_strv_contains((const char **) model->addedFeatures, feature->name)) ++ ignore = true; + + if (expected == VIR_CPU_FEATURE_DISABLE && + x86DataIsSubset(&enabled, &feature->data)) { + VIR_DEBUG("Feature '%s' enabled by the hypervisor", feature->name); + +- /* Extra features enabled by the hypervisor are ignored by +- * check='full' in case they were added to the model later for +- * backward compatibility with the older definition of the model. +- */ +- if (cpu->check == VIR_CPU_CHECK_FULL && +- !g_strv_contains((const char **) model->addedFeatures, feature->name)) { ++ if (cpu->check == VIR_CPU_CHECK_FULL && !ignore) + virBufferAsprintf(&bufAdded, "%s,", feature->name); +- } else if (virCPUDefUpdateFeature(cpu, feature->name, +- VIR_CPU_FEATURE_REQUIRE) < 0) { ++ else if (virCPUDefUpdateFeature(cpu, feature->name, ++ VIR_CPU_FEATURE_REQUIRE) < 0) + return -1; +- } + } + + if (x86DataIsSubset(&disabled, &feature->data) || + (expected == VIR_CPU_FEATURE_REQUIRE && + !x86DataIsSubset(&enabled, &feature->data))) { + VIR_DEBUG("Feature '%s' disabled by the hypervisor", feature->name); +- if (cpu->check == VIR_CPU_CHECK_FULL) ++ ++ if (cpu->check == VIR_CPU_CHECK_FULL && !ignore) { + virBufferAsprintf(&bufRemoved, "%s,", feature->name); +- else if (virCPUDefUpdateFeature(cpu, feature->name, +- VIR_CPU_FEATURE_DISABLE) < 0) +- return -1; ++ } else { ++ if (virCPUDefUpdateFeature(cpu, feature->name, VIR_CPU_FEATURE_DISABLE) < 0) ++ return -1; ++ x86DataSubtract(&disabled, &feature->data); ++ } + } + } + diff --git a/SOURCES/libvirt-rpc-ensure-temporary-GSource-is-removed-from-client-event.patch b/SOURCES/libvirt-rpc-ensure-temporary-GSource-is-removed-from-client-event.patch new file mode 100644 index 0000000..9715156 --- /dev/null +++ b/SOURCES/libvirt-rpc-ensure-temporary-GSource-is-removed-from-client-event.patch @@ -0,0 +1,96 @@ +From 8074d64dc2eca846d6a61efe1a9b7428a0ce1dd1 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] 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 +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 68098b1c8d8..147b0d661a3 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 */ diff --git a/SOURCES/libvirt-vmx-Do-not-require-DVS-Port-ID.patch b/SOURCES/libvirt-vmx-Do-not-require-DVS-Port-ID.patch new file mode 100644 index 0000000..466afd0 --- /dev/null +++ b/SOURCES/libvirt-vmx-Do-not-require-DVS-Port-ID.patch @@ -0,0 +1,83 @@ +From 2482801608b80461e67dcfcaf66ce28134688203 Mon Sep 17 00:00:00 2001 +From: Martin Kletzander +Date: Mon, 13 May 2024 12:24:19 +0200 +Subject: [PATCH] vmx: Do not require DVS Port ID +MIME-Version: 1.0 +Content-Type: text/plain; charset=UTF-8 +Content-Transfer-Encoding: 8bit + +It can be safely removed from the VMX, VMWare will still boot the +machine and once another ethernet is added it is updated in the VMX to +zero. So do not require it and default to zero too since this part of +the XML is done as best effort and it is mentioned even in our +documentation. + +Signed-off-by: Martin Kletzander +Reviewed-by: Ján Tomko +--- + src/vmx/vmx.c | 2 +- + tests/vmx2xmldata/ethernet-vds-no-portid.vmx | 10 ++++++++ + tests/vmx2xmldata/ethernet-vds-no-portid.xml | 24 ++++++++++++++++++++ + 3 files changed, 35 insertions(+), 1 deletion(-) + create mode 100644 tests/vmx2xmldata/ethernet-vds-no-portid.vmx + create mode 100644 tests/vmx2xmldata/ethernet-vds-no-portid.xml + +diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c +index f4182bc5184..d90b41d2ad1 100644 +--- a/src/vmx/vmx.c ++++ b/src/vmx/vmx.c +@@ -2854,7 +2854,7 @@ virVMXParseEthernet(virConf *conf, int controller, virDomainNetDef **def) + portId_name, + &(*def)->data.vds.port_id, + 0, +- false) < 0 || ++ true) < 0 || + virVMXGetConfigLong(conf, + connectionId_name, + &(*def)->data.vds.connection_id, +diff --git a/tests/vmx2xmldata/ethernet-vds-no-portid.vmx b/tests/vmx2xmldata/ethernet-vds-no-portid.vmx +new file mode 100644 +index 00000000000..7761accb3ab +--- /dev/null ++++ b/tests/vmx2xmldata/ethernet-vds-no-portid.vmx +@@ -0,0 +1,10 @@ ++config.version = "8" ++virtualHW.version = "4" ++ethernet0.present = "true" ++ethernet0.virtualDev = "e1000e" ++ethernet0.addressType = "vpx" ++ethernet0.generatedAddress = "00:50:56:87:65:43" ++ethernet0.dvs.switchId = "50 34 26 b2 94 e9 3b 16-1d 68 87 bf ff 4a 54 40" ++ethernet0.dvs.portgroupId = "dvportgroup-1285" ++ethernet0.dvs.connectionId = "408217997" ++displayName = "test" +diff --git a/tests/vmx2xmldata/ethernet-vds-no-portid.xml b/tests/vmx2xmldata/ethernet-vds-no-portid.xml +new file mode 100644 +index 00000000000..60fd9c99feb +--- /dev/null ++++ b/tests/vmx2xmldata/ethernet-vds-no-portid.xml +@@ -0,0 +1,24 @@ ++ ++ test ++ 00000000-0000-0000-0000-000000000000 ++ 32768 ++ 32768 ++ 1 ++ ++ hvm ++ ++ ++ destroy ++ restart ++ destroy ++ ++ ++ ++ ++ ++ ++ ++ ++ diff --git a/SOURCES/libvirt-vmx-Do-not-require-all-ID-data-for-VMWare-Distributed-Switch.patch b/SOURCES/libvirt-vmx-Do-not-require-all-ID-data-for-VMWare-Distributed-Switch.patch new file mode 100644 index 0000000..58f24f6 --- /dev/null +++ b/SOURCES/libvirt-vmx-Do-not-require-all-ID-data-for-VMWare-Distributed-Switch.patch @@ -0,0 +1,166 @@ +From db622081e0fa55b481da1fc7fb81279224a60f88 Mon Sep 17 00:00:00 2001 +From: Martin Kletzander +Date: Mon, 8 Jul 2024 13:04:13 +0200 +Subject: [PATCH] vmx: Do not require all ID data for VMWare Distributed Switch + +Similarly to commit 2482801608b8 we can safely ignore connectionId, +portId and portgroupId in both XML and VMX as they are only a blind +pass-through between XML and VMX and an ethernet without such parameters +was spotted in the wild. On top of that even our documentation says the +whole VMWare Distrubuted Switch configuration is a best-effort. + +Resolves: https://issues.redhat.com/browse/RHEL-46099 + +Signed-off-by: Martin Kletzander +Reviewed-by: Jiri Denemark +--- + src/conf/domain_conf.c | 11 ++++----- + src/conf/schemas/domaincommon.rng | 24 ++++++++++++------- + src/vmx/vmx.c | 24 ++++++++++++------- + ...-portid.vmx => ethernet-vds-no-params.vmx} | 2 -- + ...-portid.xml => ethernet-vds-no-params.xml} | 2 +- + 5 files changed, 37 insertions(+), 26 deletions(-) + rename tests/vmx2xmldata/{ethernet-vds-no-portid.vmx => ethernet-vds-no-params.vmx} (76%) + rename tests/vmx2xmldata/{ethernet-vds-no-portid.xml => ethernet-vds-no-params.xml} (82%) + +diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c +index 6080f4f90a5..bfef89e1bea 100644 +--- a/src/conf/domain_conf.c ++++ b/src/conf/domain_conf.c +@@ -9536,15 +9536,14 @@ virDomainNetDefParseXML(virDomainXMLOption *xmlopt, + def->data.vds.switch_id) < 0) + return NULL; + +- if (virXMLPropLongLong(source_node, "portid", 0, VIR_XML_PROP_REQUIRED, +- &def->data.vds.port_id, def->data.vds.port_id) < 0) ++ if (virXMLPropLongLong(source_node, "portid", 0, VIR_XML_PROP_NONE, ++ &def->data.vds.port_id, 0) < 0) + return NULL; + +- if (!(def->data.vds.portgroup_id = virXMLPropStringRequired(source_node, "portgroupid"))) +- return NULL; ++ def->data.vds.portgroup_id = virXMLPropString(source_node, "portgroupid"); + +- if (virXMLPropLongLong(source_node, "connectionid", 0, VIR_XML_PROP_REQUIRED, +- &def->data.vds.connection_id, def->data.vds.connection_id) < 0) ++ if (virXMLPropLongLong(source_node, "connectionid", 0, VIR_XML_PROP_NONE, ++ &def->data.vds.connection_id, 0) < 0) + return NULL; + + break; +diff --git a/src/conf/schemas/domaincommon.rng b/src/conf/schemas/domaincommon.rng +index b163e4eece0..2d23fcf1237 100644 +--- a/src/conf/schemas/domaincommon.rng ++++ b/src/conf/schemas/domaincommon.rng +@@ -3621,15 +3621,21 @@ + + + +- +- +- +- +- +- +- +- +- ++ ++ ++ ++ ++ ++ ++ ++ ++ ++ ++ ++ ++ ++ ++ + + + +diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c +index d082a076601..e5bc2d793c6 100644 +--- a/src/vmx/vmx.c ++++ b/src/vmx/vmx.c +@@ -2849,7 +2849,7 @@ virVMXParseEthernet(virConf *conf, int controller, virDomainNetDef **def) + if (virVMXGetConfigString(conf, + portgroupId_name, + &(*def)->data.vds.portgroup_id, +- false) < 0 || ++ true) < 0 || + virVMXGetConfigLong(conf, + portId_name, + &(*def)->data.vds.port_id, +@@ -2859,7 +2859,7 @@ virVMXParseEthernet(virConf *conf, int controller, virDomainNetDef **def) + connectionId_name, + &(*def)->data.vds.connection_id, + 0, +- false) < 0) ++ true) < 0) + goto cleanup; + } else if (connectionType == NULL && networkName == NULL) { + (*def)->type = VIR_DOMAIN_NET_TYPE_NULL; +@@ -3980,14 +3980,22 @@ virVMXFormatEthernet(virDomainNetDef *def, int controller, + uuid[5], uuid[6], uuid[7], uuid[8], uuid[9], uuid[10], + uuid[11], uuid[12], uuid[13], uuid[14], uuid[15]); + +- virBufferAsprintf(buffer, "ethernet%d.dvs.portId = \"%lld\"\n", +- controller, def->data.vds.port_id); ++ if (def->data.vds.port_id) { ++ virBufferAsprintf(buffer, "ethernet%d.dvs.portId = \"%lld\"\n", ++ controller, def->data.vds.port_id); ++ } ++ ++ if (def->data.vds.portgroup_id) { ++ virBufferAsprintf(buffer, "ethernet%d.dvs.", controller); ++ virBufferEscapeString(buffer, "portgroupId = \"%s\"\n", ++ def->data.vds.portgroup_id); ++ } + +- virBufferAsprintf(buffer, "ethernet%d.dvs.", controller); +- virBufferEscapeString(buffer, "portgroupId = \"%s\"\n", def->data.vds.portgroup_id); ++ if (def->data.vds.connection_id) { ++ virBufferAsprintf(buffer, "ethernet%d.dvs.connectionId = \"%lld\"\n", ++ controller, def->data.vds.connection_id); ++ } + +- virBufferAsprintf(buffer, "ethernet%d.dvs.connectionId = \"%lld\"\n", +- controller, def->data.vds.connection_id); + break; + } + +diff --git a/tests/vmx2xmldata/ethernet-vds-no-portid.vmx b/tests/vmx2xmldata/ethernet-vds-no-params.vmx +similarity index 76% +rename from tests/vmx2xmldata/ethernet-vds-no-portid.vmx +rename to tests/vmx2xmldata/ethernet-vds-no-params.vmx +index 7761accb3ab..90afbdac30c 100644 +--- a/tests/vmx2xmldata/ethernet-vds-no-portid.vmx ++++ b/tests/vmx2xmldata/ethernet-vds-no-params.vmx +@@ -5,6 +5,4 @@ ethernet0.virtualDev = "e1000e" + ethernet0.addressType = "vpx" + ethernet0.generatedAddress = "00:50:56:87:65:43" + ethernet0.dvs.switchId = "50 34 26 b2 94 e9 3b 16-1d 68 87 bf ff 4a 54 40" +-ethernet0.dvs.portgroupId = "dvportgroup-1285" +-ethernet0.dvs.connectionId = "408217997" + displayName = "test" +diff --git a/tests/vmx2xmldata/ethernet-vds-no-portid.xml b/tests/vmx2xmldata/ethernet-vds-no-params.xml +similarity index 82% +rename from tests/vmx2xmldata/ethernet-vds-no-portid.xml +rename to tests/vmx2xmldata/ethernet-vds-no-params.xml +index 60fd9c99feb..0011ba471a5 100644 +--- a/tests/vmx2xmldata/ethernet-vds-no-portid.xml ++++ b/tests/vmx2xmldata/ethernet-vds-no-params.xml +@@ -14,7 +14,7 @@ + + + +- ++ + + +