Re: [fuse-devel] fuse_get_context() and namespaces

Re: [fuse-devel] fuse_get_context() and namespaces

[Miklos Szeredi]

On Sat, May 2, 2015 at 5:56 PM,  <alexey@kurnosov.spb.ru> wrote:
>
> 3.10.0-229 form Scientific Linux and native 4.0.1-1 (from elrepo).
> SL 7.1 on the host and SL 6.6 on the LXC guest. At least in 3.10
> the 499dcf2024092e5cce41d05599a5b51d1f92031a is present.
> Steps to reproduce:
>
> On first console:
> [root@sl7test ~]# lxc-start  -n test-2 /bin/su -
> [root@test-2 ~]# diff -u  hello.py /usr/share/doc/fuse-python-0.2.1/example/hello.py
> --- hello.py    2015-05-02 11:12:13.963093580 -0400
> +++ /usr/share/doc/fuse-python-0.2.1/example/hello.py   2010-04-14 18:29:21.000000000 -0400
> @@ -41,8 +41,6 @@
>  class HelloFS(Fuse):
>
>      def getattr(self, path):
> -        dic = Fuse.GetContext(self)
> -        print dic
>          st = MyStat()
>          if path == '/':
>              st.st_mode = stat.S_IFDIR | 0755
> [root@test-2 ~]# python hello.py -f  /mnt/
>
> On second console:
> [root@test-2 ~]# echo $$
> 41
> [root@test-2 ~]# ls /mnt/
> hello
>
> Output of first console:
> {'gid': 0, 'pid': 12083, 'uid': 0}

Thanks.

Digging in mailbox...  There was a thread last year about adding
support for running fuse daemon in a container:

  http://thread.gmane.org/gmane.linux.kernel/1811658

Not sure what happened, but no updated patches have been posted or
maybe I just missed them.

Anyway... adding parties of that discussion to the Cc.

Thanks,
Miklos


>
>
> On Tue, Apr 14, 2015 at 10:23:50AM +0200, Miklos Szeredi wrote:
>> On Wed, Apr 1, 2015 at 5:55 PM,  <alexey@kurnosov.spb.ru> wrote:
>> >
>> > Nobody have a clue?
>> > Who is on FUSE support now?
>> >
>> > --
>> > Alexey Kurnosov
>> >
>> > On Tue, Mar 31, 2015 at 04:14:23AM +0300, alexey@kurnosov.spb.ru wrote:
>> >>
>> >> Hi All.
>> >>
>> >> In my application there is a need to filter access by PID, so i use
>> >> fuse_get_context() (over python bindings actually). The problem come
>> >> when the application runs in a LXC container, and in a separate PID
>> >> namespace (https://lwn.net/Articles/531419/) as result. fuse_get_context()
>> >> returns a caller PID in a _host_'s namespace, not in a container. Not taking
>> >> apart the fact there is broken something in namespaces isolation, is this
>> >> a correct behavior? Shouldn't FUSE be namespaces aware?  Is there a way to
>> >> get PIDs in a container's PID namespace? Maybe some workaround?
>>
>> Which kernel?  There was a fix that went in v3.8:
>>
>> commit 499dcf2024092e5cce41d05599a5b51d1f92031a
>> Author: Eric W. Biederman <ebiederm@xmission.com>
>> Date:   Tue Feb 7 16:26:03 2012 -0800
>>
>>     userns: Support fuse interacting with multiple user namespaces
>>
>>
>> Thanks,
>> Miklos
>
> --
> Alexey Kurnosov

Re: [fuse-devel] fuse_get_context() and namespaces

[Seth Forshee]

On Fri, May 22, 2015 at 04:23:55PM +0200, Miklos Szeredi wrote:
> On Sat, May 2, 2015 at 5:56 PM,  <alexey@kurnosov.spb.ru> wrote:
> >
> > 3.10.0-229 form Scientific Linux and native 4.0.1-1 (from elrepo).
> > SL 7.1 on the host and SL 6.6 on the LXC guest. At least in 3.10
> > the 499dcf2024092e5cce41d05599a5b51d1f92031a is present.
> > Steps to reproduce:
> >
> > On first console:
> > [root@sl7test ~]# lxc-start  -n test-2 /bin/su -
> > [root@test-2 ~]# diff -u  hello.py /usr/share/doc/fuse-python-0.2.1/example/hello.py
> > --- hello.py    2015-05-02 11:12:13.963093580 -0400
> > +++ /usr/share/doc/fuse-python-0.2.1/example/hello.py   2010-04-14 18:29:21.000000000 -0400
> > @@ -41,8 +41,6 @@
> >  class HelloFS(Fuse):
> >
> >      def getattr(self, path):
> > -        dic = Fuse.GetContext(self)
> > -        print dic
> >          st = MyStat()
> >          if path == '/':
> >              st.st_mode = stat.S_IFDIR | 0755
> > [root@test-2 ~]# python hello.py -f  /mnt/
> >
> > On second console:
> > [root@test-2 ~]# echo $$
> > 41
> > [root@test-2 ~]# ls /mnt/
> > hello
> >
> > Output of first console:
> > {'gid': 0, 'pid': 12083, 'uid': 0}
> 
> Thanks.
> 
> Digging in mailbox...  There was a thread last year about adding
> support for running fuse daemon in a container:
> 
>   http://thread.gmane.org/gmane.linux.kernel/1811658
> 
> Not sure what happened, but no updated patches have been posted or
> maybe I just missed them.

I haven't sent updated patches in a while. I still intend to, but I
shifted focus to first getting general support for mounts from user
namespaces into the vfs (which will give a clearer direction for some of
the concerns raised about the fuse patches).

All of this code is available in the userns-mounts branch of
git://kernel.ubuntu.com/sforshee/linux.git, and I don't think the fuse
patches actually depend on any of the stuff that precedes them. I'm
planning to start submitting some of the earlier patches from that
branch soon, and eventually get back to resubmitting the fuse patches.

This is about pid namespaces though, and the fuse pid namespace patch
from that series (see below) should be more or less independent of the
rest of the patches. Potentially that could be merged separately from
the user namespae stuff.

Seth

---

>From 8f49164266e85103a6e95759644c6c343266eb9e Mon Sep 17 00:00:00 2001
From: Seth Forshee <seth.forshee@canonical.com>
Date: Wed, 2 Jul 2014 16:29:19 -0500
Subject: [PATCH] fuse: Add support for pid namespaces

If the userspace process servicing fuse requests is running in
a pid namespace then pids passed via the fuse fd need to be
translated relative to that namespace. Capture the pid namespace
in use when the filesystem is mounted and use this for pid
translation.

As with user namespaces, since no use case currently exists for
changing namespaces so all translations are done relative to the
pid namespace in use when /dev/fuse is opened. Mounting or
/dev/fuse IO from another namespace will return errors. This
restriction can be relaxed at a later time if needed.

File locking changes based on previous work done by Eric
Biederman.

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

diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index 2ee0a7376e0e..627cbdbc63a3 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -19,6 +19,7 @@
 #include <linux/pipe_fs_i.h>
 #include <linux/swap.h>
 #include <linux/splice.h>
+#include <linux/sched.h>
 
 MODULE_ALIAS_MISCDEV(FUSE_MINOR);
 MODULE_ALIAS("devname:fuse");
@@ -127,7 +128,7 @@ static void fuse_req_init_context(struct fuse_conn *fc, struct fuse_req *req)
 {
 	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 = current->pid;
+	req->in.h.pid = pid_nr_ns(task_pid(current), fc->pid_ns);
 }
 
 void fuse_set_initialized(struct fuse_conn *fc)
@@ -1124,7 +1125,8 @@ __releases(fc->lock)
 	arg.unique = req->in.h.unique;
 
 	spin_unlock(&fc->lock);
-	if (fc->user_ns != current_user_ns())
+	if (fc->user_ns != current_user_ns() ||
+	    fc->pid_ns != task_active_pid_ns(current))
 		return -EIO;
 	if (nbytes < reqsize)
 		return -EINVAL;
@@ -1178,7 +1180,8 @@ __releases(fc->lock)
 
 	spin_unlock(&fc->lock);
 	kfree(forget);
-	if (fc->user_ns != current_user_ns())
+	if (fc->user_ns != current_user_ns() ||
+	    fc->pid_ns != task_active_pid_ns(current))
 		return -EIO;
 	if (nbytes < ih.len)
 		return -EINVAL;
@@ -1218,7 +1221,8 @@ __releases(fc->lock)
 	head = dequeue_forget(fc, max_forgets, &count);
 	spin_unlock(&fc->lock);
 
-	if (fc->user_ns != current_user_ns())
+	if (fc->user_ns != current_user_ns() ||
+	    fc->pid_ns != task_active_pid_ns(current))
 		return -EIO;
 
 	arg.count = count;
@@ -1306,7 +1310,8 @@ static ssize_t fuse_dev_do_read(struct fuse_conn *fc, struct file *file,
 	req->state = FUSE_REQ_READING;
 	list_move(&req->list, &fc->io);
 
-	if (fc->user_ns != current_user_ns()) {
+	if (fc->user_ns != current_user_ns() ||
+	    fc->pid_ns != task_active_pid_ns(current)) {
 		req->out.h.error = -EIO;
 		request_end(fc, req);
 		return -EPERM;
diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 5ef05b5c4cff..126b1568dc4c 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -2061,7 +2061,8 @@ static int fuse_direct_mmap(struct file *file, struct vm_area_struct *vma)
 	return generic_file_mmap(file, vma);
 }
 
-static int convert_fuse_file_lock(const struct fuse_file_lock *ffl,
+static int convert_fuse_file_lock(struct fuse_conn *fc,
+				  const struct fuse_file_lock *ffl,
 				  struct file_lock *fl)
 {
 	switch (ffl->type) {
@@ -2076,7 +2077,15 @@ static int convert_fuse_file_lock(const struct fuse_file_lock *ffl,
 
 		fl->fl_start = ffl->start;
 		fl->fl_end = ffl->end;
-		fl->fl_pid = ffl->pid;
+
+		/*
+		 * Convert pid into the connection's pid namespace. If the
+		 * pid does not map into the namespace fl_pid will get set
+		 * to 0.
+		 */
+		rcu_read_lock();
+		fl->fl_pid = pid_vnr(find_pid_ns(ffl->pid, fc->pid_ns));
+		rcu_read_unlock();
 		break;
 
 	default:
@@ -2125,7 +2134,7 @@ static int fuse_getlk(struct file *file, struct file_lock *fl)
 	args.out.args[0].value = &outarg;
 	err = fuse_simple_request(fc, &args);
 	if (!err)
-		err = convert_fuse_file_lock(&outarg.lk, fl);
+		err = convert_fuse_file_lock(fc, &outarg.lk, fl);
 
 	return err;
 }
@@ -2137,7 +2146,8 @@ static int fuse_setlk(struct file *file, struct file_lock *fl, int flock)
 	FUSE_ARGS(args);
 	struct fuse_lk_in inarg;
 	int opcode = (fl->fl_flags & FL_SLEEP) ? FUSE_SETLKW : FUSE_SETLK;
-	pid_t pid = fl->fl_type != F_UNLCK ? current->tgid : 0;
+	struct pid *pid = fl->fl_type != F_UNLCK ? task_tgid(current) : NULL;
+	pid_t pid_nr = pid_nr_ns(pid, fc->pid_ns);
 	int err;
 
 	if (fl->fl_lmops && fl->fl_lmops->lm_grant) {
@@ -2149,7 +2159,10 @@ static int fuse_setlk(struct file *file, struct file_lock *fl, int flock)
 	if (fl->fl_flags & FL_CLOSE)
 		return 0;
 
-	fuse_lk_fill(&args, file, fl, opcode, pid, flock, &inarg);
+	if (pid && pid_nr == 0)
+		return -EOVERFLOW;
+
+	fuse_lk_fill(&args, file, fl, opcode, pid_nr, flock, &inarg);
 	err = fuse_simple_request(fc, &args);
 
 	/* locking is restartable */
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index f6eddd8a2e06..3b7a301c0999 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/user_namespace.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
@@ -406,6 +407,9 @@ struct fuse_conn {
 	/** The user namespace for this mount */
 	struct user_namespace *user_ns;
 
+	/** 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 0e2a35507f1a..d190ce2c9d8f 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -20,6 +20,7 @@
 #include <linux/random.h>
 #include <linux/sched.h>
 #include <linux/exportfs.h>
+#include <linux/pid_namespace.h>
 
 MODULE_AUTHOR("Miklos Szeredi <miklos@szeredi.hu>");
 MODULE_DESCRIPTION("Filesystem in Userspace");
@@ -627,6 +628,7 @@ void fuse_conn_init(struct fuse_conn *fc)
 	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);
 
@@ -636,6 +638,7 @@ void fuse_conn_put(struct fuse_conn *fc)
 		if (fc->destroy_req)
 			fuse_request_free(fc->destroy_req);
 		put_user_ns(fc->user_ns);
+		put_pid_ns(fc->pid_ns);
 		fc->release(fc);
 	}
 }

Re: [fuse-devel] fuse_get_context() and namespaces

[Eric W. Biederman]

Miklos Szeredi <miklos@szeredi.hu> writes:

> On Sat, May 2, 2015 at 5:56 PM,  <alexey@kurnosov.spb.ru> wrote:
>>
>> 3.10.0-229 form Scientific Linux and native 4.0.1-1 (from elrepo).
>> SL 7.1 on the host and SL 6.6 on the LXC guest. At least in 3.10
>> the 499dcf2024092e5cce41d05599a5b51d1f92031a is present.
>> Steps to reproduce:
>>
>> On first console:
>> [root@sl7test ~]# lxc-start  -n test-2 /bin/su -
>> [root@test-2 ~]# diff -u  hello.py /usr/share/doc/fuse-python-0.2.1/example/hello.py
>> --- hello.py    2015-05-02 11:12:13.963093580 -0400
>> +++ /usr/share/doc/fuse-python-0.2.1/example/hello.py   2010-04-14 18:29:21.000000000 -0400
>> @@ -41,8 +41,6 @@
>>  class HelloFS(Fuse):
>>
>>      def getattr(self, path):
>> -        dic = Fuse.GetContext(self)
>> -        print dic
>>          st = MyStat()
>>          if path == '/':
>>              st.st_mode = stat.S_IFDIR | 0755
>> [root@test-2 ~]# python hello.py -f  /mnt/
>>
>> On second console:
>> [root@test-2 ~]# echo $$
>> 41
>> [root@test-2 ~]# ls /mnt/
>> hello
>>
>> Output of first console:
>> {'gid': 0, 'pid': 12083, 'uid': 0}
>
> Thanks.
>
> Digging in mailbox...  There was a thread last year about adding
> support for running fuse daemon in a container:
>
>   http://thread.gmane.org/gmane.linux.kernel/1811658
>
> Not sure what happened, but no updated patches have been posted or
> maybe I just missed them.

We had a discussion and decided to sort out and move as much
functionality as possible into the VFS before proceeding with fuse.
That way there are less weird corner cases to deal with in the review of
the fuse changes.

> Anyway... adding parties of that discussion to the Cc.

It is taking me a bit of work to have enough context to understand the
concern.

It seems user namespaces and unprivileged mounts are not in play which
is what Seth and I were primariliy focusing on.  So we do not have the
tricky privilege checks.

Looking at the reproducer above it appears that the issue is mounting a
fuse filesystem with global root permissions in a pid namespace.

The semantically correct behavior is to return pids to the fuse
filesystem that are in the namespace of the mounter of the fuse
filesystem, and clearly we are not doing that currently.

There are good ways and bad ways of doing that, the good ways don't
involve taking refcounts on struct pid.    I will take a look shortly
and review Seth's patch and see how well it does.  With a little luck
this should be a non-controversial fix.

Eric

Re: [fuse-devel] fuse_get_context() and namespaces

[Eric W. Biederman]

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

> On Fri, May 22, 2015 at 04:23:55PM +0200, Miklos Szeredi wrote:
>> On Sat, May 2, 2015 at 5:56 PM,  <alexey@kurnosov.spb.ru> wrote:
>> >
>> > 3.10.0-229 form Scientific Linux and native 4.0.1-1 (from elrepo).
>> > SL 7.1 on the host and SL 6.6 on the LXC guest. At least in 3.10
>> > the 499dcf2024092e5cce41d05599a5b51d1f92031a is present.
>> > Steps to reproduce:
>> >
>> > On first console:
>> > [root@sl7test ~]# lxc-start  -n test-2 /bin/su -
>> > [root@test-2 ~]# diff -u  hello.py /usr/share/doc/fuse-python-0.2.1/example/hello.py
>> > --- hello.py    2015-05-02 11:12:13.963093580 -0400
>> > +++ /usr/share/doc/fuse-python-0.2.1/example/hello.py   2010-04-14 18:29:21.000000000 -0400
>> > @@ -41,8 +41,6 @@
>> >  class HelloFS(Fuse):
>> >
>> >      def getattr(self, path):
>> > -        dic = Fuse.GetContext(self)
>> > -        print dic
>> >          st = MyStat()
>> >          if path == '/':
>> >              st.st_mode = stat.S_IFDIR | 0755
>> > [root@test-2 ~]# python hello.py -f  /mnt/
>> >
>> > On second console:
>> > [root@test-2 ~]# echo $$
>> > 41
>> > [root@test-2 ~]# ls /mnt/
>> > hello
>> >
>> > Output of first console:
>> > {'gid': 0, 'pid': 12083, 'uid': 0}
>> 
>> Thanks.
>> 
>> Digging in mailbox...  There was a thread last year about adding
>> support for running fuse daemon in a container:
>> 
>>   http://thread.gmane.org/gmane.linux.kernel/1811658
>> 
>> Not sure what happened, but no updated patches have been posted or
>> maybe I just missed them.
>
> I haven't sent updated patches in a while. I still intend to, but I
> shifted focus to first getting general support for mounts from user
> namespaces into the vfs (which will give a clearer direction for some of
> the concerns raised about the fuse patches).
>
> All of this code is available in the userns-mounts branch of
> git://kernel.ubuntu.com/sforshee/linux.git, and I don't think the fuse
> patches actually depend on any of the stuff that precedes them. I'm
> planning to start submitting some of the earlier patches from that
> branch soon, and eventually get back to resubmitting the fuse patches.
>
> This is about pid namespaces though, and the fuse pid namespace patch
> from that series (see below) should be more or less independent of the
> rest of the patches. Potentially that could be merged separately from
> the user namespae stuff.

[snip]

> @@ -2076,7 +2077,15 @@ static int convert_fuse_file_lock(const struct fuse_file_lock *ffl,
>  
>  		fl->fl_start = ffl->start;
>  		fl->fl_end = ffl->end;
> -		fl->fl_pid = ffl->pid;
> +
> +		/*
> +		 * Convert pid into the connection's pid namespace. If the
> +		 * pid does not map into the namespace fl_pid will get set
> +		 * to 0.
> +		 */
> +		rcu_read_lock();
> +		fl->fl_pid = pid_vnr(find_pid_ns(ffl->pid, fc->pid_ns));
> +		rcu_read_unlock();

Scratches my head.  This looks wrong.

I would have expected pid_nr_ns.  Am I missing something reading this
patch quickly?

Eric

Re: [fuse-devel] fuse_get_context() and namespaces

[Seth Forshee]

On Fri, May 22, 2015 at 12:44:35PM -0500, Eric W. Biederman wrote:
> Seth Forshee <seth.forshee@canonical.com> writes:
> 
> > On Fri, May 22, 2015 at 04:23:55PM +0200, Miklos Szeredi wrote:
> >> On Sat, May 2, 2015 at 5:56 PM,  <alexey@kurnosov.spb.ru> wrote:
> >> >
> >> > 3.10.0-229 form Scientific Linux and native 4.0.1-1 (from elrepo).
> >> > SL 7.1 on the host and SL 6.6 on the LXC guest. At least in 3.10
> >> > the 499dcf2024092e5cce41d05599a5b51d1f92031a is present.
> >> > Steps to reproduce:
> >> >
> >> > On first console:
> >> > [root@sl7test ~]# lxc-start  -n test-2 /bin/su -
> >> > [root@test-2 ~]# diff -u  hello.py /usr/share/doc/fuse-python-0.2.1/example/hello.py
> >> > --- hello.py    2015-05-02 11:12:13.963093580 -0400
> >> > +++ /usr/share/doc/fuse-python-0.2.1/example/hello.py   2010-04-14 18:29:21.000000000 -0400
> >> > @@ -41,8 +41,6 @@
> >> >  class HelloFS(Fuse):
> >> >
> >> >      def getattr(self, path):
> >> > -        dic = Fuse.GetContext(self)
> >> > -        print dic
> >> >          st = MyStat()
> >> >          if path == '/':
> >> >              st.st_mode = stat.S_IFDIR | 0755
> >> > [root@test-2 ~]# python hello.py -f  /mnt/
> >> >
> >> > On second console:
> >> > [root@test-2 ~]# echo $$
> >> > 41
> >> > [root@test-2 ~]# ls /mnt/
> >> > hello
> >> >
> >> > Output of first console:
> >> > {'gid': 0, 'pid': 12083, 'uid': 0}
> >> 
> >> Thanks.
> >> 
> >> Digging in mailbox...  There was a thread last year about adding
> >> support for running fuse daemon in a container:
> >> 
> >>   http://thread.gmane.org/gmane.linux.kernel/1811658
> >> 
> >> Not sure what happened, but no updated patches have been posted or
> >> maybe I just missed them.
> >
> > I haven't sent updated patches in a while. I still intend to, but I
> > shifted focus to first getting general support for mounts from user
> > namespaces into the vfs (which will give a clearer direction for some of
> > the concerns raised about the fuse patches).
> >
> > All of this code is available in the userns-mounts branch of
> > git://kernel.ubuntu.com/sforshee/linux.git, and I don't think the fuse
> > patches actually depend on any of the stuff that precedes them. I'm
> > planning to start submitting some of the earlier patches from that
> > branch soon, and eventually get back to resubmitting the fuse patches.
> >
> > This is about pid namespaces though, and the fuse pid namespace patch
> > from that series (see below) should be more or less independent of the
> > rest of the patches. Potentially that could be merged separately from
> > the user namespae stuff.
> 
> [snip]
> 
> > @@ -2076,7 +2077,15 @@ static int convert_fuse_file_lock(const struct fuse_file_lock *ffl,
> >  
> >  		fl->fl_start = ffl->start;
> >  		fl->fl_end = ffl->end;
> > -		fl->fl_pid = ffl->pid;
> > +
> > +		/*
> > +		 * Convert pid into the connection's pid namespace. If the
> > +		 * pid does not map into the namespace fl_pid will get set
> > +		 * to 0.
> > +		 */
> > +		rcu_read_lock();
> > +		fl->fl_pid = pid_vnr(find_pid_ns(ffl->pid, fc->pid_ns));
> > +		rcu_read_unlock();
> 
> Scratches my head.  This looks wrong.
> 
> I would have expected pid_nr_ns.  Am I missing something reading this
> patch quickly?

Here we're in the context of a F_GETLK operation. We've requested the
lock information from the fuse process, and ffl->pid is the pid number
in that process's pid namespace so it needs to be translated into
current's namespace. First we have to look up the struct pid, then
pid_vnr is just a wrapper for pid_nr_ns in the current pid namespace:

  pid_t pid_vnr(struct pid *pid)
  {
          return pid_nr_ns(pid, task_active_pid_ns(current));
  }

Oh, but the comment is wrong, so maybe that's what confused you.
s/connection/caller/ there and it should make more sense.

Seth

Re: [fuse-devel] fuse_get_context() and namespaces

[Miklos Szeredi]

On Fri, May 22, 2015 at 01:59:32PM -0500, Seth Forshee wrote:
> On Fri, May 22, 2015 at 12:44:35PM -0500, Eric W. Biederman wrote:
> > Seth Forshee <seth.forshee@canonical.com> writes:
> > 
> > > On Fri, May 22, 2015 at 04:23:55PM +0200, Miklos Szeredi wrote:
> > >> On Sat, May 2, 2015 at 5:56 PM,  <alexey@kurnosov.spb.ru> wrote:
> > >> >
> > >> > 3.10.0-229 form Scientific Linux and native 4.0.1-1 (from elrepo).
> > >> > SL 7.1 on the host and SL 6.6 on the LXC guest. At least in 3.10
> > >> > the 499dcf2024092e5cce41d05599a5b51d1f92031a is present.
> > >> > Steps to reproduce:
> > >> >
> > >> > On first console:
> > >> > [root@sl7test ~]# lxc-start  -n test-2 /bin/su -
> > >> > [root@test-2 ~]# diff -u  hello.py /usr/share/doc/fuse-python-0.2.1/example/hello.py
> > >> > --- hello.py    2015-05-02 11:12:13.963093580 -0400
> > >> > +++ /usr/share/doc/fuse-python-0.2.1/example/hello.py   2010-04-14 18:29:21.000000000 -0400
> > >> > @@ -41,8 +41,6 @@
> > >> >  class HelloFS(Fuse):
> > >> >
> > >> >      def getattr(self, path):
> > >> > -        dic = Fuse.GetContext(self)
> > >> > -        print dic
> > >> >          st = MyStat()
> > >> >          if path == '/':
> > >> >              st.st_mode = stat.S_IFDIR | 0755
> > >> > [root@test-2 ~]# python hello.py -f  /mnt/
> > >> >
> > >> > On second console:
> > >> > [root@test-2 ~]# echo $$
> > >> > 41
> > >> > [root@test-2 ~]# ls /mnt/
> > >> > hello
> > >> >
> > >> > Output of first console:
> > >> > {'gid': 0, 'pid': 12083, 'uid': 0}
> > >> 
> > >> Thanks.
> > >> 
> > >> Digging in mailbox...  There was a thread last year about adding
> > >> support for running fuse daemon in a container:
> > >> 
> > >>   http://thread.gmane.org/gmane.linux.kernel/1811658
> > >> 
> > >> Not sure what happened, but no updated patches have been posted or
> > >> maybe I just missed them.
> > >
> > > I haven't sent updated patches in a while. I still intend to, but I
> > > shifted focus to first getting general support for mounts from user
> > > namespaces into the vfs (which will give a clearer direction for some of
> > > the concerns raised about the fuse patches).
> > >
> > > All of this code is available in the userns-mounts branch of
> > > git://kernel.ubuntu.com/sforshee/linux.git, and I don't think the fuse
> > > patches actually depend on any of the stuff that precedes them. I'm
> > > planning to start submitting some of the earlier patches from that
> > > branch soon, and eventually get back to resubmitting the fuse patches.
> > >
> > > This is about pid namespaces though, and the fuse pid namespace patch
> > > from that series (see below) should be more or less independent of the
> > > rest of the patches. Potentially that could be merged separately from
> > > the user namespae stuff.
> > 
> > [snip]
> > 
> > > @@ -2076,7 +2077,15 @@ static int convert_fuse_file_lock(const struct fuse_file_lock *ffl,
> > >  
> > >  		fl->fl_start = ffl->start;
> > >  		fl->fl_end = ffl->end;
> > > -		fl->fl_pid = ffl->pid;
> > > +
> > > +		/*
> > > +		 * Convert pid into the connection's pid namespace. If the
> > > +		 * pid does not map into the namespace fl_pid will get set
> > > +		 * to 0.
> > > +		 */
> > > +		rcu_read_lock();
> > > +		fl->fl_pid = pid_vnr(find_pid_ns(ffl->pid, fc->pid_ns));
> > > +		rcu_read_unlock();
> > 
> > Scratches my head.  This looks wrong.
> > 
> > I would have expected pid_nr_ns.  Am I missing something reading this
> > patch quickly?
> 
> Here we're in the context of a F_GETLK operation. We've requested the
> lock information from the fuse process, and ffl->pid is the pid number
> in that process's pid namespace so it needs to be translated into
> current's namespace. First we have to look up the struct pid, then
> pid_vnr is just a wrapper for pid_nr_ns in the current pid namespace:
> 
>   pid_t pid_vnr(struct pid *pid)
>   {
>           return pid_nr_ns(pid, task_active_pid_ns(current));
>   }
> 
> Oh, but the comment is wrong, so maybe that's what confused you.
> s/connection/caller/ there and it should make more sense.

Attaching updated patch against fuse.git for-next.  Check namespace in both
device read and write.  Check them at the start (doesn't matter if requests are
stuck in the queue, if server isn't playing by the rules, then all is lost
anyway).

One thing: we return error if current tgid isn't valid in server's namespace.
That's looks good.  However we silently succeed and set in.h.pid to zero if
current pid isn't representable in the server's namespace.  That doesn't sound
quite right.

Again the question is, does it make sense to allow access by tasks whose pid
isn't representable in the server.  If not, then they should be rejected instead
of succeeding with an invalid PID, right?

Thanks,
Miklos
----

From: Seth Forshee <seth.forshee@canonical.com>
Date: Wed, 2 Jul 2014 16:29:19 -0500
Subject: fuse: Add support for pid namespaces

If the userspace process servicing fuse requests is running in
a pid namespace then pids passed via the fuse fd need to be
translated relative to that namespace. Capture the pid namespace
in use when the filesystem is mounted and use this for pid
translation.

As with user namespaces, since no use case currently exists for
changing namespaces so all translations are done relative to the
pid namespace in use when /dev/fuse is opened. Mounting or
/dev/fuse IO from another namespace will return errors. This
restriction can be relaxed at a later time if needed.

File locking changes based on previous work done by Eric
Biederman.

Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
---
 fs/fuse/dev.c    |   15 +++++++++++----
 fs/fuse/file.c   |   22 +++++++++++++++++-----
 fs/fuse/fuse_i.h |    4 ++++
 fs/fuse/inode.c  |    3 +++
 4 files changed, 35 insertions(+), 9 deletions(-)

--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -19,6 +19,7 @@
 #include <linux/pipe_fs_i.h>
 #include <linux/swap.h>
 #include <linux/splice.h>
+#include <linux/sched.h>
 
 MODULE_ALIAS_MISCDEV(FUSE_MINOR);
 MODULE_ALIAS("devname:fuse");
@@ -124,11 +125,11 @@ static void __fuse_put_request(struct fu
 	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);
 }
 
 void fuse_set_initialized(struct fuse_conn *fc)
@@ -181,7 +182,7 @@ static struct fuse_req *__fuse_get_req(s
 		goto out;
 	}
 
-	fuse_req_init_context(req);
+	fuse_req_init_context(fc, req);
 	__set_bit(FR_WAITING, &req->flags);
 	if (for_background)
 		__set_bit(FR_BACKGROUND, &req->flags);
@@ -274,7 +275,7 @@ struct fuse_req *fuse_get_req_nofail_nop
 	if (!req)
 		req = get_reserved_req(fc, file);
 
-	fuse_req_init_context(req);
+	fuse_req_init_context(fc, req);
 	__set_bit(FR_WAITING, &req->flags);
 	__clear_bit(FR_BACKGROUND, &req->flags);
 	return req;
@@ -1243,6 +1244,9 @@ static ssize_t fuse_dev_do_read(struct f
 	struct fuse_in *in;
 	unsigned reqsize;
 
+	if (task_active_pid_ns(current) != fc->pid_ns)
+		return -EIO;
+
  restart:
 	spin_lock(&fiq->waitq.lock);
 	err = -EAGAIN;
@@ -1872,6 +1876,9 @@ static ssize_t fuse_dev_do_write(struct
 	struct fuse_req *req;
 	struct fuse_out_header oh;
 
+	if (task_active_pid_ns(current) != fc->pid_ns)
+		return -EIO;
+
 	if (nbytes < sizeof(struct fuse_out_header))
 		return -EINVAL;
 
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -2061,7 +2061,8 @@ static int fuse_direct_mmap(struct file
 	return generic_file_mmap(file, vma);
 }
 
-static int convert_fuse_file_lock(const struct fuse_file_lock *ffl,
+static int convert_fuse_file_lock(struct fuse_conn *fc,
+				  const struct fuse_file_lock *ffl,
 				  struct file_lock *fl)
 {
 	switch (ffl->type) {
@@ -2076,7 +2077,14 @@ static int convert_fuse_file_lock(const
 
 		fl->fl_start = ffl->start;
 		fl->fl_end = ffl->end;
-		fl->fl_pid = ffl->pid;
+
+		/*
+		 * Convert pid into the caller's pid namespace. If the pid
+		 * does not map into the namespace fl_pid will get set to 0.
+		 */
+		rcu_read_lock();
+		fl->fl_pid = pid_vnr(find_pid_ns(ffl->pid, fc->pid_ns));
+		rcu_read_unlock();
 		break;
 
 	default:
@@ -2125,7 +2133,7 @@ static int fuse_getlk(struct file *file,
 	args.out.args[0].value = &outarg;
 	err = fuse_simple_request(fc, &args);
 	if (!err)
-		err = convert_fuse_file_lock(&outarg.lk, fl);
+		err = convert_fuse_file_lock(fc, &outarg.lk, fl);
 
 	return err;
 }
@@ -2137,7 +2145,8 @@ static int fuse_setlk(struct file *file,
 	FUSE_ARGS(args);
 	struct fuse_lk_in inarg;
 	int opcode = (fl->fl_flags & FL_SLEEP) ? FUSE_SETLKW : FUSE_SETLK;
-	pid_t pid = fl->fl_type != F_UNLCK ? current->tgid : 0;
+	struct pid *pid = fl->fl_type != F_UNLCK ? task_tgid(current) : NULL;
+	pid_t pid_nr = pid_nr_ns(pid, fc->pid_ns);
 	int err;
 
 	if (fl->fl_lmops && fl->fl_lmops->lm_grant) {
@@ -2149,7 +2158,10 @@ static int fuse_setlk(struct file *file,
 	if (fl->fl_flags & FL_CLOSE)
 		return 0;
 
-	fuse_lk_fill(&args, file, fl, opcode, pid, flock, &inarg);
+	if (pid && pid_nr == 0)
+		return -EOVERFLOW;
+
+	fuse_lk_fill(&args, file, fl, opcode, pid_nr, flock, &inarg);
 	err = fuse_simple_request(fc, &args);
 
 	/* locking is restartable */
--- 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
@@ -456,6 +457,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;
 
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -20,6 +20,7 @@
 #include <linux/random.h>
 #include <linux/sched.h>
 #include <linux/exportfs.h>
+#include <linux/pid_namespace.h>
 
 MODULE_AUTHOR("Miklos Szeredi <miklos@szeredi.hu>");
 MODULE_DESCRIPTION("Filesystem in Userspace");
@@ -609,6 +610,7 @@ void fuse_conn_init(struct fuse_conn *fc
 	fc->connected = 1;
 	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);
 
@@ -617,6 +619,7 @@ void fuse_conn_put(struct fuse_conn *fc)
 	if (atomic_dec_and_test(&fc->count)) {
 		if (fc->destroy_req)
 			fuse_request_free(fc->destroy_req);
+		put_pid_ns(fc->pid_ns);
 		fc->release(fc);
 	}
 }

Re: [fuse-devel] fuse_get_context() and namespaces

[Seth Forshee]

On Tue, May 26, 2015 at 05:21:38PM +0200, Miklos Szeredi wrote:
> On Fri, May 22, 2015 at 01:59:32PM -0500, Seth Forshee wrote:
> > On Fri, May 22, 2015 at 12:44:35PM -0500, Eric W. Biederman wrote:
> > > Seth Forshee <seth.forshee@canonical.com> writes:
> > > 
> > > > On Fri, May 22, 2015 at 04:23:55PM +0200, Miklos Szeredi wrote:
> > > >> On Sat, May 2, 2015 at 5:56 PM,  <alexey@kurnosov.spb.ru> wrote:
> > > >> >
> > > >> > 3.10.0-229 form Scientific Linux and native 4.0.1-1 (from elrepo).
> > > >> > SL 7.1 on the host and SL 6.6 on the LXC guest. At least in 3.10
> > > >> > the 499dcf2024092e5cce41d05599a5b51d1f92031a is present.
> > > >> > Steps to reproduce:
> > > >> >
> > > >> > On first console:
> > > >> > [root@sl7test ~]# lxc-start  -n test-2 /bin/su -
> > > >> > [root@test-2 ~]# diff -u  hello.py /usr/share/doc/fuse-python-0.2.1/example/hello.py
> > > >> > --- hello.py    2015-05-02 11:12:13.963093580 -0400
> > > >> > +++ /usr/share/doc/fuse-python-0.2.1/example/hello.py   2010-04-14 18:29:21.000000000 -0400
> > > >> > @@ -41,8 +41,6 @@
> > > >> >  class HelloFS(Fuse):
> > > >> >
> > > >> >      def getattr(self, path):
> > > >> > -        dic = Fuse.GetContext(self)
> > > >> > -        print dic
> > > >> >          st = MyStat()
> > > >> >          if path == '/':
> > > >> >              st.st_mode = stat.S_IFDIR | 0755
> > > >> > [root@test-2 ~]# python hello.py -f  /mnt/
> > > >> >
> > > >> > On second console:
> > > >> > [root@test-2 ~]# echo $$
> > > >> > 41
> > > >> > [root@test-2 ~]# ls /mnt/
> > > >> > hello
> > > >> >
> > > >> > Output of first console:
> > > >> > {'gid': 0, 'pid': 12083, 'uid': 0}
> > > >> 
> > > >> Thanks.
> > > >> 
> > > >> Digging in mailbox...  There was a thread last year about adding
> > > >> support for running fuse daemon in a container:
> > > >> 
> > > >>   http://thread.gmane.org/gmane.linux.kernel/1811658
> > > >> 
> > > >> Not sure what happened, but no updated patches have been posted or
> > > >> maybe I just missed them.
> > > >
> > > > I haven't sent updated patches in a while. I still intend to, but I
> > > > shifted focus to first getting general support for mounts from user
> > > > namespaces into the vfs (which will give a clearer direction for some of
> > > > the concerns raised about the fuse patches).
> > > >
> > > > All of this code is available in the userns-mounts branch of
> > > > git://kernel.ubuntu.com/sforshee/linux.git, and I don't think the fuse
> > > > patches actually depend on any of the stuff that precedes them. I'm
> > > > planning to start submitting some of the earlier patches from that
> > > > branch soon, and eventually get back to resubmitting the fuse patches.
> > > >
> > > > This is about pid namespaces though, and the fuse pid namespace patch
> > > > from that series (see below) should be more or less independent of the
> > > > rest of the patches. Potentially that could be merged separately from
> > > > the user namespae stuff.
> > > 
> > > [snip]
> > > 
> > > > @@ -2076,7 +2077,15 @@ static int convert_fuse_file_lock(const struct fuse_file_lock *ffl,
> > > >  
> > > >  		fl->fl_start = ffl->start;
> > > >  		fl->fl_end = ffl->end;
> > > > -		fl->fl_pid = ffl->pid;
> > > > +
> > > > +		/*
> > > > +		 * Convert pid into the connection's pid namespace. If the
> > > > +		 * pid does not map into the namespace fl_pid will get set
> > > > +		 * to 0.
> > > > +		 */
> > > > +		rcu_read_lock();
> > > > +		fl->fl_pid = pid_vnr(find_pid_ns(ffl->pid, fc->pid_ns));
> > > > +		rcu_read_unlock();
> > > 
> > > Scratches my head.  This looks wrong.
> > > 
> > > I would have expected pid_nr_ns.  Am I missing something reading this
> > > patch quickly?
> > 
> > Here we're in the context of a F_GETLK operation. We've requested the
> > lock information from the fuse process, and ffl->pid is the pid number
> > in that process's pid namespace so it needs to be translated into
> > current's namespace. First we have to look up the struct pid, then
> > pid_vnr is just a wrapper for pid_nr_ns in the current pid namespace:
> > 
> >   pid_t pid_vnr(struct pid *pid)
> >   {
> >           return pid_nr_ns(pid, task_active_pid_ns(current));
> >   }
> > 
> > Oh, but the comment is wrong, so maybe that's what confused you.
> > s/connection/caller/ there and it should make more sense.
> 
> Attaching updated patch against fuse.git for-next.  Check namespace in both
> device read and write.  Check them at the start (doesn't matter if requests are
> stuck in the queue, if server isn't playing by the rules, then all is lost
> anyway).
> 
> One thing: we return error if current tgid isn't valid in server's namespace.
> That's looks good.  However we silently succeed and set in.h.pid to zero if
> current pid isn't representable in the server's namespace.  That doesn't sound
> quite right.
> 
> Again the question is, does it make sense to allow access by tasks whose pid
> isn't representable in the server.  If not, then they should be rejected instead
> of succeeding with an invalid PID, right?

All of the fuse filesystems I looked at didn't pay any attention at all
to in.h.pid, and I don't see any reason to make them unusable by
processes outside the pid namespace. Filesystems which do care about
pids can reject requests when in.h.pid is 0 if they wish.

Of course it's a different matter if there are existing filesystems
which could be broken by in.h.pid == 0.

Seth

Re: [fuse-devel] fuse_get_context() and namespaces

[Eric W. Biederman]

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

> On Tue, May 26, 2015 at 05:21:38PM +0200, Miklos Szeredi wrote:
>> On Fri, May 22, 2015 at 01:59:32PM -0500, Seth Forshee wrote:
>> > On Fri, May 22, 2015 at 12:44:35PM -0500, Eric W. Biederman wrote:
>> > > Seth Forshee <seth.forshee@canonical.com> writes:
>> > > 
>> > > > On Fri, May 22, 2015 at 04:23:55PM +0200, Miklos Szeredi wrote:
>> > > >> On Sat, May 2, 2015 at 5:56 PM,  <alexey@kurnosov.spb.ru> wrote:
>> > > >> >
>> > > >> > 3.10.0-229 form Scientific Linux and native 4.0.1-1 (from elrepo).
>> > > >> > SL 7.1 on the host and SL 6.6 on the LXC guest. At least in 3.10
>> > > >> > the 499dcf2024092e5cce41d05599a5b51d1f92031a is present.
>> > > >> > Steps to reproduce:
>> > > >> >
>> > > >> > On first console:
>> > > >> > [root@sl7test ~]# lxc-start  -n test-2 /bin/su -
>> > > >> > [root@test-2 ~]# diff -u  hello.py /usr/share/doc/fuse-python-0.2.1/example/hello.py
>> > > >> > --- hello.py    2015-05-02 11:12:13.963093580 -0400
>> > > >> > +++ /usr/share/doc/fuse-python-0.2.1/example/hello.py   2010-04-14 18:29:21.000000000 -0400
>> > > >> > @@ -41,8 +41,6 @@
>> > > >> >  class HelloFS(Fuse):
>> > > >> >
>> > > >> >      def getattr(self, path):
>> > > >> > -        dic = Fuse.GetContext(self)
>> > > >> > -        print dic
>> > > >> >          st = MyStat()
>> > > >> >          if path == '/':
>> > > >> >              st.st_mode = stat.S_IFDIR | 0755
>> > > >> > [root@test-2 ~]# python hello.py -f  /mnt/
>> > > >> >
>> > > >> > On second console:
>> > > >> > [root@test-2 ~]# echo $$
>> > > >> > 41
>> > > >> > [root@test-2 ~]# ls /mnt/
>> > > >> > hello
>> > > >> >
>> > > >> > Output of first console:
>> > > >> > {'gid': 0, 'pid': 12083, 'uid': 0}
>> > > >> 
>> > > >> Thanks.
>> > > >> 
>> > > >> Digging in mailbox...  There was a thread last year about adding
>> > > >> support for running fuse daemon in a container:
>> > > >> 
>> > > >>   http://thread.gmane.org/gmane.linux.kernel/1811658
>> > > >> 
>> > > >> Not sure what happened, but no updated patches have been posted or
>> > > >> maybe I just missed them.
>> > > >
>> > > > I haven't sent updated patches in a while. I still intend to, but I
>> > > > shifted focus to first getting general support for mounts from user
>> > > > namespaces into the vfs (which will give a clearer direction for some of
>> > > > the concerns raised about the fuse patches).
>> > > >
>> > > > All of this code is available in the userns-mounts branch of
>> > > > git://kernel.ubuntu.com/sforshee/linux.git, and I don't think the fuse
>> > > > patches actually depend on any of the stuff that precedes them. I'm
>> > > > planning to start submitting some of the earlier patches from that
>> > > > branch soon, and eventually get back to resubmitting the fuse patches.
>> > > >
>> > > > This is about pid namespaces though, and the fuse pid namespace patch
>> > > > from that series (see below) should be more or less independent of the
>> > > > rest of the patches. Potentially that could be merged separately from
>> > > > the user namespae stuff.
>> > > 
>> > > [snip]
>> > > 
>> > > > @@ -2076,7 +2077,15 @@ static int convert_fuse_file_lock(const struct fuse_file_lock *ffl,
>> > > >  
>> > > >  		fl->fl_start = ffl->start;
>> > > >  		fl->fl_end = ffl->end;
>> > > > -		fl->fl_pid = ffl->pid;
>> > > > +
>> > > > +		/*
>> > > > +		 * Convert pid into the connection's pid namespace. If the
>> > > > +		 * pid does not map into the namespace fl_pid will get set
>> > > > +		 * to 0.
>> > > > +		 */
>> > > > +		rcu_read_lock();
>> > > > +		fl->fl_pid = pid_vnr(find_pid_ns(ffl->pid, fc->pid_ns));
>> > > > +		rcu_read_unlock();
>> > > 
>> > > Scratches my head.  This looks wrong.
>> > > 
>> > > I would have expected pid_nr_ns.  Am I missing something reading this
>> > > patch quickly?
>> > 
>> > Here we're in the context of a F_GETLK operation. We've requested the
>> > lock information from the fuse process, and ffl->pid is the pid number
>> > in that process's pid namespace so it needs to be translated into
>> > current's namespace. First we have to look up the struct pid, then
>> > pid_vnr is just a wrapper for pid_nr_ns in the current pid namespace:
>> > 
>> >   pid_t pid_vnr(struct pid *pid)
>> >   {
>> >           return pid_nr_ns(pid, task_active_pid_ns(current));
>> >   }
>> > 
>> > Oh, but the comment is wrong, so maybe that's what confused you.
>> > s/connection/caller/ there and it should make more sense.
>> 
>> Attaching updated patch against fuse.git for-next.  Check namespace in both
>> device read and write.  Check them at the start (doesn't matter if requests are
>> stuck in the queue, if server isn't playing by the rules, then all is lost
>> anyway).
>> 
>> One thing: we return error if current tgid isn't valid in server's namespace.
>> That's looks good.  However we silently succeed and set in.h.pid to zero if
>> current pid isn't representable in the server's namespace.  That doesn't sound
>> quite right.
>> 
>> Again the question is, does it make sense to allow access by tasks whose pid
>> isn't representable in the server.  If not, then they should be rejected instead
>> of succeeding with an invalid PID, right?
>
> All of the fuse filesystems I looked at didn't pay any attention at all
> to in.h.pid, and I don't see any reason to make them unusable by
> processes outside the pid namespace. Filesystems which do care about
> pids can reject requests when in.h.pid is 0 if they wish.

Well where we came in at was there is a fuse filesystem paying attention
to pids and is having problems running in a container because the pids
are not translated.

> Of course it's a different matter if there are existing filesystems
> which could be broken by in.h.pid == 0.

It seems there is at least one filesystem being written that has that
limitation.


Given the way fuse works it does seem reasonable to deny access outside
of a pid namespace that the fuse filesystem is in.  Set I believe you
implemented or were at least considering limiting the clients to the
same user namespace as well.

Unfortunately the test Miklos proposed is wrong.  It tested for pid
namespace identity which fails to account for nested pid namespaces.

Instead of saying:
+	if (task_active_pid_ns(current) != fc->pid_ns)
+		return -EIO;

The code should probably say:
	if (!pid_nr_ns(fc->pid_ns, task_pid(current)))
		return -EIO;

Or possibly:
static bool in_pid_ns(struct pid_namespace *ns, struct pid *pid)
{
	return ns->level <= pid->level &&
		pid->numbers[ns->level].ns == ns;
}


	if (!in_pid_ns(fc->pid_ns, task_pid(current)))
		return -EIO;

We don't have the in_pid_ns function but it would be easy enough to add.

Eric

Re: [fuse-devel] fuse_get_context() and namespaces

[Seth Forshee]

On Tue, May 26, 2015 at 10:31:40PM -0500, Eric W. Biederman wrote:
> Seth Forshee <seth.forshee@canonical.com> writes:
> 
> > On Tue, May 26, 2015 at 05:21:38PM +0200, Miklos Szeredi wrote:
> >> On Fri, May 22, 2015 at 01:59:32PM -0500, Seth Forshee wrote:
> >> > On Fri, May 22, 2015 at 12:44:35PM -0500, Eric W. Biederman wrote:
> >> > > Seth Forshee <seth.forshee@canonical.com> writes:
> >> > > 
> >> > > > On Fri, May 22, 2015 at 04:23:55PM +0200, Miklos Szeredi wrote:
> >> > > >> On Sat, May 2, 2015 at 5:56 PM,  <alexey@kurnosov.spb.ru> wrote:
> >> > > >> >
> >> > > >> > 3.10.0-229 form Scientific Linux and native 4.0.1-1 (from elrepo).
> >> > > >> > SL 7.1 on the host and SL 6.6 on the LXC guest. At least in 3.10
> >> > > >> > the 499dcf2024092e5cce41d05599a5b51d1f92031a is present.
> >> > > >> > Steps to reproduce:
> >> > > >> >
> >> > > >> > On first console:
> >> > > >> > [root@sl7test ~]# lxc-start  -n test-2 /bin/su -
> >> > > >> > [root@test-2 ~]# diff -u  hello.py /usr/share/doc/fuse-python-0.2.1/example/hello.py
> >> > > >> > --- hello.py    2015-05-02 11:12:13.963093580 -0400
> >> > > >> > +++ /usr/share/doc/fuse-python-0.2.1/example/hello.py   2010-04-14 18:29:21.000000000 -0400
> >> > > >> > @@ -41,8 +41,6 @@
> >> > > >> >  class HelloFS(Fuse):
> >> > > >> >
> >> > > >> >      def getattr(self, path):
> >> > > >> > -        dic = Fuse.GetContext(self)
> >> > > >> > -        print dic
> >> > > >> >          st = MyStat()
> >> > > >> >          if path == '/':
> >> > > >> >              st.st_mode = stat.S_IFDIR | 0755
> >> > > >> > [root@test-2 ~]# python hello.py -f  /mnt/
> >> > > >> >
> >> > > >> > On second console:
> >> > > >> > [root@test-2 ~]# echo $$
> >> > > >> > 41
> >> > > >> > [root@test-2 ~]# ls /mnt/
> >> > > >> > hello
> >> > > >> >
> >> > > >> > Output of first console:
> >> > > >> > {'gid': 0, 'pid': 12083, 'uid': 0}
> >> > > >> 
> >> > > >> Thanks.
> >> > > >> 
> >> > > >> Digging in mailbox...  There was a thread last year about adding
> >> > > >> support for running fuse daemon in a container:
> >> > > >> 
> >> > > >>   http://thread.gmane.org/gmane.linux.kernel/1811658
> >> > > >> 
> >> > > >> Not sure what happened, but no updated patches have been posted or
> >> > > >> maybe I just missed them.
> >> > > >
> >> > > > I haven't sent updated patches in a while. I still intend to, but I
> >> > > > shifted focus to first getting general support for mounts from user
> >> > > > namespaces into the vfs (which will give a clearer direction for some of
> >> > > > the concerns raised about the fuse patches).
> >> > > >
> >> > > > All of this code is available in the userns-mounts branch of
> >> > > > git://kernel.ubuntu.com/sforshee/linux.git, and I don't think the fuse
> >> > > > patches actually depend on any of the stuff that precedes them. I'm
> >> > > > planning to start submitting some of the earlier patches from that
> >> > > > branch soon, and eventually get back to resubmitting the fuse patches.
> >> > > >
> >> > > > This is about pid namespaces though, and the fuse pid namespace patch
> >> > > > from that series (see below) should be more or less independent of the
> >> > > > rest of the patches. Potentially that could be merged separately from
> >> > > > the user namespae stuff.
> >> > > 
> >> > > [snip]
> >> > > 
> >> > > > @@ -2076,7 +2077,15 @@ static int convert_fuse_file_lock(const struct fuse_file_lock *ffl,
> >> > > >  
> >> > > >  		fl->fl_start = ffl->start;
> >> > > >  		fl->fl_end = ffl->end;
> >> > > > -		fl->fl_pid = ffl->pid;
> >> > > > +
> >> > > > +		/*
> >> > > > +		 * Convert pid into the connection's pid namespace. If the
> >> > > > +		 * pid does not map into the namespace fl_pid will get set
> >> > > > +		 * to 0.
> >> > > > +		 */
> >> > > > +		rcu_read_lock();
> >> > > > +		fl->fl_pid = pid_vnr(find_pid_ns(ffl->pid, fc->pid_ns));
> >> > > > +		rcu_read_unlock();
> >> > > 
> >> > > Scratches my head.  This looks wrong.
> >> > > 
> >> > > I would have expected pid_nr_ns.  Am I missing something reading this
> >> > > patch quickly?
> >> > 
> >> > Here we're in the context of a F_GETLK operation. We've requested the
> >> > lock information from the fuse process, and ffl->pid is the pid number
> >> > in that process's pid namespace so it needs to be translated into
> >> > current's namespace. First we have to look up the struct pid, then
> >> > pid_vnr is just a wrapper for pid_nr_ns in the current pid namespace:
> >> > 
> >> >   pid_t pid_vnr(struct pid *pid)
> >> >   {
> >> >           return pid_nr_ns(pid, task_active_pid_ns(current));
> >> >   }
> >> > 
> >> > Oh, but the comment is wrong, so maybe that's what confused you.
> >> > s/connection/caller/ there and it should make more sense.
> >> 
> >> Attaching updated patch against fuse.git for-next.  Check namespace in both
> >> device read and write.  Check them at the start (doesn't matter if requests are
> >> stuck in the queue, if server isn't playing by the rules, then all is lost
> >> anyway).
> >> 
> >> One thing: we return error if current tgid isn't valid in server's namespace.
> >> That's looks good.  However we silently succeed and set in.h.pid to zero if
> >> current pid isn't representable in the server's namespace.  That doesn't sound
> >> quite right.
> >> 
> >> Again the question is, does it make sense to allow access by tasks whose pid
> >> isn't representable in the server.  If not, then they should be rejected instead
> >> of succeeding with an invalid PID, right?
> >
> > All of the fuse filesystems I looked at didn't pay any attention at all
> > to in.h.pid, and I don't see any reason to make them unusable by
> > processes outside the pid namespace. Filesystems which do care about
> > pids can reject requests when in.h.pid is 0 if they wish.
> 
> Well where we came in at was there is a fuse filesystem paying attention
> to pids and is having problems running in a container because the pids
> are not translated.
> 
> > Of course it's a different matter if there are existing filesystems
> > which could be broken by in.h.pid == 0.
> 
> It seems there is at least one filesystem being written that has that
> limitation.

I haven't seen anything to indicate that this filesystem will be broken
by this, just that it's broken by untranslated pids. Presumably it would
just reject any requests which aren't representable in its namespace.

> Given the way fuse works it does seem reasonable to deny access outside
> of a pid namespace that the fuse filesystem is in.  Set I believe you
> implemented or were at least considering limiting the clients to the
> same user namespace as well.

With user namespaces there's potential (DoS) security concern. I'm not
aware of any such concern with pid namespaces.

If it's decided as a matter of policy that we won't allow requests when
the pid can't be translated, that's fine. All I'm saying is that I know
of no technical reason that it shouldn't be permitted.

> Unfortunately the test Miklos proposed is wrong.  It tested for pid
> namespace identity which fails to account for nested pid namespaces.
> 
> Instead of saying:
> +	if (task_active_pid_ns(current) != fc->pid_ns)
> +		return -EIO;
> 
> The code should probably say:
> 	if (!pid_nr_ns(fc->pid_ns, task_pid(current)))
> 		return -EIO;
> 
> Or possibly:
> static bool in_pid_ns(struct pid_namespace *ns, struct pid *pid)
> {
> 	return ns->level <= pid->level &&
> 		pid->numbers[ns->level].ns == ns;
> }
> 
> 
> 	if (!in_pid_ns(fc->pid_ns, task_pid(current)))
> 		return -EIO;
> 
> We don't have the in_pid_ns function but it would be easy enough to add.

The proposed checks are in the server's read/write path on /dev/fuse. I
don't really think the server should be allowed to change to another pid
namespace at all, otherwise fuse needs to delay translating the pid
until the read path so it can be sure which namespace to use for the
translation.

On the other hand if requests are to be rejected based on the pid
namespace then it definitely needs to be more like what you've shown
here (the patch Miklos posted is not currently doing this). I'm pretty
sure that lxcfs [1] is mounted outside of containers for use within
them.

Seth

[1] https://github.com/lxc/lxcfs

Re: [fuse-devel] fuse_get_context() and namespaces

[Miklos Szeredi]

On Wed, May 27, 2015 at 2:55 PM, Seth Forshee
<seth.forshee@canonical.com> wrote:

> I haven't seen anything to indicate that this filesystem will be broken
> by this, just that it's broken by untranslated pids. Presumably it would
> just reject any requests which aren't representable in its namespace.

Without failing the operation there never will be any indication that
a filesystem is broken.  So I guess the safe way would be

 - deny access for untranslated pids (uids, gids, etc).

 - if this becomes an issue (possibly a perfomance issue), then add a
flag to disable pids (and/or uids, gids) completely.

Thanks,
Miklos

Re: [fuse-devel] fuse_get_context() and namespaces

[Seth Forshee]

On Mon, Jun 01, 2015 at 03:07:07PM +0200, Miklos Szeredi wrote:
> On Wed, May 27, 2015 at 2:55 PM, Seth Forshee
> <seth.forshee@canonical.com> wrote:
> 
> > I haven't seen anything to indicate that this filesystem will be broken
> > by this, just that it's broken by untranslated pids. Presumably it would
> > just reject any requests which aren't representable in its namespace.
> 
> Without failing the operation there never will be any indication that
> a filesystem is broken.  So I guess the safe way would be
> 
>  - deny access for untranslated pids (uids, gids, etc).
> 
>  - if this becomes an issue (possibly a perfomance issue), then add a
> flag to disable pids (and/or uids, gids) completely.

How about this then? I left fuse_get_req_nofail_nopages alone since it
presumably shouldn't fail, but that could be changed too.

With this pids are being translated into my container's namespace, and
access by processes whose pid won't map into the namespace is denied.

Seth

---

>From 9fa40d5084b633902c781459fe853f9234e9f67c Mon Sep 17 00:00:00 2001
From: Seth Forshee <seth.forshee@canonical.com>
Date: Wed, 2 Jul 2014 16:29:19 -0500
Subject: [PATCH] fuse: Add support for pid namespaces

If the userspace process servicing fuse requests is running in
a pid namespace then pids passed via the fuse fd need to be
translated relative to that namespace. Capture the pid namespace
in use when the filesystem is mounted and use this for pid
translation.

Since no use case currently exists for changing namespaces all
translations are done relative to the pid namespace in use when
/dev/fuse is opened. Mounting or /dev/fuse IO from another
namespace will return errors.

Requests from processes whose pid cannot be translated into the
target namespace are not permitted, except for requests
allocated via fuse_get_req_nofail_nopages. For no-fail requests
in.h.pid will be 0 if the pid translation fails.

File locking changes based on previous work done by Eric
Biederman.

Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
---
 fs/fuse/dev.c    | 19 +++++++++++++++----
 fs/fuse/file.c   | 22 +++++++++++++++++-----
 fs/fuse/fuse_i.h |  4 ++++
 fs/fuse/inode.c  |  3 +++
 4 files changed, 39 insertions(+), 9 deletions(-)

diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index 80cc1b35d460..b8b977dbdd8c 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -19,6 +19,7 @@
 #include <linux/pipe_fs_i.h>
 #include <linux/swap.h>
 #include <linux/splice.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);
 }
 
 void fuse_set_initialized(struct fuse_conn *fc)
@@ -181,10 +182,14 @@ 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);
 	__set_bit(FR_WAITING, &req->flags);
 	if (for_background)
 		__set_bit(FR_BACKGROUND, &req->flags);
+	if (req->in.h.pid == 0) {
+		fuse_put_request(fc, req);
+		return ERR_PTR(-EOVERFLOW);
+	}
 
 	return req;
 
@@ -274,7 +279,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);
 	__set_bit(FR_WAITING, &req->flags);
 	__clear_bit(FR_BACKGROUND, &req->flags);
 	return req;
@@ -1243,6 +1248,9 @@ 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)
+		return -EIO;
+
  restart:
 	spin_lock(&fiq->waitq.lock);
 	err = -EAGAIN;
@@ -1872,6 +1880,9 @@ 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)
+		return -EIO;
+
 	if (nbytes < sizeof(struct fuse_out_header))
 		return -EINVAL;
 
diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 64835cf58936..3088bf360003 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -2061,7 +2061,8 @@ static int fuse_direct_mmap(struct file *file, struct vm_area_struct *vma)
 	return generic_file_mmap(file, vma);
 }
 
-static int convert_fuse_file_lock(const struct fuse_file_lock *ffl,
+static int convert_fuse_file_lock(struct fuse_conn *fc,
+				  const struct fuse_file_lock *ffl,
 				  struct file_lock *fl)
 {
 	switch (ffl->type) {
@@ -2076,7 +2077,14 @@ static int convert_fuse_file_lock(const struct fuse_file_lock *ffl,
 
 		fl->fl_start = ffl->start;
 		fl->fl_end = ffl->end;
-		fl->fl_pid = ffl->pid;
+
+		/*
+		 * Convert pid into the caller's pid namespace. If the pid
+		 * does not map into the namespace fl_pid will get set to 0.
+		 */
+		rcu_read_lock();
+		fl->fl_pid = pid_vnr(find_pid_ns(ffl->pid, fc->pid_ns));
+		rcu_read_unlock();
 		break;
 
 	default:
@@ -2125,7 +2133,7 @@ static int fuse_getlk(struct file *file, struct file_lock *fl)
 	args.out.args[0].value = &outarg;
 	err = fuse_simple_request(fc, &args);
 	if (!err)
-		err = convert_fuse_file_lock(&outarg.lk, fl);
+		err = convert_fuse_file_lock(fc, &outarg.lk, fl);
 
 	return err;
 }
@@ -2137,7 +2145,8 @@ static int fuse_setlk(struct file *file, struct file_lock *fl, int flock)
 	FUSE_ARGS(args);
 	struct fuse_lk_in inarg;
 	int opcode = (fl->fl_flags & FL_SLEEP) ? FUSE_SETLKW : FUSE_SETLK;
-	pid_t pid = fl->fl_type != F_UNLCK ? current->tgid : 0;
+	struct pid *pid = fl->fl_type != F_UNLCK ? task_tgid(current) : NULL;
+	pid_t pid_nr = pid_nr_ns(pid, fc->pid_ns);
 	int err;
 
 	if (fl->fl_lmops && fl->fl_lmops->lm_grant) {
@@ -2149,7 +2158,10 @@ static int fuse_setlk(struct file *file, struct file_lock *fl, int flock)
 	if (fl->fl_flags & FL_CLOSE)
 		return 0;
 
-	fuse_lk_fill(&args, file, fl, opcode, pid, flock, &inarg);
+	if (pid && pid_nr == 0)
+		return -EOVERFLOW;
+
+	fuse_lk_fill(&args, file, fl, opcode, pid_nr, flock, &inarg);
 	err = fuse_simple_request(fc, &args);
 
 	/* locking is restartable */
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index 405113101db8..143b595197b6 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
@@ -456,6 +457,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 ac81f48ab2f4..4a03ac733be9 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -20,6 +20,7 @@
 #include <linux/random.h>
 #include <linux/sched.h>
 #include <linux/exportfs.h>
+#include <linux/pid_namespace.h>
 
 MODULE_AUTHOR("Miklos Szeredi <miklos@szeredi.hu>");
 MODULE_DESCRIPTION("Filesystem in Userspace");
@@ -609,6 +610,7 @@ void fuse_conn_init(struct fuse_conn *fc)
 	fc->connected = 1;
 	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);
 
@@ -617,6 +619,7 @@ void fuse_conn_put(struct fuse_conn *fc)
 	if (atomic_dec_and_test(&fc->count)) {
 		if (fc->destroy_req)
 			fuse_request_free(fc->destroy_req);
+		put_pid_ns(fc->pid_ns);
 		fc->release(fc);
 	}
 }