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 :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list