From b54ac47536056f26c724c7997e2aa96009b79ebd Mon Sep 17 00:00:00 2001
Message-Id: <b54ac47536056f26c724c7997e2aa96009b79ebd@dist-git>
From: Jonathon Jongsma <jjongsma@redhat.com>
Date: Fri, 1 May 2020 16:53:38 -0500
Subject: [PATCH] qemu: don't hold a monitor and agent job for reboot

We have to assume that the guest agent may be malicious so we don't want
to allow any agent queries to block any other libvirt API. By holding
a monitor job while we're querying the agent, we open ourselves up to a
DoS.

Split the function so that we only hold the appropriate type of job
while rebooting.

Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
(cherry picked from commit 0a9893121187c0c3f9807e9164366e1f6977619c)

CVE-2019-20485

Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Message-Id: <20200501215341.27683-3-jjongsma@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/qemu/qemu_driver.c | 112 +++++++++++++++++++++++++----------------
 1 file changed, 70 insertions(+), 42 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index bdc955e1fa..888c586a79 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2049,6 +2049,72 @@ static int qemuDomainShutdown(virDomainPtr dom)
 }
 
 
+static int
+qemuDomainRebootAgent(virQEMUDriverPtr driver,
+                      virDomainObjPtr vm,
+                      bool isReboot,
+                      bool agentForced)
+{
+    qemuAgentPtr agent;
+    int ret = -1;
+    int agentFlag = QEMU_AGENT_SHUTDOWN_REBOOT;
+
+    if (!isReboot)
+        agentFlag = QEMU_AGENT_SHUTDOWN_POWERDOWN;
+
+    if (qemuDomainObjBeginAgentJob(driver, vm,
+                                   QEMU_AGENT_JOB_MODIFY) < 0)
+        return -1;
+
+    if (!qemuDomainAgentAvailable(vm, agentForced))
+        goto endjob;
+
+    if (virDomainObjCheckActive(vm) < 0)
+        goto endjob;
+
+    qemuDomainSetFakeReboot(driver, vm, false);
+    agent = qemuDomainObjEnterAgent(vm);
+    ret = qemuAgentShutdown(agent, agentFlag);
+    qemuDomainObjExitAgent(vm, agent);
+
+ endjob:
+    qemuDomainObjEndAgentJob(vm);
+    return ret;
+}
+
+
+static int
+qemuDomainRebootMonitor(virQEMUDriverPtr driver,
+                        virDomainObjPtr vm,
+                        bool isReboot)
+{
+    qemuDomainObjPrivatePtr priv = vm->privateData;
+    int ret = -1;
+
+    if (qemuDomainObjBeginJob(driver, vm,
+                              QEMU_JOB_MODIFY) < 0)
+        return -1;
+
+    if (virDomainObjCheckActive(vm) < 0)
+        goto endjob;
+
+#if !WITH_YAJL
+    virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+                   _("ACPI reboot is not supported without the JSON monitor"));
+    goto endjob;
+#endif
+    qemuDomainSetFakeReboot(driver, vm, isReboot);
+    qemuDomainObjEnterMonitor(driver, vm);
+    ret = qemuMonitorSystemPowerdown(priv->mon);
+    if (qemuDomainObjExitMonitor(driver, vm) < 0)
+        ret = -1;
+
+ endjob:
+    qemuDomainObjEndJob(driver, vm);
+    return ret;
+}
+
+
 static int
 qemuDomainReboot(virDomainPtr dom, unsigned int flags)
 {
@@ -2059,8 +2125,6 @@ qemuDomainReboot(virDomainPtr dom, unsigned int flags)
     bool useAgent = false, agentRequested, acpiRequested;
     bool isReboot = true;
     bool agentForced;
-    qemuDomainAgentJob agentJob = QEMU_AGENT_JOB_NONE;
-    int agentFlag = QEMU_AGENT_SHUTDOWN_REBOOT;
 
     virCheckFlags(VIR_DOMAIN_REBOOT_ACPI_POWER_BTN |
                   VIR_DOMAIN_REBOOT_GUEST_AGENT, -1);
@@ -2070,7 +2134,6 @@ qemuDomainReboot(virDomainPtr dom, unsigned int flags)
 
     if (vm->def->onReboot == VIR_DOMAIN_LIFECYCLE_ACTION_DESTROY ||
         vm->def->onReboot == VIR_DOMAIN_LIFECYCLE_ACTION_PRESERVE) {
-        agentFlag = QEMU_AGENT_SHUTDOWN_POWERDOWN;
         isReboot = false;
         VIR_INFO("Domain on_reboot setting overridden, shutting down");
     }
@@ -2086,56 +2149,21 @@ qemuDomainReboot(virDomainPtr dom, unsigned int flags)
     if (virDomainRebootEnsureACL(dom->conn, vm->def, flags) < 0)
         goto cleanup;
 
+    agentForced = agentRequested && !acpiRequested;
     if (useAgent)
-        agentJob = QEMU_AGENT_JOB_MODIFY;
+        ret = qemuDomainRebootAgent(driver, vm, isReboot, agentForced);
 
-    if (qemuDomainObjBeginJobWithAgent(driver, vm,
-                                       QEMU_JOB_MODIFY,
-                                       agentJob) < 0)
+    if (ret < 0 && agentForced)
         goto cleanup;
 
-    agentForced = agentRequested && !acpiRequested;
-    if (!qemuDomainAgentAvailable(vm, agentForced)) {
-        if (agentForced)
-            goto endjob;
-        useAgent = false;
-    }
-
-    if (virDomainObjCheckActive(vm) < 0)
-        goto endjob;
-
-    if (useAgent) {
-        qemuAgentPtr agent;
-
-        qemuDomainSetFakeReboot(driver, vm, false);
-        agent = qemuDomainObjEnterAgent(vm);
-        ret = qemuAgentShutdown(agent, agentFlag);
-        qemuDomainObjExitAgent(vm, agent);
-    }
-
     /* If we are not enforced to use just an agent, try ACPI
      * shutdown as well in case agent did not succeed.
      */
     if ((!useAgent) ||
         (ret < 0 && (acpiRequested || !flags))) {
-#if !WITH_YAJL
-        virReportError(VIR_ERR_OPERATION_INVALID, "%s",
-                       _("ACPI reboot is not supported without the JSON monitor"));
-        goto endjob;
-#endif
-        qemuDomainSetFakeReboot(driver, vm, isReboot);
-        qemuDomainObjEnterMonitor(driver, vm);
-        ret = qemuMonitorSystemPowerdown(priv->mon);
-        if (qemuDomainObjExitMonitor(driver, vm) < 0)
-            ret = -1;
+        ret = qemuDomainRebootMonitor(driver, vm, isReboot);
     }
 
- endjob:
-    if (agentJob)
-        qemuDomainObjEndJobWithAgent(driver, vm);
-    else
-        qemuDomainObjEndJob(driver, vm);
-
  cleanup:
     virDomainObjEndAPI(&vm);
     return ret;
-- 
2.26.2