[PATCH] conf: Fix out-of-bounds write during cleanup of virDomainNumaDefNodeDistanceParseXML
by Rayhan Faizel
mem_nodes[i].ndistances is written outside the loop causing an out-of-bounds
write leading to heap corruption.
While we are at it, the entire cleanup portion can be removed as it can be
handled in virDomainNumaFree. One instance of VIR_FREE is also removed and
replaced with g_autofree.
This patch also adds a testcase which would be picked up by ASAN, if this
portion regresses.
Signed-off-by: Rayhan Faizel <rayhan.faizel(a)gmail.com>
---
src/conf/numa_conf.c | 30 ++++++-------------
...ance-nonexistent-sibling.x86_64-latest.err | 1 +
.../cpu-numa-distance-nonexistent-sibling.xml | 29 ++++++++++++++++++
tests/qemuxmlconftest.c | 1 +
4 files changed, 40 insertions(+), 21 deletions(-)
create mode 100644 tests/qemuxmlconfdata/cpu-numa-distance-nonexistent-sibling.x86_64-latest.err
create mode 100644 tests/qemuxmlconfdata/cpu-numa-distance-nonexistent-sibling.xml
diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c
index d8120de6d2..0a0e2911f7 100644
--- a/src/conf/numa_conf.c
+++ b/src/conf/numa_conf.c
@@ -343,8 +343,7 @@ virDomainNumaFree(virDomainNuma *numa)
virBitmapFree(numa->mem_nodes[i].cpumask);
virBitmapFree(numa->mem_nodes[i].nodeset);
- if (numa->mem_nodes[i].ndistances > 0)
- g_free(numa->mem_nodes[i].distances);
+ g_free(numa->mem_nodes[i].distances);
g_free(numa->mem_nodes[i].caches);
}
@@ -685,9 +684,8 @@ virDomainNumaDefNodeDistanceParseXML(virDomainNuma *def,
xmlXPathContextPtr ctxt,
unsigned int cur_cell)
{
- int ret = -1;
int sibling;
- xmlNodePtr *nodes = NULL;
+ g_autofree xmlNodePtr *nodes = NULL;
size_t i, ndistances = def->nmem_nodes;
if (ndistances == 0)
@@ -698,12 +696,12 @@ virDomainNumaDefNodeDistanceParseXML(virDomainNuma *def,
return 0;
if ((sibling = virXPathNodeSet("./distances[1]/sibling", ctxt, &nodes)) < 0)
- goto cleanup;
+ return -1;
if (sibling == 0) {
virReportError(VIR_ERR_XML_ERROR, "%s",
_("NUMA distances defined without siblings"));
- goto cleanup;
+ return -1;
}
for (i = 0; i < sibling; i++) {
@@ -713,19 +711,19 @@ virDomainNumaDefNodeDistanceParseXML(virDomainNuma *def,
if (virXMLPropUInt(nodes[i], "id", 10, VIR_XML_PROP_REQUIRED,
&sibling_id) < 0)
- goto cleanup;
+ return -1;
/* The "id" needs to be within numa/cell range */
if (sibling_id >= ndistances) {
virReportError(VIR_ERR_XML_ERROR,
_("'sibling_id %1$d' does not refer to a valid cell within NUMA 'cell id %2$d'"),
sibling_id, cur_cell);
- goto cleanup;
+ return -1;
}
if (virXMLPropUInt(nodes[i], "value", 10, VIR_XML_PROP_REQUIRED,
&sibling_value) < 0)
- goto cleanup;
+ return -1;
/* Assure LOCAL_DISTANCE <= "value" <= UNREACHABLE
* and correct LOCAL_DISTANCE setting if such applies.
@@ -739,7 +737,7 @@ virDomainNumaDefNodeDistanceParseXML(virDomainNuma *def,
virReportError(VIR_ERR_XML_ERROR,
_("'value %1$d' is invalid for 'sibling id %2$d' under NUMA 'cell id %3$d'"),
sibling_value, sibling_id, cur_cell);
- goto cleanup;
+ return -1;
}
/* Apply the local / remote distance */
@@ -770,17 +768,7 @@ virDomainNumaDefNodeDistanceParseXML(virDomainNuma *def,
rdist[cur_cell].value = sibling_value;
}
- ret = 0;
-
- cleanup:
- if (ret < 0) {
- for (i = 0; i < ndistances; i++)
- VIR_FREE(def->mem_nodes[i].distances);
- def->mem_nodes[i].ndistances = 0;
- }
- VIR_FREE(nodes);
-
- return ret;
+ return 0;
}
diff --git a/tests/qemuxmlconfdata/cpu-numa-distance-nonexistent-sibling.x86_64-latest.err b/tests/qemuxmlconfdata/cpu-numa-distance-nonexistent-sibling.x86_64-latest.err
new file mode 100644
index 0000000000..4866ff5e80
--- /dev/null
+++ b/tests/qemuxmlconfdata/cpu-numa-distance-nonexistent-sibling.x86_64-latest.err
@@ -0,0 +1 @@
+XML error: 'sibling_id 2' does not refer to a valid cell within NUMA 'cell id 1'
diff --git a/tests/qemuxmlconfdata/cpu-numa-distance-nonexistent-sibling.xml b/tests/qemuxmlconfdata/cpu-numa-distance-nonexistent-sibling.xml
new file mode 100644
index 0000000000..62a6c32fbe
--- /dev/null
+++ b/tests/qemuxmlconfdata/cpu-numa-distance-nonexistent-sibling.xml
@@ -0,0 +1,29 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory unit='KiB'>219100</memory>
+ <currentMemory unit='KiB'>219100</currentMemory>
+ <vcpu placement='static'>16</vcpu>
+ <os>
+ <type arch='x86_64' machine='pc'>hvm</type>
+ <boot dev='network'/>
+ </os>
+ <cpu>
+ <topology sockets='2' dies='1' cores='4' threads='2'/>
+ <numa>
+ <cell id='1' cpus='8-15' memory='109550' unit='KiB'>
+ <distances>
+ <sibling id='2' value='10'/>
+ </distances>
+ </cell>
+ <cell id='0' cpus='0-7' memory='109550' unit='KiB'/>
+ </numa>
+ </cpu>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-x86_64</emulator>
+ </devices>
+</domain>
diff --git a/tests/qemuxmlconftest.c b/tests/qemuxmlconftest.c
index 4a711fceeb..af49934c33 100644
--- a/tests/qemuxmlconftest.c
+++ b/tests/qemuxmlconftest.c
@@ -2160,6 +2160,7 @@ mymain(void)
DO_TEST_CAPS_LATEST_PARSE_ERROR("cpu-numa3");
DO_TEST_CAPS_LATEST("cpu-numa-disjoint");
DO_TEST_CAPS_LATEST("cpu-numa-memshared");
+ DO_TEST_CAPS_LATEST_PARSE_ERROR("cpu-numa-distance-nonexistent-sibling");
/* host-model cpu expansion depends on the cpu reported by qemu and thus
* we invoke it for all real capability dumps we have */
--
2.34.1
3 months, 3 weeks
[PATCH] test_driver: add testUpdateDeviceFlags implementation
by John Levon
Add basic coverage of device update; for now, only support disk updates
until other types are needed or tested.
Signed-off-by: John Levon <john.levon(a)nutanix.com>
---
src/test/test_driver.c | 127 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 127 insertions(+)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 40fb8a467d..213fbdbccb 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -41,6 +41,7 @@
#include "domain_conf.h"
#include "domain_driver.h"
#include "domain_event.h"
+#include "domain_postparse.h"
#include "domain_validate.h"
#include "network_event.h"
#include "snapshot_conf.h"
@@ -10237,6 +10238,131 @@ testDomainAttachDevice(virDomainPtr domain, const char *xml)
}
+static int
+testDomainUpdateDeviceConfig(virDomainDef *vmdef,
+ virDomainDeviceDef *dev,
+ unsigned int parse_flags,
+ virDomainXMLOption *xmlopt)
+{
+ virDomainDiskDef *newDisk;
+ virDomainDeviceDef oldDev = { .type = dev->type };
+ int pos;
+
+ switch (dev->type) {
+ case VIR_DOMAIN_DEVICE_DISK:
+ newDisk = dev->data.disk;
+ if ((pos = virDomainDiskIndexByName(vmdef, newDisk->dst, false)) < 0) {
+ virReportError(VIR_ERR_INVALID_ARG,
+ _("target %1$s doesn't exist."), newDisk->dst);
+ return -1;
+ }
+
+ oldDev.data.disk = vmdef->disks[pos];
+ if (virDomainDefCompatibleDevice(vmdef, dev, &oldDev,
+ VIR_DOMAIN_DEVICE_ACTION_UPDATE,
+ false) < 0)
+ return -1;
+
+ virDomainDiskDefFree(vmdef->disks[pos]);
+ vmdef->disks[pos] = newDisk;
+ dev->data.disk = NULL;
+ break;
+
+ // TODO: support these here once tested.
+ case VIR_DOMAIN_DEVICE_GRAPHICS:
+ case VIR_DOMAIN_DEVICE_NET:
+ case VIR_DOMAIN_DEVICE_MEMORY:
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
+ _("persistent update of device '%1$s' is not supported by test driver"),
+ virDomainDeviceTypeToString(dev->type));
+ return -1;
+
+ case VIR_DOMAIN_DEVICE_FS:
+ case VIR_DOMAIN_DEVICE_INPUT:
+ case VIR_DOMAIN_DEVICE_SOUND:
+ case VIR_DOMAIN_DEVICE_VIDEO:
+ case VIR_DOMAIN_DEVICE_WATCHDOG:
+ case VIR_DOMAIN_DEVICE_HUB:
+ case VIR_DOMAIN_DEVICE_SMARTCARD:
+ case VIR_DOMAIN_DEVICE_MEMBALLOON:
+ case VIR_DOMAIN_DEVICE_NVRAM:
+ case VIR_DOMAIN_DEVICE_RNG:
+ case VIR_DOMAIN_DEVICE_SHMEM:
+ case VIR_DOMAIN_DEVICE_LEASE:
+ case VIR_DOMAIN_DEVICE_HOSTDEV:
+ case VIR_DOMAIN_DEVICE_CONTROLLER:
+ case VIR_DOMAIN_DEVICE_REDIRDEV:
+ case VIR_DOMAIN_DEVICE_CHR:
+ case VIR_DOMAIN_DEVICE_NONE:
+ case VIR_DOMAIN_DEVICE_TPM:
+ case VIR_DOMAIN_DEVICE_PANIC:
+ case VIR_DOMAIN_DEVICE_IOMMU:
+ case VIR_DOMAIN_DEVICE_VSOCK:
+ case VIR_DOMAIN_DEVICE_AUDIO:
+ case VIR_DOMAIN_DEVICE_CRYPTO:
+ case VIR_DOMAIN_DEVICE_LAST:
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
+ _("persistent update of device '%1$s' is not supported"),
+ virDomainDeviceTypeToString(dev->type));
+ return -1;
+ }
+
+ if (virDomainDefPostParse(vmdef, parse_flags, xmlopt, NULL) < 0)
+ return -1;
+
+ return 0;
+}
+
+
+static int
+testDomainUpdateDeviceFlags(virDomainPtr dom,
+ const char *xml,
+ unsigned int flags)
+{
+ testDriver *driver = dom->conn->privateData;
+ virDomainObj *vm = NULL;
+ g_autoptr(virDomainDef) vmdef = NULL;
+ g_autoptr(virDomainDeviceDef) dev = NULL;
+ int ret = -1;
+ unsigned int parse_flags = 0;
+
+ virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
+ VIR_DOMAIN_AFFECT_CONFIG |
+ VIR_DOMAIN_DEVICE_MODIFY_FORCE, -1);
+
+ if (!(vm = testDomObjFromDomain(dom)))
+ goto cleanup;
+
+ if (virDomainObjBeginJob(vm, VIR_JOB_MODIFY) < 0)
+ goto cleanup;
+
+ if (virDomainObjUpdateModificationImpact(vm, &flags) < 0)
+ goto endjob;
+
+ if ((flags & VIR_DOMAIN_AFFECT_CONFIG) &&
+ !(flags & VIR_DOMAIN_AFFECT_LIVE))
+ parse_flags |= VIR_DOMAIN_DEF_PARSE_INACTIVE;
+
+ if (!(dev = virDomainDeviceDefParse(xml, vm->def, driver->xmlopt,
+ NULL, parse_flags)))
+ goto endjob;
+
+ /* virDomainDefCompatibleDevice call is delayed until we know the
+ * device we're going to update. */
+ if ((ret = testDomainUpdateDeviceConfig(vm->def, dev,
+ parse_flags,
+ driver->xmlopt)) < 0)
+ goto endjob;
+
+ endjob:
+ virDomainObjEndJob(vm);
+
+ cleanup:
+ virDomainObjEndAPI(&vm);
+ return ret;
+}
+
+
/* search for a hostdev matching dev and detach it */
static int
testDomainDetachPrepHostdev(virDomainObj *vm,
@@ -10456,6 +10582,7 @@ static virHypervisorDriver testHypervisorDriver = {
.domainAttachDevice = testDomainAttachDevice, /* 10.0.0 */
.domainAttachDeviceFlags = testDomainAttachDeviceFlags, /* 10.0.0 */
.domainDetachDeviceAlias = testDomainDetachDeviceAlias, /* 10.0.0 */
+ .domainUpdateDeviceFlags = testDomainUpdateDeviceFlags, /* 10.6.0 */
.domainCreateXML = testDomainCreateXML, /* 0.1.4 */
.domainCreateXMLWithFiles = testDomainCreateXMLWithFiles, /* 5.7.0 */
.domainLookupByID = testDomainLookupByID, /* 0.1.1 */
--
2.34.1
3 months, 3 weeks
[PATCH v2] tests: Move domainEventState initialization to qemuTestDriverInit
by Rayhan Faizel
Under the test environment, driver->domainEventState is uninitialized. If a
disk gets dropped, it will attempt to queue an event which will cause a
segmentation fault. This crash does not occur during normal use.
This patch moves driver->domainEventState initialization from qemuhotplugtest
to qemuTestDriverInit in testutilsqemu (Credit goes to Michal Privoznik as he
had already provided the diff).
An additional test case is added to test dropping of disks with startupPolicy
set as optional.
Signed-off-by: Rayhan Faizel <rayhan.faizel(a)gmail.com>
---
As the patch title has completely changed,
v1: https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/44...
[Changes in v2]
- Move domainEventState initialization instead of adding a NULL check
tests/qemuhotplugtest.c | 3 --
...tuppolicy-optional-drop.x86_64-latest.args | 33 ++++++++++++++++
...rtuppolicy-optional-drop.x86_64-latest.xml | 38 +++++++++++++++++++
.../disk-startuppolicy-optional-drop.xml | 23 +++++++++++
tests/qemuxmlconftest.c | 1 +
tests/testutilsqemu.c | 4 ++
6 files changed, 99 insertions(+), 3 deletions(-)
create mode 100644 tests/qemuxmlconfdata/disk-startuppolicy-optional-drop.x86_64-latest.args
create mode 100644 tests/qemuxmlconfdata/disk-startuppolicy-optional-drop.x86_64-latest.xml
create mode 100644 tests/qemuxmlconfdata/disk-startuppolicy-optional-drop.xml
diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c
index d935ad58ed..f707121c47 100644
--- a/tests/qemuhotplugtest.c
+++ b/tests/qemuhotplugtest.c
@@ -501,9 +501,6 @@ mymain(void)
virEventRegisterDefaultImpl();
- if (!(driver.domainEventState = virObjectEventStateNew()))
- return EXIT_FAILURE;
-
driver.lockManager = virLockManagerPluginNew("nop", "qemu",
driver.config->configBaseDir,
0);
diff --git a/tests/qemuxmlconfdata/disk-startuppolicy-optional-drop.x86_64-latest.args b/tests/qemuxmlconfdata/disk-startuppolicy-optional-drop.x86_64-latest.args
new file mode 100644
index 0000000000..13ddbc1a5d
--- /dev/null
+++ b/tests/qemuxmlconfdata/disk-startuppolicy-optional-drop.x86_64-latest.args
@@ -0,0 +1,33 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1 \
+USER=test \
+LOGNAME=test \
+XDG_DATA_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.local/share \
+XDG_CACHE_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.cache \
+XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.config \
+/usr/bin/qemu-system-x86_64 \
+-name guest=QEMUGuest1,debug-threads=on \
+-S \
+-object '{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/var/lib/libvirt/qemu/domain--1-QEMUGuest1/master-key.aes"}' \
+-machine pc,usb=off,dump-guest-core=off,memory-backend=pc.ram,acpi=off \
+-accel tcg \
+-cpu qemu64 \
+-m size=219136k \
+-object '{"qom-type":"memory-backend-ram","id":"pc.ram","size":224395264}' \
+-overcommit mem-lock=off \
+-smp 1,sockets=1,cores=1,threads=1 \
+-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
+-display none \
+-no-user-config \
+-nodefaults \
+-chardev socket,id=charmonitor,fd=1729,server=on,wait=off \
+-mon chardev=charmonitor,id=monitor,mode=control \
+-rtc base=utc \
+-no-shutdown \
+-boot strict=on \
+-device '{"driver":"piix3-usb-uhci","id":"usb","bus":"pci.0","addr":"0x1.0x2"}' \
+-device '{"driver":"lsi","id":"scsi0","bus":"pci.0","addr":"0x2"}' \
+-audiodev '{"id":"audio1","driver":"none"}' \
+-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
+-msg timestamp=on
diff --git a/tests/qemuxmlconfdata/disk-startuppolicy-optional-drop.x86_64-latest.xml b/tests/qemuxmlconfdata/disk-startuppolicy-optional-drop.x86_64-latest.xml
new file mode 100644
index 0000000000..27d0639109
--- /dev/null
+++ b/tests/qemuxmlconfdata/disk-startuppolicy-optional-drop.x86_64-latest.xml
@@ -0,0 +1,38 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory unit='KiB'>219100</memory>
+ <currentMemory unit='KiB'>219100</currentMemory>
+ <vcpu placement='static'>1</vcpu>
+ <os>
+ <type arch='x86_64' machine='pc'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <cpu mode='custom' match='exact' check='none'>
+ <model fallback='forbid'>qemu64</model>
+ </cpu>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-x86_64</emulator>
+ <disk type='volume' device='disk'>
+ <driver name='qemu'/>
+ <source pool='inactive' volume='inactive' startupPolicy='optional'/>
+ <target dev='sda' bus='scsi'/>
+ <address type='drive' controller='0' bus='0' target='0' unit='0'/>
+ </disk>
+ <controller type='usb' index='0' model='piix3-uhci'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>
+ </controller>
+ <controller type='pci' index='0' model='pci-root'/>
+ <controller type='scsi' index='0' model='lsilogic'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/>
+ </controller>
+ <input type='mouse' bus='ps2'/>
+ <input type='keyboard' bus='ps2'/>
+ <audio id='1' type='none'/>
+ <memballoon model='none'/>
+ </devices>
+</domain>
diff --git a/tests/qemuxmlconfdata/disk-startuppolicy-optional-drop.xml b/tests/qemuxmlconfdata/disk-startuppolicy-optional-drop.xml
new file mode 100644
index 0000000000..c6c59978c6
--- /dev/null
+++ b/tests/qemuxmlconfdata/disk-startuppolicy-optional-drop.xml
@@ -0,0 +1,23 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory unit='KiB'>219100</memory>
+ <currentMemory unit='KiB'>219100</currentMemory>
+ <vcpu placement='static'>1</vcpu>
+ <os>
+ <type arch='x86_64' machine='pc'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-x86_64</emulator>
+ <disk type='volume' device='disk'>
+ <source pool='inactive' volume='inactive' startupPolicy='optional'/>
+ <target dev='sda'/>
+ </disk>
+ <memballoon model='none'/>
+ </devices>
+</domain>
diff --git a/tests/qemuxmlconftest.c b/tests/qemuxmlconftest.c
index 4a711fceeb..2a495cc892 100644
--- a/tests/qemuxmlconftest.c
+++ b/tests/qemuxmlconftest.c
@@ -2947,6 +2947,7 @@ mymain(void)
DO_TEST_CAPS_LATEST_FAILURE("disk-network-iscsi-zero-hosts-invalid")
DO_TEST_CAPS_LATEST_PARSE_ERROR("hostdev-scsi-vhost-rawio-invalid")
DO_TEST_CAPS_LATEST_PARSE_ERROR("hostdev-scsi-vhost-sgio-invalid")
+ DO_TEST_CAPS_LATEST("disk-startuppolicy-optional-drop")
/* check that all input files were actually used here */
if (testConfXMLCheck(existingTestCases) < 0)
diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c
index d70850cb5d..ee6cae218a 100644
--- a/tests/testutilsqemu.c
+++ b/tests/testutilsqemu.c
@@ -236,6 +236,7 @@ void qemuTestDriverFree(virQEMUDriver *driver)
virObjectUnref(driver->caps);
virObjectUnref(driver->config);
virObjectUnref(driver->securityManager);
+ virObjectUnref(driver->domainEventState);
g_clear_object(&driver->nbdkitCapsCache);
virCPUDefFree(cpuDefault);
@@ -369,6 +370,9 @@ int qemuTestDriverInit(virQEMUDriver *driver)
if (!(driver->securityManager = virSecurityManagerNewStack(mgr)))
goto error;
+ if (!(driver->domainEventState = virObjectEventStateNew()))
+ goto error;
+
qemuTestSetHostCPU(driver, driver->hostarch, NULL);
VIR_FREE(cfg->vncTLSx509certdir);
--
2.34.1
3 months, 3 weeks
[PATCH v1] security_manager: Ensure top lock is acquired before nested locks
by hongmianquan
We need to ensure top lock is acquired before nested lock. Otherwise deadlock
issues may arise. We have the stack security driver, which internally manages
other security drivers, we call them "top" and "nested".
We call virSecurityStackPreFork() to lock the top one, and it also locks
and then unlocks the nested drivers prior to fork. Then in qemuSecurityPostFork(),
it unlocks the top one, but not the nested ones. Thus, if one of the nested
drivers ("dac" or "selinux") is still locked, it will cause a deadlock.
We discovered this case: the nested list obtained through the qemuSecurityGetNested()
will be locked for subsequent use, such as in virQEMUDriverCreateCapabilities(),
where the nested list is locked using qemuSecurityGetDOI, but the top one is not locked beforehand.
The problem stack is as follows:
libvirtd thread1 libvirtd thread2 child libvirtd
| | |
| | |
virsh capabilities qemuProcessLanuch |
| | |
| lock top |
| | |
lock nested | |
| | |
| fork------------------->|(held nested lock)
| | |
| | |
unlock nested unlock top unlock top
|
|
qemuSecuritySetSocketLabel
|
|
lock nested (deadlock)
In this commit, we ensure that the top lock is acquired before the nested lock,
so during fork, it's not possible for another task to acquire the nested lock.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1303031
Signed-off-by: hongmianquan <hongmianquan(a)bytedance.com>
---
src/libvirt_private.syms | 3 ++-
src/qemu/qemu_conf.c | 9 ++++++++-
src/qemu/qemu_driver.c | 16 +++++++++-------
src/qemu/qemu_security.h | 2 ++
src/security/security_manager.c | 22 ++++++++++++++++++++++
src/security/security_manager.h | 2 ++
6 files changed, 45 insertions(+), 9 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index bac4a8a366..39cdb90772 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1806,7 +1806,8 @@ virSecurityManagerTransactionAbort;
virSecurityManagerTransactionCommit;
virSecurityManagerTransactionStart;
virSecurityManagerVerify;
-
+virSecurityManagerStackLock;
+virSecurityManagerStackUnlock;
# security/security_util.h
virSecurityXATTRNamespaceDefined;
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 4050a82341..21f0739fd5 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -1380,6 +1380,9 @@ virCaps *virQEMUDriverCreateCapabilities(virQEMUDriver *driver)
return NULL;
}
+ /* Ensure top lock is acquired before nested locks */
+ qemuSecurityStackLock(driver->securityManager);
+
/* access sec drivers and create a sec model for each one */
if (!(sec_managers = qemuSecurityGetNested(driver->securityManager)))
return NULL;
@@ -1402,8 +1405,10 @@ virCaps *virQEMUDriverCreateCapabilities(virQEMUDriver *driver)
lbl = qemuSecurityGetBaseLabel(sec_managers[i], virtTypes[j]);
type = virDomainVirtTypeToString(virtTypes[j]);
if (lbl &&
- virCapabilitiesHostSecModelAddBaseLabel(sm, type, lbl) < 0)
+ virCapabilitiesHostSecModelAddBaseLabel(sm, type, lbl) < 0) {
+ qemuSecurityStackUnlock(driver->securityManager);
return NULL;
+ }
}
VIR_DEBUG("Initialized caps for security driver \"%s\" with "
@@ -1412,6 +1417,8 @@ virCaps *virQEMUDriverCreateCapabilities(virQEMUDriver *driver)
caps->host.numa = virCapabilitiesHostNUMANewHost();
caps->host.cpu = virQEMUDriverGetHostCPU(driver);
+
+ qemuSecurityStackUnlock(driver->securityManager);
return g_steal_pointer(&caps);
}
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index fc1704f4fc..c980a0990f 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -560,7 +560,6 @@ qemuStateInitialize(bool privileged,
bool autostart = true;
size_t i;
const char *defsecmodel = NULL;
- g_autofree virSecurityManager **sec_managers = NULL;
g_autoptr(virIdentity) identity = virIdentityGetCurrent();
qemu_driver = g_new0(virQEMUDriver, 1);
@@ -835,11 +834,8 @@ qemuStateInitialize(bool privileged,
if (!qemu_driver->qemuCapsCache)
goto error;
- if (!(sec_managers = qemuSecurityGetNested(qemu_driver->securityManager)))
- goto error;
-
- if (sec_managers[0] != NULL)
- defsecmodel = qemuSecurityGetModel(sec_managers[0]);
+ if (qemu_driver->securityManager != NULL)
+ defsecmodel = qemuSecurityGetModel(qemu_driver->securityManager);
if (!(qemu_driver->xmlopt = virQEMUDriverCreateXMLConf(qemu_driver,
defsecmodel)))
@@ -5663,7 +5659,12 @@ static int qemuDomainGetSecurityLabelList(virDomainPtr dom,
ret = 0;
} else {
int len = 0;
- virSecurityManager ** mgrs = qemuSecurityGetNested(driver->securityManager);
+ virSecurityManager ** mgrs = NULL;
+
+ /* Ensure top lock is acquired before nested locks */
+ qemuSecurityStackLock(driver->securityManager);
+
+ mgrs = qemuSecurityGetNested(driver->securityManager);
if (!mgrs)
goto cleanup;
@@ -5688,6 +5689,7 @@ static int qemuDomainGetSecurityLabelList(virDomainPtr dom,
}
cleanup:
+ qemuSecurityStackUnlock(driver->securityManager);
virDomainObjEndAPI(&vm);
return ret;
}
diff --git a/src/qemu/qemu_security.h b/src/qemu/qemu_security.h
index 41da33debc..19fcb3c939 100644
--- a/src/qemu/qemu_security.h
+++ b/src/qemu/qemu_security.h
@@ -151,3 +151,5 @@ int qemuSecurityCommandRun(virQEMUDriver *driver,
#define qemuSecuritySetTapFDLabel virSecurityManagerSetTapFDLabel
#define qemuSecurityStackAddNested virSecurityManagerStackAddNested
#define qemuSecurityVerify virSecurityManagerVerify
+#define qemuSecurityStackLock virSecurityManagerStackLock
+#define qemuSecurityStackUnlock virSecurityManagerStackUnlock
\ No newline at end of file
diff --git a/src/security/security_manager.c b/src/security/security_manager.c
index 24f2f3d3dc..c49c4f708f 100644
--- a/src/security/security_manager.c
+++ b/src/security/security_manager.c
@@ -989,6 +989,28 @@ virSecurityManagerGetNested(virSecurityManager *mgr)
return list;
}
+/*
+ * Usually called before virSecurityManagerGetNested().
+ * We need to ensure locking the stack security manager before
+ * locking the nested security manager to maintain the correct
+ * synchronization state.
+ * It must be followed by a call virSecurityManagerStackUnlock().
+ */
+void
+virSecurityManagerStackLock(virSecurityManager *mgr)
+{
+ if (STREQ("stack", mgr->drv->name))
+ virObjectLock(mgr);
+}
+
+
+void
+virSecurityManagerStackUnlock(virSecurityManager *mgr)
+{
+ if (STREQ("stack", mgr->drv->name))
+ virObjectUnlock(mgr);
+}
+
/**
* virSecurityManagerDomainSetPathLabel:
diff --git a/src/security/security_manager.h b/src/security/security_manager.h
index a416af3215..bb6d22bc31 100644
--- a/src/security/security_manager.h
+++ b/src/security/security_manager.h
@@ -158,6 +158,8 @@ int virSecurityManagerSetTapFDLabel(virSecurityManager *mgr,
char *virSecurityManagerGetMountOptions(virSecurityManager *mgr,
virDomainDef *vm);
virSecurityManager ** virSecurityManagerGetNested(virSecurityManager *mgr);
+void virSecurityManagerStackLock(virSecurityManager *mgr);
+void virSecurityManagerStackUnlock(virSecurityManager *mgr);
typedef enum {
VIR_SECURITY_DOMAIN_IMAGE_LABEL_BACKING_CHAIN = 1 << 0,
--
2.11.0
3 months, 3 weeks
Re: [PATCH v1] security_manager: Ensure top lock is acquired before
nested locks
by ByteDance
Yes, we have recently encountered this issue, and it is exactly the same as the scenario described in the bug report. We investigated the code and found this potential problem. The issue occurs when collecting information through the ‘virsh capabilities' while concurrently creating vm. The higher the frequency and concurrency, the more likely the issue will reproduce. We believe this is a very rare bug; our environment has been running for three years, and we've only encountered this issue once. To successfully reproduce the problem, we constructed a test case by adding a sleep between the lock nested and unlock. But we still need to try to fix this issue, even though the probability is very low.
3 months, 3 weeks
[PATCH v2] conf: Fix rawio/sgio checks for non-scsi hostdev devices
by Rayhan Faizel
The current hostdev parsing logic sets rawio or sgio even if the hostdev type
is not 'scsi'. The rawio field in virDomainHostdevSubsysSCSI overlaps with
wwpn field in virDomainHostdevSubsysSCSIVHost, consequently setting a bogus
pointer value such as 0x1 or 0x2 from virDomainHostdevSubsysSCSIVHost's
point of view. This leads to a segmentation fault when it attempts to free
wwpn.
While setting sgio does not appear to crash, it shares the same flawed logic
as setting rawio.
Instead, we ensure these are set only after the hostdev type check succeeds.
This patch also adds two test cases to exercise both scenarios.
Fixes: bdb95b520c53f9bacc6504fc51381bac4813be38
Signed-off-by: Rayhan Faizel <rayhan.faizel(a)gmail.com>
---
[Changes in v2]
- Reworked fix to use temporary variables instead of xpath queries.
- Added comments for future reference.
- Mention fixed commit.
src/conf/domain_conf.c | 26 +++++++++---
...scsi-vhost-rawio-invalid.x86_64-latest.err | 1 +
.../hostdev-scsi-vhost-rawio-invalid.xml | 41 +++++++++++++++++++
...-scsi-vhost-sgio-invalid.x86_64-latest.err | 1 +
.../hostdev-scsi-vhost-sgio-invalid.xml | 41 +++++++++++++++++++
tests/qemuxmlconftest.c | 2 +
6 files changed, 106 insertions(+), 6 deletions(-)
create mode 100644 tests/qemuxmlconfdata/hostdev-scsi-vhost-rawio-invalid.x86_64-latest.err
create mode 100644 tests/qemuxmlconfdata/hostdev-scsi-vhost-rawio-invalid.xml
create mode 100644 tests/qemuxmlconfdata/hostdev-scsi-vhost-sgio-invalid.x86_64-latest.err
create mode 100644 tests/qemuxmlconfdata/hostdev-scsi-vhost-sgio-invalid.xml
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 334152c4ff..7033b4e9fe 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6220,6 +6220,8 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node,
virDomainHostdevSubsysMediatedDev *mdevsrc = &def->source.subsys.u.mdev;
virTristateBool managed;
g_autofree char *model = NULL;
+ virDomainDeviceSGIO sgio;
+ virTristateBool rawio;
int rv;
/* @managed can be read from the xml document - it is always an
@@ -6259,7 +6261,7 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node,
if ((rv = virXMLPropEnum(node, "sgio",
virDomainDeviceSGIOTypeFromString,
VIR_XML_PROP_NONZERO,
- &scsisrc->sgio)) < 0) {
+ &sgio)) < 0) {
return -1;
}
@@ -6269,18 +6271,30 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node,
_("sgio is only supported for scsi host device"));
return -1;
}
+
+ /* Set sgio only after checking if hostdev type is 'scsi' to avoid
+ * clobbering other union structures.
+ */
+ scsisrc->sgio = sgio;
}
if ((rv = virXMLPropTristateBool(node, "rawio",
VIR_XML_PROP_NONE,
- &scsisrc->rawio)) < 0) {
+ &rawio)) < 0) {
return -1;
}
- if (rv > 0 && def->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("rawio is only supported for scsi host device"));
- return -1;
+ if (rv > 0) {
+ if (def->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("rawio is only supported for scsi host device"));
+ return -1;
+ }
+
+ /* Set rawio only after checking if hostdev type is 'scsi' to avoid
+ * clobbering other union structures.
+ */
+ scsisrc->rawio = rawio;
}
if (def->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV &&
diff --git a/tests/qemuxmlconfdata/hostdev-scsi-vhost-rawio-invalid.x86_64-latest.err b/tests/qemuxmlconfdata/hostdev-scsi-vhost-rawio-invalid.x86_64-latest.err
new file mode 100644
index 0000000000..ddaf449658
--- /dev/null
+++ b/tests/qemuxmlconfdata/hostdev-scsi-vhost-rawio-invalid.x86_64-latest.err
@@ -0,0 +1 @@
+XML error: rawio is only supported for scsi host device
diff --git a/tests/qemuxmlconfdata/hostdev-scsi-vhost-rawio-invalid.xml b/tests/qemuxmlconfdata/hostdev-scsi-vhost-rawio-invalid.xml
new file mode 100644
index 0000000000..80760ab941
--- /dev/null
+++ b/tests/qemuxmlconfdata/hostdev-scsi-vhost-rawio-invalid.xml
@@ -0,0 +1,41 @@
+<domain type='qemu'>
+ <name>QEMUGuest2</name>
+ <uuid>c7a5fdbd-edaf-9466-926a-d65c16db1809</uuid>
+ <memory unit='KiB'>219100</memory>
+ <currentMemory unit='KiB'>219100</currentMemory>
+ <vcpu placement='static'>1</vcpu>
+ <os>
+ <type arch='x86_64' machine='pc'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-x86_64</emulator>
+ <disk type='block' device='disk'>
+ <source dev='/dev/HostVG/QEMUGuest2'/>
+ <target dev='hda' bus='ide'/>
+ <address type='drive' controller='0' bus='0' target='0' unit='0'/>
+ </disk>
+ <controller type='scsi' index='0' model='virtio-scsi'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
+ </controller>
+ <controller type='usb' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>
+ </controller>
+ <controller type='ide' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/>
+ </controller>
+ <controller type='pci' index='0' model='pci-root'/>
+ <input type='mouse' bus='ps2'/>
+ <input type='keyboard' bus='ps2'/>
+ <hostdev mode='subsystem' type='scsi_host' rawio='yes'>
+ <source protocol='vhost' wwpn='naa.5123456789abcde0'/>
+ </hostdev>
+ <memballoon model='virtio'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/>
+ </memballoon>
+ </devices>
+</domain>
diff --git a/tests/qemuxmlconfdata/hostdev-scsi-vhost-sgio-invalid.x86_64-latest.err b/tests/qemuxmlconfdata/hostdev-scsi-vhost-sgio-invalid.x86_64-latest.err
new file mode 100644
index 0000000000..32256aad1c
--- /dev/null
+++ b/tests/qemuxmlconfdata/hostdev-scsi-vhost-sgio-invalid.x86_64-latest.err
@@ -0,0 +1 @@
+XML error: sgio is only supported for scsi host device
diff --git a/tests/qemuxmlconfdata/hostdev-scsi-vhost-sgio-invalid.xml b/tests/qemuxmlconfdata/hostdev-scsi-vhost-sgio-invalid.xml
new file mode 100644
index 0000000000..c549bddc1d
--- /dev/null
+++ b/tests/qemuxmlconfdata/hostdev-scsi-vhost-sgio-invalid.xml
@@ -0,0 +1,41 @@
+<domain type='qemu'>
+ <name>QEMUGuest2</name>
+ <uuid>c7a5fdbd-edaf-9466-926a-d65c16db1809</uuid>
+ <memory unit='KiB'>219100</memory>
+ <currentMemory unit='KiB'>219100</currentMemory>
+ <vcpu placement='static'>1</vcpu>
+ <os>
+ <type arch='x86_64' machine='pc'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-x86_64</emulator>
+ <disk type='block' device='disk'>
+ <source dev='/dev/HostVG/QEMUGuest2'/>
+ <target dev='hda' bus='ide'/>
+ <address type='drive' controller='0' bus='0' target='0' unit='0'/>
+ </disk>
+ <controller type='scsi' index='0' model='virtio-scsi'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
+ </controller>
+ <controller type='usb' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>
+ </controller>
+ <controller type='ide' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/>
+ </controller>
+ <controller type='pci' index='0' model='pci-root'/>
+ <input type='mouse' bus='ps2'/>
+ <input type='keyboard' bus='ps2'/>
+ <hostdev mode='subsystem' type='scsi_host' sgio='filtered'>
+ <source protocol='vhost' wwpn='naa.5123456789abcde0'/>
+ </hostdev>
+ <memballoon model='virtio'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/>
+ </memballoon>
+ </devices>
+</domain>
diff --git a/tests/qemuxmlconftest.c b/tests/qemuxmlconftest.c
index 8e0d47c6fd..1236ed5df1 100644
--- a/tests/qemuxmlconftest.c
+++ b/tests/qemuxmlconftest.c
@@ -2993,6 +2993,8 @@ mymain(void)
DO_TEST_CAPS_LATEST("sound-device-virtio")
DO_TEST_CAPS_LATEST_FAILURE("disk-network-iscsi-zero-hosts-invalid")
+ DO_TEST_CAPS_LATEST_PARSE_ERROR("hostdev-scsi-vhost-rawio-invalid")
+ DO_TEST_CAPS_LATEST_PARSE_ERROR("hostdev-scsi-vhost-sgio-invalid")
/* check that all input files were actually used here */
if (testConfXMLCheck(existingTestCases) < 0)
--
2.34.1
3 months, 3 weeks
[PATCH] qemuDomainChangeNet: forbid changing portgroup
by Adam Julis
Changing the postgroup attribute caused unexpected behavior.
Although it can be implemented, it has a non-trivial solution.
No requirement or use has yet been found for implementing this
feature, so it has been disabled for hot-plug.
Resolves: https://issues.redhat.com/browse/RHEL-7299
Signed-off-by: Adam Julis <ajulis(a)redhat.com>
---
src/qemu/qemu_hotplug.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 4a3f4f657e..08ca7ab973 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -3937,6 +3937,13 @@ qemuDomainChangeNet(virQEMUDriver *driver,
else
needBridgeChange = true;
}
+
+ if (STRNEQ_NULLABLE(olddev->data.network.portgroup, newdev->data.network.portgroup)) {
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+ _("cannot modify network device portgroup attribute"));
+ goto cleanup;
+ }
+
/* other things handled in common code directly below this switch */
break;
--
2.45.2
3 months, 3 weeks
[PATCH] qemu_domain: Check if driver->domainEventState is NULL
by Rayhan Faizel
Under the test environment, driver->domainEventState is uninitialized. If a
disk gets dropped, it will attempt to queue an event which will cause a
segmentation fault. This crash does not occur during normal use.
This patch adds a quick check to ensure driver->domainEventState is not NULL
along with a testcase exercising the dropping of disks with startupPolicy set
as 'optional'.
Signed-off-by: Rayhan Faizel <rayhan.faizel(a)gmail.com>
---
src/qemu/qemu_domain.c | 3 +-
...tuppolicy-optional-drop.x86_64-latest.args | 33 ++++++++++++++++
...rtuppolicy-optional-drop.x86_64-latest.xml | 38 +++++++++++++++++++
.../disk-startuppolicy-optional-drop.xml | 23 +++++++++++
tests/qemuxmlconftest.c | 2 +
5 files changed, 98 insertions(+), 1 deletion(-)
create mode 100644 tests/qemuxmlconfdata/disk-startuppolicy-optional-drop.x86_64-latest.args
create mode 100644 tests/qemuxmlconfdata/disk-startuppolicy-optional-drop.x86_64-latest.xml
create mode 100644 tests/qemuxmlconfdata/disk-startuppolicy-optional-drop.xml
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 7ba2ea4a5e..109c5bbd52 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -7592,7 +7592,8 @@ qemuDomainCheckRemoveOptionalDisk(virQEMUDriver *driver,
virDomainDiskDefFree(disk);
}
- virObjectEventStateQueue(driver->domainEventState, event);
+ if (driver->domainEventState)
+ virObjectEventStateQueue(driver->domainEventState, event);
}
diff --git a/tests/qemuxmlconfdata/disk-startuppolicy-optional-drop.x86_64-latest.args b/tests/qemuxmlconfdata/disk-startuppolicy-optional-drop.x86_64-latest.args
new file mode 100644
index 0000000000..13ddbc1a5d
--- /dev/null
+++ b/tests/qemuxmlconfdata/disk-startuppolicy-optional-drop.x86_64-latest.args
@@ -0,0 +1,33 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1 \
+USER=test \
+LOGNAME=test \
+XDG_DATA_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.local/share \
+XDG_CACHE_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.cache \
+XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.config \
+/usr/bin/qemu-system-x86_64 \
+-name guest=QEMUGuest1,debug-threads=on \
+-S \
+-object '{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/var/lib/libvirt/qemu/domain--1-QEMUGuest1/master-key.aes"}' \
+-machine pc,usb=off,dump-guest-core=off,memory-backend=pc.ram,acpi=off \
+-accel tcg \
+-cpu qemu64 \
+-m size=219136k \
+-object '{"qom-type":"memory-backend-ram","id":"pc.ram","size":224395264}' \
+-overcommit mem-lock=off \
+-smp 1,sockets=1,cores=1,threads=1 \
+-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
+-display none \
+-no-user-config \
+-nodefaults \
+-chardev socket,id=charmonitor,fd=1729,server=on,wait=off \
+-mon chardev=charmonitor,id=monitor,mode=control \
+-rtc base=utc \
+-no-shutdown \
+-boot strict=on \
+-device '{"driver":"piix3-usb-uhci","id":"usb","bus":"pci.0","addr":"0x1.0x2"}' \
+-device '{"driver":"lsi","id":"scsi0","bus":"pci.0","addr":"0x2"}' \
+-audiodev '{"id":"audio1","driver":"none"}' \
+-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
+-msg timestamp=on
diff --git a/tests/qemuxmlconfdata/disk-startuppolicy-optional-drop.x86_64-latest.xml b/tests/qemuxmlconfdata/disk-startuppolicy-optional-drop.x86_64-latest.xml
new file mode 100644
index 0000000000..27d0639109
--- /dev/null
+++ b/tests/qemuxmlconfdata/disk-startuppolicy-optional-drop.x86_64-latest.xml
@@ -0,0 +1,38 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory unit='KiB'>219100</memory>
+ <currentMemory unit='KiB'>219100</currentMemory>
+ <vcpu placement='static'>1</vcpu>
+ <os>
+ <type arch='x86_64' machine='pc'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <cpu mode='custom' match='exact' check='none'>
+ <model fallback='forbid'>qemu64</model>
+ </cpu>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-x86_64</emulator>
+ <disk type='volume' device='disk'>
+ <driver name='qemu'/>
+ <source pool='inactive' volume='inactive' startupPolicy='optional'/>
+ <target dev='sda' bus='scsi'/>
+ <address type='drive' controller='0' bus='0' target='0' unit='0'/>
+ </disk>
+ <controller type='usb' index='0' model='piix3-uhci'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>
+ </controller>
+ <controller type='pci' index='0' model='pci-root'/>
+ <controller type='scsi' index='0' model='lsilogic'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/>
+ </controller>
+ <input type='mouse' bus='ps2'/>
+ <input type='keyboard' bus='ps2'/>
+ <audio id='1' type='none'/>
+ <memballoon model='none'/>
+ </devices>
+</domain>
diff --git a/tests/qemuxmlconfdata/disk-startuppolicy-optional-drop.xml b/tests/qemuxmlconfdata/disk-startuppolicy-optional-drop.xml
new file mode 100644
index 0000000000..c6c59978c6
--- /dev/null
+++ b/tests/qemuxmlconfdata/disk-startuppolicy-optional-drop.xml
@@ -0,0 +1,23 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory unit='KiB'>219100</memory>
+ <currentMemory unit='KiB'>219100</currentMemory>
+ <vcpu placement='static'>1</vcpu>
+ <os>
+ <type arch='x86_64' machine='pc'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-x86_64</emulator>
+ <disk type='volume' device='disk'>
+ <source pool='inactive' volume='inactive' startupPolicy='optional'/>
+ <target dev='sda'/>
+ </disk>
+ <memballoon model='none'/>
+ </devices>
+</domain>
diff --git a/tests/qemuxmlconftest.c b/tests/qemuxmlconftest.c
index 5700ea314f..16ecc1b7e4 100644
--- a/tests/qemuxmlconftest.c
+++ b/tests/qemuxmlconftest.c
@@ -2986,6 +2986,8 @@ mymain(void)
DO_TEST_CAPS_LATEST("net-usb")
DO_TEST_CAPS_LATEST("sound-device-virtio")
+ DO_TEST_CAPS_LATEST("disk-startuppolicy-optional-drop")
+
/* check that all input files were actually used here */
if (testConfXMLCheck(existingTestCases) < 0)
ret = -1;
--
2.34.1
3 months, 3 weeks
[PATCH] conf: Fix rawio/sgio checks for non-scsi hostdev devices
by Rayhan Faizel
The current hostdev parsing logic sets rawio or sgio even if the hostdev type
is not 'scsi'. The rawio field in virDomainHostdevSubsysSCSI overlaps with
wwpn field in virDomainHostdevSubsysSCSIVHost, consequently setting a bogus
pointer value such as 0x1 or 0x2 from virDomainHostdevSubsysSCSIVHost's
point of view. This leads to a segmentation fault when it attempts to free
wwpn.
While setting sgio does not appear to crash, it shares the same flawed logic
as setting rawio.
Instead, we test the presence of their xpaths along with the hostdev type
before setting either attributes. This patch also adds two test cases to
exercise both scenarios.
Signed-off-by: Rayhan Faizel <rayhan.faizel(a)gmail.com>
---
src/conf/domain_conf.c | 33 ++++++++-------
...scsi-vhost-rawio-invalid.x86_64-latest.err | 1 +
.../hostdev-scsi-vhost-rawio-invalid.xml | 41 +++++++++++++++++++
...-scsi-vhost-sgio-invalid.x86_64-latest.err | 1 +
.../hostdev-scsi-vhost-sgio-invalid.xml | 41 +++++++++++++++++++
tests/qemuxmlconftest.c | 2 +
6 files changed, 102 insertions(+), 17 deletions(-)
create mode 100644 tests/qemuxmlconfdata/hostdev-scsi-vhost-rawio-invalid.x86_64-latest.err
create mode 100644 tests/qemuxmlconfdata/hostdev-scsi-vhost-rawio-invalid.xml
create mode 100644 tests/qemuxmlconfdata/hostdev-scsi-vhost-sgio-invalid.x86_64-latest.err
create mode 100644 tests/qemuxmlconfdata/hostdev-scsi-vhost-sgio-invalid.xml
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 334152c4ff..5f8c6f2672 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6220,7 +6220,6 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node,
virDomainHostdevSubsysMediatedDev *mdevsrc = &def->source.subsys.u.mdev;
virTristateBool managed;
g_autofree char *model = NULL;
- int rv;
/* @managed can be read from the xml document - it is always an
* attribute of the toplevel element, no matter what type of
@@ -6256,33 +6255,33 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node,
return -1;
}
- if ((rv = virXMLPropEnum(node, "sgio",
- virDomainDeviceSGIOTypeFromString,
- VIR_XML_PROP_NONZERO,
- &scsisrc->sgio)) < 0) {
+ if (def->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI &&
+ virXPathBoolean("boolean(./@sgio)", ctxt)) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("sgio is only supported for scsi host device"));
return -1;
}
- if (rv > 0) {
- if (def->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("sgio is only supported for scsi host device"));
- return -1;
- }
- }
-
- if ((rv = virXMLPropTristateBool(node, "rawio",
- VIR_XML_PROP_NONE,
- &scsisrc->rawio)) < 0) {
+ if (virXMLPropEnum(node, "sgio",
+ virDomainDeviceSGIOTypeFromString,
+ VIR_XML_PROP_NONZERO,
+ &scsisrc->sgio) < 0) {
return -1;
}
- if (rv > 0 && def->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) {
+ if (def->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI &&
+ virXPathBoolean("boolean(./@rawio)", ctxt)) {
virReportError(VIR_ERR_XML_ERROR, "%s",
_("rawio is only supported for scsi host device"));
return -1;
}
+ if (virXMLPropTristateBool(node, "rawio",
+ VIR_XML_PROP_NONE,
+ &scsisrc->rawio) < 0) {
+ return -1;
+ }
+
if (def->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV &&
def->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI_HOST) {
if (model) {
diff --git a/tests/qemuxmlconfdata/hostdev-scsi-vhost-rawio-invalid.x86_64-latest.err b/tests/qemuxmlconfdata/hostdev-scsi-vhost-rawio-invalid.x86_64-latest.err
new file mode 100644
index 0000000000..ddaf449658
--- /dev/null
+++ b/tests/qemuxmlconfdata/hostdev-scsi-vhost-rawio-invalid.x86_64-latest.err
@@ -0,0 +1 @@
+XML error: rawio is only supported for scsi host device
diff --git a/tests/qemuxmlconfdata/hostdev-scsi-vhost-rawio-invalid.xml b/tests/qemuxmlconfdata/hostdev-scsi-vhost-rawio-invalid.xml
new file mode 100644
index 0000000000..80760ab941
--- /dev/null
+++ b/tests/qemuxmlconfdata/hostdev-scsi-vhost-rawio-invalid.xml
@@ -0,0 +1,41 @@
+<domain type='qemu'>
+ <name>QEMUGuest2</name>
+ <uuid>c7a5fdbd-edaf-9466-926a-d65c16db1809</uuid>
+ <memory unit='KiB'>219100</memory>
+ <currentMemory unit='KiB'>219100</currentMemory>
+ <vcpu placement='static'>1</vcpu>
+ <os>
+ <type arch='x86_64' machine='pc'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-x86_64</emulator>
+ <disk type='block' device='disk'>
+ <source dev='/dev/HostVG/QEMUGuest2'/>
+ <target dev='hda' bus='ide'/>
+ <address type='drive' controller='0' bus='0' target='0' unit='0'/>
+ </disk>
+ <controller type='scsi' index='0' model='virtio-scsi'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
+ </controller>
+ <controller type='usb' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>
+ </controller>
+ <controller type='ide' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/>
+ </controller>
+ <controller type='pci' index='0' model='pci-root'/>
+ <input type='mouse' bus='ps2'/>
+ <input type='keyboard' bus='ps2'/>
+ <hostdev mode='subsystem' type='scsi_host' rawio='yes'>
+ <source protocol='vhost' wwpn='naa.5123456789abcde0'/>
+ </hostdev>
+ <memballoon model='virtio'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/>
+ </memballoon>
+ </devices>
+</domain>
diff --git a/tests/qemuxmlconfdata/hostdev-scsi-vhost-sgio-invalid.x86_64-latest.err b/tests/qemuxmlconfdata/hostdev-scsi-vhost-sgio-invalid.x86_64-latest.err
new file mode 100644
index 0000000000..32256aad1c
--- /dev/null
+++ b/tests/qemuxmlconfdata/hostdev-scsi-vhost-sgio-invalid.x86_64-latest.err
@@ -0,0 +1 @@
+XML error: sgio is only supported for scsi host device
diff --git a/tests/qemuxmlconfdata/hostdev-scsi-vhost-sgio-invalid.xml b/tests/qemuxmlconfdata/hostdev-scsi-vhost-sgio-invalid.xml
new file mode 100644
index 0000000000..c549bddc1d
--- /dev/null
+++ b/tests/qemuxmlconfdata/hostdev-scsi-vhost-sgio-invalid.xml
@@ -0,0 +1,41 @@
+<domain type='qemu'>
+ <name>QEMUGuest2</name>
+ <uuid>c7a5fdbd-edaf-9466-926a-d65c16db1809</uuid>
+ <memory unit='KiB'>219100</memory>
+ <currentMemory unit='KiB'>219100</currentMemory>
+ <vcpu placement='static'>1</vcpu>
+ <os>
+ <type arch='x86_64' machine='pc'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-x86_64</emulator>
+ <disk type='block' device='disk'>
+ <source dev='/dev/HostVG/QEMUGuest2'/>
+ <target dev='hda' bus='ide'/>
+ <address type='drive' controller='0' bus='0' target='0' unit='0'/>
+ </disk>
+ <controller type='scsi' index='0' model='virtio-scsi'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
+ </controller>
+ <controller type='usb' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>
+ </controller>
+ <controller type='ide' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/>
+ </controller>
+ <controller type='pci' index='0' model='pci-root'/>
+ <input type='mouse' bus='ps2'/>
+ <input type='keyboard' bus='ps2'/>
+ <hostdev mode='subsystem' type='scsi_host' sgio='filtered'>
+ <source protocol='vhost' wwpn='naa.5123456789abcde0'/>
+ </hostdev>
+ <memballoon model='virtio'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/>
+ </memballoon>
+ </devices>
+</domain>
diff --git a/tests/qemuxmlconftest.c b/tests/qemuxmlconftest.c
index 8e0d47c6fd..1236ed5df1 100644
--- a/tests/qemuxmlconftest.c
+++ b/tests/qemuxmlconftest.c
@@ -2993,6 +2993,8 @@ mymain(void)
DO_TEST_CAPS_LATEST("sound-device-virtio")
DO_TEST_CAPS_LATEST_FAILURE("disk-network-iscsi-zero-hosts-invalid")
+ DO_TEST_CAPS_LATEST_PARSE_ERROR("hostdev-scsi-vhost-rawio-invalid")
+ DO_TEST_CAPS_LATEST_PARSE_ERROR("hostdev-scsi-vhost-sgio-invalid")
/* check that all input files were actually used here */
if (testConfXMLCheck(existingTestCases) < 0)
--
2.34.1
3 months, 3 weeks