[libvirt] [PATCH] qemu: read backing chain names from qemu

[Daniel P. Berrange]

On Thu, Mar 12, 2015 at 08:09:35AM -0600, Eric Blake wrote:
> On 03/12/2015 03:14 AM, Daniel P. Berrange wrote:
> 
> >> even though the file exists.  Trying to teach libvirt the rules on
> >> which name qemu will expect is not worth the effort (besides, we'd
> >> have to remember it across libvirtd restarts, and track whether a
> >> file was opened via metadata or via snapshot creation for a given
> >> qemu process); it is easier to just always directly ask qemu what
> >> string it expects to see in the first place.
> > 
> > If you're going to ask QEMU for the names, then libvirt needs to
> > validate that the name it gets back resolves to the same inode
> > as the name it originally had. We cannot trust QEMU to tell the
> > truth and cannot let it trick us into relabelling the wrong files
> > by giving us the name of something completely different.
> > 
> 
> We are NOT relabelling files based on the information.  I agree that if
> we act on a name returned by qemu, then we are vulnerable to mistakes if
> a guest manages to compromise qemu; but I don't think this is one of
> those situations.
> 
> > 
> >>      qemuDomainObjEnterMonitor(driver, vm);
> >> -    ret = qemuMonitorBlockJob(priv->mon, device, basePath, backingPath,
> >> -                              speed, mode, async);
> >> +    if (baseSource)
> >> +        basePath = qemuMonitorDiskNameLookup(priv->mon, device, disk->src,
> >> +                                             baseSource);
> > 
> > So this needs to valid basePath vs baseSource inodes.
> 
> The only use of basePath is the string passed right back to qemu, to
> kick off the commit or pull operation.  Since qemu is using strcmp()
> (rather than inode comparison), feeding it the name it just told us will
> make the commit operation work on the correct node.  Everything else
> that libvirt does, such as relabelling files, is done solely on the
> information libvirt has been tracking (and not reliant on the name
> returned by qemu).
> 
> But if it would satisfy your paranoia, I can certainly add a
> verification step that the string being returned by qemu resolves to the
> same inode being tracked by libvirt, at least in the case where the
> <disk> element resolves to a filename rather than a network disk.

I think it would be desirable, because while your current usage
may be safe with these assumptions, if someone refactors this
6 months later they may not realize the security implications
of this code.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|