conf: Fix rawio/sgio checks for non-scsi hostdev devices

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@gmail.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 334152c4ff7f6512f8e666c2b7e15f430a91b64d..7033b4e9fe258d93cc26dba390fba610954fee89 100644 (file)
--- 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 (file)
index 0000000..ddaf449
--- /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 (file)
index 0000000..80760ab
--- /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 (file)
index 0000000..32256aa
--- /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 (file)
index 0000000..c549bdd
--- /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 2ede8cd29dd290202ef3b9181fef5a1cf7376617..4a711fceebacba31dc9ac3a157e2b52acbb27eb4 100644 (file)
--- a/tests/qemuxmlconftest.c
+++ b/tests/qemuxmlconftest.c
@@ -2945,6 +2945,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)