[PATCH v2] fuse: Add module param for non-descendant userns access to allow_other

[PATCH v2] fuse: Add module param for non-descendant userns access to allow_other

[Dave Marchevsky]

Since commit 73f03c2b4b52 ("fuse: Restrict allow_other to the
superblock's namespace or a descendant"), access to allow_other FUSE
filesystems has been limited to users in the mounting user namespace or
descendants. This prevents a process that is privileged in its userns -
but not its parent namespaces - from mounting a FUSE fs w/ allow_other
that is accessible to processes in parent namespaces.

While this restriction makes sense overall it breaks a legitimate
usecase: I have a tracing daemon which needs to peek into
process' open files in order to symbolicate - similar to 'perf'. The
daemon is a privileged process in the root userns, but is unable to peek
into FUSE filesystems mounted with allow_other by processes in child
namespaces.

This patch adds a module param, allow_other_parent_userns, to act as an
escape hatch for this descendant userns logic. Setting
allow_other_parent_userns allows non-descendant-userns processes to
access FUSEs mounted with allow_other. A sysadmin setting this param
must trust allow_other FUSEs on the host to not DoS processes as
described in 73f03c2b4b52.

Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
---

This is a followup to a previous attempt to solve same problem in a
different way: "fuse: allow CAP_SYS_ADMIN in root userns to access
allow_other mount" [0].

v1 -> v2:
  * Use module param instead of capability check

  [0]: lore.kernel.org/linux-fsdevel/20211111221142.4096653-1-davemarchevsky@fb.com

 fs/fuse/dir.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index 9dfee44e97ad..3d593ae7dc66 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -11,6 +11,7 @@
 #include <linux/pagemap.h>
 #include <linux/file.h>
 #include <linux/fs_context.h>
+#include <linux/moduleparam.h>
 #include <linux/sched.h>
 #include <linux/namei.h>
 #include <linux/slab.h>
@@ -21,6 +22,12 @@
 #include <linux/types.h>
 #include <linux/kernel.h>
 
+static bool __read_mostly allow_other_parent_userns;
+module_param(allow_other_parent_userns, bool, 0644);
+MODULE_PARM_DESC(allow_other_parent_userns,
+ "Allow users not in mounting or descendant userns "
+ "to access FUSE with allow_other set");
+
 static void fuse_advise_use_readdirplus(struct inode *dir)
 {
 	struct fuse_inode *fi = get_fuse_inode(dir);
@@ -1230,7 +1237,7 @@ int fuse_allow_current_process(struct fuse_conn *fc)
 	const struct cred *cred;
 
 	if (fc->allow_other)
-		return current_in_userns(fc->user_ns);
+		return current_in_userns(fc->user_ns) || allow_other_parent_userns;
 
 	cred = current_cred();
 	if (uid_eq(cred->euid, fc->user_id) &&
-- 
2.30.2

Re: [PATCH v2] fuse: Add module param for non-descendant userns access to allow_other

[Christian Brauner]

On Wed, Jun 01, 2022 at 11:44:07AM -0700, Dave Marchevsky wrote:
> Since commit 73f03c2b4b52 ("fuse: Restrict allow_other to the
> superblock's namespace or a descendant"), access to allow_other FUSE
> filesystems has been limited to users in the mounting user namespace or
> descendants. This prevents a process that is privileged in its userns -
> but not its parent namespaces - from mounting a FUSE fs w/ allow_other
> that is accessible to processes in parent namespaces.
> 
> While this restriction makes sense overall it breaks a legitimate
> usecase: I have a tracing daemon which needs to peek into
> process' open files in order to symbolicate - similar to 'perf'. The
> daemon is a privileged process in the root userns, but is unable to peek
> into FUSE filesystems mounted with allow_other by processes in child
> namespaces.
> 
> This patch adds a module param, allow_other_parent_userns, to act as an
> escape hatch for this descendant userns logic. Setting
> allow_other_parent_userns allows non-descendant-userns processes to
> access FUSEs mounted with allow_other. A sysadmin setting this param
> must trust allow_other FUSEs on the host to not DoS processes as
> described in 73f03c2b4b52.
> 
> Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
> ---
> 
> This is a followup to a previous attempt to solve same problem in a
> different way: "fuse: allow CAP_SYS_ADMIN in root userns to access
> allow_other mount" [0].
> 
> v1 -> v2:
>   * Use module param instead of capability check
> 
>   [0]: lore.kernel.org/linux-fsdevel/20211111221142.4096653-1-davemarchevsky@fb.com
> 
>  fs/fuse/dir.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> index 9dfee44e97ad..3d593ae7dc66 100644
> --- a/fs/fuse/dir.c
> +++ b/fs/fuse/dir.c
> @@ -11,6 +11,7 @@
>  #include <linux/pagemap.h>
>  #include <linux/file.h>
>  #include <linux/fs_context.h>
> +#include <linux/moduleparam.h>
>  #include <linux/sched.h>
>  #include <linux/namei.h>
>  #include <linux/slab.h>
> @@ -21,6 +22,12 @@
>  #include <linux/types.h>
>  #include <linux/kernel.h>
>  
> +static bool __read_mostly allow_other_parent_userns;
> +module_param(allow_other_parent_userns, bool, 0644);
> +MODULE_PARM_DESC(allow_other_parent_userns,
> + "Allow users not in mounting or descendant userns "
> + "to access FUSE with allow_other set");

The name of the parameter also suggests that access is granted to parent
userns tasks whereas the change seems to me to allows every task access
to that fuse filesystem independent of what userns they are in.

So even a task in a sibling userns could - probably with rather
elaborate mount propagation trickery - access that fuse filesystem.

AFaict, either the module parameter is misnamed or the patch doesn't
implement the behavior expressed in the name.

The original patch restricted access to a CAP_SYS_ADMIN capable task.
Did we agree that it was a good idea to weaken it to all tasks?
Shouldn't we still just restrict this to CAP_SYS_ADMIN capable tasks in
the initial userns?

> +
>  static void fuse_advise_use_readdirplus(struct inode *dir)
>  {
>  	struct fuse_inode *fi = get_fuse_inode(dir);
> @@ -1230,7 +1237,7 @@ int fuse_allow_current_process(struct fuse_conn *fc)
>  	const struct cred *cred;
>  
>  	if (fc->allow_other)
> -		return current_in_userns(fc->user_ns);
> +		return current_in_userns(fc->user_ns) || allow_other_parent_userns;
>  
>  	cred = current_cred();
>  	if (uid_eq(cred->euid, fc->user_id) &&
> -- 
> 2.30.2
>

Re: [PATCH v2] fuse: Add module param for non-descendant userns access to allow_other

[Andrii Nakryiko]

On 6/7/22 1:47 AM, Christian Brauner wrote:
> On Wed, Jun 01, 2022 at 11:44:07AM -0700, Dave Marchevsky wrote:
>> Since commit 73f03c2b4b52 ("fuse: Restrict allow_other to the
>> superblock's namespace or a descendant"), access to allow_other FUSE
>> filesystems has been limited to users in the mounting user namespace or
>> descendants. This prevents a process that is privileged in its userns -
>> but not its parent namespaces - from mounting a FUSE fs w/ allow_other
>> that is accessible to processes in parent namespaces.
>>
>> While this restriction makes sense overall it breaks a legitimate
>> usecase: I have a tracing daemon which needs to peek into
>> process' open files in order to symbolicate - similar to 'perf'. The
>> daemon is a privileged process in the root userns, but is unable to peek
>> into FUSE filesystems mounted with allow_other by processes in child
>> namespaces.
>>
>> This patch adds a module param, allow_other_parent_userns, to act as an
>> escape hatch for this descendant userns logic. Setting
>> allow_other_parent_userns allows non-descendant-userns processes to
>> access FUSEs mounted with allow_other. A sysadmin setting this param
>> must trust allow_other FUSEs on the host to not DoS processes as
>> described in 73f03c2b4b52.
>>
>> Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
>> ---
>>
>> This is a followup to a previous attempt to solve same problem in a
>> different way: "fuse: allow CAP_SYS_ADMIN in root userns to access
>> allow_other mount" [0].
>>
>> v1 -> v2:
>>    * Use module param instead of capability check
>>
>>    [0]: lore.kernel.org/linux-fsdevel/20211111221142.4096653-1-davemarchevsky@fb.com
>>
>>   fs/fuse/dir.c | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
>> index 9dfee44e97ad..3d593ae7dc66 100644
>> --- a/fs/fuse/dir.c
>> +++ b/fs/fuse/dir.c
>> @@ -11,6 +11,7 @@
>>   #include <linux/pagemap.h>
>>   #include <linux/file.h>
>>   #include <linux/fs_context.h>
>> +#include <linux/moduleparam.h>
>>   #include <linux/sched.h>
>>   #include <linux/namei.h>
>>   #include <linux/slab.h>
>> @@ -21,6 +22,12 @@
>>   #include <linux/types.h>
>>   #include <linux/kernel.h>
>>   
>> +static bool __read_mostly allow_other_parent_userns;
>> +module_param(allow_other_parent_userns, bool, 0644);
>> +MODULE_PARM_DESC(allow_other_parent_userns,
>> + "Allow users not in mounting or descendant userns "
>> + "to access FUSE with allow_other set");
> 
> The name of the parameter also suggests that access is granted to parent
> userns tasks whereas the change seems to me to allows every task access
> to that fuse filesystem independent of what userns they are in.
> 
> So even a task in a sibling userns could - probably with rather
> elaborate mount propagation trickery - access that fuse filesystem.
> 
> AFaict, either the module parameter is misnamed or the patch doesn't
> implement the behavior expressed in the name.
> 
> The original patch restricted access to a CAP_SYS_ADMIN capable task.
> Did we agree that it was a good idea to weaken it to all tasks?
> Shouldn't we still just restrict this to CAP_SYS_ADMIN capable tasks in
> the initial userns?

I think it's fine to allow for CAP_SYS_ADMIN only, but can we then 
ignore the allow_other mount option in such case? The idea is that 
CAP_SYS_ADMIN allows you to read FUSE-backed contents no matter what, so 
user not mounting with allow_other preventing root from reading contents 
defeats the purpose at least partially.

> 
>> +
>>   static void fuse_advise_use_readdirplus(struct inode *dir)
>>   {
>>   	struct fuse_inode *fi = get_fuse_inode(dir);
>> @@ -1230,7 +1237,7 @@ int fuse_allow_current_process(struct fuse_conn *fc)
>>   	const struct cred *cred;
>>   
>>   	if (fc->allow_other)
>> -		return current_in_userns(fc->user_ns);
>> +		return current_in_userns(fc->user_ns) || allow_other_parent_userns;
>>   
>>   	cred = current_cred();
>>   	if (uid_eq(cred->euid, fc->user_id) &&
>> -- 
>> 2.30.2
>>

Re: [PATCH v2] fuse: Add module param for non-descendant userns access to allow_other

[Miklos Szeredi]

On Fri, 10 Jun 2022 at 23:39, Andrii Nakryiko <andriin@fb.com> wrote:
>
>
>
> On 6/7/22 1:47 AM, Christian Brauner wrote:
> > On Wed, Jun 01, 2022 at 11:44:07AM -0700, Dave Marchevsky wrote:

[...]

> >> +static bool __read_mostly allow_other_parent_userns;
> >> +module_param(allow_other_parent_userns, bool, 0644);
> >> +MODULE_PARM_DESC(allow_other_parent_userns,
> >> + "Allow users not in mounting or descendant userns "
> >> + "to access FUSE with allow_other set");
> >
> > The name of the parameter also suggests that access is granted to parent
> > userns tasks whereas the change seems to me to allows every task access
> > to that fuse filesystem independent of what userns they are in.
> >
> > So even a task in a sibling userns could - probably with rather
> > elaborate mount propagation trickery - access that fuse filesystem.
> >
> > AFaict, either the module parameter is misnamed or the patch doesn't
> > implement the behavior expressed in the name.
> >
> > The original patch restricted access to a CAP_SYS_ADMIN capable task.
> > Did we agree that it was a good idea to weaken it to all tasks?
> > Shouldn't we still just restrict this to CAP_SYS_ADMIN capable tasks in
> > the initial userns?
>
> I think it's fine to allow for CAP_SYS_ADMIN only, but can we then
> ignore the allow_other mount option in such case? The idea is that
> CAP_SYS_ADMIN allows you to read FUSE-backed contents no matter what, so
> user not mounting with allow_other preventing root from reading contents
> defeats the purpose at least partially.

If we want to be compatible with "user_allow_other", then it should be
checking if the uid/gid of the current task is mapped in the
filesystems user_ns (fsuidgid_has_mapping()).  Right?

Thanks,
Miklos

Re: [PATCH v2] fuse: Add module param for non-descendant userns access to allow_other

[Christian Brauner]

On Mon, Jun 13, 2022 at 10:23:47AM +0200, Miklos Szeredi wrote:
> On Fri, 10 Jun 2022 at 23:39, Andrii Nakryiko <andriin@fb.com> wrote:
> >
> >
> >
> > On 6/7/22 1:47 AM, Christian Brauner wrote:
> > > On Wed, Jun 01, 2022 at 11:44:07AM -0700, Dave Marchevsky wrote:
> 
> [...]
> 
> > >> +static bool __read_mostly allow_other_parent_userns;
> > >> +module_param(allow_other_parent_userns, bool, 0644);
> > >> +MODULE_PARM_DESC(allow_other_parent_userns,
> > >> + "Allow users not in mounting or descendant userns "
> > >> + "to access FUSE with allow_other set");
> > >
> > > The name of the parameter also suggests that access is granted to parent
> > > userns tasks whereas the change seems to me to allows every task access
> > > to that fuse filesystem independent of what userns they are in.
> > >
> > > So even a task in a sibling userns could - probably with rather
> > > elaborate mount propagation trickery - access that fuse filesystem.
> > >
> > > AFaict, either the module parameter is misnamed or the patch doesn't
> > > implement the behavior expressed in the name.
> > >
> > > The original patch restricted access to a CAP_SYS_ADMIN capable task.
> > > Did we agree that it was a good idea to weaken it to all tasks?
> > > Shouldn't we still just restrict this to CAP_SYS_ADMIN capable tasks in
> > > the initial userns?
> >
> > I think it's fine to allow for CAP_SYS_ADMIN only, but can we then
> > ignore the allow_other mount option in such case? The idea is that
> > CAP_SYS_ADMIN allows you to read FUSE-backed contents no matter what, so
> > user not mounting with allow_other preventing root from reading contents
> > defeats the purpose at least partially.
> 
> If we want to be compatible with "user_allow_other", then it should be
> checking if the uid/gid of the current task is mapped in the
> filesystems user_ns (fsuidgid_has_mapping()).  Right?

I think that's doable. So assuming we're still talking about requiring
cap_sys_admin then we'd roughly have sm like:

	if (fc->allow_other)
		return current_in_userns(fc->user_ns) ||
			(capable(CAP_SYS_ADMIN) &&
			fsuidgid_has_mapping(..., &init_user_ns));

so say a fuse filesystem is mounted in a userns with a mapping of
0:10000:100. Assume root in init_user_ns is trying to access that fuse
filesystem:

fuse_sb->s_user_ns = 0:10000:100
current_fsuid() = 0
current_fsgid() = 0
capable(CAP_SYS_ADMIN)

that would fail as

fsuidgid_has_mapping() {
	kuid_has_mapping(0:10000:1000, 0) -> INVALID_UID
	kgid_has_mapping(0:10000:1000, 0) -> INVALID_GID
}

so root would have to do:

setfsuid(100000)
setfsgid(100000)

// This transition will cost you at least
// CAP_CHOWN
// CAP_MKNOD
// CAP_DAC_OVERRIDE
// CAP_DAC_READ_SEARCH
// CAP_FOWNER
// CAP_FSETID
// but those are regained when transitioning back to fsuid/fsgid 0.

fuse_sb->s_user_ns = 0:10000:100
current_fsuid() = 100000
current_fsgid() = 100000
capable(CAP_SYS_ADMIN)

that would succeed as

fsuidgid_has_mapping() {
	kuid_has_mapping(0:10000:1000, 0) -> 0
	kgid_has_mapping(0:10000:1000, 0) -> 0
}

Re: [PATCH v2] fuse: Add module param for non-descendant userns access to allow_other

[Miklos Szeredi]

On Mon, 13 Jun 2022 at 11:37, Christian Brauner <brauner@kernel.org> wrote:
>
> On Mon, Jun 13, 2022 at 10:23:47AM +0200, Miklos Szeredi wrote:
> > On Fri, 10 Jun 2022 at 23:39, Andrii Nakryiko <andriin@fb.com> wrote:
> > >
> > >
> > >
> > > On 6/7/22 1:47 AM, Christian Brauner wrote:
> > > > On Wed, Jun 01, 2022 at 11:44:07AM -0700, Dave Marchevsky wrote:
> >
> > [...]
> >
> > > >> +static bool __read_mostly allow_other_parent_userns;
> > > >> +module_param(allow_other_parent_userns, bool, 0644);
> > > >> +MODULE_PARM_DESC(allow_other_parent_userns,
> > > >> + "Allow users not in mounting or descendant userns "
> > > >> + "to access FUSE with allow_other set");
> > > >
> > > > The name of the parameter also suggests that access is granted to parent
> > > > userns tasks whereas the change seems to me to allows every task access
> > > > to that fuse filesystem independent of what userns they are in.
> > > >
> > > > So even a task in a sibling userns could - probably with rather
> > > > elaborate mount propagation trickery - access that fuse filesystem.
> > > >
> > > > AFaict, either the module parameter is misnamed or the patch doesn't
> > > > implement the behavior expressed in the name.
> > > >
> > > > The original patch restricted access to a CAP_SYS_ADMIN capable task.
> > > > Did we agree that it was a good idea to weaken it to all tasks?
> > > > Shouldn't we still just restrict this to CAP_SYS_ADMIN capable tasks in
> > > > the initial userns?
> > >
> > > I think it's fine to allow for CAP_SYS_ADMIN only, but can we then
> > > ignore the allow_other mount option in such case? The idea is that
> > > CAP_SYS_ADMIN allows you to read FUSE-backed contents no matter what, so
> > > user not mounting with allow_other preventing root from reading contents
> > > defeats the purpose at least partially.
> >
> > If we want to be compatible with "user_allow_other", then it should be
> > checking if the uid/gid of the current task is mapped in the
> > filesystems user_ns (fsuidgid_has_mapping()).  Right?
>
> I think that's doable. So assuming we're still talking about requiring
> cap_sys_admin then we'd roughly have sm like:
>
>         if (fc->allow_other)
>                 return current_in_userns(fc->user_ns) ||
>                         (capable(CAP_SYS_ADMIN) &&
>                         fsuidgid_has_mapping(..., &init_user_ns));

No, I meant this:

        if (fc->allow_other)
                return current_in_userns(fc->user_ns) ||
                        (userns_allow_other &&
                        fsuidgid_has_mapping(..., &init_user_ns));

But I think the OP wanted to allow real root to access the fs, which
this doesn't allow (since 0 will have no mapping in the user ns), so
I'm not sure what's the right solution...

Maybe the original patch is fine: this check isn't meant to protect
the filesystem from access, it's meant to protect the accessor.

Thanks,
Miklos

Re: [PATCH v2] fuse: Add module param for non-descendant userns access to allow_other

[Christian Brauner]

On Mon, Jun 13, 2022 at 12:34:05PM +0200, Miklos Szeredi wrote:
> On Mon, 13 Jun 2022 at 11:37, Christian Brauner <brauner@kernel.org> wrote:
> >
> > On Mon, Jun 13, 2022 at 10:23:47AM +0200, Miklos Szeredi wrote:
> > > On Fri, 10 Jun 2022 at 23:39, Andrii Nakryiko <andriin@fb.com> wrote:
> > > >
> > > >
> > > >
> > > > On 6/7/22 1:47 AM, Christian Brauner wrote:
> > > > > On Wed, Jun 01, 2022 at 11:44:07AM -0700, Dave Marchevsky wrote:
> > >
> > > [...]
> > >
> > > > >> +static bool __read_mostly allow_other_parent_userns;
> > > > >> +module_param(allow_other_parent_userns, bool, 0644);
> > > > >> +MODULE_PARM_DESC(allow_other_parent_userns,
> > > > >> + "Allow users not in mounting or descendant userns "
> > > > >> + "to access FUSE with allow_other set");
> > > > >
> > > > > The name of the parameter also suggests that access is granted to parent
> > > > > userns tasks whereas the change seems to me to allows every task access
> > > > > to that fuse filesystem independent of what userns they are in.
> > > > >
> > > > > So even a task in a sibling userns could - probably with rather
> > > > > elaborate mount propagation trickery - access that fuse filesystem.
> > > > >
> > > > > AFaict, either the module parameter is misnamed or the patch doesn't
> > > > > implement the behavior expressed in the name.
> > > > >
> > > > > The original patch restricted access to a CAP_SYS_ADMIN capable task.
> > > > > Did we agree that it was a good idea to weaken it to all tasks?
> > > > > Shouldn't we still just restrict this to CAP_SYS_ADMIN capable tasks in
> > > > > the initial userns?
> > > >
> > > > I think it's fine to allow for CAP_SYS_ADMIN only, but can we then
> > > > ignore the allow_other mount option in such case? The idea is that
> > > > CAP_SYS_ADMIN allows you to read FUSE-backed contents no matter what, so
> > > > user not mounting with allow_other preventing root from reading contents
> > > > defeats the purpose at least partially.
> > >
> > > If we want to be compatible with "user_allow_other", then it should be
> > > checking if the uid/gid of the current task is mapped in the
> > > filesystems user_ns (fsuidgid_has_mapping()).  Right?
> >
> > I think that's doable. So assuming we're still talking about requiring
> > cap_sys_admin then we'd roughly have sm like:
> >
> >         if (fc->allow_other)
> >                 return current_in_userns(fc->user_ns) ||
> >                         (capable(CAP_SYS_ADMIN) &&
> >                         fsuidgid_has_mapping(..., &init_user_ns));
> 
> No, I meant this:
> 
>         if (fc->allow_other)
>                 return current_in_userns(fc->user_ns) ||
>                         (userns_allow_other &&
>                         fsuidgid_has_mapping(..., &init_user_ns));
> 
> But I think the OP wanted to allow real root to access the fs, which
> this doesn't allow (since 0 will have no mapping in the user ns), so
> I'm not sure what's the right solution...

I aimed to show that. You can setfs*id() and retain capabilities and
still access the filesystem.

> 
> Maybe the original patch is fine: this check isn't meant to protect
> the filesystem from access, it's meant to protect the accessor.

I don't have specific worries here. I'm just a bit hesitant to just let
anyone access the fs. But if we go for allow other semantics then that's
probably fine. Though I wonder why then we don't just do:

if (fc->allow_other)
        return current_in_userns(fc->user_ns) ||
                (userns_allow_other &&
                ns_capable(fc->user_ns, CAP_SYS_ADMIN));

? That'll let any ancestor userns access the fs not just descendants of
fc->user_ns.

Re: [PATCH v2] fuse: Add module param for non-descendant userns access to allow_other

[Miklos Szeredi]

On Mon, 13 Jun 2022 at 12:46, Christian Brauner <brauner@kernel.org> wrote:
>
> On Mon, Jun 13, 2022 at 12:34:05PM +0200, Miklos Szeredi wrote:
> > On Mon, 13 Jun 2022 at 11:37, Christian Brauner <brauner@kernel.org> wrote:
> > >
> > > On Mon, Jun 13, 2022 at 10:23:47AM +0200, Miklos Szeredi wrote:
> > > > On Fri, 10 Jun 2022 at 23:39, Andrii Nakryiko <andriin@fb.com> wrote:
> > > > >
> > > > >
> > > > >
> > > > > On 6/7/22 1:47 AM, Christian Brauner wrote:
> > > > > > On Wed, Jun 01, 2022 at 11:44:07AM -0700, Dave Marchevsky wrote:
> > > >
> > > > [...]
> > > >
> > > > > >> +static bool __read_mostly allow_other_parent_userns;
> > > > > >> +module_param(allow_other_parent_userns, bool, 0644);
> > > > > >> +MODULE_PARM_DESC(allow_other_parent_userns,
> > > > > >> + "Allow users not in mounting or descendant userns "
> > > > > >> + "to access FUSE with allow_other set");
> > > > > >
> > > > > > The name of the parameter also suggests that access is granted to parent
> > > > > > userns tasks whereas the change seems to me to allows every task access
> > > > > > to that fuse filesystem independent of what userns they are in.
> > > > > >
> > > > > > So even a task in a sibling userns could - probably with rather
> > > > > > elaborate mount propagation trickery - access that fuse filesystem.
> > > > > >
> > > > > > AFaict, either the module parameter is misnamed or the patch doesn't
> > > > > > implement the behavior expressed in the name.
> > > > > >
> > > > > > The original patch restricted access to a CAP_SYS_ADMIN capable task.
> > > > > > Did we agree that it was a good idea to weaken it to all tasks?
> > > > > > Shouldn't we still just restrict this to CAP_SYS_ADMIN capable tasks in
> > > > > > the initial userns?
> > > > >
> > > > > I think it's fine to allow for CAP_SYS_ADMIN only, but can we then
> > > > > ignore the allow_other mount option in such case? The idea is that
> > > > > CAP_SYS_ADMIN allows you to read FUSE-backed contents no matter what, so
> > > > > user not mounting with allow_other preventing root from reading contents
> > > > > defeats the purpose at least partially.
> > > >
> > > > If we want to be compatible with "user_allow_other", then it should be
> > > > checking if the uid/gid of the current task is mapped in the
> > > > filesystems user_ns (fsuidgid_has_mapping()).  Right?
> > >
> > > I think that's doable. So assuming we're still talking about requiring
> > > cap_sys_admin then we'd roughly have sm like:
> > >
> > >         if (fc->allow_other)
> > >                 return current_in_userns(fc->user_ns) ||
> > >                         (capable(CAP_SYS_ADMIN) &&
> > >                         fsuidgid_has_mapping(..., &init_user_ns));
> >
> > No, I meant this:
> >
> >         if (fc->allow_other)
> >                 return current_in_userns(fc->user_ns) ||
> >                         (userns_allow_other &&
> >                         fsuidgid_has_mapping(..., &init_user_ns));
> >
> > But I think the OP wanted to allow real root to access the fs, which
> > this doesn't allow (since 0 will have no mapping in the user ns), so
> > I'm not sure what's the right solution...
>
> I aimed to show that. You can setfs*id() and retain capabilities and
> still access the filesystem.
>
> >
> > Maybe the original patch is fine: this check isn't meant to protect
> > the filesystem from access, it's meant to protect the accessor.
>
> I don't have specific worries here. I'm just a bit hesitant to just let
> anyone access the fs. But if we go for allow other semantics then that's
> probably fine. Though I wonder why then we don't just do:
>
> if (fc->allow_other)
>         return current_in_userns(fc->user_ns) ||
>                 (userns_allow_other &&
>                 ns_capable(fc->user_ns, CAP_SYS_ADMIN));
>
> ? That'll let any ancestor userns access the fs not just descendants of
> fc->user_ns.

Looks good to me.

Thanks,
Miklos

Re: [PATCH v2] fuse: Add module param for non-descendant userns access to allow_other

[Andrii Nakryiko]

On Mon, Jun 13, 2022 at 3:34 AM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Mon, 13 Jun 2022 at 11:37, Christian Brauner <brauner@kernel.org> wrote:
> >
> > On Mon, Jun 13, 2022 at 10:23:47AM +0200, Miklos Szeredi wrote:
> > > On Fri, 10 Jun 2022 at 23:39, Andrii Nakryiko <andriin@fb.com> wrote:
> > > >
> > > >
> > > >
> > > > On 6/7/22 1:47 AM, Christian Brauner wrote:
> > > > > On Wed, Jun 01, 2022 at 11:44:07AM -0700, Dave Marchevsky wrote:
> > >
> > > [...]
> > >
> > > > >> +static bool __read_mostly allow_other_parent_userns;
> > > > >> +module_param(allow_other_parent_userns, bool, 0644);
> > > > >> +MODULE_PARM_DESC(allow_other_parent_userns,
> > > > >> + "Allow users not in mounting or descendant userns "
> > > > >> + "to access FUSE with allow_other set");
> > > > >
> > > > > The name of the parameter also suggests that access is granted to parent
> > > > > userns tasks whereas the change seems to me to allows every task access
> > > > > to that fuse filesystem independent of what userns they are in.
> > > > >
> > > > > So even a task in a sibling userns could - probably with rather
> > > > > elaborate mount propagation trickery - access that fuse filesystem.
> > > > >
> > > > > AFaict, either the module parameter is misnamed or the patch doesn't
> > > > > implement the behavior expressed in the name.
> > > > >
> > > > > The original patch restricted access to a CAP_SYS_ADMIN capable task.
> > > > > Did we agree that it was a good idea to weaken it to all tasks?
> > > > > Shouldn't we still just restrict this to CAP_SYS_ADMIN capable tasks in
> > > > > the initial userns?
> > > >
> > > > I think it's fine to allow for CAP_SYS_ADMIN only, but can we then
> > > > ignore the allow_other mount option in such case? The idea is that
> > > > CAP_SYS_ADMIN allows you to read FUSE-backed contents no matter what, so
> > > > user not mounting with allow_other preventing root from reading contents
> > > > defeats the purpose at least partially.
> > >
> > > If we want to be compatible with "user_allow_other", then it should be
> > > checking if the uid/gid of the current task is mapped in the
> > > filesystems user_ns (fsuidgid_has_mapping()).  Right?
> >
> > I think that's doable. So assuming we're still talking about requiring
> > cap_sys_admin then we'd roughly have sm like:
> >
> >         if (fc->allow_other)
> >                 return current_in_userns(fc->user_ns) ||
> >                         (capable(CAP_SYS_ADMIN) &&
> >                         fsuidgid_has_mapping(..., &init_user_ns));
>
> No, I meant this:
>
>         if (fc->allow_other)
>                 return current_in_userns(fc->user_ns) ||
>                         (userns_allow_other &&
>                         fsuidgid_has_mapping(..., &init_user_ns));
>
> But I think the OP wanted to allow real root to access the fs, which
> this doesn't allow (since 0 will have no mapping in the user ns), so
> I'm not sure what's the right solution...

Right, so I was basically asking why not do something like this:

$ git diff
diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index 74303d6e987b..8c04955eb26e 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -1224,6 +1224,9 @@ int fuse_allow_current_process(struct fuse_conn *fc)
 {
        const struct cred *cred;

+       if (fuse_allow_sys_admin_access && capable(CAP_SYS_ADMIN))
+               return 1;
+
        if (fc->allow_other)
                return current_in_userns(fc->user_ns);


where fuse_allow_sys_admin_access is module param which has to be
opted into through sysfs?

>
> Maybe the original patch is fine: this check isn't meant to protect
> the filesystem from access, it's meant to protect the accessor.
>
> Thanks,
> Miklos

Re: [PATCH v2] fuse: Add module param for non-descendant userns access to allow_other

[Christian Brauner]

On Mon, Jun 13, 2022 at 11:21:24AM -0700, Andrii Nakryiko wrote:
> On Mon, Jun 13, 2022 at 3:34 AM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Mon, 13 Jun 2022 at 11:37, Christian Brauner <brauner@kernel.org> wrote:
> > >
> > > On Mon, Jun 13, 2022 at 10:23:47AM +0200, Miklos Szeredi wrote:
> > > > On Fri, 10 Jun 2022 at 23:39, Andrii Nakryiko <andriin@fb.com> wrote:
> > > > >
> > > > >
> > > > >
> > > > > On 6/7/22 1:47 AM, Christian Brauner wrote:
> > > > > > On Wed, Jun 01, 2022 at 11:44:07AM -0700, Dave Marchevsky wrote:
> > > >
> > > > [...]
> > > >
> > > > > >> +static bool __read_mostly allow_other_parent_userns;
> > > > > >> +module_param(allow_other_parent_userns, bool, 0644);
> > > > > >> +MODULE_PARM_DESC(allow_other_parent_userns,
> > > > > >> + "Allow users not in mounting or descendant userns "
> > > > > >> + "to access FUSE with allow_other set");
> > > > > >
> > > > > > The name of the parameter also suggests that access is granted to parent
> > > > > > userns tasks whereas the change seems to me to allows every task access
> > > > > > to that fuse filesystem independent of what userns they are in.
> > > > > >
> > > > > > So even a task in a sibling userns could - probably with rather
> > > > > > elaborate mount propagation trickery - access that fuse filesystem.
> > > > > >
> > > > > > AFaict, either the module parameter is misnamed or the patch doesn't
> > > > > > implement the behavior expressed in the name.
> > > > > >
> > > > > > The original patch restricted access to a CAP_SYS_ADMIN capable task.
> > > > > > Did we agree that it was a good idea to weaken it to all tasks?
> > > > > > Shouldn't we still just restrict this to CAP_SYS_ADMIN capable tasks in
> > > > > > the initial userns?
> > > > >
> > > > > I think it's fine to allow for CAP_SYS_ADMIN only, but can we then
> > > > > ignore the allow_other mount option in such case? The idea is that
> > > > > CAP_SYS_ADMIN allows you to read FUSE-backed contents no matter what, so
> > > > > user not mounting with allow_other preventing root from reading contents
> > > > > defeats the purpose at least partially.
> > > >
> > > > If we want to be compatible with "user_allow_other", then it should be
> > > > checking if the uid/gid of the current task is mapped in the
> > > > filesystems user_ns (fsuidgid_has_mapping()).  Right?
> > >
> > > I think that's doable. So assuming we're still talking about requiring
> > > cap_sys_admin then we'd roughly have sm like:
> > >
> > >         if (fc->allow_other)
> > >                 return current_in_userns(fc->user_ns) ||
> > >                         (capable(CAP_SYS_ADMIN) &&
> > >                         fsuidgid_has_mapping(..., &init_user_ns));
> >
> > No, I meant this:
> >
> >         if (fc->allow_other)
> >                 return current_in_userns(fc->user_ns) ||
> >                         (userns_allow_other &&
> >                         fsuidgid_has_mapping(..., &init_user_ns));
> >
> > But I think the OP wanted to allow real root to access the fs, which
> > this doesn't allow (since 0 will have no mapping in the user ns), so
> > I'm not sure what's the right solution...
> 
> Right, so I was basically asking why not do something like this:
> 
> $ git diff
> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> index 74303d6e987b..8c04955eb26e 100644
> --- a/fs/fuse/dir.c
> +++ b/fs/fuse/dir.c
> @@ -1224,6 +1224,9 @@ int fuse_allow_current_process(struct fuse_conn *fc)
>  {
>         const struct cred *cred;
> 
> +       if (fuse_allow_sys_admin_access && capable(CAP_SYS_ADMIN))
> +               return 1;
> +
>         if (fc->allow_other)
>                 return current_in_userns(fc->user_ns);
> 
> 
> where fuse_allow_sys_admin_access is module param which has to be
> opted into through sysfs?

You can either do this or do what I suggested in:
https://lore.kernel.org/linux-fsdevel/20220613104604.t5ptuhrl2d4l7kbl@wittgenstein
which is a bit more lax.

If you make it module load parameter only it has the advantage that it
can't be changed after fuse has been loaded which in this case might be
an advantage. It's likely that users might not be too happy if module
semantics can be changed that drastically at runtime. But I have no
strong opinions here.

Christian

Re: [PATCH v2] fuse: Add module param for non-descendant userns access to allow_other

[Andrii Nakryiko]

On Tue, Jun 14, 2022 at 7:33 AM Christian Brauner <brauner@kernel.org> wrote:
>
> On Mon, Jun 13, 2022 at 11:21:24AM -0700, Andrii Nakryiko wrote:
> > On Mon, Jun 13, 2022 at 3:34 AM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > >
> > > On Mon, 13 Jun 2022 at 11:37, Christian Brauner <brauner@kernel.org> wrote:
> > > >
> > > > On Mon, Jun 13, 2022 at 10:23:47AM +0200, Miklos Szeredi wrote:
> > > > > On Fri, 10 Jun 2022 at 23:39, Andrii Nakryiko <andriin@fb.com> wrote:
> > > > > >
> > > > > >
> > > > > >
> > > > > > On 6/7/22 1:47 AM, Christian Brauner wrote:
> > > > > > > On Wed, Jun 01, 2022 at 11:44:07AM -0700, Dave Marchevsky wrote:
> > > > >
> > > > > [...]
> > > > >
> > > > > > >> +static bool __read_mostly allow_other_parent_userns;
> > > > > > >> +module_param(allow_other_parent_userns, bool, 0644);
> > > > > > >> +MODULE_PARM_DESC(allow_other_parent_userns,
> > > > > > >> + "Allow users not in mounting or descendant userns "
> > > > > > >> + "to access FUSE with allow_other set");
> > > > > > >
> > > > > > > The name of the parameter also suggests that access is granted to parent
> > > > > > > userns tasks whereas the change seems to me to allows every task access
> > > > > > > to that fuse filesystem independent of what userns they are in.
> > > > > > >
> > > > > > > So even a task in a sibling userns could - probably with rather
> > > > > > > elaborate mount propagation trickery - access that fuse filesystem.
> > > > > > >
> > > > > > > AFaict, either the module parameter is misnamed or the patch doesn't
> > > > > > > implement the behavior expressed in the name.
> > > > > > >
> > > > > > > The original patch restricted access to a CAP_SYS_ADMIN capable task.
> > > > > > > Did we agree that it was a good idea to weaken it to all tasks?
> > > > > > > Shouldn't we still just restrict this to CAP_SYS_ADMIN capable tasks in
> > > > > > > the initial userns?
> > > > > >
> > > > > > I think it's fine to allow for CAP_SYS_ADMIN only, but can we then
> > > > > > ignore the allow_other mount option in such case? The idea is that
> > > > > > CAP_SYS_ADMIN allows you to read FUSE-backed contents no matter what, so
> > > > > > user not mounting with allow_other preventing root from reading contents
> > > > > > defeats the purpose at least partially.
> > > > >
> > > > > If we want to be compatible with "user_allow_other", then it should be
> > > > > checking if the uid/gid of the current task is mapped in the
> > > > > filesystems user_ns (fsuidgid_has_mapping()).  Right?
> > > >
> > > > I think that's doable. So assuming we're still talking about requiring
> > > > cap_sys_admin then we'd roughly have sm like:
> > > >
> > > >         if (fc->allow_other)
> > > >                 return current_in_userns(fc->user_ns) ||
> > > >                         (capable(CAP_SYS_ADMIN) &&
> > > >                         fsuidgid_has_mapping(..., &init_user_ns));
> > >
> > > No, I meant this:
> > >
> > >         if (fc->allow_other)
> > >                 return current_in_userns(fc->user_ns) ||
> > >                         (userns_allow_other &&
> > >                         fsuidgid_has_mapping(..., &init_user_ns));
> > >
> > > But I think the OP wanted to allow real root to access the fs, which
> > > this doesn't allow (since 0 will have no mapping in the user ns), so
> > > I'm not sure what's the right solution...
> >
> > Right, so I was basically asking why not do something like this:
> >
> > $ git diff
> > diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> > index 74303d6e987b..8c04955eb26e 100644
> > --- a/fs/fuse/dir.c
> > +++ b/fs/fuse/dir.c
> > @@ -1224,6 +1224,9 @@ int fuse_allow_current_process(struct fuse_conn *fc)
> >  {
> >         const struct cred *cred;
> >
> > +       if (fuse_allow_sys_admin_access && capable(CAP_SYS_ADMIN))
> > +               return 1;
> > +
> >         if (fc->allow_other)
> >                 return current_in_userns(fc->user_ns);
> >
> >
> > where fuse_allow_sys_admin_access is module param which has to be
> > opted into through sysfs?
>
> You can either do this or do what I suggested in:
> https://lore.kernel.org/linux-fsdevel/20220613104604.t5ptuhrl2d4l7kbl@wittgenstein
> which is a bit more lax.

My logic was that given we require opt-in and we are root, we
shouldn't be prevented from reading contents just because someone
didn't know about allow_other mount option. So I'd go with a simple
check before we even check fc-allow_other.

>
> If you make it module load parameter only it has the advantage that it
> can't be changed after fuse has been loaded which in this case might be
> an advantage. It's likely that users might not be too happy if module
> semantics can be changed that drastically at runtime. But I have no
> strong opinions here.
>

I'm not too familiar with this, whatever Dave was doing with
MODULE_PARM_DESC seems to be working fine? Did you have some other
preference for a specific param mechanism?

> Christian

Re: [PATCH v2] fuse: Add module param for non-descendant userns access to allow_other

[Christian Brauner]

On Wed, Jun 15, 2022 at 04:36:35PM -0700, Andrii Nakryiko wrote:
> On Tue, Jun 14, 2022 at 7:33 AM Christian Brauner <brauner@kernel.org> wrote:
> >
> > On Mon, Jun 13, 2022 at 11:21:24AM -0700, Andrii Nakryiko wrote:
> > > On Mon, Jun 13, 2022 at 3:34 AM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > > >
> > > > On Mon, 13 Jun 2022 at 11:37, Christian Brauner <brauner@kernel.org> wrote:
> > > > >
> > > > > On Mon, Jun 13, 2022 at 10:23:47AM +0200, Miklos Szeredi wrote:
> > > > > > On Fri, 10 Jun 2022 at 23:39, Andrii Nakryiko <andriin@fb.com> wrote:
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > On 6/7/22 1:47 AM, Christian Brauner wrote:
> > > > > > > > On Wed, Jun 01, 2022 at 11:44:07AM -0700, Dave Marchevsky wrote:
> > > > > >
> > > > > > [...]
> > > > > >
> > > > > > > >> +static bool __read_mostly allow_other_parent_userns;
> > > > > > > >> +module_param(allow_other_parent_userns, bool, 0644);
> > > > > > > >> +MODULE_PARM_DESC(allow_other_parent_userns,
> > > > > > > >> + "Allow users not in mounting or descendant userns "
> > > > > > > >> + "to access FUSE with allow_other set");
> > > > > > > >
> > > > > > > > The name of the parameter also suggests that access is granted to parent
> > > > > > > > userns tasks whereas the change seems to me to allows every task access
> > > > > > > > to that fuse filesystem independent of what userns they are in.
> > > > > > > >
> > > > > > > > So even a task in a sibling userns could - probably with rather
> > > > > > > > elaborate mount propagation trickery - access that fuse filesystem.
> > > > > > > >
> > > > > > > > AFaict, either the module parameter is misnamed or the patch doesn't
> > > > > > > > implement the behavior expressed in the name.
> > > > > > > >
> > > > > > > > The original patch restricted access to a CAP_SYS_ADMIN capable task.
> > > > > > > > Did we agree that it was a good idea to weaken it to all tasks?
> > > > > > > > Shouldn't we still just restrict this to CAP_SYS_ADMIN capable tasks in
> > > > > > > > the initial userns?
> > > > > > >
> > > > > > > I think it's fine to allow for CAP_SYS_ADMIN only, but can we then
> > > > > > > ignore the allow_other mount option in such case? The idea is that
> > > > > > > CAP_SYS_ADMIN allows you to read FUSE-backed contents no matter what, so
> > > > > > > user not mounting with allow_other preventing root from reading contents
> > > > > > > defeats the purpose at least partially.
> > > > > >
> > > > > > If we want to be compatible with "user_allow_other", then it should be
> > > > > > checking if the uid/gid of the current task is mapped in the
> > > > > > filesystems user_ns (fsuidgid_has_mapping()).  Right?
> > > > >
> > > > > I think that's doable. So assuming we're still talking about requiring
> > > > > cap_sys_admin then we'd roughly have sm like:
> > > > >
> > > > >         if (fc->allow_other)
> > > > >                 return current_in_userns(fc->user_ns) ||
> > > > >                         (capable(CAP_SYS_ADMIN) &&
> > > > >                         fsuidgid_has_mapping(..., &init_user_ns));
> > > >
> > > > No, I meant this:
> > > >
> > > >         if (fc->allow_other)
> > > >                 return current_in_userns(fc->user_ns) ||
> > > >                         (userns_allow_other &&
> > > >                         fsuidgid_has_mapping(..., &init_user_ns));
> > > >
> > > > But I think the OP wanted to allow real root to access the fs, which
> > > > this doesn't allow (since 0 will have no mapping in the user ns), so
> > > > I'm not sure what's the right solution...
> > >
> > > Right, so I was basically asking why not do something like this:
> > >
> > > $ git diff
> > > diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> > > index 74303d6e987b..8c04955eb26e 100644
> > > --- a/fs/fuse/dir.c
> > > +++ b/fs/fuse/dir.c
> > > @@ -1224,6 +1224,9 @@ int fuse_allow_current_process(struct fuse_conn *fc)
> > >  {
> > >         const struct cred *cred;
> > >
> > > +       if (fuse_allow_sys_admin_access && capable(CAP_SYS_ADMIN))
> > > +               return 1;
> > > +
> > >         if (fc->allow_other)
> > >                 return current_in_userns(fc->user_ns);
> > >
> > >
> > > where fuse_allow_sys_admin_access is module param which has to be
> > > opted into through sysfs?
> >
> > You can either do this or do what I suggested in:
> > https://lore.kernel.org/linux-fsdevel/20220613104604.t5ptuhrl2d4l7kbl@wittgenstein
> > which is a bit more lax.
> 
> My logic was that given we require opt-in and we are root, we
> shouldn't be prevented from reading contents just because someone
> didn't know about allow_other mount option. So I'd go with a simple
> check before we even check fc-allow_other.

I don't see a problem with this but it other than that it subverts the
allow_other mount option a bit tbh...

> 
> >
> > If you make it module load parameter only it has the advantage that it
> > can't be changed after fuse has been loaded which in this case might be
> > an advantage. It's likely that users might not be too happy if module
> > semantics can be changed that drastically at runtime. But I have no
> > strong opinions here.
> >
> 
> I'm not too familiar with this, whatever Dave was doing with
> MODULE_PARM_DESC seems to be working fine? Did you have some other
> preference for a specific param mechanism?

Nope, that one seems fine.

Re: [PATCH v2] fuse: Add module param for non-descendant userns access to allow_other

[Dave Marchevsky]

On 6/16/22 4:01 AM, Christian Brauner wrote:   
> On Wed, Jun 15, 2022 at 04:36:35PM -0700, Andrii Nakryiko wrote:
>> On Tue, Jun 14, 2022 at 7:33 AM Christian Brauner <brauner@kernel.org> wrote:
>>>
>>> On Mon, Jun 13, 2022 at 11:21:24AM -0700, Andrii Nakryiko wrote:
>>>> On Mon, Jun 13, 2022 at 3:34 AM Miklos Szeredi <miklos@szeredi.hu> wrote:
>>>>>
>>>>> On Mon, 13 Jun 2022 at 11:37, Christian Brauner <brauner@kernel.org> wrote:
>>>>>>
>>>>>> On Mon, Jun 13, 2022 at 10:23:47AM +0200, Miklos Szeredi wrote:
>>>>>>> On Fri, 10 Jun 2022 at 23:39, Andrii Nakryiko <andriin@fb.com> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 6/7/22 1:47 AM, Christian Brauner wrote:
>>>>>>>>> On Wed, Jun 01, 2022 at 11:44:07AM -0700, Dave Marchevsky wrote:
>>>>>>>
>>>>>>> [...]
>>>>>>>
>>>>>>>>>> +static bool __read_mostly allow_other_parent_userns;
>>>>>>>>>> +module_param(allow_other_parent_userns, bool, 0644);
>>>>>>>>>> +MODULE_PARM_DESC(allow_other_parent_userns,
>>>>>>>>>> + "Allow users not in mounting or descendant userns "
>>>>>>>>>> + "to access FUSE with allow_other set");
>>>>>>>>>
>>>>>>>>> The name of the parameter also suggests that access is granted to parent
>>>>>>>>> userns tasks whereas the change seems to me to allows every task access
>>>>>>>>> to that fuse filesystem independent of what userns they are in.
>>>>>>>>>
>>>>>>>>> So even a task in a sibling userns could - probably with rather
>>>>>>>>> elaborate mount propagation trickery - access that fuse filesystem.
>>>>>>>>>
>>>>>>>>> AFaict, either the module parameter is misnamed or the patch doesn't
>>>>>>>>> implement the behavior expressed in the name.
>>>>>>>>>
>>>>>>>>> The original patch restricted access to a CAP_SYS_ADMIN capable task.
>>>>>>>>> Did we agree that it was a good idea to weaken it to all tasks?
>>>>>>>>> Shouldn't we still just restrict this to CAP_SYS_ADMIN capable tasks in
>>>>>>>>> the initial userns?
>>>>>>>>
>>>>>>>> I think it's fine to allow for CAP_SYS_ADMIN only, but can we then
>>>>>>>> ignore the allow_other mount option in such case? The idea is that
>>>>>>>> CAP_SYS_ADMIN allows you to read FUSE-backed contents no matter what, so
>>>>>>>> user not mounting with allow_other preventing root from reading contents
>>>>>>>> defeats the purpose at least partially.
>>>>>>>
>>>>>>> If we want to be compatible with "user_allow_other", then it should be
>>>>>>> checking if the uid/gid of the current task is mapped in the
>>>>>>> filesystems user_ns (fsuidgid_has_mapping()).  Right?
>>>>>>
>>>>>> I think that's doable. So assuming we're still talking about requiring
>>>>>> cap_sys_admin then we'd roughly have sm like:
>>>>>>
>>>>>>         if (fc->allow_other)
>>>>>>                 return current_in_userns(fc->user_ns) ||
>>>>>>                         (capable(CAP_SYS_ADMIN) &&
>>>>>>                         fsuidgid_has_mapping(..., &init_user_ns));
>>>>>
>>>>> No, I meant this:
>>>>>
>>>>>         if (fc->allow_other)
>>>>>                 return current_in_userns(fc->user_ns) ||
>>>>>                         (userns_allow_other &&
>>>>>                         fsuidgid_has_mapping(..., &init_user_ns));
>>>>>
>>>>> But I think the OP wanted to allow real root to access the fs, which
>>>>> this doesn't allow (since 0 will have no mapping in the user ns), so
>>>>> I'm not sure what's the right solution...
>>>>
>>>> Right, so I was basically asking why not do something like this:
>>>>
>>>> $ git diff
>>>> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
>>>> index 74303d6e987b..8c04955eb26e 100644
>>>> --- a/fs/fuse/dir.c
>>>> +++ b/fs/fuse/dir.c
>>>> @@ -1224,6 +1224,9 @@ int fuse_allow_current_process(struct fuse_conn *fc)
>>>>  {
>>>>         const struct cred *cred;
>>>>
>>>> +       if (fuse_allow_sys_admin_access && capable(CAP_SYS_ADMIN))
>>>> +               return 1;
>>>> +
>>>>         if (fc->allow_other)
>>>>                 return current_in_userns(fc->user_ns);
>>>>
>>>>
>>>> where fuse_allow_sys_admin_access is module param which has to be
>>>> opted into through sysfs?
>>>
>>> You can either do this or do what I suggested in:
>>> https://lore.kernel.org/linux-fsdevel/20220613104604.t5ptuhrl2d4l7kbl@wittgenstein
>>> which is a bit more lax.
>>
>> My logic was that given we require opt-in and we are root, we
>> shouldn't be prevented from reading contents just because someone
>> didn't know about allow_other mount option. So I'd go with a simple
>> check before we even check fc-allow_other.
> 
> I don't see a problem with this but it other than that it subverts the
> allow_other mount option a bit tbh...

Will send v3 doing this shortly.

>>
>>>
>>> If you make it module load parameter only it has the advantage that it
>>> can't be changed after fuse has been loaded which in this case might be
>>> an advantage. It's likely that users might not be too happy if module
>>> semantics can be changed that drastically at runtime. But I have no
>>> strong opinions here.
>>>
>>
>> I'm not too familiar with this, whatever Dave was doing with
>> MODULE_PARM_DESC seems to be working fine? Did you have some other
>> preference for a specific param mechanism?
> 
> Nope, that one seems fine.

For our usecase, changing the behavior after FUSE module has been loaded is
preferable. Otherwise perf - or our internal tool - would have to emit some
message like "can't symbolicate these paths, please reload FUSE and try again"