[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 2/2] blockjob: fix use-after-free in blockcopy



On 08/06/14 23:12, Eric Blake wrote:
> 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>
> ---
>  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 96835bc..82a82aa 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -14886,23 +14886,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
> 

In the cleanup section there's the original place where oldsrc was
returned back to disk->src. That would now be dead code.

ACK with that part removed.

Peter

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]