From mboxrd@z Thu Jan 1 00:00:00 1970 From: Miklos Szeredi Subject: Re: [PATCH RESEND v2 16/18] fuse: Support fuse filesystems outside of init_user_ns Date: Wed, 9 Mar 2016 12:29:23 +0100 Message-ID: <20160309112923.GF8655@tucsk> References: <1451930639-94331-1-git-send-email-seth.forshee@canonical.com> <1451930639-94331-17-git-send-email-seth.forshee@canonical.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <1451930639-94331-17-git-send-email-seth.forshee@canonical.com> Sender: linux-fsdevel-owner@vger.kernel.org To: Seth Forshee Cc: "Eric W. Biederman" , Alexander Viro , Serge Hallyn , Richard Weinberger , Austin S Hemmelgarn , linux-kernel@vger.kernel.org, linux-bcache@vger.kernel.org, dm-devel@redhat.com, linux-raid@vger.kernel.org, linux-mtd@lists.infradead.org, linux-fsdevel@vger.kernel.org, fuse-devel@lists.sourceforge.net, linux-security-module@vger.kernel.org, selinux@tycho.nsa.gov List-Id: linux-raid.ids On Mon, Jan 04, 2016 at 12:03:55PM -0600, Seth Forshee wrote: > 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. > > For cuse the namespace used for the connection is also simply > current_user_ns() at the time /dev/cuse is opened. > > Signed-off-by: Seth Forshee > --- > fs/fuse/cuse.c | 3 ++- > fs/fuse/dev.c | 13 ++++++++----- > fs/fuse/dir.c | 14 +++++++------- > fs/fuse/fuse_i.h | 6 +++++- > fs/fuse/inode.c | 35 +++++++++++++++++++++++------------ > 5 files changed, 45 insertions(+), 26 deletions(-) > > diff --git a/fs/fuse/cuse.c b/fs/fuse/cuse.c > index eae2c11268bc..a10aca57bfe4 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 a4f6f30d6d86..11b4cb0a0e2f 100644 > --- a/fs/fuse/dev.c > +++ b/fs/fuse/dev.c > @@ -127,8 +127,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); > } > > @@ -186,7 +186,8 @@ 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.pid == 0) { > + if (req->in.h.pid == 0 || req->in.h.uid == (uid_t)-1 || > + req->in.h.gid == (gid_t)-1) { > fuse_put_request(fc, req); > return ERR_PTR(-EOVERFLOW); > } > @@ -1248,7 +1249,8 @@ static ssize_t fuse_dev_do_read(struct fuse_dev *fud, struct file *file, > struct fuse_in *in; > unsigned reqsize; > > - if (task_active_pid_ns(current) != fc->pid_ns) > + if (task_active_pid_ns(current) != fc->pid_ns || > + current_user_ns() != fc->user_ns) > return -EIO; > > restart: > @@ -1880,7 +1882,8 @@ static ssize_t fuse_dev_do_write(struct fuse_dev *fud, > struct fuse_req *req; > struct fuse_out_header oh; > > - if (task_active_pid_ns(current) != fc->pid_ns) > + if (task_active_pid_ns(current) != fc->pid_ns || > + current_user_ns() != fc->user_ns) > return -EIO; > > if (nbytes < sizeof(struct fuse_out_header)) > diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c > index 5e2e08712d3b..8fd9fe4dcd43 100644 > --- a/fs/fuse/dir.c > +++ b/fs/fuse/dir.c > @@ -841,8 +841,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 = inode->i_uid; > + stat->gid = inode->i_gid; This breaks the attr_version logic in fuse_change_attributes(). So just use make_k[ug]id() here as well. > stat->rdev = inode->i_rdev; > stat->atime.tv_sec = attr->atime; > stat->atime.tv_nsec = attr->atimensec; > @@ -1455,17 +1455,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) { > @@ -1625,7 +1625,7 @@ int fuse_do_setattr(struct inode *inode, 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 143b595197b6..5897805405ba 100644 > --- a/fs/fuse/fuse_i.h > +++ b/fs/fuse/fuse_i.h > @@ -23,6 +23,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 > @@ -460,6 +461,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; > + > /** The fuse mount flags for this mount */ > unsigned flags; > > @@ -855,7 +859,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 2f31874ea9db..b7bdfdac3521 100644 > --- a/fs/fuse/inode.c > +++ b/fs/fuse/inode.c > @@ -167,8 +167,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; > @@ -467,12 +467,15 @@ 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)); > d->max_read = ~0; > d->blksize = FUSE_DEFAULT_BLKSIZE; > + d->user_id = make_kuid(user_ns, 0); > + d->group_id = make_kgid(user_ns, 0); It is true that if "user_id=" or "group_id" options were omitted we used the zero uid/gid values. However, this isn't actually used by anybody AFAIK, and generalizing it for userns doesn't seem to make much sense. So I suggest we that we instead return an error if mounting from a userns AND neither "allow_other" nor both "user_id" and "group_id" are specified. > > while ((p = strsep(&opt, ",")) != NULL) { > int token; > @@ -503,7 +506,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; > @@ -512,7 +515,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; > @@ -555,8 +558,10 @@ 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->flags & FUSE_DEFAULT_PERMISSIONS) > seq_puts(m, ",default_permissions"); > if (fc->flags & FUSE_ALLOW_OTHER) > @@ -587,7 +592,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); > @@ -611,6 +616,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); > > @@ -620,6 +626,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); > } > } > @@ -1046,7 +1053,7 @@ static int fuse_fill_super(struct super_block *sb, void *data, int silent) > > sb->s_flags &= ~(MS_NOSEC | MS_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) { > @@ -1070,8 +1077,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); > @@ -1079,7 +1090,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); > -- > 1.9.1 >