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

[Peter Krempa]

On 08/07/14 18:36, Eric Blake wrote:
> 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?
> 

Ah right :)

Peter

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