From da27be1b096cfcbc54d7b30379b7a6fb0d2de69d Mon Sep 17 00:00:00 2001 From: Michal Privoznik Date: Fri, 28 Jun 2019 08:17:45 +0200 Subject: [PATCH] qemu: Don't leak storage perms on failure in qemuDomainAttachDiskGeneric At the very beginning of the attach function the qemuDomainStorageSourceChainAccessAllow() is called which modifies CGroups, locks and seclabels for new disk and its backing chain. This must be followed by a counterpart which reverts back all the changes if something goes wrong. This boils down to calling qemuDomainStorageSourceChainAccessRevoke() which is done under 'error' label. But not all failure branches jump there. They just jump onto 'cleanup' label where no revoke is done. Such mistake is easy to do because 'cleanup' label does exist. Therefore, dissolve 'error' block in 'cleanup' and have everything jump onto 'cleanup' label. Signed-off-by: Michal Privoznik Reviewed-by: Cole Robinson --- src/qemu/qemu_hotplug.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 930f4283c8..55b09942cf 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -683,13 +683,13 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, bool blockdev = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV); if (qemuDomainStorageSourceChainAccessAllow(driver, vm, disk->src) < 0) - goto cleanup; + return -1; if (qemuAssignDeviceDiskAlias(vm->def, disk, priv->qemuCaps) < 0) - goto error; + goto cleanup; if (qemuDomainPrepareDiskSource(disk, priv, cfg) < 0) - goto error; + goto cleanup; if (blockdev) { if (disk->copy_on_read == VIR_TRISTATE_SWITCH_ON && @@ -706,13 +706,13 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, } if (!(devstr = qemuBuildDiskDeviceStr(vm->def, disk, 0, priv->qemuCaps))) - goto error; + goto cleanup; if (VIR_REALLOC_N(vm->def->disks, vm->def->ndisks + 1) < 0) - goto error; + goto cleanup; if (qemuHotplugAttachManagedPR(driver, vm, disk->src, QEMU_ASYNC_JOB_NONE) < 0) - goto error; + goto cleanup; qemuDomainObjEnterMonitor(driver, vm); @@ -748,7 +748,7 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, if (qemuDomainObjExitMonitor(driver, vm) < 0) { ret = -2; - goto error; + goto cleanup; } virDomainAuditDisk(vm, NULL, disk->src, "attach", true); @@ -757,6 +757,8 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, ret = 0; cleanup: + if (ret < 0) + ignore_value(qemuDomainStorageSourceChainAccessRevoke(driver, vm, disk->src)); qemuDomainSecretDiskDestroy(disk); return ret; @@ -773,9 +775,6 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, ret = -2; virDomainAuditDisk(vm, NULL, disk->src, "attach", false); - - error: - ignore_value(qemuDomainStorageSourceChainAccessRevoke(driver, vm, disk->src)); goto cleanup; }