From mboxrd@z Thu Jan 1 00:00:00 1970 From: Seth Forshee Subject: Re: [PATCH RESEND v2 16/18] fuse: Support fuse filesystems outside of init_user_ns Date: Wed, 9 Mar 2016 08:18:41 -0600 Message-ID: <20160309141840.GC23399@ubuntu-xps13> References: <1451930639-94331-1-git-send-email-seth.forshee@canonical.com> <1451930639-94331-17-git-send-email-seth.forshee@canonical.com> <20160309112923.GF8655@tucsk> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <20160309112923.GF8655@tucsk> Sender: linux-bcache-owner@vger.kernel.org To: Miklos Szeredi 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 Wed, Mar 09, 2016 at 12:29:23PM +0100, Miklos Szeredi wrote: > 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. Okay. > > 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. But those are also used for ownership of the connection files in fusectl. In an allow_other mount shouldn't those files by owned by namespace root and not global root? > > 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 > >