render / rpms / libvirt

Forked from rpms/libvirt 10 months ago
Clone
d76c62
From bac1c96fbf2bd9d6ef728a813fda793ce1e99267 Mon Sep 17 00:00:00 2001
d76c62
Message-Id: <bac1c96fbf2bd9d6ef728a813fda793ce1e99267@dist-git>
d76c62
From: Jonathon Jongsma <jjongsma@redhat.com>
d76c62
Date: Thu, 20 Feb 2020 10:52:27 -0600
d76c62
Subject: [PATCH] qemu: remove qemuDomainObjBegin/EndJobWithAgent()
d76c62
MIME-Version: 1.0
d76c62
Content-Type: text/plain; charset=UTF-8
d76c62
Content-Transfer-Encoding: 8bit
d76c62
d76c62
This function potentially grabs both a monitor job and an agent job at
d76c62
the same time. This is problematic because it means that a malicious (or
d76c62
just buggy) guest agent can cause a denial of service on the host. The
d76c62
presence of this function makes it easy to do the wrong thing and hold
d76c62
both jobs at the same time. All existing uses have already been removed
d76c62
by previous commits.
d76c62
d76c62
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
d76c62
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
d76c62
(cherry picked from commit 3c436c22a4f94c85c2b5e7b5fb84af48219d78e3)
d76c62
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
d76c62
https://bugzilla.redhat.com/show_bug.cgi?id=1759566
d76c62
Message-Id: <20200220165227.11491-6-jjongsma@redhat.com>
d76c62
Reviewed-by: Ján Tomko <jtomko@redhat.com>
d76c62
---
d76c62
 src/qemu/THREADS.txt   | 58 +++++-------------------------------------
d76c62
 src/qemu/qemu_domain.c | 56 ++++------------------------------------
d76c62
 src/qemu/qemu_domain.h |  7 -----
d76c62
 3 files changed, 11 insertions(+), 110 deletions(-)
d76c62
d76c62
diff --git a/src/qemu/THREADS.txt b/src/qemu/THREADS.txt
d76c62
index aa428fda6a..a7d8709a43 100644
d76c62
--- a/src/qemu/THREADS.txt
d76c62
+++ b/src/qemu/THREADS.txt
d76c62
@@ -61,11 +61,12 @@ There are a number of locks on various objects
d76c62
 
d76c62
     Agent job condition is then used when thread wishes to talk to qemu
d76c62
     agent monitor. It is possible to acquire just agent job
d76c62
-    (qemuDomainObjBeginAgentJob), or only normal job
d76c62
-    (qemuDomainObjBeginJob) or both at the same time
d76c62
-    (qemuDomainObjBeginJobWithAgent). Which type of job to grab depends
d76c62
-    whether caller wishes to communicate only with agent socket, or only
d76c62
-    with qemu monitor socket or both, respectively.
d76c62
+    (qemuDomainObjBeginAgentJob), or only normal job (qemuDomainObjBeginJob)
d76c62
+    but not both at the same time. Holding an agent job and a normal job would
d76c62
+    allow an unresponsive or malicious agent to block normal libvirt API and
d76c62
+    potentially result in a denial of service. Which type of job to grab
d76c62
+    depends whether caller wishes to communicate only with agent socket, or
d76c62
+    only with qemu monitor socket.
d76c62
 
d76c62
     Immediately after acquiring the virDomainObjPtr lock, any method
d76c62
     which intends to update state must acquire asynchronous, normal or
d76c62
@@ -141,18 +142,6 @@ To acquire the agent job condition
d76c62
 
d76c62
 
d76c62
 
d76c62
-To acquire both normal and agent job condition
d76c62
-
d76c62
-  qemuDomainObjBeginJobWithAgent()
d76c62
-    - Waits until there is no normal and no agent job set
d76c62
-    - Sets both job.active and job.agentActive with required job types
d76c62
-
d76c62
-  qemuDomainObjEndJobWithAgent()
d76c62
-    - Sets both job.active and job.agentActive to 0
d76c62
-    - Signals on job.cond condition
d76c62
-
d76c62
-
d76c62
-
d76c62
 To acquire the asynchronous job condition
d76c62
 
d76c62
   qemuDomainObjBeginAsyncJob()
d76c62
@@ -292,41 +281,6 @@ Design patterns
d76c62
      virDomainObjEndAPI(&obj);
d76c62
 
d76c62
 
d76c62
- * Invoking both monitor and agent commands on a virDomainObjPtr
d76c62
-
d76c62
-     virDomainObjPtr obj;
d76c62
-     qemuAgentPtr agent;
d76c62
-
d76c62
-     obj = qemuDomObjFromDomain(dom);
d76c62
-
d76c62
-     qemuDomainObjBeginJobWithAgent(obj, QEMU_JOB_TYPE, QEMU_AGENT_JOB_TYPE);
d76c62
-
d76c62
-     if (!virDomainObjIsActive(dom))
d76c62
-         goto cleanup;
d76c62
-
d76c62
-     ...do prep work...
d76c62
-
d76c62
-     if (!qemuDomainAgentAvailable(obj, true))
d76c62
-         goto cleanup;
d76c62
-
d76c62
-     agent = qemuDomainObjEnterAgent(obj);
d76c62
-     qemuAgentXXXX(agent, ..);
d76c62
-     qemuDomainObjExitAgent(obj, agent);
d76c62
-
d76c62
-     ...
d76c62
-
d76c62
-     qemuDomainObjEnterMonitor(obj);
d76c62
-     qemuMonitorXXXX(priv->mon);
d76c62
-     qemuDomainObjExitMonitor(obj);
d76c62
-
d76c62
-     /* Alternatively, talk to the monitor first and then talk to the agent. */
d76c62
-
d76c62
-     ...do final work...
d76c62
-
d76c62
-     qemuDomainObjEndJobWithAgent(obj);
d76c62
-     virDomainObjEndAPI(&obj);
d76c62
-
d76c62
-
d76c62
  * Running asynchronous job
d76c62
 
d76c62
      virDomainObjPtr obj;
d76c62
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
d76c62
index 0baa80582c..f037f0812e 100644
d76c62
--- a/src/qemu/qemu_domain.c
d76c62
+++ b/src/qemu/qemu_domain.c
d76c62
@@ -9734,26 +9734,6 @@ qemuDomainObjBeginAgentJob(virQEMUDriverPtr driver,
d76c62
                                          QEMU_ASYNC_JOB_NONE, false);
d76c62
 }
d76c62
 
d76c62
-/**
d76c62
- * qemuDomainObjBeginJobWithAgent:
d76c62
- *
d76c62
- * Grabs both monitor and agent types of job. Use if caller talks to
d76c62
- * both monitor and guest agent. However, if @job (or @agentJob) is
d76c62
- * QEMU_JOB_NONE (or QEMU_AGENT_JOB_NONE) only agent job is acquired (or
d76c62
- * monitor job).
d76c62
- *
d76c62
- * To end job call qemuDomainObjEndJobWithAgent.
d76c62
- */
d76c62
-int
d76c62
-qemuDomainObjBeginJobWithAgent(virQEMUDriverPtr driver,
d76c62
-                               virDomainObjPtr obj,
d76c62
-                               qemuDomainJob job,
d76c62
-                               qemuDomainAgentJob agentJob)
d76c62
-{
d76c62
-    return qemuDomainObjBeginJobInternal(driver, obj, job, agentJob,
d76c62
-                                         QEMU_ASYNC_JOB_NONE, false);
d76c62
-}
d76c62
-
d76c62
 int qemuDomainObjBeginAsyncJob(virQEMUDriverPtr driver,
d76c62
                                virDomainObjPtr obj,
d76c62
                                qemuDomainAsyncJob asyncJob,
d76c62
@@ -9868,31 +9848,6 @@ qemuDomainObjEndAgentJob(virDomainObjPtr obj)
d76c62
     virCondBroadcast(&priv->job.cond);
d76c62
 }
d76c62
 
d76c62
-void
d76c62
-qemuDomainObjEndJobWithAgent(virQEMUDriverPtr driver,
d76c62
-                             virDomainObjPtr obj)
d76c62
-{
d76c62
-    qemuDomainObjPrivatePtr priv = obj->privateData;
d76c62
-    qemuDomainJob job = priv->job.active;
d76c62
-    qemuDomainAgentJob agentJob = priv->job.agentActive;
d76c62
-
d76c62
-    priv->jobs_queued--;
d76c62
-
d76c62
-    VIR_DEBUG("Stopping both jobs: %s %s (async=%s vm=%p name=%s)",
d76c62
-              qemuDomainJobTypeToString(job),
d76c62
-              qemuDomainAgentJobTypeToString(agentJob),
d76c62
-              qemuDomainAsyncJobTypeToString(priv->job.asyncJob),
d76c62
-              obj, obj->def->name);
d76c62
-
d76c62
-    qemuDomainObjResetJob(priv);
d76c62
-    qemuDomainObjResetAgentJob(priv);
d76c62
-    if (qemuDomainTrackJob(job))
d76c62
-        qemuDomainObjSaveStatus(driver, obj);
d76c62
-    /* We indeed need to wake up ALL threads waiting because
d76c62
-     * grabbing a job requires checking more variables. */
d76c62
-    virCondBroadcast(&priv->job.cond);
d76c62
-}
d76c62
-
d76c62
 void
d76c62
 qemuDomainObjEndAsyncJob(virQEMUDriverPtr driver, virDomainObjPtr obj)
d76c62
 {
d76c62
@@ -9926,9 +9881,9 @@ qemuDomainObjAbortAsyncJob(virDomainObjPtr obj)
d76c62
  * obj must be locked before calling
d76c62
  *
d76c62
  * To be called immediately before any QEMU monitor API call
d76c62
- * Must have already either called qemuDomainObjBeginJob() or
d76c62
- * qemuDomainObjBeginJobWithAgent() and checked that the VM is
d76c62
- * still active; may not be used for nested async jobs.
d76c62
+ * Must have already called qemuDomainObjBeginJob() and checked
d76c62
+ * that the VM is still active; may not be used for nested async
d76c62
+ * jobs.
d76c62
  *
d76c62
  * To be followed with qemuDomainObjExitMonitor() once complete
d76c62
  */
d76c62
@@ -10050,9 +10005,8 @@ qemuDomainObjEnterMonitorAsync(virQEMUDriverPtr driver,
d76c62
  * obj must be locked before calling
d76c62
  *
d76c62
  * To be called immediately before any QEMU agent API call.
d76c62
- * Must have already called qemuDomainObjBeginAgentJob() or
d76c62
- * qemuDomainObjBeginJobWithAgent() and checked that the VM is
d76c62
- * still active.
d76c62
+ * Must have already called qemuDomainObjBeginAgentJob() and
d76c62
+ * checked that the VM is still active.
d76c62
  *
d76c62
  * To be followed with qemuDomainObjExitAgent() once complete
d76c62
  */
d76c62
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
d76c62
index cdcb6ecc7a..c581b3a162 100644
d76c62
--- a/src/qemu/qemu_domain.h
d76c62
+++ b/src/qemu/qemu_domain.h
d76c62
@@ -651,11 +651,6 @@ int qemuDomainObjBeginAgentJob(virQEMUDriverPtr driver,
d76c62
                                virDomainObjPtr obj,
d76c62
                                qemuDomainAgentJob agentJob)
d76c62
     G_GNUC_WARN_UNUSED_RESULT;
d76c62
-int qemuDomainObjBeginJobWithAgent(virQEMUDriverPtr driver,
d76c62
-                                   virDomainObjPtr obj,
d76c62
-                                   qemuDomainJob job,
d76c62
-                                   qemuDomainAgentJob agentJob)
d76c62
-    G_GNUC_WARN_UNUSED_RESULT;
d76c62
 int qemuDomainObjBeginAsyncJob(virQEMUDriverPtr driver,
d76c62
                                virDomainObjPtr obj,
d76c62
                                qemuDomainAsyncJob asyncJob,
d76c62
@@ -674,8 +669,6 @@ int qemuDomainObjBeginJobNowait(virQEMUDriverPtr driver,
d76c62
 void qemuDomainObjEndJob(virQEMUDriverPtr driver,
d76c62
                          virDomainObjPtr obj);
d76c62
 void qemuDomainObjEndAgentJob(virDomainObjPtr obj);
d76c62
-void qemuDomainObjEndJobWithAgent(virQEMUDriverPtr driver,
d76c62
-                                  virDomainObjPtr obj);
d76c62
 void qemuDomainObjEndAsyncJob(virQEMUDriverPtr driver,
d76c62
                               virDomainObjPtr obj);
d76c62
 void qemuDomainObjAbortAsyncJob(virDomainObjPtr obj);
d76c62
-- 
d76c62
2.25.0
d76c62