[libvirt] [PATCH 2/2] qemu: agent: fix unsafe agent access
Nikolay Shirokovskiy
nshirokovskiy at virtuozzo.com
Mon Nov 14 14:24:23 UTC 2016
qemuDomainObjExitAgent is unsafe.
First it accesses domain object without domain lock.
Second it uses outdated logic that goes back to commit 79533da1 of
year 2009 when code was quite different. (unref function
instead of unreferencing only unlocked and disposed object
in case of last reference and leaved unlocking to the caller otherwise).
Nowadays this logic may lead to disposing locked object
i guess.
Another problem is that the callers of qemuDomainObjEnterAgent
use domain object again (namely priv->agent) without domain lock.
This patch address these two problems.
qemuDomainGetAgent is dropped as unused.
---
I wonder should we make qemuDomainObj(Enter|Exit)Agent be macros. So
we don't need to declare a variable to hold agent reference and zero
it out at the end for safety.
Also the qemu monitor code has probably same problems as the agent monitor code
seems to be copied from there.
src/qemu/qemu_domain.c | 41 ++++---------
src/qemu/qemu_domain.h | 7 +--
src/qemu/qemu_driver.c | 153 ++++++++++++++++++++++++-------------------------
3 files changed, 91 insertions(+), 110 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index f121152..e272699 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3626,19 +3626,6 @@ qemuDomainObjEnterMonitorAsync(virQEMUDriverPtr driver,
}
-/**
- * qemuDomainGetAgent:
- * @vm: domain object
- *
- * Returns the agent pointer of @vm;
- */
-qemuAgentPtr
-qemuDomainGetAgent(virDomainObjPtr vm)
-{
- return (((qemuDomainObjPrivatePtr)(vm->privateData))->agent);
-}
-
-
/*
* obj must be locked before calling
*
@@ -3648,16 +3635,20 @@ qemuDomainGetAgent(virDomainObjPtr vm)
*
* To be followed with qemuDomainObjExitAgent() once complete
*/
-void
+qemuAgentPtr
qemuDomainObjEnterAgent(virDomainObjPtr obj)
{
qemuDomainObjPrivatePtr priv = obj->privateData;
+ qemuAgentPtr agent = priv->agent;
VIR_DEBUG("Entering agent (agent=%p vm=%p name=%s)",
priv->agent, obj, obj->def->name);
- virObjectLock(priv->agent);
- virObjectRef(priv->agent);
+
+ virObjectLock(agent);
+ virObjectRef(agent);
virObjectUnlock(obj);
+
+ return agent;
}
@@ -3666,22 +3657,14 @@ qemuDomainObjEnterAgent(virDomainObjPtr obj)
* Should be paired with an earlier qemuDomainObjEnterAgent() call
*/
void
-qemuDomainObjExitAgent(virDomainObjPtr obj)
+qemuDomainObjExitAgent(virDomainObjPtr obj, qemuAgentPtr agent)
{
- qemuDomainObjPrivatePtr priv = obj->privateData;
- bool hasRefs;
-
- hasRefs = virObjectUnref(priv->agent);
-
- if (hasRefs)
- virObjectUnlock(priv->agent);
-
+ virObjectUnlock(agent);
+ virObjectUnref(agent);
virObjectLock(obj);
+
VIR_DEBUG("Exited agent (agent=%p vm=%p name=%s)",
- priv->agent, obj, obj->def->name);
-
- if (!hasRefs)
- priv->agent = NULL;
+ agent, obj, obj->def->name);
}
void qemuDomainObjEnterRemote(virDomainObjPtr obj)
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 8d3688c..e675bb2 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -445,11 +445,10 @@ int qemuDomainObjEnterMonitorAsync(virQEMUDriverPtr driver,
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
-qemuAgentPtr qemuDomainGetAgent(virDomainObjPtr vm);
-void qemuDomainObjEnterAgent(virDomainObjPtr obj)
- ATTRIBUTE_NONNULL(1);
-void qemuDomainObjExitAgent(virDomainObjPtr obj)
+qemuAgentPtr qemuDomainObjEnterAgent(virDomainObjPtr obj)
ATTRIBUTE_NONNULL(1);
+void qemuDomainObjExitAgent(virDomainObjPtr obj, qemuAgentPtr agent)
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
void qemuDomainObjEnterRemote(virDomainObjPtr obj)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 38c8414..20c419c 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2013,10 +2013,11 @@ static int qemuDomainShutdownFlags(virDomainPtr dom, unsigned int flags)
if (useAgent) {
+ qemuAgentPtr agent;
qemuDomainSetFakeReboot(driver, vm, false);
- qemuDomainObjEnterAgent(vm);
- ret = qemuAgentShutdown(priv->agent, agentFlag);
- qemuDomainObjExitAgent(vm);
+ agent = qemuDomainObjEnterAgent(vm);
+ ret = qemuAgentShutdown(agent, agentFlag);
+ qemuDomainObjExitAgent(vm, agent);
}
/* If we are not enforced to use just an agent, try ACPI
@@ -2106,10 +2107,12 @@ qemuDomainReboot(virDomainPtr dom, unsigned int flags)
}
if (useAgent) {
+ qemuAgentPtr agent;
+
qemuDomainSetFakeReboot(driver, vm, false);
- qemuDomainObjEnterAgent(vm);
- ret = qemuAgentShutdown(priv->agent, agentFlag);
- qemuDomainObjExitAgent(vm);
+ agent = qemuDomainObjEnterAgent(vm);
+ ret = qemuAgentShutdown(agent, agentFlag);
+ qemuDomainObjExitAgent(vm, agent);
}
/* If we are not enforced to use just an agent, try ACPI
@@ -4685,6 +4688,7 @@ qemuDomainSetVcpusAgent(virDomainObjPtr vm,
unsigned int nvcpus)
{
qemuAgentCPUInfoPtr cpuinfo = NULL;
+ qemuAgentPtr agent;
int ncpuinfo;
int ret = -1;
@@ -4699,9 +4703,10 @@ qemuDomainSetVcpusAgent(virDomainObjPtr vm,
goto cleanup;
}
- qemuDomainObjEnterAgent(vm);
- ncpuinfo = qemuAgentGetVCPUs(qemuDomainGetAgent(vm), &cpuinfo);
- qemuDomainObjExitAgent(vm);
+ agent = qemuDomainObjEnterAgent(vm);
+ ncpuinfo = qemuAgentGetVCPUs(agent, &cpuinfo);
+ qemuDomainObjExitAgent(vm, agent);
+ agent = NULL;
if (ncpuinfo < 0)
goto cleanup;
@@ -4712,9 +4717,9 @@ qemuDomainSetVcpusAgent(virDomainObjPtr vm,
if (!qemuDomainAgentAvailable(vm, true))
goto cleanup;
- qemuDomainObjEnterAgent(vm);
- ret = qemuAgentSetVCPUs(qemuDomainGetAgent(vm), cpuinfo, ncpuinfo);
- qemuDomainObjExitAgent(vm);
+ agent = qemuDomainObjEnterAgent(vm);
+ ret = qemuAgentSetVCPUs(agent, cpuinfo, ncpuinfo);
+ qemuDomainObjExitAgent(vm, agent);
cleanup:
VIR_FREE(cpuinfo);
@@ -5487,11 +5492,11 @@ static int
qemuDomainGetVcpusFlags(virDomainPtr dom, unsigned int flags)
{
virQEMUDriverPtr driver = dom->conn->privateData;
- qemuDomainObjPrivatePtr priv;
virDomainObjPtr vm;
virDomainDefPtr def;
int ret = -1;
qemuAgentCPUInfoPtr cpuinfo = NULL;
+ qemuAgentPtr agent;
int ncpuinfo = -1;
size_t i;
@@ -5503,8 +5508,6 @@ qemuDomainGetVcpusFlags(virDomainPtr dom, unsigned int flags)
if (!(vm = qemuDomObjFromDomain(dom)))
return -1;
- priv = vm->privateData;
-
if (virDomainGetVcpusFlagsEnsureACL(dom->conn, vm->def, flags) < 0)
goto cleanup;
@@ -5525,9 +5528,9 @@ qemuDomainGetVcpusFlags(virDomainPtr dom, unsigned int flags)
if (!qemuDomainAgentAvailable(vm, true))
goto endjob;
- qemuDomainObjEnterAgent(vm);
- ncpuinfo = qemuAgentGetVCPUs(priv->agent, &cpuinfo);
- qemuDomainObjExitAgent(vm);
+ agent = qemuDomainObjEnterAgent(vm);
+ ncpuinfo = qemuAgentGetVCPUs(agent, &cpuinfo);
+ qemuDomainObjExitAgent(vm, agent);
endjob:
qemuDomainObjEndJob(driver, vm);
@@ -13461,15 +13464,15 @@ qemuDomainSnapshotFSFreeze(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
const char **mountpoints,
unsigned int nmountpoints)
{
- qemuDomainObjPrivatePtr priv = vm->privateData;
+ qemuAgentPtr agent;
int frozen;
if (!qemuDomainAgentAvailable(vm, true))
return -1;
- qemuDomainObjEnterAgent(vm);
- frozen = qemuAgentFSFreeze(priv->agent, mountpoints, nmountpoints);
- qemuDomainObjExitAgent(vm);
+ agent = qemuDomainObjEnterAgent(vm);
+ frozen = qemuAgentFSFreeze(agent, mountpoints, nmountpoints);
+ qemuDomainObjExitAgent(vm, agent);
return frozen < 0 ? -2 : frozen;
}
@@ -13480,20 +13483,20 @@ qemuDomainSnapshotFSThaw(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
virDomainObjPtr vm,
bool report)
{
- qemuDomainObjPrivatePtr priv = vm->privateData;
+ qemuAgentPtr agent;
int thawed;
virErrorPtr err = NULL;
if (!qemuDomainAgentAvailable(vm, report))
return -1;
- qemuDomainObjEnterAgent(vm);
+ agent = qemuDomainObjEnterAgent(vm);
if (!report)
err = virSaveLastError();
- thawed = qemuAgentFSThaw(priv->agent);
+ thawed = qemuAgentFSThaw(agent);
if (!report)
virSetError(err);
- qemuDomainObjExitAgent(vm);
+ qemuDomainObjExitAgent(vm, agent);
virFreeError(err);
@@ -18044,6 +18047,7 @@ qemuDomainPMSuspendForDuration(virDomainPtr dom,
virQEMUDriverPtr driver = dom->conn->privateData;
qemuDomainObjPrivatePtr priv;
virDomainObjPtr vm;
+ qemuAgentPtr agent;
int ret = -1;
virCheckFlags(0, -1);
@@ -18109,9 +18113,9 @@ qemuDomainPMSuspendForDuration(virDomainPtr dom,
if (!qemuDomainAgentAvailable(vm, true))
goto endjob;
- qemuDomainObjEnterAgent(vm);
- ret = qemuAgentSuspend(priv->agent, target);
- qemuDomainObjExitAgent(vm);
+ agent = qemuDomainObjEnterAgent(vm);
+ ret = qemuAgentSuspend(agent, target);
+ qemuDomainObjExitAgent(vm, agent);
endjob:
qemuDomainObjEndJob(driver, vm);
@@ -18199,15 +18203,13 @@ qemuDomainQemuAgentCommand(virDomainPtr domain,
virDomainObjPtr vm;
int ret = -1;
char *result = NULL;
- qemuDomainObjPrivatePtr priv;
+ qemuAgentPtr agent;
virCheckFlags(0, NULL);
if (!(vm = qemuDomObjFromDomain(domain)))
goto cleanup;
- priv = vm->privateData;
-
if (virDomainQemuAgentCommandEnsureACL(domain->conn, vm->def) < 0)
goto cleanup;
@@ -18223,9 +18225,9 @@ qemuDomainQemuAgentCommand(virDomainPtr domain,
if (!qemuDomainAgentAvailable(vm, true))
goto endjob;
- qemuDomainObjEnterAgent(vm);
- ret = qemuAgentArbitraryCommand(priv->agent, cmd, &result, timeout);
- qemuDomainObjExitAgent(vm);
+ agent = qemuDomainObjEnterAgent(vm);
+ ret = qemuAgentArbitraryCommand(agent, cmd, &result, timeout);
+ qemuDomainObjExitAgent(vm, agent);
if (ret < 0)
VIR_FREE(result);
@@ -18294,8 +18296,8 @@ qemuDomainFSTrim(virDomainPtr dom,
{
virQEMUDriverPtr driver = dom->conn->privateData;
virDomainObjPtr vm;
+ qemuAgentPtr agent;
int ret = -1;
- qemuDomainObjPrivatePtr priv;
virCheckFlags(0, -1);
@@ -18309,8 +18311,6 @@ qemuDomainFSTrim(virDomainPtr dom,
if (!(vm = qemuDomObjFromDomain(dom)))
goto cleanup;
- priv = vm->privateData;
-
if (virDomainFSTrimEnsureACL(dom->conn, vm->def) < 0)
goto cleanup;
@@ -18326,9 +18326,9 @@ qemuDomainFSTrim(virDomainPtr dom,
goto endjob;
}
- qemuDomainObjEnterAgent(vm);
- ret = qemuAgentFSTrim(priv->agent, minimum);
- qemuDomainObjExitAgent(vm);
+ agent = qemuDomainObjEnterAgent(vm);
+ ret = qemuAgentFSTrim(agent, minimum);
+ qemuDomainObjExitAgent(vm, agent);
endjob:
qemuDomainObjEndJob(driver, vm);
@@ -18487,7 +18487,7 @@ qemuDomainGetTime(virDomainPtr dom,
{
virQEMUDriverPtr driver = dom->conn->privateData;
virDomainObjPtr vm = NULL;
- qemuDomainObjPrivatePtr priv;
+ qemuAgentPtr agent;
int ret = -1;
int rv;
@@ -18499,8 +18499,6 @@ qemuDomainGetTime(virDomainPtr dom,
if (virDomainGetTimeEnsureACL(dom->conn, vm->def) < 0)
goto cleanup;
- priv = vm->privateData;
-
if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
goto cleanup;
@@ -18513,9 +18511,9 @@ qemuDomainGetTime(virDomainPtr dom,
if (!qemuDomainAgentAvailable(vm, true))
goto endjob;
- qemuDomainObjEnterAgent(vm);
- rv = qemuAgentGetTime(priv->agent, seconds, nseconds);
- qemuDomainObjExitAgent(vm);
+ agent = qemuDomainObjEnterAgent(vm);
+ rv = qemuAgentGetTime(agent, seconds, nseconds);
+ qemuDomainObjExitAgent(vm, agent);
if (rv < 0)
goto endjob;
@@ -18539,6 +18537,7 @@ qemuDomainSetTime(virDomainPtr dom,
virQEMUDriverPtr driver = dom->conn->privateData;
qemuDomainObjPrivatePtr priv;
virDomainObjPtr vm;
+ qemuAgentPtr agent;
bool rtcSync = flags & VIR_DOMAIN_TIME_SYNC;
int ret = -1;
int rv;
@@ -18577,9 +18576,9 @@ qemuDomainSetTime(virDomainPtr dom,
if (!qemuDomainAgentAvailable(vm, true))
goto endjob;
- qemuDomainObjEnterAgent(vm);
- rv = qemuAgentSetTime(priv->agent, seconds, nseconds, rtcSync);
- qemuDomainObjExitAgent(vm);
+ agent = qemuDomainObjEnterAgent(vm);
+ rv = qemuAgentSetTime(agent, seconds, nseconds, rtcSync);
+ qemuDomainObjExitAgent(vm, agent);
if (rv < 0)
goto endjob;
@@ -19676,8 +19675,8 @@ qemuDomainGetFSInfo(virDomainPtr dom,
unsigned int flags)
{
virQEMUDriverPtr driver = dom->conn->privateData;
- qemuDomainObjPrivatePtr priv;
virDomainObjPtr vm;
+ qemuAgentPtr agent;
int ret = -1;
virCheckFlags(0, ret);
@@ -19688,8 +19687,6 @@ qemuDomainGetFSInfo(virDomainPtr dom,
if (virDomainGetFSInfoEnsureACL(dom->conn, vm->def) < 0)
goto cleanup;
- priv = vm->privateData;
-
if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
goto cleanup;
@@ -19702,9 +19699,9 @@ qemuDomainGetFSInfo(virDomainPtr dom,
if (!qemuDomainAgentAvailable(vm, true))
goto endjob;
- qemuDomainObjEnterAgent(vm);
- ret = qemuAgentGetFSInfo(priv->agent, info, vm->def);
- qemuDomainObjExitAgent(vm);
+ agent = qemuDomainObjEnterAgent(vm);
+ ret = qemuAgentGetFSInfo(agent, info, vm->def);
+ qemuDomainObjExitAgent(vm, agent);
endjob:
qemuDomainObjEndJob(driver, vm);
@@ -19721,8 +19718,8 @@ qemuDomainInterfaceAddresses(virDomainPtr dom,
unsigned int flags)
{
virQEMUDriverPtr driver = dom->conn->privateData;
- qemuDomainObjPrivatePtr priv = NULL;
virDomainObjPtr vm = NULL;
+ qemuAgentPtr agent;
int ret = -1;
virCheckFlags(0, -1);
@@ -19730,8 +19727,6 @@ qemuDomainInterfaceAddresses(virDomainPtr dom,
if (!(vm = qemuDomObjFromDomain(dom)))
goto cleanup;
- priv = vm->privateData;
-
if (virDomainInterfaceAddressesEnsureACL(dom->conn, vm->def) < 0)
goto cleanup;
@@ -19753,9 +19748,9 @@ qemuDomainInterfaceAddresses(virDomainPtr dom,
if (!qemuDomainAgentAvailable(vm, true))
goto endjob;
- qemuDomainObjEnterAgent(vm);
- ret = qemuAgentGetInterfaces(priv->agent, ifaces);
- qemuDomainObjExitAgent(vm);
+ agent = qemuDomainObjEnterAgent(vm);
+ ret = qemuAgentGetInterfaces(agent, ifaces);
+ qemuDomainObjExitAgent(vm, agent);
endjob:
qemuDomainObjEndJob(driver, vm);
@@ -19879,8 +19874,8 @@ qemuDomainSetUserPassword(virDomainPtr dom,
unsigned int flags)
{
virQEMUDriverPtr driver = dom->conn->privateData;
- qemuDomainObjPrivatePtr priv;
virDomainObjPtr vm;
+ qemuAgentPtr agent;
int ret = -1;
int rv;
@@ -19892,8 +19887,6 @@ qemuDomainSetUserPassword(virDomainPtr dom,
if (virDomainSetUserPasswordEnsureACL(dom->conn, vm->def) < 0)
goto cleanup;
- priv = vm->privateData;
-
if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
goto cleanup;
@@ -19906,10 +19899,10 @@ qemuDomainSetUserPassword(virDomainPtr dom,
if (!qemuDomainAgentAvailable(vm, true))
goto endjob;
- qemuDomainObjEnterAgent(vm);
- rv = qemuAgentSetUserPassword(priv->agent, user, password,
+ agent = qemuDomainObjEnterAgent(vm);
+ rv = qemuAgentSetUserPassword(agent, user, password,
flags & VIR_DOMAIN_PASSWORD_ENCRYPTED);
- qemuDomainObjExitAgent(vm);
+ qemuDomainObjExitAgent(vm, agent);
if (rv < 0)
goto endjob;
@@ -20139,6 +20132,7 @@ qemuDomainGetGuestVcpus(virDomainPtr dom,
{
virQEMUDriverPtr driver = dom->conn->privateData;
virDomainObjPtr vm = NULL;
+ qemuAgentPtr agent;
qemuAgentCPUInfoPtr info = NULL;
int ninfo = 0;
int ret = -1;
@@ -20157,9 +20151,9 @@ qemuDomainGetGuestVcpus(virDomainPtr dom,
if (!qemuDomainAgentAvailable(vm, true))
goto endjob;
- qemuDomainObjEnterAgent(vm);
- ninfo = qemuAgentGetVCPUs(qemuDomainGetAgent(vm), &info);
- qemuDomainObjExitAgent(vm);
+ agent = qemuDomainObjEnterAgent(vm);
+ ninfo = qemuAgentGetVCPUs(agent, &info);
+ qemuDomainObjExitAgent(vm, agent);
if (ninfo < 0)
goto endjob;
@@ -20189,6 +20183,7 @@ qemuDomainSetGuestVcpus(virDomainPtr dom,
virDomainObjPtr vm = NULL;
virBitmapPtr map = NULL;
qemuAgentCPUInfoPtr info = NULL;
+ qemuAgentPtr agent;
int ninfo = 0;
size_t i;
int ret = -1;
@@ -20215,9 +20210,10 @@ qemuDomainSetGuestVcpus(virDomainPtr dom,
if (!qemuDomainAgentAvailable(vm, true))
goto endjob;
- qemuDomainObjEnterAgent(vm);
- ninfo = qemuAgentGetVCPUs(qemuDomainGetAgent(vm), &info);
- qemuDomainObjExitAgent(vm);
+ agent = qemuDomainObjEnterAgent(vm);
+ ninfo = qemuAgentGetVCPUs(agent, &info);
+ qemuDomainObjExitAgent(vm, agent);
+ agent = NULL;
if (ninfo < 0)
goto endjob;
@@ -20246,9 +20242,12 @@ qemuDomainSetGuestVcpus(virDomainPtr dom,
goto endjob;
}
- qemuDomainObjEnterAgent(vm);
- ret = qemuAgentSetVCPUs(qemuDomainGetAgent(vm), info, ninfo);
- qemuDomainObjExitAgent(vm);
+ if (!qemuDomainAgentAvailable(vm, true))
+ goto endjob;
+
+ agent = qemuDomainObjEnterAgent(vm);
+ ret = qemuAgentSetVCPUs(agent, info, ninfo);
+ qemuDomainObjExitAgent(vm, agent);
endjob:
qemuDomainObjEndJob(driver, vm);
--
1.8.3.1
More information about the libvir-list
mailing list