conf: Fix out-of-bounds write during cleanup of virDomainNumaDefNodeDistanceParseXML

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.

Fixes: 742494eed8dbdde8b1d05a306032334e6226beea
Signed-off-by: Rayhan Faizel <rayhan.faizel@gmail.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c
index d8120de6d2aca9db919e217d0a22d1d034246da1..0a0e2911f7cc7e6e3314502c446d5f7da83185ce 100644 (file)
--- 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 (file)
index 0000000..4866ff5
--- /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 (file)
index 0000000..62a6c32
--- /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 2a495cc892c33822d2ab64572045f41df49b0935..4f2966109d8da59d49e9cdceb4ca885e3c06943c 100644 (file)
--- 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 */