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

[Eric Blake]

On 08/07/2014 01:58 AM, Peter Krempa wrote:
> 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.
>>

>>
>> -    if (qemuDomainDetermineDiskChain(driver, vm, disk, false) < 0)
>> -        goto cleanup;
>> +        if (qemuDomainDetermineDiskChain(driver, vm, disk, false) < 0)
>> +            goto cleanup;
>>

If we go to cleanup here...

>> -    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;

...then we miss the restore of disk->src here...

>> +    }
>>
>>      /* 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.

...so the cleanup label is not dead code.

> 
> ACK with that part removed.

Is the patch okay as-is, with my explanation?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 539 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140807/ec37ce26/attachment-0001.sig>