109 lines
5.1 KiB
Diff
109 lines
5.1 KiB
Diff
From 9617e31b5349b193469874706abcbcb013e6a6fd Mon Sep 17 00:00:00 2001
|
|
Message-Id: <9617e31b5349b193469874706abcbcb013e6a6fd.1407860168.git.crobinso@redhat.com>
|
|
In-Reply-To: <2151695a5119a8d7f44d416c730df50a1e42695a.1407860168.git.crobinso@redhat.com>
|
|
References: <2151695a5119a8d7f44d416c730df50a1e42695a.1407860168.git.crobinso@redhat.com>
|
|
From: Eric Blake <eblake@redhat.com>
|
|
Date: Wed, 6 Aug 2014 14:06:23 -0600
|
|
Subject: [PATCH 3/3] blockjob: fix use-after-free in blockcopy
|
|
|
|
Commit febf84c2 tried to delay in-memory modification of the actual
|
|
domain disk structure until after the qemu event was received.
|
|
However, I missed that the code for block pivot had been temporarily
|
|
setting disk->src = disk->mirror prior to the qemu command, in order
|
|
to label the backing chain of a reused external blockcopy disk;
|
|
and calls into qemu while still in that state before finally undoing
|
|
things at the cleanup label. Since the qemu event handler then does:
|
|
virStorageSourceFree(disk->src);
|
|
disk->src = disk->mirror;
|
|
we have the sad race that a fast enough qemu event can cause a leak of
|
|
the original disk->src, as well as a use-after-free of the disk->mirror
|
|
contents, bad enough to crash libvirtd in some of my test runs, even
|
|
though the common case of the qemu event being much later won't trip
|
|
the race.
|
|
|
|
I'll go wear the brown paper bag of shame, for introducing a crasher
|
|
in between rc1 and rc2 of the freeze for 1.2.7 :( My only
|
|
consolation is that virDomainBlockJobAbort requires the domain:write
|
|
ACL, so it is not a CVE.
|
|
|
|
The valgrind report when the race occurs looks like:
|
|
|
|
==25612== Invalid read of size 4
|
|
==25612== at 0x50E7C90: virStorageSourceGetActualType (virstoragefile.c:1948)
|
|
==25612== by 0x209C0B18: qemuDomainDetermineDiskChain (qemu_domain.c:2473)
|
|
==25612== by 0x209D7F6A: qemuProcessHandleBlockJob (qemu_process.c:1087)
|
|
==25612== by 0x209F40C9: qemuMonitorEmitBlockJob (qemu_monitor.c:1357)
|
|
...
|
|
==25612== Address 0xe4b5610 is 0 bytes inside a block of size 200 free'd
|
|
==25612== at 0x4A07577: free (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
|
|
==25612== by 0x50839E9: virFree (viralloc.c:582)
|
|
==25612== by 0x50E7E51: virStorageSourceFree (virstoragefile.c:2015)
|
|
==25612== by 0x209D7EFF: qemuProcessHandleBlockJob (qemu_process.c:1073)
|
|
==25612== by 0x209F40C9: qemuMonitorEmitBlockJob (qemu_monitor.c:1357)
|
|
|
|
* src/qemu/qemu_driver.c (qemuDomainBlockPivot): Don't corrupt
|
|
disk->src, and only label chain for blockcopy.
|
|
|
|
Signed-off-by: Eric Blake <eblake@redhat.com>
|
|
(cherry picked from commit 265680c58ebbee30bb70369e7d9905a599afbd6a)
|
|
---
|
|
src/qemu/qemu_driver.c | 40 +++++++++++++++++++++++++---------------
|
|
1 file changed, 25 insertions(+), 15 deletions(-)
|
|
|
|
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
|
|
index 57cc913..a050dbc 100644
|
|
--- a/src/qemu/qemu_driver.c
|
|
+++ b/src/qemu/qemu_driver.c
|
|
@@ -14888,23 +14888,33 @@ qemuDomainBlockPivot(virConnectPtr conn,
|
|
}
|
|
}
|
|
|
|
- /* We previously labeled only the top-level image; but if the
|
|
- * image includes a relative backing file, the pivot may result in
|
|
- * qemu needing to open the entire backing chain, so we need to
|
|
- * label the entire chain. This action is safe even if the
|
|
- * backing chain has already been labeled; but only necessary when
|
|
- * we know for sure that there is a backing chain. */
|
|
- oldsrc = disk->src;
|
|
- disk->src = disk->mirror;
|
|
+ /* For active commit, the mirror is part of the already labeled
|
|
+ * chain. For blockcopy, we previously labeled only the top-level
|
|
+ * image; but if the user is reusing an external image that
|
|
+ * includes a backing file, the pivot may result in qemu needing
|
|
+ * to open the entire backing chain, so we need to label the
|
|
+ * entire chain. This action is safe even if the backing chain
|
|
+ * has already been labeled; but only necessary when we know for
|
|
+ * sure that there is a backing chain. */
|
|
+ if (disk->mirrorJob == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY) {
|
|
+ oldsrc = disk->src;
|
|
+ disk->src = disk->mirror;
|
|
+
|
|
+ if (qemuDomainDetermineDiskChain(driver, vm, disk, false) < 0)
|
|
+ goto cleanup;
|
|
|
|
- if (qemuDomainDetermineDiskChain(driver, vm, disk, false) < 0)
|
|
- goto cleanup;
|
|
+ if (disk->mirror->format &&
|
|
+ disk->mirror->format != VIR_STORAGE_FILE_RAW &&
|
|
+ (virDomainLockDiskAttach(driver->lockManager, cfg->uri, vm,
|
|
+ disk) < 0 ||
|
|
+ qemuSetupDiskCgroup(vm, disk) < 0 ||
|
|
+ virSecurityManagerSetDiskLabel(driver->securityManager, vm->def,
|
|
+ disk) < 0))
|
|
+ goto cleanup;
|
|
|
|
- if (disk->mirror->format && disk->mirror->format != VIR_STORAGE_FILE_RAW &&
|
|
- (virDomainLockDiskAttach(driver->lockManager, cfg->uri, vm, disk) < 0 ||
|
|
- qemuSetupDiskCgroup(vm, disk) < 0 ||
|
|
- virSecurityManagerSetDiskLabel(driver->securityManager, vm->def, disk) < 0))
|
|
- goto cleanup;
|
|
+ disk->src = oldsrc;
|
|
+ oldsrc = NULL;
|
|
+ }
|
|
|
|
/* Attempt the pivot. Record the attempt now, to prevent duplicate
|
|
* attempts; but the actual disk change will be made when emitting
|
|
--
|
|
1.9.3
|
|
|