[PATCH 0/3] fuse: Allow mounts in containers

[PATCH 0/3] fuse: Allow mounts in containers

[Seth Forshee]

These patches allow unprivileged users to mount with fuse from within
containers. The first patch is really just a bug fix and related only
because the bug allows unprivileged users to crash the system. The
second patch translates the pid which is making a request into the
server's pid namespace, and the third adds user namespace support to
fuse. This is limited only to the "fuse" fs type. fuseblk could likely
be supported as well, but I haven't spent any time testing it, and I
haven't really given cuse much consideration at all (though cuse ioctls
look rather frightening).

The server's pid and user namespaces are both assumed to be those of the
process which calls mount. This does't necessarily have to be the same
as those of the server, especially since fuse mounts are routinely done
by a process other than the server. However I didn't find any way to
ensure that we use those of the server with the information currently
available to fuse in the kernel. If the mount is done from a different
namespace it could result in reduced functionality, however it should
not result in any privileges not already available to the user.

In preparing these patches I spent some time considering the security
aspects of allowing fuse mounts from containers. fuse is already
sufficiently untrusting of input from userspace, and it has mechanisms
to prevent several types of attacks. However some of these mechanisms
rely on having a trusted setuid root helper (fusermount) to enforce
policy, such as forcing certain mount options for unprivileged monts. In
a container we can't rely on a userspace helper to enforce policy. Here
are details about how these issues work out:

* devices: fusermount forces nodev for unprivileged mounts. In these
  patches I use the existing kernel support for forcing nodev for fuse
  mounts from user namspaces.

* set[ug]id files: fusermount also forces nosuid for unprivileged
  mounts. In a user namespace all file uids and gids are treated as
  being mapped into the user ns, so it's not possible to setuid to
  anything outside the server's namespace. This means setuid can't be
  used to gain elevated privileges, and thus the kernel doesn't need to
  force nosuid.

* mounting over files or directories: fusermount ensures that the
  unprivileged user has write permissions to the mountpoint before
  mounting. But since mounting is only allowed by CAP_SYS_ADMIN in the
  user ns of the mount ns, a user cannot use a namespace to mount over
  any files or directories unless the user already had the ability to do
  so, or if it does so in a different mount ns. Namespaces therefore
  don't open the door to this type of attack, and kernel enforcment is
  not needed.

* affecting behavior of other users' processes: A user could DoS other
  users' processes if those processes accessed files or directories
  within a fuse mount. For this reason the default behavoior of fuse is
  that only the mount owner can access the filesystem. This can be
  overridden with the allow_other mount option, but fusermount forbids
  this option unless allowed by system policy in /etc/fuse.conf.

  To protect against this, these patches patches change the meaning of
  allow_other slightly, from "any user can access this filesystem" to
  "users in the mount owner's namespace or a child namespace can access
  this filesystem." This protects more privileged contexts while
  maintaining the existing behavior.

* {user,group}_id mount options: These are being mapped into the user
  ns, which prevents specifying any user outside the ns. Any ids which
  do not map to the user ns wil cause the mount to fail.

That represents everything I could think of that would be possible as a
consequence of allowing mounts from user namespaces. I also read through
the fuse kernel code (espeically the parts handling input from
userspace) looking for additional vectors for attack or any other
weaknesses, but I didn't find anything. So I believe these should be all
the changes needed to make fuse mounts from user namespaces safe, but
please let me know if I missed anything.

Thanks,
Seth


Seth Forshee (3):
  fuse/dev: Fix unbalanced calls to kunmap_atomic() during splice I/O
  fuse: Translate pid making a request into the server's pid namespace
  fuse: Allow mounts from user namespaces

 fs/fuse/dev.c    | 19 +++++++++----------
 fs/fuse/dir.c    | 30 +++++++++++++++++++-----------
 fs/fuse/fuse_i.h |  8 ++++++++
 fs/fuse/inode.c  | 21 ++++++++++++++-------
 4 files changed, 50 insertions(+), 28 deletions(-)

[PATCH 1/3] fuse/dev: Fix unbalanced calls to kunmap_atomic() during splice I/O

[Seth Forshee]

fuse_copy_finish() assumes that mapaddr in fuse_copy_state refers
to a valid mapping if currbuf is non-NULL, but this isn't always
true when moving pages for splice I/O. This results in an
unbalanced call to kunmap_atomic() and thus an unbalanced
decrement of the preempt count. Avoid this by checking that
mapaddr is non-NULL before calling kunmap_atomic().

This can be reproduced easily with fusexmp_fh:

  $ mkdir data mount
  $ dd if=/dev/urandom of=data/rand.bin bs=1M count=1
  $ fusexmp_fh -omodules=subdir,subdir=$PWD/data,splice_write,splice_move mount
  $ cat mount/rand.bin >/dev/null

The bug has existed in its current form since 58bda1da4
"fuse/dev: use atomic maps" and fbb32750a "pipe: kill ->map()
and ->unmap()" converted all unmaps to kunmap_atomic() in 3.15.
The fundamental problem of unmapping a page which hasn't been
mapped goes back farther, probably to ce534fb05 "fuse: allow
splice to move pages," but likely with a different impact.

Cc: <stable@vger.kernel.org>
Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
---
 fs/fuse/dev.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index 098f97bdcf1b..219d1e685183 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -666,12 +666,10 @@ static void fuse_copy_finish(struct fuse_copy_state *cs)
 	if (cs->currbuf) {
 		struct pipe_buffer *buf = cs->currbuf;
 
-		if (!cs->write) {
-			kunmap_atomic(cs->mapaddr);
-		} else {
+		if (cs->mapaddr)
 			kunmap_atomic(cs->mapaddr);
+		if (cs->write)
 			buf->len = PAGE_SIZE - cs->len;
-		}
 		cs->currbuf = NULL;
 		cs->mapaddr = NULL;
 	} else if (cs->mapaddr) {
-- 
1.9.1

[PATCH 2/3] fuse: Translate pid making a request into the server's pid namespace

[Seth Forshee]

If the server is executing in a pid namespace then then giving it
the global pid of the process making the request is useless.
Translate the pid into the server's pid namespace.

Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
---
 fs/fuse/dev.c    | 9 +++++----
 fs/fuse/fuse_i.h | 4 ++++
 fs/fuse/inode.c  | 2 ++
 3 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index 219d1e685183..db781ff6392b 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -20,6 +20,7 @@
 #include <linux/swap.h>
 #include <linux/splice.h>
 #include <linux/aio.h>
+#include <linux/sched.h>
 
 MODULE_ALIAS_MISCDEV(FUSE_MINOR);
 MODULE_ALIAS("devname:fuse");
@@ -124,11 +125,11 @@ static void __fuse_put_request(struct fuse_req *req)
 	atomic_dec(&req->count);
 }
 
-static void fuse_req_init_context(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.pid = current->pid;
+	req->in.h.pid = pid_nr_ns(task_pid(current), fc->pid_ns);
 }
 
 static bool fuse_block_alloc(struct fuse_conn *fc, bool for_background)
@@ -168,7 +169,7 @@ static struct fuse_req *__fuse_get_req(struct fuse_conn *fc, unsigned npages,
 		goto out;
 	}
 
-	fuse_req_init_context(req);
+	fuse_req_init_context(fc, req);
 	req->waiting = 1;
 	req->background = for_background;
 	return req;
@@ -257,7 +258,7 @@ struct fuse_req *fuse_get_req_nofail_nopages(struct fuse_conn *fc,
 	if (!req)
 		req = get_reserved_req(fc, file);
 
-	fuse_req_init_context(req);
+	fuse_req_init_context(fc, req);
 	req->waiting = 1;
 	req->background = 0;
 	return req;
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index e8e47a6ab518..a3ded071e2c6 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -22,6 +22,7 @@
 #include <linux/rbtree.h>
 #include <linux/poll.h>
 #include <linux/workqueue.h>
+#include <linux/pid_namespace.h>
 
 /** Max number of pages that can be used in a single read request */
 #define FUSE_MAX_PAGES_PER_REQ 32
@@ -386,6 +387,9 @@ struct fuse_conn {
 	/** The group id for this mount */
 	kgid_t group_id;
 
+	/** The pid namespace for this mount */
+	struct pid_namespace *pid_ns;
+
 	/** The fuse mount flags for this mount */
 	unsigned flags;
 
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 754dcf23de8a..3655152f770d 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -604,6 +604,7 @@ void fuse_conn_init(struct fuse_conn *fc)
 	fc->initialized = 0;
 	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));
 }
 EXPORT_SYMBOL_GPL(fuse_conn_init);
 
@@ -944,6 +945,7 @@ static void fuse_send_init(struct fuse_conn *fc, struct fuse_req *req)
 
 static void fuse_free_conn(struct fuse_conn *fc)
 {
+	put_pid_ns(fc->pid_ns);
 	kfree_rcu(fc, rcu);
 }
 
-- 
1.9.1

[PATCH 3/3] fuse: Allow mounts from user namespaces

[Seth Forshee]

Update fuse to allow mounts from user namespaces. During mount
current_user_ns() is stashed away, and this is used for all uid
and gid mappings. The restriction on mounting from only
init_user_ns is lifted, and the FS_USERNS_MOUNT flag is added to
the flags for the fuse fs type (but not for fuseblk).

The allow_others option becomes a problem when allowing mounts
from user namespaces as it could allow users to override system
policy for this option, however prohibiting this option in
namespaces may limit the utility of fuse in containers. Here we
compromise by permitting allow_other in user namespaces but
restricting filesystem access to users in that namespace and its
children. This gives the expected behavior in containers without
allowing DoS attacks on more priveleged contexts.

Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
---
 fs/fuse/dev.c    |  4 ++--
 fs/fuse/dir.c    | 30 +++++++++++++++++++-----------
 fs/fuse/fuse_i.h |  4 ++++
 fs/fuse/inode.c  | 19 ++++++++++++-------
 4 files changed, 37 insertions(+), 20 deletions(-)

diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index db781ff6392b..18f2d9a0eb30 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_munged(fc->user_ns, current_fsuid());
+	req->in.h.gid = from_kgid_munged(fc->user_ns, current_fsgid());
 	req->in.h.pid = pid_nr_ns(task_pid(current), fc->pid_ns);
 }
 
diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index 42198359fa1b..9895b342b7c0 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -904,8 +904,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;
@@ -1084,12 +1084,20 @@ int fuse_reverse_inval_entry(struct super_block *sb, u64 parent_nodeid,
  */
 int fuse_allow_current_process(struct fuse_conn *fc)
 {
-	const struct cred *cred;
+	const struct cred *cred = current_cred();
 
-	if (fc->flags & FUSE_ALLOW_OTHER)
-		return 1;
+	if (fc->flags & FUSE_ALLOW_OTHER) {
+		if (kuid_has_mapping(fc->user_ns, cred->euid) &&
+		    kuid_has_mapping(fc->user_ns, cred->suid) &&
+		    kuid_has_mapping(fc->user_ns, cred->uid) &&
+		    kgid_has_mapping(fc->user_ns, cred->egid) &&
+		    kgid_has_mapping(fc->user_ns, cred->sgid) &&
+		    kgid_has_mapping(fc->user_ns, cred->gid))
+			return 1;
+
+		return 0;
+	}
 
-	cred = current_cred();
 	if (uid_eq(cred->euid, fc->user_id) &&
 	    uid_eq(cred->suid, fc->user_id) &&
 	    uid_eq(cred->uid,  fc->user_id) &&
@@ -1555,17 +1563,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) {
@@ -1740,7 +1748,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 a3ded071e2c6..2cfd0ca3407a 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -22,6 +22,7 @@
 #include <linux/rbtree.h>
 #include <linux/poll.h>
 #include <linux/workqueue.h>
+#include <linux/user_namespace.h>
 #include <linux/pid_namespace.h>
 
 /** Max number of pages that can be used in a single read request */
@@ -387,6 +388,9 @@ struct fuse_conn {
 	/** The group id for this mount */
 	kgid_t group_id;
 
+	/** The user namespace for this mount */
+	struct user_namespace *user_ns;
+
 	/** The pid namespace for this mount */
 	struct pid_namespace *pid_ns;
 
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 3655152f770d..14fc96e0b408 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -166,8 +166,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;
@@ -484,6 +484,8 @@ static int parse_fuse_opt(char *opt, struct fuse_mount_data *d, int is_bdev)
 	memset(d, 0, sizeof(struct fuse_mount_data));
 	d->max_read = ~0;
 	d->blksize = FUSE_DEFAULT_BLKSIZE;
+	d->user_id = make_kuid(current_user_ns(), 0);
+	d->group_id = make_kgid(current_user_ns(), 0);
 
 	while ((p = strsep(&opt, ",")) != NULL) {
 		int token;
@@ -565,8 +567,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)
@@ -604,6 +608,7 @@ void fuse_conn_init(struct fuse_conn *fc)
 	fc->initialized = 0;
 	fc->attr_version = 1;
 	get_random_bytes(&fc->scramble_key, sizeof(fc->scramble_key));
+	fc->user_ns = get_user_ns(current_user_ns());
 	fc->pid_ns = get_pid_ns(task_active_pid_ns(current));
 }
 EXPORT_SYMBOL_GPL(fuse_conn_init);
@@ -945,6 +950,7 @@ static void fuse_send_init(struct fuse_conn *fc, struct fuse_req *req)
 
 static void fuse_free_conn(struct fuse_conn *fc)
 {
+	put_user_ns(fc->user_ns);
 	put_pid_ns(fc->pid_ns);
 	kfree_rcu(fc, rcu);
 }
@@ -1032,8 +1038,7 @@ 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))
+	if (file->f_op != &fuse_dev_operations)
 		goto err_fput;
 
 	fc = kmalloc(sizeof(*fc), GFP_KERNEL);
@@ -1147,7 +1152,7 @@ static void fuse_kill_sb_anon(struct super_block *sb)
 static struct file_system_type fuse_fs_type = {
 	.owner		= THIS_MODULE,
 	.name		= "fuse",
-	.fs_flags	= FS_HAS_SUBTYPE,
+	.fs_flags	= FS_HAS_SUBTYPE | FS_USERNS_MOUNT,
 	.mount		= fuse_mount,
 	.kill_sb	= fuse_kill_sb_anon,
 };
-- 
1.9.1

Re: [PATCH 1/3] fuse/dev: Fix unbalanced calls to kunmap_atomic() during splice I/O

[Miklos Szeredi]

On Mon, Jul 14, 2014 at 9:18 PM, Seth Forshee
<seth.forshee@canonical.com> wrote:
> fuse_copy_finish() assumes that mapaddr in fuse_copy_state refers
> to a valid mapping if currbuf is non-NULL, but this isn't always
> true when moving pages for splice I/O. This results in an
> unbalanced call to kunmap_atomic() and thus an unbalanced
> decrement of the preempt count. Avoid this by checking that
> mapaddr is non-NULL before calling kunmap_atomic().

I guess this is obsoleted by:

  c55a01d360af  fuse: avoid scheduling while atomic

which moves the kmap/kunmap closer to the actual use of the mapping.

Can you please verify?

Thanks,
Miklos

Re: [PATCH 2/3] fuse: Translate pid making a request into the server's pid namespace

[Miklos Szeredi]

On Mon, Jul 14, 2014 at 9:18 PM, Seth Forshee
<seth.forshee@canonical.com> wrote:
> If the server is executing in a pid namespace then then giving it
> the global pid of the process making the request is useless.
> Translate the pid into the server's pid namespace.
>
> Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> ---
>  fs/fuse/dev.c    | 9 +++++----
>  fs/fuse/fuse_i.h | 4 ++++
>  fs/fuse/inode.c  | 2 ++
>  3 files changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> index 219d1e685183..db781ff6392b 100644
> --- a/fs/fuse/dev.c
> +++ b/fs/fuse/dev.c
> @@ -20,6 +20,7 @@
>  #include <linux/swap.h>
>  #include <linux/splice.h>
>  #include <linux/aio.h>
> +#include <linux/sched.h>
>
>  MODULE_ALIAS_MISCDEV(FUSE_MINOR);
>  MODULE_ALIAS("devname:fuse");
> @@ -124,11 +125,11 @@ static void __fuse_put_request(struct fuse_req *req)
>         atomic_dec(&req->count);
>  }
>
> -static void fuse_req_init_context(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.pid = current->pid;
> +       req->in.h.pid = pid_nr_ns(task_pid(current), fc->pid_ns);

I think this is wrong.  We should store struct pid* in req->in and
translate into the server's pid in fuse_dev_do_read() before we copy
in.h.

Thanks,
Miklos

Re: [PATCH 3/3] fuse: Allow mounts from user namespaces

[Miklos Szeredi]

On Mon, Jul 14, 2014 at 9:18 PM, Seth Forshee
<seth.forshee@canonical.com> wrote:
> Update fuse to allow mounts from user namespaces. During mount
> current_user_ns() is stashed away,

Same thing here.  While practically this may work, it's theoretically
wrong, and possibly may go wrong in special situations.   In fuse
there's no official "server process", so storing information, like
namespace, about one is going to be wrong.

Thanks,
Miklos

Re: [PATCH 1/3] fuse/dev: Fix unbalanced calls to kunmap_atomic() during splice I/O

[Seth Forshee]

On Fri, Jul 18, 2014 at 05:21:55PM +0200, Miklos Szeredi wrote:
> On Mon, Jul 14, 2014 at 9:18 PM, Seth Forshee
> <seth.forshee@canonical.com> wrote:
> > fuse_copy_finish() assumes that mapaddr in fuse_copy_state refers
> > to a valid mapping if currbuf is non-NULL, but this isn't always
> > true when moving pages for splice I/O. This results in an
> > unbalanced call to kunmap_atomic() and thus an unbalanced
> > decrement of the preempt count. Avoid this by checking that
> > mapaddr is non-NULL before calling kunmap_atomic().
> 
> I guess this is obsoleted by:
> 
>   c55a01d360af  fuse: avoid scheduling while atomic
> 
> which moves the kmap/kunmap closer to the actual use of the mapping.
> 
> Can you please verify?

Yes, that commit does seem to fix the problem.

Re: [PATCH 3/3] fuse: Allow mounts from user namespaces

[Seth Forshee]

On Fri, Jul 18, 2014 at 05:33:23PM +0200, Miklos Szeredi wrote:
> On Mon, Jul 14, 2014 at 9:18 PM, Seth Forshee
> <seth.forshee@canonical.com> wrote:
> > Update fuse to allow mounts from user namespaces. During mount
> > current_user_ns() is stashed away,
> 
> Same thing here.  While practically this may work, it's theoretically
> wrong, and possibly may go wrong in special situations.   In fuse
> there's no official "server process", so storing information, like
> namespace, about one is going to be wrong.

What you're suggesting would probably work fine when dealing with pids.
It's not going to work though for the checks I've added in
fuse_allow_current_process() that the process is in the mount owner's
user ns, and without those checks or something similar I don't think
it's safe to permit allow_other for user ns mounts.

I realize that "server process" isn't perfect terminology, it was just
the shorthand I came up with for "the process reading/writing on
/dev/fuse for this super block," which I assumed would be a single
process and certainly not multiple processes in different namespaces.
Can you elaborate on what special situations might violate these
assumptions or otherwise cause problems?

Maybe an alternative approach for the user namspace would be to use the
one which owns the mount namespace of the mount. I'm just not sure off
the top of my head whether or not there's a way for fuse to get to that
information.

Re: [PATCH 3/3] fuse: Allow mounts from user namespaces

[Miklos Szeredi]

On Mon, Jul 21, 2014 at 2:47 PM, Seth Forshee
<seth.forshee@canonical.com> wrote:
> On Fri, Jul 18, 2014 at 05:33:23PM +0200, Miklos Szeredi wrote:
>> On Mon, Jul 14, 2014 at 9:18 PM, Seth Forshee
>> <seth.forshee@canonical.com> wrote:
>> > Update fuse to allow mounts from user namespaces. During mount
>> > current_user_ns() is stashed away,
>>
>> Same thing here.  While practically this may work, it's theoretically
>> wrong, and possibly may go wrong in special situations.   In fuse
>> there's no official "server process", so storing information, like
>> namespace, about one is going to be wrong.
>
> What you're suggesting would probably work fine when dealing with pids.
> It's not going to work though for the checks I've added in
> fuse_allow_current_process() that the process is in the mount owner's
> user ns, and without those checks or something similar I don't think
> it's safe to permit allow_other for user ns mounts.

You can add that check in fuse_dev_do_read() as well.  If the
fsuid/fsgid doesn't exist in the "server's" namespace, then set
req->out.h.error and call request_end().

> Can you elaborate on what special situations might violate these
> assumptions or otherwise cause problems?

What's preventing a fuse fs implementation from handling FUSE_INIT in
one process and then handling the rest in a different process
(possibly in a different namespace)?

Thanks,
Miklos

Re: [PATCH 3/3] fuse: Allow mounts from user namespaces

[Seth Forshee]

On Mon, Jul 21, 2014 at 03:09:14PM +0200, Miklos Szeredi wrote:
> On Mon, Jul 21, 2014 at 2:47 PM, Seth Forshee
> <seth.forshee@canonical.com> wrote:
> > On Fri, Jul 18, 2014 at 05:33:23PM +0200, Miklos Szeredi wrote:
> >> On Mon, Jul 14, 2014 at 9:18 PM, Seth Forshee
> >> <seth.forshee@canonical.com> wrote:
> >> > Update fuse to allow mounts from user namespaces. During mount
> >> > current_user_ns() is stashed away,
> >>
> >> Same thing here.  While practically this may work, it's theoretically
> >> wrong, and possibly may go wrong in special situations.   In fuse
> >> there's no official "server process", so storing information, like
> >> namespace, about one is going to be wrong.
> >
> > What you're suggesting would probably work fine when dealing with pids.
> > It's not going to work though for the checks I've added in
> > fuse_allow_current_process() that the process is in the mount owner's
> > user ns, and without those checks or something similar I don't think
> > it's safe to permit allow_other for user ns mounts.
> 
> You can add that check in fuse_dev_do_read() as well.  If the
> fsuid/fsgid doesn't exist in the "server's" namespace, then set
> req->out.h.error and call request_end().

Okay, that seems like it should work.

> > Can you elaborate on what special situations might violate these
> > assumptions or otherwise cause problems?
> 
> What's preventing a fuse fs implementation from handling FUSE_INIT in
> one process and then handling the rest in a different process
> (possibly in a different namespace)?

Nothing, but I'm having a hard time imagining why that would ever be
useful. The user/group ids passed in the mount options would have to be
mapped into that namespace, otherwise all requests will just fail in the
check you suggest above. The only thing I can think of would be if
someone wanted to proxy mounts trough a process in a more privileged
context, but then the main point of these patches is to make that
unnecessary.

But I also think your approach should work just as well as mine for the
use cases that do make sense to me, so I'll go ahead and give it a try.

Thanks,
Seth

Re: [PATCH 3/3] fuse: Allow mounts from user namespaces

[Eric W. Biederman]

Seth Forshee <seth.forshee@canonical.com> writes:

> On Mon, Jul 21, 2014 at 03:09:14PM +0200, Miklos Szeredi wrote:
>> On Mon, Jul 21, 2014 at 2:47 PM, Seth Forshee
>> <seth.forshee@canonical.com> wrote:
>> > On Fri, Jul 18, 2014 at 05:33:23PM +0200, Miklos Szeredi wrote:
>> >> On Mon, Jul 14, 2014 at 9:18 PM, Seth Forshee
>> >> <seth.forshee@canonical.com> wrote:
>> >> > Update fuse to allow mounts from user namespaces. During mount
>> >> > current_user_ns() is stashed away,
>> >>
>> >> Same thing here.  While practically this may work, it's theoretically
>> >> wrong, and possibly may go wrong in special situations.   In fuse
>> >> there's no official "server process", so storing information, like
>> >> namespace, about one is going to be wrong.
>> >
>> > What you're suggesting would probably work fine when dealing with pids.
>> > It's not going to work though for the checks I've added in
>> > fuse_allow_current_process() that the process is in the mount owner's
>> > user ns, and without those checks or something similar I don't think
>> > it's safe to permit allow_other for user ns mounts.
>> 
>> You can add that check in fuse_dev_do_read() as well.  If the
>> fsuid/fsgid doesn't exist in the "server's" namespace, then set
>> req->out.h.error and call request_end().
>
> Okay, that seems like it should work.
>
>> > Can you elaborate on what special situations might violate these
>> > assumptions or otherwise cause problems?
>> 
>> What's preventing a fuse fs implementation from handling FUSE_INIT in
>> one process and then handling the rest in a different process
>> (possibly in a different namespace)?
>
> Nothing, but I'm having a hard time imagining why that would ever be
> useful. The user/group ids passed in the mount options would have to be
> mapped into that namespace, otherwise all requests will just fail in the
> check you suggest above. The only thing I can think of would be if
> someone wanted to proxy mounts trough a process in a more privileged
> context, but then the main point of these patches is to make that
> unnecessary.
>
> But I also think your approach should work just as well as mine for the
> use cases that do make sense to me, so I'll go ahead and give it a
> try.

A few observations that I don't think I have seen come up in this
thread.

In my earlier experiments with mounting filesystems in other user
namespaces it did wind up making sense to have a notion of this is
the user namespace that things are represented in on disk, and
that wound up covering odd corner cases like acls.  For fuse
I don't recall if any of those corner cases exists.

At the same time my conversion experience also showed that performing
the conversion to/from kuid and kgids as close to the user space
interface as close as possible was the lease error prone and most
secure way of handling things.

For the file descriptors used for talking to a fuse server I would be
inclined to capture a user and pid namespace at open time to use for
your conversions.   This avoids using current and getting into the
problem of file descriptors that change behavior when passed from
process to process.

I definitely agree that using kuids kgids, and struct pid through the
fuse filesystem internally is the least error prone way to go.

Eric

Re: [PATCH 3/3] fuse: Allow mounts from user namespaces

[Seth Forshee]

On Mon, Jul 21, 2014 at 11:02:53AM -0700, Eric W. Biederman wrote:
> Seth Forshee <seth.forshee@canonical.com> writes:
> 
> > On Mon, Jul 21, 2014 at 03:09:14PM +0200, Miklos Szeredi wrote:
> >> On Mon, Jul 21, 2014 at 2:47 PM, Seth Forshee
> >> <seth.forshee@canonical.com> wrote:
> >> > On Fri, Jul 18, 2014 at 05:33:23PM +0200, Miklos Szeredi wrote:
> >> >> On Mon, Jul 14, 2014 at 9:18 PM, Seth Forshee
> >> >> <seth.forshee@canonical.com> wrote:
> >> >> > Update fuse to allow mounts from user namespaces. During mount
> >> >> > current_user_ns() is stashed away,
> >> >>
> >> >> Same thing here.  While practically this may work, it's theoretically
> >> >> wrong, and possibly may go wrong in special situations.   In fuse
> >> >> there's no official "server process", so storing information, like
> >> >> namespace, about one is going to be wrong.
> >> >
> >> > What you're suggesting would probably work fine when dealing with pids.
> >> > It's not going to work though for the checks I've added in
> >> > fuse_allow_current_process() that the process is in the mount owner's
> >> > user ns, and without those checks or something similar I don't think
> >> > it's safe to permit allow_other for user ns mounts.
> >> 
> >> You can add that check in fuse_dev_do_read() as well.  If the
> >> fsuid/fsgid doesn't exist in the "server's" namespace, then set
> >> req->out.h.error and call request_end().
> >
> > Okay, that seems like it should work.
> >
> >> > Can you elaborate on what special situations might violate these
> >> > assumptions or otherwise cause problems?
> >> 
> >> What's preventing a fuse fs implementation from handling FUSE_INIT in
> >> one process and then handling the rest in a different process
> >> (possibly in a different namespace)?
> >
> > Nothing, but I'm having a hard time imagining why that would ever be
> > useful. The user/group ids passed in the mount options would have to be
> > mapped into that namespace, otherwise all requests will just fail in the
> > check you suggest above. The only thing I can think of would be if
> > someone wanted to proxy mounts trough a process in a more privileged
> > context, but then the main point of these patches is to make that
> > unnecessary.
> >
> > But I also think your approach should work just as well as mine for the
> > use cases that do make sense to me, so I'll go ahead and give it a
> > try.
> 
> A few observations that I don't think I have seen come up in this
> thread.
> 
> In my earlier experiments with mounting filesystems in other user
> namespaces it did wind up making sense to have a notion of this is
> the user namespace that things are represented in on disk, and
> that wound up covering odd corner cases like acls.  For fuse
> I don't recall if any of those corner cases exists.
> 
> At the same time my conversion experience also showed that performing
> the conversion to/from kuid and kgids as close to the user space
> interface as close as possible was the lease error prone and most
> secure way of handling things.

My approach and the one suggested by Miklos will both do the conversion
only when copying the ids into/from the structs which are directly
exchanged with userspace, so we're already as close to userspace as
possible. The difference between the approaches is that one does it
before inserting the data into the queue that services userspace reads
whereas the other does it after dequeueing (and thus in the context of
the process doing the read).

> For the file descriptors used for talking to a fuse server I would be
> inclined to capture a user and pid namespace at open time to use for
> your conversions.   This avoids using current and getting into the
> problem of file descriptors that change behavior when passed from
> process to process.

This seems best to me too. It would require restructuring the way fuse
handles file private data for /dev/fuse, but that should be doable.

Miklos, after looking at the code again I noticed that to move all the
id conversions into the context of the userspace process we also have to
handle it for FUSE_GETATTR and FUSE_SETATTR, probably by special casing
these ops in the read/write code. Do you favor this approach or Eric's
suggestion?

Thanks,
Seth

Re: [PATCH 3/3] fuse: Allow mounts from user namespaces

[Seth Forshee]

On Mon, Jul 21, 2014 at 10:30:44PM -0500, Seth Forshee wrote:
> On Mon, Jul 21, 2014 at 11:02:53AM -0700, Eric W. Biederman wrote:
> > Seth Forshee <seth.forshee@canonical.com> writes:
> > 
> > > On Mon, Jul 21, 2014 at 03:09:14PM +0200, Miklos Szeredi wrote:
> > >> On Mon, Jul 21, 2014 at 2:47 PM, Seth Forshee
> > >> <seth.forshee@canonical.com> wrote:
> > >> > On Fri, Jul 18, 2014 at 05:33:23PM +0200, Miklos Szeredi wrote:
> > >> >> On Mon, Jul 14, 2014 at 9:18 PM, Seth Forshee
> > >> >> <seth.forshee@canonical.com> wrote:
> > >> >> > Update fuse to allow mounts from user namespaces. During mount
> > >> >> > current_user_ns() is stashed away,
> > >> >>
> > >> >> Same thing here.  While practically this may work, it's theoretically
> > >> >> wrong, and possibly may go wrong in special situations.   In fuse
> > >> >> there's no official "server process", so storing information, like
> > >> >> namespace, about one is going to be wrong.
> > >> >
> > >> > What you're suggesting would probably work fine when dealing with pids.
> > >> > It's not going to work though for the checks I've added in
> > >> > fuse_allow_current_process() that the process is in the mount owner's
> > >> > user ns, and without those checks or something similar I don't think
> > >> > it's safe to permit allow_other for user ns mounts.
> > >> 
> > >> You can add that check in fuse_dev_do_read() as well.  If the
> > >> fsuid/fsgid doesn't exist in the "server's" namespace, then set
> > >> req->out.h.error and call request_end().
> > >
> > > Okay, that seems like it should work.
> > >
> > >> > Can you elaborate on what special situations might violate these
> > >> > assumptions or otherwise cause problems?
> > >> 
> > >> What's preventing a fuse fs implementation from handling FUSE_INIT in
> > >> one process and then handling the rest in a different process
> > >> (possibly in a different namespace)?
> > >
> > > Nothing, but I'm having a hard time imagining why that would ever be
> > > useful. The user/group ids passed in the mount options would have to be
> > > mapped into that namespace, otherwise all requests will just fail in the
> > > check you suggest above. The only thing I can think of would be if
> > > someone wanted to proxy mounts trough a process in a more privileged
> > > context, but then the main point of these patches is to make that
> > > unnecessary.
> > >
> > > But I also think your approach should work just as well as mine for the
> > > use cases that do make sense to me, so I'll go ahead and give it a
> > > try.
> > 
> > A few observations that I don't think I have seen come up in this
> > thread.
> > 
> > In my earlier experiments with mounting filesystems in other user
> > namespaces it did wind up making sense to have a notion of this is
> > the user namespace that things are represented in on disk, and
> > that wound up covering odd corner cases like acls.  For fuse
> > I don't recall if any of those corner cases exists.
> > 
> > At the same time my conversion experience also showed that performing
> > the conversion to/from kuid and kgids as close to the user space
> > interface as close as possible was the lease error prone and most
> > secure way of handling things.
> 
> My approach and the one suggested by Miklos will both do the conversion
> only when copying the ids into/from the structs which are directly
> exchanged with userspace, so we're already as close to userspace as
> possible. The difference between the approaches is that one does it
> before inserting the data into the queue that services userspace reads
> whereas the other does it after dequeueing (and thus in the context of
> the process doing the read).
> 
> > For the file descriptors used for talking to a fuse server I would be
> > inclined to capture a user and pid namespace at open time to use for
> > your conversions.   This avoids using current and getting into the
> > problem of file descriptors that change behavior when passed from
> > process to process.
> 
> This seems best to me too. It would require restructuring the way fuse
> handles file private data for /dev/fuse, but that should be doable.
> 
> Miklos, after looking at the code again I noticed that to move all the
> id conversions into the context of the userspace process we also have to
> handle it for FUSE_GETATTR and FUSE_SETATTR, probably by special casing
> these ops in the read/write code. Do you favor this approach or Eric's
> suggestion?

Miklos,

I went ahead and tried out both of these approaches. I've pushed code
for both to the fuse-ns-on-open and fuse-ns-on-read branches of
git://kernel.ubuntu.com/sforshee/linux.git. The code isn't polished yet,
but both are working for simple test cases at least.

The code for converting ids and pids in the read/write paths turned out
to be quite a bit more complicated than that for capturing the
namespaces on open. I ended up capturing the struct cred * when filling
in a request to save all the ids needed for the checks and conversions
in fuse_dev_do_read(), but that turned out to be the simpler case.
Converting attributes written from userspace is what accounts for the
bulk of the changes. It didn't really make sense to duplicate some of
the code that processes the data from userspace in the write path, so
instead I captured the struct user_namespace * during the write for use
when processing the outargs. I still need to go back over that code
carefully to verify that it doesn't have any leaks.

I'd appreciate it if you could take a look at these branches and let me
know which way you want to go so I'll know where to focus my time.

Thanks,
Seth

Re: [PATCH 3/3] fuse: Allow mounts from user namespaces

[Miklos Szeredi]

On Fri, Jul 25, 2014 at 9:46 PM, Seth Forshee
<seth.forshee@canonical.com> wrote:

>
> I'd appreciate it if you could take a look at these branches and let me
> know which way you want to go so I'll know where to focus my time.

I'll be offline for the following week, and will have a look after that.

Thanks,
Mikos

Re: [PATCH 3/3] fuse: Allow mounts from user namespaces

[Seth Forshee]

On Sat, Jul 26, 2014 at 06:27:36PM +0200, Miklos Szeredi wrote:
> On Fri, Jul 25, 2014 at 9:46 PM, Seth Forshee
> <seth.forshee@canonical.com> wrote:
> 
> >
> > I'd appreciate it if you could take a look at these branches and let me
> > know which way you want to go so I'll know where to focus my time.
> 
> I'll be offline for the following week, and will have a look after that.

Ping. It's been a couple of weeks now, so I'm wondering if this might
have fallen off your radar.

Thanks,
Seth