Received: by 10.223.185.116 with SMTP id b49csp2399549wrg; Mon, 12 Feb 2018 08:58:08 -0800 (PST) X-Google-Smtp-Source: AH8x224Lz4XkW77hmbCyJAK7FPxsleYltyDItD2crG+ke2xJzt9KtUM0P8AS5RBl1WpdswL7ZD+c X-Received: by 2002:a17:902:b28b:: with SMTP id u11-v6mr4711398plr.146.1518454688739; Mon, 12 Feb 2018 08:58:08 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1518454688; cv=none; d=google.com; s=arc-20160816; b=MaAhbMDT1HVDqE10dDmCzYq8fmcK8klhyI5FVrTAsmFEHUZaqk7Kj9VqG4r5X4v2ae nbCkN/FhHDKWgvBzair13ijd84otADwULK3WQdYnsyoM0zjvCIIOH00ygiCJ6V/bgSgj ecwaOL9ehFD3ohFA5vlSVoAoe4I0k5NXrYQeVRHZoTWhaL3rwl1/vsPLT0B48/OMD1wS TF4RhRa2VaBPC9PkybQpuwjUCnNWaVxdUs7Y5pgynv86BOebv03ltfX7ouN3B7CZ3mm4 W93L+CoG1+Oe+VquKmJqkfP0q4ncW0ggro8m20dLMB6X0YJCXyRzFXwmzI1ItlIfpBPt lESg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:cc:to:subject:message-id:date:from :references:in-reply-to:mime-version:arc-authentication-results; bh=r/3xUcyZjwZoplVy9T9EDM+Bf9ZBu91xHlm7YiXtvPE=; b=fmOQMI7urLAiWbz6xZNzljuoap3mchLlPP2LRkzEu2u8U0yI+zwuI1CJMl5iYVlGRU 3tlvJJGJl+xcDqM7uqgPXwptQNcnwnGfImffdSvRG6KbPwRLDMFqZBFivKVJ8hh/Fx/c 2N7Gor0h3Nb5Ho47oGTpi5pb/DO7I+PIpTsW3dTX5gwC7yRK65z3Lkd95fqerAFFttin svdtxBbs0ng7wTqHLIMAB/PjQs2W79uxHaS81PVaTVKYRHHZRhcSlumjULJMOTV5QHGz 7EVHN9RcAp4oMqOVdGw543ljgl5XkhzahhpwDdLiXiNtJT1WG81NBTtarNs3L0LuUBn3 4ySQ== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=redhat.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id g7-v6si3625018pll.826.2018.02.12.08.57.53; Mon, 12 Feb 2018 08:58:08 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=redhat.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S964854AbeBLP5q (ORCPT + 99 others); Mon, 12 Feb 2018 10:57:46 -0500 Received: from mail-qk0-f194.google.com ([209.85.220.194]:46527 "EHLO mail-qk0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934541AbeBLP5c (ORCPT ); Mon, 12 Feb 2018 10:57:32 -0500 Received: by mail-qk0-f194.google.com with SMTP id g129so4365303qkb.13 for ; Mon, 12 Feb 2018 07:57:32 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=r/3xUcyZjwZoplVy9T9EDM+Bf9ZBu91xHlm7YiXtvPE=; b=rjMKBAg6BkUt5JD9Zs1Nu3DmNx2Edtipn/e2q8pLrD7w03sL1AcZR7IZulFIEPr71m ETVEps2tPc86hXnG8Wwdn7JNVq0f2HgheKhIOjOtlMKlm3BH1udxjl/zkZP3sq8gWFcZ FYWQyhgwBYbsowfG6DbiKgfWUg38wddAH50iegPSCHvU/Vujy8xX8zARuGGMLMiVzEiz wy/rHDmpLb1wVk9e/cZcQaxeDXHl6xIxXmJxTZ8qkw1fLXOBPL9fQiHxFTmQf3zk20Do 7pxK5tA3X37DINaKV3cxKWMAeU26c04qAz72jUXAVJniMcenJ9FnqeFFMzJWYpHF9Eyc fD9A== X-Gm-Message-State: APf1xPBk9zlvi5X8YrZgR7IYy2zwg5yCXYAxjSSxZszhErz32C2zQeyc b5q0QVLTalYSM5eWg5xwFWoB8hMwlpAaTFf2fMlrZw== X-Received: by 10.55.157.76 with SMTP id g73mr7128637qke.107.1518451051664; Mon, 12 Feb 2018 07:57:31 -0800 (PST) MIME-Version: 1.0 Received: by 10.140.95.17 with HTTP; Mon, 12 Feb 2018 07:57:31 -0800 (PST) In-Reply-To: References: From: Miklos Szeredi Date: Mon, 12 Feb 2018 16:57:31 +0100 Message-ID: Subject: Re: [PATCH 08/11] fuse: Support fuse filesystems outside of init_user_ns To: Dongsu Park Cc: lkml , containers@lists.linux-foundation.org, Alban Crequy , "Eric W . Biederman" , Seth Forshee , Sargun Dhillon , linux-fsdevel Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Dec 22, 2017 at 3:32 PM, Dongsu Park wrote: > From: Seth Forshee > > In order to support mounts from namespaces other than > init_user_ns, fuse must translate uids and gids to/from the > userns of the process servicing requests on /dev/fuse. This > patch does that, with a couple of restrictions on the namespace: > > - The userns for the fuse connection is fixed to the namespace > from which /dev/fuse is opened. > > - The namespace must be the same as s_user_ns. > > These restrictions simplify the implementation by avoiding the > need to pass around userns references and by allowing fuse to > rely on the checks in inode_change_ok for ownership changes. > Either restriction could be relaxed in the future if needed. Can we not introduce potential userspace interface regressions? The issue with pid namespaces fixed in commit 5d6d3a301c4e ("fuse: allow server to run in different pid_ns") will probably bite us here as well. We basically need two modes of operation: a) old, backward compatible (not introducing any new failure mores), created with privileged mount b) new, non-backward compatible, created with unprivileged mount Technically there would still be a risk from breaking userspace, since we are using the same entry point for both, but let's hope that no practical problems come from that. > For cuse the namespace used for the connection is also simply > current_user_ns() at the time /dev/cuse is opened. > > Patch v4 is available: https://patchwork.kernel.org/patch/8944661/ > > Cc: linux-fsdevel@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > Cc: Miklos Szeredi > Signed-off-by: Seth Forshee > Signed-off-by: Dongsu Park > --- > fs/fuse/cuse.c | 3 ++- > fs/fuse/dev.c | 11 ++++++++--- > fs/fuse/dir.c | 14 +++++++------- > fs/fuse/fuse_i.h | 6 +++++- > fs/fuse/inode.c | 31 +++++++++++++++++++------------ > 5 files changed, 41 insertions(+), 24 deletions(-) > > diff --git a/fs/fuse/cuse.c b/fs/fuse/cuse.c > index e9e97803..b1b83259 100644 > --- a/fs/fuse/cuse.c > +++ b/fs/fuse/cuse.c > @@ -48,6 +48,7 @@ > #include > #include > #include > +#include > > #include "fuse_i.h" > > @@ -498,7 +499,7 @@ static int cuse_channel_open(struct inode *inode, struct file *file) > if (!cc) > return -ENOMEM; > > - fuse_conn_init(&cc->fc); > + fuse_conn_init(&cc->fc, current_user_ns()); > > fud = fuse_dev_alloc(&cc->fc); > if (!fud) { > diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c > index 17f0d05b..0f780e16 100644 > --- a/fs/fuse/dev.c > +++ b/fs/fuse/dev.c > @@ -114,8 +114,8 @@ static void __fuse_put_request(struct fuse_req *req) > > static void fuse_req_init_context(struct fuse_conn *fc, struct fuse_req *req) > { > - req->in.h.uid = from_kuid_munged(&init_user_ns, current_fsuid()); > - req->in.h.gid = from_kgid_munged(&init_user_ns, current_fsgid()); > + req->in.h.uid = from_kuid(fc->user_ns, current_fsuid()); > + req->in.h.gid = from_kgid(fc->user_ns, current_fsgid()); > req->in.h.pid = pid_nr_ns(task_pid(current), fc->pid_ns); > } > > @@ -167,6 +167,10 @@ static struct fuse_req *__fuse_get_req(struct fuse_conn *fc, unsigned npages, > __set_bit(FR_WAITING, &req->flags); > if (for_background) > __set_bit(FR_BACKGROUND, &req->flags); > + if (req->in.h.uid == (uid_t)-1 || req->in.h.gid == (gid_t)-1) { > + fuse_put_request(fc, req); > + return ERR_PTR(-EOVERFLOW); > + } > > return req; > > @@ -1260,7 +1264,8 @@ static ssize_t fuse_dev_do_read(struct fuse_dev *fud, struct file *file, > in = &req->in; > reqsize = in->h.len; > > - if (task_active_pid_ns(current) != fc->pid_ns) { > + if (task_active_pid_ns(current) != fc->pid_ns || > + current_user_ns() != fc->user_ns) { I don't get it. Why recalculate the pid if the user_ns does not match? > rcu_read_lock(); > in->h.pid = pid_vnr(find_pid_ns(in->h.pid, fc->pid_ns)); > rcu_read_unlock(); > diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c > index 24967382..ad1cfac1 100644 > --- a/fs/fuse/dir.c > +++ b/fs/fuse/dir.c > @@ -858,8 +858,8 @@ static void fuse_fillattr(struct inode *inode, struct fuse_attr *attr, > stat->ino = attr->ino; > stat->mode = (inode->i_mode & S_IFMT) | (attr->mode & 07777); > stat->nlink = attr->nlink; > - stat->uid = make_kuid(&init_user_ns, attr->uid); > - stat->gid = make_kgid(&init_user_ns, attr->gid); > + stat->uid = make_kuid(fc->user_ns, attr->uid); > + stat->gid = make_kgid(fc->user_ns, attr->gid); > stat->rdev = inode->i_rdev; > stat->atime.tv_sec = attr->atime; > stat->atime.tv_nsec = attr->atimensec; > @@ -1475,17 +1475,17 @@ static bool update_mtime(unsigned ivalid, bool trust_local_mtime) > return true; > } > > -static void iattr_to_fattr(struct iattr *iattr, struct fuse_setattr_in *arg, > - bool trust_local_cmtime) > +static void iattr_to_fattr(struct fuse_conn *fc, struct iattr *iattr, > + struct fuse_setattr_in *arg, bool trust_local_cmtime) > { > unsigned ivalid = iattr->ia_valid; > > if (ivalid & ATTR_MODE) > arg->valid |= FATTR_MODE, arg->mode = iattr->ia_mode; > if (ivalid & ATTR_UID) > - arg->valid |= FATTR_UID, arg->uid = from_kuid(&init_user_ns, iattr->ia_uid); > + arg->valid |= FATTR_UID, arg->uid = from_kuid(fc->user_ns, iattr->ia_uid); > if (ivalid & ATTR_GID) > - arg->valid |= FATTR_GID, arg->gid = from_kgid(&init_user_ns, iattr->ia_gid); > + arg->valid |= FATTR_GID, arg->gid = from_kgid(fc->user_ns, iattr->ia_gid); > if (ivalid & ATTR_SIZE) > arg->valid |= FATTR_SIZE, arg->size = iattr->ia_size; > if (ivalid & ATTR_ATIME) { > @@ -1646,7 +1646,7 @@ int fuse_do_setattr(struct dentry *dentry, struct iattr *attr, > > memset(&inarg, 0, sizeof(inarg)); > memset(&outarg, 0, sizeof(outarg)); > - iattr_to_fattr(attr, &inarg, trust_local_cmtime); > + iattr_to_fattr(fc, attr, &inarg, trust_local_cmtime); > if (file) { > struct fuse_file *ff = file->private_data; > inarg.valid |= FATTR_FH; > diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h > index d5773ca6..364e65c8 100644 > --- a/fs/fuse/fuse_i.h > +++ b/fs/fuse/fuse_i.h > @@ -26,6 +26,7 @@ > #include > #include > #include > +#include > > /** Max number of pages that can be used in a single read request */ > #define FUSE_MAX_PAGES_PER_REQ 32 > @@ -466,6 +467,9 @@ struct fuse_conn { > /** The pid namespace for this mount */ > struct pid_namespace *pid_ns; > > + /** The user namespace for this mount */ > + struct user_namespace *user_ns; > + > /** Maximum read size */ > unsigned max_read; > > @@ -870,7 +874,7 @@ struct fuse_conn *fuse_conn_get(struct fuse_conn *fc); > /** > * Initialize fuse_conn > */ > -void fuse_conn_init(struct fuse_conn *fc); > +void fuse_conn_init(struct fuse_conn *fc, struct user_namespace *user_ns); > > /** > * Release reference to fuse_conn > diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c > index 2f504d61..7f6b2e55 100644 > --- a/fs/fuse/inode.c > +++ b/fs/fuse/inode.c > @@ -171,8 +171,8 @@ void fuse_change_attributes_common(struct inode *inode, struct fuse_attr *attr, > inode->i_ino = fuse_squash_ino(attr->ino); > inode->i_mode = (inode->i_mode & S_IFMT) | (attr->mode & 07777); > set_nlink(inode, attr->nlink); > - inode->i_uid = make_kuid(&init_user_ns, attr->uid); > - inode->i_gid = make_kgid(&init_user_ns, attr->gid); > + inode->i_uid = make_kuid(fc->user_ns, attr->uid); > + inode->i_gid = make_kgid(fc->user_ns, attr->gid); > inode->i_blocks = attr->blocks; > inode->i_atime.tv_sec = attr->atime; > inode->i_atime.tv_nsec = attr->atimensec; > @@ -477,7 +477,8 @@ static int fuse_match_uint(substring_t *s, unsigned int *res) > return err; > } > > -static int parse_fuse_opt(char *opt, struct fuse_mount_data *d, int is_bdev) > +static int parse_fuse_opt(char *opt, struct fuse_mount_data *d, int is_bdev, > + struct user_namespace *user_ns) > { > char *p; > memset(d, 0, sizeof(struct fuse_mount_data)); > @@ -513,7 +514,7 @@ static int parse_fuse_opt(char *opt, struct fuse_mount_data *d, int is_bdev) > case OPT_USER_ID: > if (fuse_match_uint(&args[0], &uv)) > return 0; > - d->user_id = make_kuid(current_user_ns(), uv); > + d->user_id = make_kuid(user_ns, uv); > if (!uid_valid(d->user_id)) > return 0; > d->user_id_present = 1; > @@ -522,7 +523,7 @@ static int parse_fuse_opt(char *opt, struct fuse_mount_data *d, int is_bdev) > case OPT_GROUP_ID: > if (fuse_match_uint(&args[0], &uv)) > return 0; > - d->group_id = make_kgid(current_user_ns(), uv); > + d->group_id = make_kgid(user_ns, uv); > if (!gid_valid(d->group_id)) > return 0; > d->group_id_present = 1; > @@ -565,8 +566,8 @@ static int fuse_show_options(struct seq_file *m, struct dentry *root) > struct super_block *sb = root->d_sb; > struct fuse_conn *fc = get_fuse_conn_super(sb); > > - seq_printf(m, ",user_id=%u", from_kuid_munged(&init_user_ns, fc->user_id)); > - seq_printf(m, ",group_id=%u", from_kgid_munged(&init_user_ns, fc->group_id)); > + seq_printf(m, ",user_id=%u", from_kuid_munged(fc->user_ns, fc->user_id)); > + seq_printf(m, ",group_id=%u", from_kgid_munged(fc->user_ns, fc->group_id)); > if (fc->default_permissions) > seq_puts(m, ",default_permissions"); > if (fc->allow_other) > @@ -597,7 +598,7 @@ static void fuse_pqueue_init(struct fuse_pqueue *fpq) > fpq->connected = 1; > } > > -void fuse_conn_init(struct fuse_conn *fc) > +void fuse_conn_init(struct fuse_conn *fc, struct user_namespace *user_ns) > { > memset(fc, 0, sizeof(*fc)); > spin_lock_init(&fc->lock); > @@ -621,6 +622,7 @@ void fuse_conn_init(struct fuse_conn *fc) > fc->attr_version = 1; > get_random_bytes(&fc->scramble_key, sizeof(fc->scramble_key)); > fc->pid_ns = get_pid_ns(task_active_pid_ns(current)); > + fc->user_ns = get_user_ns(user_ns); > } > EXPORT_SYMBOL_GPL(fuse_conn_init); > > @@ -630,6 +632,7 @@ void fuse_conn_put(struct fuse_conn *fc) > if (fc->destroy_req) > fuse_request_free(fc->destroy_req); > put_pid_ns(fc->pid_ns); > + put_user_ns(fc->user_ns); > fc->release(fc); > } > } > @@ -1061,7 +1064,7 @@ static int fuse_fill_super(struct super_block *sb, void *data, int silent) > > sb->s_flags &= ~(MS_NOSEC | SB_I_VERSION); > > - if (!parse_fuse_opt(data, &d, is_bdev)) > + if (!parse_fuse_opt(data, &d, is_bdev, sb->s_user_ns)) > goto err; > > if (is_bdev) { > @@ -1086,8 +1089,12 @@ static int fuse_fill_super(struct super_block *sb, void *data, int silent) > if (!file) > goto err; > > - if ((file->f_op != &fuse_dev_operations) || > - (file->f_cred->user_ns != &init_user_ns)) > + /* > + * Require mount to happen from the same user namespace which > + * opened /dev/fuse to prevent potential attacks. > + */ > + if (file->f_op != &fuse_dev_operations || > + file->f_cred->user_ns != sb->s_user_ns) > goto err_fput; > > fc = kmalloc(sizeof(*fc), GFP_KERNEL); > @@ -1095,7 +1102,7 @@ static int fuse_fill_super(struct super_block *sb, void *data, int silent) > if (!fc) > goto err_fput; > > - fuse_conn_init(fc); > + fuse_conn_init(fc, sb->s_user_ns); > fc->release = fuse_free_conn; > > fud = fuse_dev_alloc(fc); > -- > 2.13.6 >