qemu: remove qemuDomainObjBegin/EndJobWithAgent()

This function potentially grabs both a monitor job and an agent job at
the same time. This is problematic because it means that a malicious (or
just buggy) guest agent can cause a denial of service on the host. The
presence of this function makes it easy to do the wrong thing and hold
both jobs at the same time. All existing uses have already been removed
by previous commits.

Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
This commit is contained in:
Jonathon Jongsma 2020-01-10 17:32:14 -06:00 committed by Michal Privoznik
parent 599ae372d8
commit 3c436c22a4
3 changed files with 11 additions and 110 deletions

View File

@ -61,11 +61,12 @@ There are a number of locks on various objects
Agent job condition is then used when thread wishes to talk to qemu Agent job condition is then used when thread wishes to talk to qemu
agent monitor. It is possible to acquire just agent job agent monitor. It is possible to acquire just agent job
(qemuDomainObjBeginAgentJob), or only normal job (qemuDomainObjBeginAgentJob), or only normal job (qemuDomainObjBeginJob)
(qemuDomainObjBeginJob) or both at the same time but not both at the same time. Holding an agent job and a normal job would
(qemuDomainObjBeginJobWithAgent). Which type of job to grab depends allow an unresponsive or malicious agent to block normal libvirt API and
whether caller wishes to communicate only with agent socket, or only potentially result in a denial of service. Which type of job to grab
with qemu monitor socket or both, respectively. depends whether caller wishes to communicate only with agent socket, or
only with qemu monitor socket.
Immediately after acquiring the virDomainObjPtr lock, any method Immediately after acquiring the virDomainObjPtr lock, any method
which intends to update state must acquire asynchronous, normal or which intends to update state must acquire asynchronous, normal or
@ -141,18 +142,6 @@ To acquire the agent job condition
To acquire both normal and agent job condition
qemuDomainObjBeginJobWithAgent()
- Waits until there is no normal and no agent job set
- Sets both job.active and job.agentActive with required job types
qemuDomainObjEndJobWithAgent()
- Sets both job.active and job.agentActive to 0
- Signals on job.cond condition
To acquire the asynchronous job condition To acquire the asynchronous job condition
qemuDomainObjBeginAsyncJob() qemuDomainObjBeginAsyncJob()
@ -292,41 +281,6 @@ Design patterns
virDomainObjEndAPI(&obj); virDomainObjEndAPI(&obj);
* Invoking both monitor and agent commands on a virDomainObjPtr
virDomainObjPtr obj;
qemuAgentPtr agent;
obj = qemuDomObjFromDomain(dom);
qemuDomainObjBeginJobWithAgent(obj, QEMU_JOB_TYPE, QEMU_AGENT_JOB_TYPE);
if (!virDomainObjIsActive(dom))
goto cleanup;
...do prep work...
if (!qemuDomainAgentAvailable(obj, true))
goto cleanup;
agent = qemuDomainObjEnterAgent(obj);
qemuAgentXXXX(agent, ..);
qemuDomainObjExitAgent(obj, agent);
...
qemuDomainObjEnterMonitor(obj);
qemuMonitorXXXX(priv->mon);
qemuDomainObjExitMonitor(obj);
/* Alternatively, talk to the monitor first and then talk to the agent. */
...do final work...
qemuDomainObjEndJobWithAgent(obj);
virDomainObjEndAPI(&obj);
* Running asynchronous job * Running asynchronous job
virDomainObjPtr obj; virDomainObjPtr obj;

View File

@ -9640,26 +9640,6 @@ qemuDomainObjBeginAgentJob(virQEMUDriverPtr driver,
QEMU_ASYNC_JOB_NONE, false); QEMU_ASYNC_JOB_NONE, false);
} }
/**
* qemuDomainObjBeginJobWithAgent:
*
* Grabs both monitor and agent types of job. Use if caller talks to
* both monitor and guest agent. However, if @job (or @agentJob) is
* QEMU_JOB_NONE (or QEMU_AGENT_JOB_NONE) only agent job is acquired (or
* monitor job).
*
* To end job call qemuDomainObjEndJobWithAgent.
*/
int
qemuDomainObjBeginJobWithAgent(virQEMUDriverPtr driver,
virDomainObjPtr obj,
qemuDomainJob job,
qemuDomainAgentJob agentJob)
{
return qemuDomainObjBeginJobInternal(driver, obj, job, agentJob,
QEMU_ASYNC_JOB_NONE, false);
}
int qemuDomainObjBeginAsyncJob(virQEMUDriverPtr driver, int qemuDomainObjBeginAsyncJob(virQEMUDriverPtr driver,
virDomainObjPtr obj, virDomainObjPtr obj,
qemuDomainAsyncJob asyncJob, qemuDomainAsyncJob asyncJob,
@ -9774,31 +9754,6 @@ qemuDomainObjEndAgentJob(virDomainObjPtr obj)
virCondBroadcast(&priv->job.cond); virCondBroadcast(&priv->job.cond);
} }
void
qemuDomainObjEndJobWithAgent(virQEMUDriverPtr driver,
virDomainObjPtr obj)
{
qemuDomainObjPrivatePtr priv = obj->privateData;
qemuDomainJob job = priv->job.active;
qemuDomainAgentJob agentJob = priv->job.agentActive;
priv->jobs_queued--;
VIR_DEBUG("Stopping both jobs: %s %s (async=%s vm=%p name=%s)",
qemuDomainJobTypeToString(job),
qemuDomainAgentJobTypeToString(agentJob),
qemuDomainAsyncJobTypeToString(priv->job.asyncJob),
obj, obj->def->name);
qemuDomainObjResetJob(priv);
qemuDomainObjResetAgentJob(priv);
if (qemuDomainTrackJob(job))
qemuDomainObjSaveStatus(driver, obj);
/* We indeed need to wake up ALL threads waiting because
* grabbing a job requires checking more variables. */
virCondBroadcast(&priv->job.cond);
}
void void
qemuDomainObjEndAsyncJob(virQEMUDriverPtr driver, virDomainObjPtr obj) qemuDomainObjEndAsyncJob(virQEMUDriverPtr driver, virDomainObjPtr obj)
{ {
@ -9832,9 +9787,9 @@ qemuDomainObjAbortAsyncJob(virDomainObjPtr obj)
* obj must be locked before calling * obj must be locked before calling
* *
* To be called immediately before any QEMU monitor API call * To be called immediately before any QEMU monitor API call
* Must have already either called qemuDomainObjBeginJob() or * Must have already called qemuDomainObjBeginJob() and checked
* qemuDomainObjBeginJobWithAgent() and checked that the VM is * that the VM is still active; may not be used for nested async
* still active; may not be used for nested async jobs. * jobs.
* *
* To be followed with qemuDomainObjExitMonitor() once complete * To be followed with qemuDomainObjExitMonitor() once complete
*/ */
@ -9956,9 +9911,8 @@ qemuDomainObjEnterMonitorAsync(virQEMUDriverPtr driver,
* obj must be locked before calling * obj must be locked before calling
* *
* To be called immediately before any QEMU agent API call. * To be called immediately before any QEMU agent API call.
* Must have already called qemuDomainObjBeginAgentJob() or * Must have already called qemuDomainObjBeginAgentJob() and
* qemuDomainObjBeginJobWithAgent() and checked that the VM is * checked that the VM is still active.
* still active.
* *
* To be followed with qemuDomainObjExitAgent() once complete * To be followed with qemuDomainObjExitAgent() once complete
*/ */

View File

@ -651,11 +651,6 @@ int qemuDomainObjBeginAgentJob(virQEMUDriverPtr driver,
virDomainObjPtr obj, virDomainObjPtr obj,
qemuDomainAgentJob agentJob) qemuDomainAgentJob agentJob)
G_GNUC_WARN_UNUSED_RESULT; G_GNUC_WARN_UNUSED_RESULT;
int qemuDomainObjBeginJobWithAgent(virQEMUDriverPtr driver,
virDomainObjPtr obj,
qemuDomainJob job,
qemuDomainAgentJob agentJob)
G_GNUC_WARN_UNUSED_RESULT;
int qemuDomainObjBeginAsyncJob(virQEMUDriverPtr driver, int qemuDomainObjBeginAsyncJob(virQEMUDriverPtr driver,
virDomainObjPtr obj, virDomainObjPtr obj,
qemuDomainAsyncJob asyncJob, qemuDomainAsyncJob asyncJob,
@ -674,8 +669,6 @@ int qemuDomainObjBeginJobNowait(virQEMUDriverPtr driver,
void qemuDomainObjEndJob(virQEMUDriverPtr driver, void qemuDomainObjEndJob(virQEMUDriverPtr driver,
virDomainObjPtr obj); virDomainObjPtr obj);
void qemuDomainObjEndAgentJob(virDomainObjPtr obj); void qemuDomainObjEndAgentJob(virDomainObjPtr obj);
void qemuDomainObjEndJobWithAgent(virQEMUDriverPtr driver,
virDomainObjPtr obj);
void qemuDomainObjEndAsyncJob(virQEMUDriverPtr driver, void qemuDomainObjEndAsyncJob(virQEMUDriverPtr driver,
virDomainObjPtr obj); virDomainObjPtr obj);
void qemuDomainObjAbortAsyncJob(virDomainObjPtr obj); void qemuDomainObjAbortAsyncJob(virDomainObjPtr obj);