From mboxrd@z Thu Jan 1 00:00:00 1970 From: ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org (Eric W. Biederman) Subject: Re: [PATCH 08/11] fuse: Support fuse filesystems outside of init_user_ns Date: Mon, 12 Feb 2018 10:35:10 -0600 Message-ID: <87lgfy5fpd.fsf__20789.1147736975$1518453237$gmane$org@xmission.com> References: Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: (Miklos Szeredi's message of "Mon, 12 Feb 2018 16:57:31 +0100") List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: containers-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org Errors-To: containers-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org To: Miklos Szeredi Cc: containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org, lkml , Seth Forshee , Alban Crequy , Sargun Dhillon , linux-fsdevel List-Id: containers.vger.kernel.org Miklos Szeredi writes: > 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. Maybe, but unlike the pid namespace no one has been able to mount fuse outside of init_user_ns so we are much less exposed. I agree we should be careful. > 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. Answering from a 10,000 foot perspective: There are two cases. Requests to read/write the filesystem from outside of s_user_ns. These run no risk of breaking userspace as this mode has not been implemented before. Restrictions at mount time to ensure we are not dealing with a crazy mix of namespaces. This has a small chance of breaking someone's crazy setup. Dropping requests to read/write the filesystem when the requester does not map into s_user_ns should not be a problem to enable universally. If s_user_ns is init_user_ns everything maps so there is no restriction. What we can do if we want to ensure maximum backwards compatibility is if the fuse filesystem is mounted in init_user_ns but if device for the communication channel is opened in some other user namespace we can just force the communication channel to operate in init_user_ns. That will be 100% backwards compatible in all cases and as far as I can see remove the need for having different ``modes'' of operation. This does look like the time to give all of this a hard look and see if we can get these patches in shape to be merged. Eric >> 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org >> Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.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 >>