| Message ID | 1451930639-94331-17-git-send-email-seth.forshee@canonical.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <seth.forshee@canonical.com> > --- > 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 <linux/stat.h> > #include <linux/module.h> > #include <linux/uio.h> > +#include <linux/user_namespace.h> > > #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 <linux/poll.h> > #include <linux/workqueue.h> > #include <linux/pid_namespace.h> > +#include <linux/user_namespace.h> > > /** 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 > -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <seth.forshee@canonical.com> > > --- > > 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 <linux/stat.h> > > #include <linux/module.h> > > #include <linux/uio.h> > > +#include <linux/user_namespace.h> > > > > #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 <linux/poll.h> > > #include <linux/workqueue.h> > > #include <linux/pid_namespace.h> > > +#include <linux/user_namespace.h> > > > > /** 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 > > -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 9, 2016 at 3:18 PM, Seth Forshee <seth.forshee@canonical.com> wrote: > 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: >> > -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? Yes. Can't we use current_cred()->uid/gid? Or fsuid/fsgid maybe? When we have true unprivileged mounts, the user_id/group_id options become redundant anyway and we can just use the current credentials. Thanks, Miklos -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 09, 2016 at 03:48:22PM +0100, Miklos Szeredi wrote: > On Wed, Mar 9, 2016 at 3:18 PM, Seth Forshee <seth.forshee@canonical.com> wrote: > > 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: > > >> > -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? > > Yes. > > Can't we use current_cred()->uid/gid? Or fsuid/fsgid maybe? That would be a departure from the current behavior in the !allow_other case for unprivileged users. Since those mounts are done by an suid helper all of those ids would be root in the userns, wouldn't they? > When we have true unprivileged mounts, the user_id/group_id options > become redundant anyway and we can just use the current credentials. True, but we don't yet have that. Thanks, Seth -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 9, 2016 at 4:25 PM, Seth Forshee <seth.forshee@canonical.com> wrote: > On Wed, Mar 09, 2016 at 03:48:22PM +0100, Miklos Szeredi wrote: >> Can't we use current_cred()->uid/gid? Or fsuid/fsgid maybe? > > That would be a departure from the current behavior in the !allow_other > case for unprivileged users. Since those mounts are done by an suid > helper all of those ids would be root in the userns, wouldn't they? Well, actually this is what the helper does: sprintf(d, "fd=%i,rootmode=%o,user_id=%u,group_id=%u", fd, rootmode, getuid(), getgid()); So it just uses the current uid/gid. Apparently no reason to do this in userland, we could just as well set these in the kernel. Except for possible backward compatibility problems for things not using the helper. BUT if the mount is unprivileged or it's a userns mount, or anything previously not possible, then we are not constrained by the backward compatibility issues, and can go with the saner solution. Does that not make sense? >> When we have true unprivileged mounts, the user_id/group_id options >> become redundant anyway and we can just use the current credentials. > > True, but we don't yet have that. What's missing? Thanks, Miklos -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 09, 2016 at 04:51:42PM +0100, Miklos Szeredi wrote: > On Wed, Mar 9, 2016 at 4:25 PM, Seth Forshee <seth.forshee@canonical.com> wrote: > > On Wed, Mar 09, 2016 at 03:48:22PM +0100, Miklos Szeredi wrote: > > >> Can't we use current_cred()->uid/gid? Or fsuid/fsgid maybe? > > > > That would be a departure from the current behavior in the !allow_other > > case for unprivileged users. Since those mounts are done by an suid > > helper all of those ids would be root in the userns, wouldn't they? > > Well, actually this is what the helper does: > > sprintf(d, "fd=%i,rootmode=%o,user_id=%u,group_id=%u", > fd, rootmode, getuid(), getgid()); Sorry, I was thinking of euid. So this may not be a problem. > So it just uses the current uid/gid. Apparently no reason to do this > in userland, we could just as well set these in the kernel. Except > for possible backward compatibility problems for things not using the > helper. > > BUT if the mount is unprivileged or it's a userns mount, or anything > previously not possible, then we are not constrained by the backward > compatibility issues, and can go with the saner solution. > > Does that not make sense? But we generally do want backwards compatibility, and we want userspace software to be able to expect the same behavior whether or not it's running in a user namespaced container. Obviously we can't always have things 100% identical, but we shouldn't break things unless we really need to. However it may be that this isn't actually going to break assumptions of existing software like I had feared. My preference is still to not change any userspace-visible behaviors since we never know what software might have made assumptions based on those behaviors. But if you're confident that it won't break anything I'm willing to give it a try. > >> When we have true unprivileged mounts, the user_id/group_id options > >> become redundant anyway and we can just use the current credentials. > > > > True, but we don't yet have that. > > What's missing? A user must still be privileged to mount, even if only towards their own user and mount namespaces. Maybe that's what you meant though and I just misunderstood. Thanks, Seth -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 9, 2016 at 6:07 PM, Seth Forshee <seth.forshee@canonical.com> wrote: > On Wed, Mar 09, 2016 at 04:51:42PM +0100, Miklos Szeredi wrote: >> On Wed, Mar 9, 2016 at 4:25 PM, Seth Forshee <seth.forshee@canonical.com> wrote: >> > On Wed, Mar 09, 2016 at 03:48:22PM +0100, Miklos Szeredi wrote: >> >> >> Can't we use current_cred()->uid/gid? Or fsuid/fsgid maybe? >> > >> > That would be a departure from the current behavior in the !allow_other >> > case for unprivileged users. Since those mounts are done by an suid >> > helper all of those ids would be root in the userns, wouldn't they? >> >> Well, actually this is what the helper does: >> >> sprintf(d, "fd=%i,rootmode=%o,user_id=%u,group_id=%u", >> fd, rootmode, getuid(), getgid()); > > Sorry, I was thinking of euid. So this may not be a problem. > >> So it just uses the current uid/gid. Apparently no reason to do this >> in userland, we could just as well set these in the kernel. Except >> for possible backward compatibility problems for things not using the >> helper. >> >> BUT if the mount is unprivileged or it's a userns mount, or anything >> previously not possible, then we are not constrained by the backward >> compatibility issues, and can go with the saner solution. >> >> Does that not make sense? > > But we generally do want backwards compatibility, and we want userspace > software to be able to expect the same behavior whether or not it's > running in a user namespaced container. Obviously we can't always have > things 100% identical, but we shouldn't break things unless we really > need to. > > However it may be that this isn't actually going to break assumptions of > existing software like I had feared. My preference is still to not > change any userspace-visible behaviors since we never know what software > might have made assumptions based on those behaviors. But if you're > confident that it won't break anything I'm willing to give it a try. I'm quite confident it won't make a difference. Thanks, Miklos -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Mar 14, 2016 at 09:58:43PM +0100, Miklos Szeredi wrote: > On Wed, Mar 9, 2016 at 6:07 PM, Seth Forshee <seth.forshee@canonical.com> wrote: > > On Wed, Mar 09, 2016 at 04:51:42PM +0100, Miklos Szeredi wrote: > >> On Wed, Mar 9, 2016 at 4:25 PM, Seth Forshee <seth.forshee@canonical.com> wrote: > >> > On Wed, Mar 09, 2016 at 03:48:22PM +0100, Miklos Szeredi wrote: > >> > >> >> Can't we use current_cred()->uid/gid? Or fsuid/fsgid maybe? > >> > > >> > That would be a departure from the current behavior in the !allow_other > >> > case for unprivileged users. Since those mounts are done by an suid > >> > helper all of those ids would be root in the userns, wouldn't they? > >> > >> Well, actually this is what the helper does: > >> > >> sprintf(d, "fd=%i,rootmode=%o,user_id=%u,group_id=%u", > >> fd, rootmode, getuid(), getgid()); > > > > Sorry, I was thinking of euid. So this may not be a problem. > > > >> So it just uses the current uid/gid. Apparently no reason to do this > >> in userland, we could just as well set these in the kernel. Except > >> for possible backward compatibility problems for things not using the > >> helper. > >> > >> BUT if the mount is unprivileged or it's a userns mount, or anything > >> previously not possible, then we are not constrained by the backward > >> compatibility issues, and can go with the saner solution. > >> > >> Does that not make sense? > > > > But we generally do want backwards compatibility, and we want userspace > > software to be able to expect the same behavior whether or not it's > > running in a user namespaced container. Obviously we can't always have > > things 100% identical, but we shouldn't break things unless we really > > need to. > > > > However it may be that this isn't actually going to break assumptions of > > existing software like I had feared. My preference is still to not > > change any userspace-visible behaviors since we never know what software > > might have made assumptions based on those behaviors. But if you're > > confident that it won't break anything I'm willing to give it a try. > > I'm quite confident it won't make a difference. I was just about to go make these changes and discovered that the user_id and group_id options are already mandatory, due to this check at the bottom of parse_fuse_opt(): if (!d->fd_present || !d->rootmode_present || !d->user_id_present || !d->group_id_present) return 0; So I'll simply drop those two lines which supply default values for these options. Thanks, Seth -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <linux/stat.h> #include <linux/module.h> #include <linux/uio.h> +#include <linux/user_namespace.h> #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; 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 <linux/poll.h> #include <linux/workqueue.h> #include <linux/pid_namespace.h> +#include <linux/user_namespace.h> /** 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); 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);
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 <seth.forshee@canonical.com> --- 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(-)