[Qemu-devel] [RFC PATCH] virtio-9p: Use clone approach to fix TOCTOU vulnerability

[M. Mohan Kumar]

[RFC PATCH] virtio-9p: Use clone approach to fix TOCTOU vulnerability

In passthrough security model, following a symbolic link in the server
side could result in TOCTTOU vulnerability.

Use clone system call to create a thread which runs in chrooted
environment. All passthrough model file operations are done from this
thread to avoid TOCTTOU vulnerability.

Signed-off-by: Venkateswararao Jujjuri <address@hidden>
Signed-off-by: M. Mohan Kumar <address@hidden>
---
 fsdev/file-op-9p.h         |    1 +
 hw/9pfs/virtio-9p-coth.c   |  105 +++++++++++++++++++++++++++++++++++++++++--
 hw/9pfs/virtio-9p-coth.h   |   13 +++++-
 hw/9pfs/virtio-9p-device.c |    7 +++-
 hw/9pfs/virtio-9p.h        |    6 ++-
 5 files changed, 124 insertions(+), 8 deletions(-)

diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h
index 5d088d4..c54f6bf 100644
--- a/fsdev/file-op-9p.h
+++ b/fsdev/file-op-9p.h
@@ -60,6 +60,7 @@ typedef struct FsContext
     SecModel fs_sm;
     uid_t uid;
     int open_flags;
+    int enable_chroot;
     struct xattr_operations **xops;
     /* fs driver specific data */
     void *private;
diff --git a/hw/9pfs/virtio-9p-coth.c b/hw/9pfs/virtio-9p-coth.c
index e61b656..aa71a83 100644
--- a/hw/9pfs/virtio-9p-coth.c
+++ b/hw/9pfs/virtio-9p-coth.c
@@ -17,6 +17,8 @@
 #include "qemu-thread.h"
 #include "qemu-coroutine.h"
 #include "virtio-9p-coth.h"
+#include <sys/syscall.h>
+#include "qemu-error.h"
 
 /* v9fs glib thread pool */
 static V9fsThPool v9fs_pool;
@@ -56,7 +58,96 @@ static void v9fs_thread_routine(gpointer data, gpointer 
user_data)
     } while (len == -1 && errno == EINTR);
 }
 
-int v9fs_init_worker_threads(void)
+static int v9fs_chroot_function(void *arg)
+{
+    GError *err;
+    V9fsChrootState *csp = arg;
+    FsContext *fs_ctx = csp->fs_ctx;
+    V9fsThPool *p = &v9fs_pool;
+    int notifier_fds[2];
+
+    if (chroot(fs_ctx->fs_root) < 0) {
+        error_report("chroot");
+        goto error;
+    }
+
+    if (qemu_pipe(notifier_fds) == -1) {
+        return -1;
+    }
+    p->pool = g_thread_pool_new(v9fs_thread_routine, p, 10, TRUE, &err);
+    if (!p->pool) {
+        error_report("g_thread_pool_new");
+        goto error;
+    }
+
+    p->completed = g_async_queue_new();
+    if (!p->completed) {
+        /*
+         * We are going to terminate.
+         * So don't worry about cleanup
+         */
+        goto error;
+    }
+    p->rfd = notifier_fds[0];
+    p->wfd = notifier_fds[1];
+
+    fcntl(p->rfd, F_SETFL, O_NONBLOCK);
+    fcntl(p->wfd, F_SETFL, O_NONBLOCK);
+
+    qemu_set_fd_handler(p->rfd, v9fs_qemu_process_req_done, NULL, NULL);
+
+    /* Signal parent thread that thread pools are created */
+    g_cond_signal(csp->cond);
+    g_mutex_lock(csp->mutex_term);
+    /* TODO: Wake up cs->terminate during 9p hotplug support */
+    g_cond_wait(csp->terminate, csp->mutex);
+    g_mutex_unlock(csp->mutex_term);
+    g_mutex_free(csp->mutex);
+    g_cond_free(csp->cond);
+    g_mutex_free(csp->mutex_term);
+    g_cond_free(csp->terminate);
+    qemu_free(csp->stack);
+    qemu_free(csp);
+    return 0;
+error:
+    p->pool = NULL;
+    g_cond_signal(csp->cond);
+    return 0;
+}
+
+static int v9fs_clone_chroot_th(FsContext *fs_ctx)
+{
+    pid_t pid;
+    V9fsChrootState *cs;
+
+    cs = qemu_malloc(sizeof(*cs));
+    cs->stack = qemu_malloc(THREAD_STACK);
+    cs->mutex = g_mutex_new();
+    cs->cond = g_cond_new();
+    cs->mutex_term = g_mutex_new();
+    cs->terminate = g_cond_new();
+    cs->fs_ctx = fs_ctx;
+
+    g_mutex_lock(cs->mutex);
+    pid = clone(&v9fs_chroot_function, cs->stack + THREAD_STACK, CLONE_FILES |
+                                CLONE_SIGHAND | CLONE_VM | CLONE_THREAD, cs);
+    if (pid == -1) {
+        error_report("clone");
+        g_mutex_unlock(cs->mutex);
+        g_mutex_free(cs->mutex);
+        g_cond_free(cs->cond);
+        g_mutex_free(cs->mutex_term);
+        g_cond_free(cs->terminate);
+        qemu_free(cs->stack);
+        qemu_free(cs);
+        return pid;
+    }
+    g_cond_wait(cs->cond, cs->mutex);
+    g_mutex_unlock(cs->mutex);
+    return pid;
+}
+
+int v9fs_init_worker_threads(FsContext *fs_ctx)
 {
     int notifier_fds[2];
     V9fsThPool *p = &v9fs_pool;
@@ -66,14 +157,18 @@ int v9fs_init_worker_threads(void)
         g_thread_init(NULL);
     }
 
-    if (qemu_pipe(notifier_fds) == -1) {
-        return -1;
+    if (fs_ctx->enable_chroot) {
+        return v9fs_clone_chroot_th(fs_ctx);
+    } else {
+        p->pool = g_thread_pool_new(v9fs_thread_routine, p, -1, FALSE, NULL);
     }
-
-    p->pool = g_thread_pool_new(v9fs_thread_routine, p, -1, FALSE, NULL);
     if (!p->pool) {
         return -1;
     }
+    if (qemu_pipe(notifier_fds) == -1) {
+        return -1;
+    }
+
     p->completed = g_async_queue_new();
     if (!p->completed) {
         /*
diff --git a/hw/9pfs/virtio-9p-coth.h b/hw/9pfs/virtio-9p-coth.h
index 4630080..422354a 100644
--- a/hw/9pfs/virtio-9p-coth.h
+++ b/hw/9pfs/virtio-9p-coth.h
@@ -20,6 +20,8 @@
 #include "virtio-9p.h"
 #include <glib.h>
 
+#define THREAD_STACK (16 * 1024)
+
 typedef struct V9fsThPool {
     int rfd;
     int wfd;
@@ -27,6 +29,15 @@ typedef struct V9fsThPool {
     GAsyncQueue *completed;
 } V9fsThPool;
 
+typedef struct V9fsChrootState {
+    FsContext *fs_ctx;
+    GMutex *mutex;
+    GCond *cond;
+    GMutex *mutex_term;
+    GCond *terminate;
+    void *stack;
+} V9fsChrootState;
+
 /*
  * we want to use bottom half because we want to make sure the below
  * sequence of events.
@@ -55,7 +66,7 @@ typedef struct V9fsThPool {
     } while (0)
 
 extern void co_run_in_worker_bh(void *);
-extern int v9fs_init_worker_threads(void);
+extern int v9fs_init_worker_threads(FsContext *fs_ctx);
 extern int v9fs_co_readlink(V9fsPDU *, V9fsPath *, V9fsString *);
 extern int v9fs_co_readdir_r(V9fsPDU *, V9fsFidState *,
                            struct dirent *, struct dirent **result);
diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
index c7a7f44..5e2cc5a 100644
--- a/hw/9pfs/virtio-9p-device.c
+++ b/hw/9pfs/virtio-9p-device.c
@@ -82,10 +82,15 @@ VirtIODevice *virtio_9p_init(DeviceState *dev, V9fsConf 
*conf)
         exit(1);
     }
 
+    s->ctx.enable_chroot = 0;
+
     if (!strcmp(fse->security_model, "passthrough")) {
         /* Files on the Fileserver set to client user credentials */
         s->ctx.fs_sm = SM_PASSTHROUGH;
         s->ctx.xops = passthrough_xattr_ops;
+
+        /* TODO: Add a configuration option to enable this */
+        s->ctx.enable_chroot = 1;
     } else if (!strcmp(fse->security_model, "mapped")) {
         /* Files on the fileserver are set to QEMU credentials.
          * Client user credentials are saved in extended attributes.
@@ -146,7 +151,7 @@ VirtIODevice *virtio_9p_init(DeviceState *dev, V9fsConf 
*conf)
                 " and export path:%s\n", conf->fsdev_id, s->ctx.fs_root);
         exit(1);
     }
-    if (v9fs_init_worker_threads() < 0) {
+    if (v9fs_init_worker_threads(&s->ctx) < 0) {
         fprintf(stderr, "worker thread initialization failed\n");
         exit(1);
     }
diff --git a/hw/9pfs/virtio-9p.h b/hw/9pfs/virtio-9p.h
index 9e3d4d3..ffe4f60 100644
--- a/hw/9pfs/virtio-9p.h
+++ b/hw/9pfs/virtio-9p.h
@@ -113,7 +113,11 @@ enum p9_proto_version {
 #define FID_NON_RECLAIMABLE     0x2
 static inline const char *rpath(FsContext *ctx, const char *path, char *buffer)
 {
-    snprintf(buffer, PATH_MAX, "%s/%s", ctx->fs_root, path);
+    if (ctx->enable_chroot) {
+        snprintf(buffer, PATH_MAX, "%s", path);
+    } else {
+        snprintf(buffer, PATH_MAX, "%s/%s", ctx->fs_root, path);
+    }
     return buffer;
 }
 
-- 
1.7.5.1