On Thu, Aug 23, 2012 at 02:36:40PM +0530, M. Mohan Kumar wrote: > From: "M. Mohan Kumar" <mohan@xxxxxxxxxx> > > A new FS driver type 'proxy' is added to QEMU 9p server. This patch adds > support for using proxy FS driver from libvirt. > > QEMU proxy FS driver uses socket for communicating between helper and qemu > proxy FS driver. Proxy helper (a stand alone binary part of qemu) is invoked > with one of the descriptors created using socketpair call and the share path. > Similarly QEMU is invoked with another descriptor created using the same > socketpair system call and with other required FS driver parameters. > > Need for proxy FS driver > ======================== > Pass through security model in QEMU 9p server has following issues: > 1) TOCTTOU vulnerability: Following symbolic links in the server could > provide access to files beyond 9p export path. > > 2) Running QEMU with root privilege could be a security issue (pass > through security model needs root privilege). > > Proxy FS driver is implemented to solve these issues. > > Proxy FS uses chroot + socket combination for securing the vulnerability > known with following symbolic links. Intention of adding a new filesystem > type is to allow qemu to run in non-root mode, but doing privileged > operations in a chroot environment using socket IO. > > Proxy helper is invoked with root privileges and chroots into 9p export path. > QEMU proxy fs driver sends filesystem request to proxy helper and receives the > response from it. > > Proxy helper is designed such a way that it needs only few capabilities related > to filesystem operations (such as CAP_DAC_OVERRIDE, CAP_FOWNER, etc) and all > other capabilities are dropped (CAP_SYS_CHROOT, etc) > > Proxy patches > http://permalink.gmane.org/gmane.comp.emulators.qemu/128735 > > Signed-off-by: M. Mohan Kumar <mohan@xxxxxxxxxx> > --- > Changes from previous version V3 > * Addressed Daniel's review comments > > Changes from previous version V2 > * Rebased on top of current libvirt git level > > Changes from previous version > * Remove the xml node for specifying the virtfs-proxy-helper path, now it is > determined from qemu binary path. > > docs/formatdomain.html.in | 2 + > src/conf/domain_conf.c | 3 +- > src/conf/domain_conf.h | 2 +- Opps, forgot to mention in previous reviews that you must update docs/schemas/domaincommon.rng to list the new attribute > diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c > index 5472267..88b7e87 100644 > --- a/src/qemu/qemu_capabilities.c > +++ b/src/qemu/qemu_capabilities.c > @@ -144,7 +144,6 @@ VIR_ENUM_IMPL(qemuCaps, QEMU_CAPS_LAST, > "ich9-ahci", > "no-acpi", > "fsdev-readonly", > - > "virtio-blk-pci.scsi", /* 80 */ > "blk-sg-io", > "drive-copy-on-read", Bogus whitespace removal > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index 4ca3047..3f78635 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -46,6 +46,7 @@ > #include <sys/utsname.h> > #include <sys/stat.h> > #include <fcntl.h> > +#include <libgen.h> You don't use any functions from this header, so it can be removed. > @@ -2632,9 +2634,59 @@ error: > return NULL; > } > > +/* > + * Invokes the Proxy Helper with one of the socketpair as its parameter > + * > + */ > +static int qemuInvokeProxyHelper(const char *emulator, int sock, const char *path) > +{ > +#define HELPER "virtfs-proxy-helper" > + int ret_val, status; > + virCommandPtr cmd; > + char *helper, *dname, *dp; > + > + dp = strdup(emulator); > + if (!dp) { > + virReportOOMError(); > + return -1; This doesn't close 'sock' like other error paths do > + } > + dname = strrchr(dp, '/'); > + if (!dname) { > + VIR_FREE(dp); > + VIR_FORCE_CLOSE(sock); > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("Unable to get path details from %s"), dp); > + return -1; > + } > + *dname = '\0'; > + if (virAsprintf(&helper, "%s/%s", dp, HELPER) < 0) { > + VIR_FREE(dp); > + VIR_FORCE_CLOSE(sock); > + virReportOOMError(); > + return -1; > + } > + > + cmd = virCommandNewArgList(helper, NULL); > + virCommandAddArg(cmd, "-f"); > + virCommandAddArgFormat(cmd, "%d", sock); > + virCommandAddArg(cmd, "-p"); > + virCommandAddArgFormat(cmd, "%s", path); > + virCommandTransferFD(cmd, sock); > + virCommandDaemonize(cmd); > + ret_val = virCommandRun(cmd, &status); > + if (ret_val < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("%s can't execute"), helper); > + VIR_FORCE_CLOSE(sock); > + } > + virCommandFree(cmd); > + VIR_FREE(helper); > + VIR_FREE(dp); > + return ret_val; > +} > > char *qemuBuildFSStr(virDomainFSDefPtr fs, > - virBitmapPtr qemuCaps ATTRIBUTE_UNUSED) > + virBitmapPtr qemuCaps ATTRIBUTE_UNUSED, int qemuSocket) > { > virBuffer opt = VIR_BUFFER_INITIALIZER; > const char *driver = qemuDomainFSDriverTypeToString(fs->fsdriver); > @@ -2683,6 +2735,10 @@ char *qemuBuildFSStr(virDomainFSDefPtr fs, > } > > virBufferAsprintf(&opt, ",id=%s%s", QEMU_FSDEV_HOST_PREFIX, fs->info.alias); > + > + if (fs->fsdriver == VIR_DOMAIN_FS_DRIVER_TYPE_PROXY) > + virBufferAsprintf(&opt, ",sock_fd=%d", qemuSocket); > + > virBufferAsprintf(&opt, ",path=%s", fs->src); > > if (fs->readonly) { > @@ -5153,10 +5209,31 @@ qemuBuildCommandLine(virConnectPtr conn, > if (qemuCapsGet(qemuCaps, QEMU_CAPS_FSDEV)) { > for (i = 0 ; i < def->nfss ; i++) { > char *optstr; > + int sockets[2] = {-1, -1}; > virDomainFSDefPtr fs = def->fss[i]; > > + /* > + * If its a proxy FS, we need to create a socket pair > + * and invoke proxy_helper > + */ > + if (fs->fsdriver == VIR_DOMAIN_FS_DRIVER_TYPE_PROXY) { > + if (qemuCapsGet(qemuCaps, QEMU_CAPS_FSDEV_PROXY) < 0) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("proxy helper not supported")); > + goto error; > + } > + /* create a socket pair */ > + if (socketpair(PF_UNIX, SOCK_STREAM, 0, sockets) < 0) { > + virReportSystemError(errno, _("socketpair %s"), "failed"); > + goto error; > + } > + virCommandTransferFD(cmd, sockets[1]); > + if (qemuInvokeProxyHelper(def->emulator, sockets[0], > + fs->src) < 0) > + goto error; > + } Hmm, right here we are in the middle of constructing the command line args for QEMU. We should not be launching at processes at this point, not least since we could be running this from the test suite. Also you don't appear to have any code to tell the daemonized proxy to shutdown when QEMU is stopped ? We really need to push the invocation of the proxy up into the code which actually launches QEMU, and merely have this part construct the command line arguments. > virCommandAddArg(cmd, "-fsdev"); > - if (!(optstr = qemuBuildFSStr(fs, qemuCaps))) > + if (!(optstr = qemuBuildFSStr(fs, qemuCaps, sockets[1]))) > goto error; > virCommandAddArg(cmd, optstr); > VIR_FREE(optstr); > diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h > index e999bc7..2da5ad0 100644 > --- a/src/qemu/qemu_command.h > +++ b/src/qemu/qemu_command.h > @@ -89,7 +89,8 @@ char *qemuBuildDriveStr(virConnectPtr conn, > bool bootable, > virBitmapPtr qemuCaps); > char *qemuBuildFSStr(virDomainFSDefPtr fs, > - virBitmapPtr qemuCaps); > + virBitmapPtr qemuCaps, > + int qemuSocket); > > /* Current, best practice */ > char * qemuBuildDriveDevStr(virDomainDefPtr def, 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