From patchwork Tue Jun 14 08:12:45 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mohan Kumar M X-Patchwork-Id: 100290 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [140.186.70.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 2621EB6FA8 for ; Tue, 14 Jun 2011 18:18:40 +1000 (EST) Received: from localhost ([::1]:42286 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QWOpY-000806-Kn for incoming@patchwork.ozlabs.org; Tue, 14 Jun 2011 04:18:36 -0400 Received: from eggs.gnu.org ([140.186.70.92]:56979) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QWOkD-0007NC-3L for qemu-devel@nongnu.org; Tue, 14 Jun 2011 04:13:06 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QWOkA-0001St-Qn for qemu-devel@nongnu.org; Tue, 14 Jun 2011 04:13:04 -0400 Received: from e28smtp04.in.ibm.com ([122.248.162.4]:41777) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QWOk9-0001SO-79 for qemu-devel@nongnu.org; Tue, 14 Jun 2011 04:13:02 -0400 Received: from d28relay03.in.ibm.com (d28relay03.in.ibm.com [9.184.220.60]) by e28smtp04.in.ibm.com (8.14.4/8.13.1) with ESMTP id p5E8Cpvb008044 for ; Tue, 14 Jun 2011 13:42:51 +0530 Received: from d28av01.in.ibm.com (d28av01.in.ibm.com [9.184.220.63]) by d28relay03.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p5E8CoXs4288528 for ; Tue, 14 Jun 2011 13:42:51 +0530 Received: from d28av01.in.ibm.com (loopback [127.0.0.1]) by d28av01.in.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p5E8CnO7031088 for ; Tue, 14 Jun 2011 13:42:49 +0530 Received: from in.ibm.com ([9.124.35.234]) by d28av01.in.ibm.com (8.14.4/8.13.1/NCO v10.0 AVin) with SMTP id p5E8CnGA031080; Tue, 14 Jun 2011 13:42:49 +0530 Date: Tue, 14 Jun 2011 13:42:45 +0530 From: "M. Mohan Kumar" To: qemu-devel@nongnu.org Message-ID: <20110614081244.GB3428@in.ibm.com> MIME-Version: 1.0 Content-Disposition: inline User-Agent: Mutt/1.5.19 (2009-01-05) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6, seldom 2.4 (older, 4) X-Received-From: 122.248.162.4 Cc: jvrao@linux.vnet.ibm.com Subject: [Qemu-devel] [RFC PATCH] virtio-9p: Use clone approach to fix TOCTOU vulnerability X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list Reply-To: mohan@in.ibm.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org [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 Signed-off-by: M. Mohan Kumar --- 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 +#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 +#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; }