[libvirt] [PATCH] CVE-2014-8131: Fix possible deadlock and segfault in qemuConnectGetAllDomainStats()

[Martin Kletzander]

When user doesn't have read access on one of the domains he requested,
the for loop could exit abruptly or continue and override pointer which
pointed to locked object.

This patch fixed two issues at once.  One is that domflags might have
had QEMU_DOMAIN_STATS_HAVE_JOB even when there was no job started (this
is fixed by doing domflags |= QEMU_DOMAIN_STATS_HAVE_JOB only when the
job was acquired and cleaning domflags on every start of the loop.
Second one is that the domain is kept locked when
virConnectGetAllDomainStatsCheckACL() fails and continues the loop when
it didn't end.  Adding a simple virObjectUnlock() and clearing the
pointer ought to do.

Signed-off-by: Martin Kletzander <mkletzan at redhat.com>
---

Since this is very low priority and it was mistakenly disclosed in
another patch there is no embargo on this CVE.  Patches to maint
branches will be pushed after back-porting (couple of minutes).

Having said that, I am probably not the right one to write up the
libvirt security notice, but I'll try drafting one, eventually.


 src/qemu/qemu_driver.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index be37c8f..ae6225b 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -18740,20 +18740,23 @@ qemuConnectGetAllDomainStats(virConnectPtr conn,
         privflags |= QEMU_DOMAIN_STATS_HAVE_JOB;

     for (i = 0; i < ndoms; i++) {
-        domflags = privflags;
         virDomainStatsRecordPtr tmp = NULL;
+        domflags = 0;

         if (!(dom = qemuDomObjFromDomain(doms[i])))
             continue;

         if (doms != domlist &&
-            !virConnectGetAllDomainStatsCheckACL(conn, dom->def))
+            !virConnectGetAllDomainStatsCheckACL(conn, dom->def)) {
+            virObjectUnlock(dom);
+            dom = NULL;
             continue;
+        }

-        if (HAVE_JOB(domflags) &&
+        if (HAVE_JOB(privflags) &&
             qemuDomainObjBeginJob(driver, dom, QEMU_JOB_QUERY) < 0)
             /* As it was never requested. Gather as much as possible anyway. */
-            domflags &= ~QEMU_DOMAIN_STATS_HAVE_JOB;
+            domflags |= QEMU_DOMAIN_STATS_HAVE_JOB;

         if (qemuDomainGetStats(conn, dom, stats, &tmp, domflags) < 0)
             goto endjob;
@@ -18761,9 +18764,12 @@ qemuConnectGetAllDomainStats(virConnectPtr conn,
         if (tmp)
             tmpstats[nstats++] = tmp;

-        if (HAVE_JOB(domflags) && !qemuDomainObjEndJob(driver, dom)) {
-            dom = NULL;
-            continue;
+        if (HAVE_JOB(domflags)) {
+            domflags = 0;
+            if (!qemuDomainObjEndJob(driver, dom)) {
+                dom = NULL;
+                continue;
+            }
         }

         virObjectUnlock(dom);
--
2.2.0