Date:	Wed, 9 Mar 2016 08:18:41 -0600
From:	Seth Forshee <seth.forshee@...onical.com>
To:	Miklos Szeredi <miklos@...redi.hu>
Cc:	"Eric W. Biederman" <ebiederm@...ssion.com>,
	Alexander Viro <viro@...iv.linux.org.uk>,
	Serge Hallyn <serge.hallyn@...onical.com>,
	Richard Weinberger <richard.weinberger@...il.com>,
	Austin S Hemmelgarn <ahferroin7@...il.com>,
	linux-kernel@...r.kernel.org, linux-bcache@...r.kernel.org,
	dm-devel@...hat.com, linux-raid@...r.kernel.org,
	linux-mtd@...ts.infradead.org, linux-fsdevel@...r.kernel.org,
	fuse-devel@...ts.sourceforge.net,
	linux-security-module@...r.kernel.org, selinux@...ho.nsa.gov
Subject: Re: [PATCH RESEND v2 16/18] fuse: Support fuse filesystems outside
 of init_user_ns

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@...onical.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
> > 

Powered by blists - more mailing lists