[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 <jv...@linux.vnet.ibm.com> Signed-off-by: M. Mohan Kumar <mo...@in.ibm.com> --- 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