From f2110c4d26ec4cef5384edbdd23a31d5691cb924 Mon Sep 17 00:00:00 2001
From: Xu Yandong <xuyandong2@huawei.com>
Date: Sat, 9 May 2020 14:54:30 +0800
Subject: [PATCH] qemu: don't hold both jobs for suspend

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.

So split the function up a bit to only hold the monitor job while
querying qemu for whether the domain supports suspend. Then acquire only
an agent job while issuing the agent suspend command.

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 7317a83f6024dd6b4e1fa19436e24db0132b6cb9)
Signed-off-by: Xu Yandong <xuyandong2@huawei.com>
---
 src/qemu/qemu_driver.c | 94 ++++++++++++++++++++++++++----------------
 1 file changed, 59 insertions(+), 35 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 8749c53..dad0710 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -19088,6 +19088,59 @@ qemuDomainProbeQMPCurrentMachine(virQEMUDriverPtr driver,
 }
 
 
+/* returns -1 on error, or if query is not supported, 0 if query was successful */
+static int
+qemuDomainQueryWakeupSuspendSupport(virQEMUDriverPtr driver,
+                                    virDomainObjPtr vm,
+                                    bool *wakeupSupported)
+{
+    qemuDomainObjPrivatePtr priv = vm->privateData;
+    int ret = -1;
+
+    if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_QUERY_CURRENT_MACHINE))
+        return -1;
+
+    if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
+        return -1;
+
+    if ((ret = virDomainObjCheckActive(vm)) < 0)
+        goto endjob;
+
+    ret = qemuDomainProbeQMPCurrentMachine(driver, vm, wakeupSupported);
+
+ endjob:
+    qemuDomainObjEndJob(driver, vm);
+    return ret;
+}
+
+
+static int
+qemuDomainPMSuspendAgent(virQEMUDriverPtr driver,
+                         virDomainObjPtr vm,
+                         unsigned int target)
+{
+    qemuAgentPtr agent;
+    int ret = -1;
+
+    if (qemuDomainObjBeginAgentJob(driver, vm, QEMU_AGENT_JOB_MODIFY) < 0)
+        return -1;
+
+    if ((ret = virDomainObjCheckActive(vm)) < 0)
+        goto endjob;
+
+    if (!qemuDomainAgentAvailable(vm, true))
+        goto endjob;
+
+    agent = qemuDomainObjEnterAgent(vm);
+    ret = qemuAgentSuspend(agent, target);
+    qemuDomainObjExitAgent(vm, agent);
+
+ endjob:
+    qemuDomainObjEndAgentJob(vm);
+    return ret;
+}
+
+
 static int
 qemuDomainPMSuspendForDuration(virDomainPtr dom,
                                unsigned int target,
@@ -19095,11 +19148,9 @@ qemuDomainPMSuspendForDuration(virDomainPtr dom,
                                unsigned int flags)
 {
     virQEMUDriverPtr driver = dom->conn->privateData;
-    qemuDomainObjPrivatePtr priv;
     virDomainObjPtr vm;
-    qemuAgentPtr agent;
-    qemuDomainJob job = QEMU_JOB_NONE;
     int ret = -1;
+    bool wakeupSupported;
 
     virCheckFlags(0, -1);
 
@@ -19124,17 +19175,6 @@ qemuDomainPMSuspendForDuration(virDomainPtr dom,
     if (virDomainPMSuspendForDurationEnsureACL(dom->conn, vm->def) < 0)
         goto cleanup;
 
-    priv = vm->privateData;
-
-    if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_QUERY_CURRENT_MACHINE))
-        job = QEMU_JOB_MODIFY;
-
-    if (qemuDomainObjBeginJobWithAgent(driver, vm, job, QEMU_AGENT_JOB_MODIFY) < 0)
-        goto cleanup;
-
-    if (virDomainObjCheckActive(vm) < 0)
-        goto endjob;
-
     /*
      * The case we want to handle here is when QEMU has the API (i.e.
      * QEMU_CAPS_QUERY_CURRENT_MACHINE is set). Otherwise, do not interfere
@@ -19142,16 +19182,11 @@ qemuDomainPMSuspendForDuration(virDomainPtr dom,
      * that don't know about this cap, will keep their old behavior of
      * suspending 'in the dark'.
      */
-    if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_QUERY_CURRENT_MACHINE)) {
-        bool wakeupSupported;
-
-        if (qemuDomainProbeQMPCurrentMachine(driver, vm, &wakeupSupported) < 0)
-            goto endjob;
-
+    if (qemuDomainQueryWakeupSuspendSupport(driver, vm, &wakeupSupported) == 0) {
         if (!wakeupSupported) {
             virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
                            _("Domain does not have suspend support"));
-            goto endjob;
+            goto cleanup;
         }
     }
 
@@ -19161,29 +19196,18 @@ qemuDomainPMSuspendForDuration(virDomainPtr dom,
              target == VIR_NODE_SUSPEND_TARGET_HYBRID)) {
             virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                            _("S3 state is disabled for this domain"));
-            goto endjob;
+            goto cleanup;
         }
 
         if (vm->def->pm.s4 == VIR_TRISTATE_BOOL_NO &&
             target == VIR_NODE_SUSPEND_TARGET_DISK) {
             virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                            _("S4 state is disabled for this domain"));
-            goto endjob;
+            goto cleanup;
         }
     }
 
-    if (!qemuDomainAgentAvailable(vm, true))
-        goto endjob;
-
-    agent = qemuDomainObjEnterAgent(vm);
-    ret = qemuAgentSuspend(agent, target);
-    qemuDomainObjExitAgent(vm, agent);
-
- endjob:
-    if (job)
-        qemuDomainObjEndJobWithAgent(driver, vm);
-    else
-        qemuDomainObjEndAgentJob(vm);
+    ret = qemuDomainPMSuspendAgent(driver, vm, target);
 
  cleanup:
     virDomainObjEndAPI(&vm);
-- 
2.21.0

