On Thu, Aug 23, 2012 at 02:36:40PM +0530, M. Mohan Kumar wrote:
From: "M. Mohan Kumar" <mohan(a)in.ibm.com>
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(a)in.ibm.com>
---
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 :|