Re: [Qemu-devel] [PATCH] Add support for fd: protocol

[Daniel P. Berrange]

On Mon, May 23, 2011 at 11:19:15AM +0100, Stefan Hajnoczi wrote:
> On Mon, May 23, 2011 at 10:45 AM, Daniel P. Berrange
> <address@hidden> wrote:
> > On Fri, May 20, 2011 at 02:48:23PM -0400, Corey Bryant wrote:
> >> sVirt provides SELinux MAC isolation for Qemu guest processes and their
> >> corresponding resources (image files). sVirt provides this support
> >> by labeling guests and resources with security labels that are stored
> >> in file system extended attributes. Some file systems, such as NFS, do
> >> not support the extended attribute security namespace, which is needed
> >> for image file isolation when using the sVirt SELinux security driver
> >> in libvirt.
> >>
> >> The proposed solution entails a combination of Qemu, libvirt, and
> >> SELinux patches that work together to isolate multiple guests' images
> >> when they're stored in the same NFS mount. This results in an
> >> environment where sVirt isolation and NFS image file isolation can both
> >> be provided.
> >>
> >> Currently, Qemu opens an image file in addition to performing the
> >> necessary read and write operations. The proposed solution will move
> >> the open out of Qemu and into libvirt. Once libvirt opens an image
> >> file for the guest, it will pass the file descriptor to Qemu via a
> >> new fd: protocol.
> >>
> >> If the image file resides in an NFS mount, the following SELinux policy
> >> changes will provide image isolation:
> >>
> >>   - A new SELinux boolean is created (e.g. virt_read_write_nfs) to
> >>     allow Qemu (svirt_t) to only have SELinux read and write
> >>     permissions on nfs_t files
> >>
> >>   - Qemu (svirt_t) also gets SELinux use permissions on libvirt
> >>     (virtd_t) file descriptors
> >>
> >> Following is a sample invocation of Qemu using the fd: protocol:
> >>
> >>     qemu -drive file=fd:4,format=qcow2
> >>
> >> This patch contains the Qemu code to support this solution. I would
> >> like to solicit input from the libvirt community prior to starting
> >> the libvirt patch.
> >>
> >> This patch was tested with the following formats: raw, cow, qcow,
> >> qcow2, vmdk, using the fd: protocol as well as existing file name
> >> support. Non-valid file descriptors were also tested.
> >
> > How can backing files work ?  The '-drive' syntax doesn't provide
> > any way to set properties against the backing files (which may be
> > nested to arbitrary depth).  AFAICT, we need to the often discussed
> > -blockdev advanced syntax to be able to set a 'fd' property against
> > nested backing files.  I'm not sure it is worth supporting fd: if
> > we only have -drive syntax, since backing files are an important
> > feature for most mgmt apps.
> >
> > Also, there are a few places in QEMU, where it re-opens the existing
> > block driver on the fly. What is the plan for supporting this, without
> > having QEMU block on waiting for libvirt to pass it a new FD ?
> 
> QEMU could ask for a file over QMP.  So you bootstrap it using fd: but
> when a reopen or backing file is needed, QEMU raises a
> QEVENT_BLOCK_REQUEST_FILE event.  Of course waiting around for the
> reopen to complete it not ideal as it may temporarily pause the guest.

It feels to me that turning the current block driver code which just does
open(2) on files, into something which issues events & asynchronously
waits for a file would potentially be quite complex.

You also need to be much more careful from a security POV if the mgmt
app is accepting requests to open arbitrary files from QEMU, to ensure
the filenames are correctly/strictly validated before opening them and
giving them back to QEMU. An architecture where the mgmt app decides
what FDs to supply upfront, has less potential for security errors.

To me the ideal would thus be that we can supply FDs for the backing
store with -blockdev syntax, and that places where QEMU re-opens files
would be enhanced to avoid that need. If there are things we can't do
without closing & re-opening the same file, then perhaps we need some
new ioctl()/fcntl() calls to change those file attributes on the fly.

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