proc: subset=pid: Relax check of mount visibility

Hello Alexey,

Would it be possible to revive this patch series?

I wanted to add an additional downstream use case that would benefit from this work. In particular, I am trying to run the sandbox sunwalker-box [1] without root privileges and/or inside a container.

The sandbox aims to prevent cross-run communication via side channels, and PID allocation is one such channel. Therefore, it creates a new PID namespace and mounts the corresponding procfs instance inside of the sandbox. This currently works without a real root when procfs is fully accessible, but obviously fails otherwise.

Thanks,
Dan Klishch

[1] https://github.com/purplesyringa/sunwalker-box/

It is much easier to implement file access
restrictions in procfs using an ebpf controller.

Quoted 2 lines

It is much easier to implement file access
restrictions in procfs using an ebpf controller.

But if we already have a masked /proc from podman/docker/user who decided to run mount --bind /dev/null /proc/smth, the sandbox will not have a choice other than to bail out.

Also, correct me if I am wrong, installing ebpf controller requires CAP_BPF in initial userns, so rootless podman will not be able to mask /proc "properly" even if someone sends a patch switching it to ebpf.

But then, if I understand you correctly, this patch will not be enough for you. procfs with subset=pid will not allow you to have /proc/meminfo, /proc/cpuinfo, etc.

Quoted 3 lines

Also, correct me if I am wrong, installing ebpf controller requires CAP_BPF in initial userns, so rootless podman will not be able to mask /proc "properly" even if someone sends a patch switching it to ebpf.

You can turn on /proc/sys/kernel/unprivileged_bpf_disabled.

Quoted 3 lines · Alexey Gladkov

But then, if I understand you correctly, this patch will not be enough for you. procfs with subset=pid will not allow you to have /proc/meminfo, /proc/cpuinfo, etc.

Hmm, I didn't think of this. sunwalker-box only exposes cpuinfo and PID tree to the sandboxed programs (empirically, this is enough for most of programs you want sandboxing for). With that in mind, this patch and a FUSE providing an overlay with cpuinfo / seccomp intercepting opens of /proc/cpuinfo / a small kernel patch with a new mount option for procfs to expose more static files still look like a clean solution to me.

Quoted 5 lines

Quoted 3 lines

Also, correct me if I am wrong, installing ebpf controller requires CAP_BPF in initial userns, so rootless podman will not be able to mask /proc "properly" even if someone sends a patch switching it to ebpf.

You can turn on /proc/sys/kernel/unprivileged_bpf_disabled.

$ cat /proc/sys/kernel/unprivileged_bpf_disabled
0
$ unshare -pfr --mount-proc
$ ./proc-controller -p deny /proc/cpuinfo
libbpf: prog 'proc_access_restrict': BPF program load failed: Operation not permitted
libbpf: prog 'proc_access_restrict': failed to load: -1
libbpf: failed to load object './proc-controller.bpf.o'
proc-controller: ERROR: loading BPF object file failed

I think only packet filters are allowed to be installed by non-root.

Quoted 3 lines · Alexey Gladkov

But then, if I understand you correctly, this patch will not be enough for you. procfs with subset=pid will not allow you to have /proc/meminfo, /proc/cpuinfo, etc.

Hmm, I didn't think of this. sunwalker-box only exposes cpuinfo and PID tree to the sandboxed programs (empirically, this is enough for most of programs you want sandboxing for). With that in mind, this patch and a FUSE providing an overlay with cpuinfo / seccomp intercepting opens of /proc/cpuinfo / a small kernel patch with a new mount option for procfs to expose more static files still look like a clean solution to me.

Quoted 4 lines

Quoted 4 lines

Also, correct me if I am wrong, installing ebpf controller requires CAP_BPF in initial userns, so rootless podman will not be able to mask /proc "properly" even if someone sends a patch switching it to ebpf.

Quoted 11 lines · Dan Klishch

Quoted 3 lines · Alexey Gladkov

But then, if I understand you correctly, this patch will not be enough for you. procfs with subset=pid will not allow you to have /proc/meminfo, /proc/cpuinfo, etc.

Hmm, I didn't think of this. sunwalker-box only exposes cpuinfo and PID tree to the sandboxed programs (empirically, this is enough for most of programs you want sandboxing for). With that in mind, this patch and a FUSE providing an overlay with cpuinfo / seccomp intercepting opens of /proc/cpuinfo / a small kernel patch with a new mount option for procfs to expose more static files still look like a clean solution to me.

I don't think you'll be able to do that. procfs doesn't allow itself to be overlayed [1]. What should block mounting overlayfs and fuse on top of procfs.

[1] https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/proc/root.c#n274

The standard way of making it possible to mount procfs inside of a container with a separate mount namespace that has a procfs inside it with overmounted entries is to ensure that a fully-visible procfs instance is present.

The container needs to inherit a fully-visible instance somehow if you want nesting. Using an unprivileged LSM such as landlock to prevent any access to the fully visible procfs instance is usually the better way.

My hope is that once signed bpf is more widely adopted that distros will just start enabling blessed bpf programs that will just take on the access protecting instead of the clumsy bind-mount protection mechanism.

Quoted 13 lines · Alexey Gladkov

Quoted 11 lines · Dan Klishch

Quoted 3 lines · Alexey Gladkov

But then, if I understand you correctly, this patch will not be enough for you. procfs with subset=pid will not allow you to have /proc/meminfo, /proc/cpuinfo, etc.

Hmm, I didn't think of this. sunwalker-box only exposes cpuinfo and PID tree to the sandboxed programs (empirically, this is enough for most of programs you want sandboxing for). With that in mind, this patch and a FUSE providing an overlay with cpuinfo / seccomp intercepting opens of /proc/cpuinfo / a small kernel patch with a new mount option for procfs to expose more static files still look like a clean solution to me.

Quoted 5 lines

I don't think you'll be able to do that. procfs doesn't allow itself to be overlayed [1]. What should block mounting overlayfs and fuse on top of procfs.

[1] https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/proc/root.c#n274

This is why I have been careful not to say overlayfs. With [2] (warning:
zero-shot ChatGPT output), I can do:

$ ./fuse-overlay target --source=/proc
$ ls target
1   88   194   1374    889840  908552
2   90   195   1375    889987  908619
3   91   196   1379    890031  908658
4   92   203   1412    890063  908756
5   93   205   1590    890085  908804
6   94   233   1644    890139  908951
7   96   237   1802    890246  909848
8   97   239   1850    890271  909914
10  98   240   1852    894665  909924
13  99   243   1865    895854  909926
15  100  244   1888    895864  910005
16  102  246   1889    896030  acpi
17  103  262   1891    896205  asound
18  104  263   1895    896508  bus
19  105  264   1896    896544  driver
20  106  265   1899    896706  dynamic_debug
<...>

[2] https://gist.github.com/DanShaders/547eeb74a90315356b98472feae47474

This requires a much more careful thought wrt magic symlinks and permission checks. The fact that I am highly unlikely to 100% correctly reimplement the checks and special behavior of procfs makes me not want to proceed with the FUSE route.

Quoted 4 lines · Christian Brauner

The standard way of making it possible to mount procfs inside of a container with a separate mount namespace that has a procfs inside it with overmounted entries is to ensure that a fully-visible procfs instance is present.

Yes, this is a solution. However, this is only marginally better than passing --privileged to the outer container (in a sense that we require outer sandbox to remove some protections for the inner sandbox to work).

Quoted 7 lines

The container needs to inherit a fully-visible instance somehow if you want nesting. Using an unprivileged LSM such as landlock to prevent any access to the fully visible procfs instance is usually the better way.

My hope is that once signed bpf is more widely adopted that distros will just start enabling blessed bpf programs that will just take on the access protecting instead of the clumsy bind-mount protection mechanism.

These are big changes to container runtimes that are unlikely to happen soon. In contrast, the patch we are discussing will be available in 2 months after the merge for me to use on ArchLinux, and in a couple more months on Ubuntu.

So, is there any way forward with the patch or should I continue trying to find a userspace solution?

Quoted 68 lines · Dan Klishch

Quoted 13 lines · Alexey Gladkov

Quoted 11 lines · Dan Klishch

Quoted 3 lines · Alexey Gladkov

But then, if I understand you correctly, this patch will not be enough for you. procfs with subset=pid will not allow you to have /proc/meminfo, /proc/cpuinfo, etc.

Hmm, I didn't think of this. sunwalker-box only exposes cpuinfo and PID tree to the sandboxed programs (empirically, this is enough for most of programs you want sandboxing for). With that in mind, this patch and a FUSE providing an overlay with cpuinfo / seccomp intercepting opens of /proc/cpuinfo / a small kernel patch with a new mount option for procfs to expose more static files still look like a clean solution to me.

Quoted 5 lines

I don't think you'll be able to do that. procfs doesn't allow itself to be overlayed [1]. What should block mounting overlayfs and fuse on top of procfs.

[1] https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/proc/root.c#n274

This is why I have been careful not to say overlayfs. With [2] (warning:
zero-shot ChatGPT output), I can do:

$ ./fuse-overlay target --source=/proc
$ ls target
1   88   194   1374    889840  908552
2   90   195   1375    889987  908619
3   91   196   1379    890031  908658
4   92   203   1412    890063  908756
5   93   205   1590    890085  908804
6   94   233   1644    890139  908951
7   96   237   1802    890246  909848
8   97   239   1850    890271  909914
10  98   240   1852    894665  909924
13  99   243   1865    895854  909926
15  100  244   1888    895864  910005
16  102  246   1889    896030  acpi
17  103  262   1891    896205  asound
18  104  263   1895    896508  bus
19  105  264   1896    896544  driver
20  106  265   1899    896706  dynamic_debug
<...>

[2] https://gist.github.com/DanShaders/547eeb74a90315356b98472feae47474

This requires a much more careful thought wrt magic symlinks and permission checks. The fact that I am highly unlikely to 100% correctly reimplement the checks and special behavior of procfs makes me not want to proceed with the FUSE route.

Quoted 4 lines · Christian Brauner

The standard way of making it possible to mount procfs inside of a container with a separate mount namespace that has a procfs inside it with overmounted entries is to ensure that a fully-visible procfs instance is present.

Yes, this is a solution. However, this is only marginally better than passing --privileged to the outer container (in a sense that we require outer sandbox to remove some protections for the inner sandbox to work).

Quoted 7 lines

The container needs to inherit a fully-visible instance somehow if you want nesting. Using an unprivileged LSM such as landlock to prevent any access to the fully visible procfs instance is usually the better way.

My hope is that once signed bpf is more widely adopted that distros will just start enabling blessed bpf programs that will just take on the access protecting instead of the clumsy bind-mount protection mechanism.

Quoted 7 lines

These are big changes to container runtimes that are unlikely to happen soon. In contrast, the patch we are discussing will be available in 2 months after the merge for me to use on ArchLinux, and in a couple more months on Ubuntu.

So, is there any way forward with the patch or should I continue trying to find a userspace solution?

I still consider these patches useful. I made them precisely to remove some of the restrictions we have for procfs because of global files in the root of this filesystem.

I can update and prepare a new version of patchset if Christian thinks it's useful too.

Quoted 77 lines · Alexey Gladkov

Quoted 75 lines · Dan Klishch

Quoted 13 lines · Alexey Gladkov

Quoted 11 lines · Dan Klishch

Quoted 3 lines · Alexey Gladkov

But then, if I understand you correctly, this patch will not be enough for you. procfs with subset=pid will not allow you to have /proc/meminfo, /proc/cpuinfo, etc.

Hmm, I didn't think of this. sunwalker-box only exposes cpuinfo and PID tree to the sandboxed programs (empirically, this is enough for most of programs you want sandboxing for). With that in mind, this patch and a FUSE providing an overlay with cpuinfo / seccomp intercepting opens of /proc/cpuinfo / a small kernel patch with a new mount option for procfs to expose more static files still look like a clean solution to me.

Quoted 5 lines

I don't think you'll be able to do that. procfs doesn't allow itself to be overlayed [1]. What should block mounting overlayfs and fuse on top of procfs.

[1] https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/proc/root.c#n274

This is why I have been careful not to say overlayfs. With [2] (warning:
zero-shot ChatGPT output), I can do:

$ ./fuse-overlay target --source=/proc
$ ls target
1   88   194   1374    889840  908552
2   90   195   1375    889987  908619
3   91   196   1379    890031  908658
4   92   203   1412    890063  908756
5   93   205   1590    890085  908804
6   94   233   1644    890139  908951
7   96   237   1802    890246  909848
8   97   239   1850    890271  909914
10  98   240   1852    894665  909924
13  99   243   1865    895854  909926
15  100  244   1888    895864  910005
16  102  246   1889    896030  acpi
17  103  262   1891    896205  asound
18  104  263   1895    896508  bus
19  105  264   1896    896544  driver
20  106  265   1899    896706  dynamic_debug
<...>

[2] https://gist.github.com/DanShaders/547eeb74a90315356b98472feae47474

This requires a much more careful thought wrt magic symlinks and permission checks. The fact that I am highly unlikely to 100% correctly reimplement the checks and special behavior of procfs makes me not want to proceed with the FUSE route.

Quoted 4 lines · Christian Brauner

The standard way of making it possible to mount procfs inside of a container with a separate mount namespace that has a procfs inside it with overmounted entries is to ensure that a fully-visible procfs instance is present.

Yes, this is a solution. However, this is only marginally better than passing --privileged to the outer container (in a sense that we require outer sandbox to remove some protections for the inner sandbox to work).

Quoted 7 lines

The container needs to inherit a fully-visible instance somehow if you want nesting. Using an unprivileged LSM such as landlock to prevent any access to the fully visible procfs instance is usually the better way.

My hope is that once signed bpf is more widely adopted that distros will just start enabling blessed bpf programs that will just take on the access protecting instead of the clumsy bind-mount protection mechanism.

These are big changes to container runtimes that are unlikely to happen soon. In contrast, the patch we are discussing will be available in 2 months after the merge for me to use on ArchLinux, and in a couple more months on Ubuntu.

So, is there any way forward with the patch or should I continue trying to find a userspace solution?

Quoted 6 lines

I still consider these patches useful. I made them precisely to remove some of the restrictions we have for procfs because of global files in the root of this filesystem.

I can update and prepare a new version of patchset if Christian thinks it's useful too.

Let's see it at least! No need to preemptively dismiss it. :)

Cache the mounters credentials and allow access to the net directories contingent of the permissions of the mounter of proc.

Do not show /proc/self/net when proc is mounted with subset=pid option and the mounter does not have CAP_NET_ADMIN.

Signed-off-by Alexey Gladkov <[email protected]>

---
 fs/proc/proc_net.c      | 8 ++++++++
 fs/proc/root.c          | 5 +++++
 include/linux/proc_fs.h | 1 +
 3 files changed, 14 insertions(+)

fs/proc/proc_net.c +8 -0

index 52f0b75cbce2..6e0ccef0169f 100644
@@ -23,6 +23,7 @@
 #include <linux/uidgid.h>
 #include <net/net_namespace.h>
 #include <linux/seq_file.h>
+#include <linux/security.h>
 
 #include "internal.h"
 

@@ -270,6 +271,7 @@ static struct net *get_proc_task_net(struct inode *dir)
 	struct task_struct *task;
 	struct nsproxy *ns;
 	struct net *net = NULL;
+	struct proc_fs_info *fs_info = proc_sb_info(dir->i_sb);
 
 	rcu_read_lock();
 	task = pid_task(proc_pid(dir), PIDTYPE_PID);

@@ -282,6 +284,12 @@ static struct net *get_proc_task_net(struct inode *dir)
 	}
 	rcu_read_unlock();
 
+	if (net && (fs_info->pidonly == PROC_PIDONLY_ON) &&
+	    security_capable(fs_info->mounter_cred, net->user_ns, CAP_NET_ADMIN, CAP_OPT_NONE) < 0) {
+		put_net(net);
+		net = NULL;
+	}
+
 	return net;
 }
 

fs/proc/root.c +1 -0

index d8ca41d823e4..ed8a101d09d3 100644
@@ -254,6 +254,7 @@ static int proc_fill_super(struct super_block *s, struct fs_context *fc)
 		return -ENOMEM;
 
 	fs_info->pid_ns = get_pid_ns(ctx->pid_ns);
+	fs_info->mounter_cred = get_cred(fc->cred);
 	proc_apply_options(fs_info, fc, current_user_ns());
 
 	/* User space would break if executables or devices appear on proc */

Quoted 59 lines · Alexey Gladkov

Cache the mounters credentials and allow access to the net directories contingent of the permissions of the mounter of proc.

Do not show /proc/self/net when proc is mounted with subset=pid option and the mounter does not have CAP_NET_ADMIN.

Signed-off-by Alexey Gladkov <[email protected]>

---
 fs/proc/proc_net.c      | 8 ++++++++
 fs/proc/root.c          | 5 +++++
 include/linux/proc_fs.h | 1 +
 3 files changed, 14 insertions(+)

fs/proc/proc_net.c +8 -0

index 52f0b75cbce2..6e0ccef0169f 100644
@@ -23,6 +23,7 @@
 #include <linux/uidgid.h>
 #include <net/net_namespace.h>
 #include <linux/seq_file.h>
+#include <linux/security.h>
 
 #include "internal.h"
 

@@ -270,6 +271,7 @@ static struct net *get_proc_task_net(struct inode *dir)
 	struct task_struct *task;
 	struct nsproxy *ns;
 	struct net *net = NULL;
+	struct proc_fs_info *fs_info = proc_sb_info(dir->i_sb);
 
 	rcu_read_lock();
 	task = pid_task(proc_pid(dir), PIDTYPE_PID);

@@ -282,6 +284,12 @@ static struct net *get_proc_task_net(struct inode *dir)
 	}
 	rcu_read_unlock();
 
+	if (net && (fs_info->pidonly == PROC_PIDONLY_ON) &&
+	    security_capable(fs_info->mounter_cred, net->user_ns, CAP_NET_ADMIN, CAP_OPT_NONE) < 0) {
+		put_net(net);
+		net = NULL;
+	}
+
 	return net;
 }
 

fs/proc/root.c +1 -0

index d8ca41d823e4..ed8a101d09d3 100644
@@ -254,6 +254,7 @@ static int proc_fill_super(struct super_block *s, struct fs_context *fc)
 		return -ENOMEM;
 
 	fs_info->pid_ns = get_pid_ns(ctx->pid_ns);
+	fs_info->mounter_cred = get_cred(fc->cred);
 	proc_apply_options(fs_info, fc, current_user_ns());
 
 	/* User space would break if executables or devices appear on proc */

fs/proc/root.c static int proc_reconfigure(struct fs_context *fc) · line 308

Quoted 5 lines

fs/proc/root.c +2 -0

@@ -303,6 +304,9 @@ static int proc_reconfigure(struct fs_context *fc)
 
 	sync_filesystem(sb);
 
+	put_cred(fs_info->mounter_cred);
+	fs_info->mounter_cred = get_cred(fc->cred);

Afaict, this races with get_proc_task_net(). You need a synchronization mechanism here so that get_proc_task_net() doesn't risk accessing invalid mounter creds while someone concurrently updates the creds. Proposal how to fix that below.

But I'm kinda torn here anyway whether we want that credential change on remount. The problem is that someone might inadvertently allow access to /proc/<pid>/net as a side-effect simply because they remounted procfs. But they never had a chance to prevent this.

I think it's best if mounter_creds stays fixed just as they do for overlayfs. So we don't allow them to change on reconfigure. That also makes all of the code I hinted at below pointless.

If we ever want to change the credentials it's easier to add a mount option to procfs like I did for overlayfs.

_Untested_ patches:

First, the preparatory patch diff (no functional changes intended):

fs/proc/proc_net.c +10 -10

index 52f0b75cbce2..81825e5819b8 100644
@@ -268,19 +268,19 @@ EXPORT_SYMBOL_GPL(proc_create_net_single_write);
 static struct net *get_proc_task_net(struct inode *dir)
 {
        struct task_struct *task;
-       struct nsproxy *ns;
-       struct net *net = NULL;
+       struct net *net;
 
-       rcu_read_lock();
+       guard(rcu)();
        task = pid_task(proc_pid(dir), PIDTYPE_PID);
-       if (task != NULL) {
-               task_lock(task);
-               ns = task->nsproxy;
-               if (ns != NULL)
-                       net = get_net(ns->net_ns);
-               task_unlock(task);
+       if (!task)
+               return NULL;
+
+       scoped_guard(task_lock, task) {
+               struct nsproxy *ns = task->nsproxy;
+               if (!ns)
+                       return NULL;
+               net = get_net(ns->net_ns);
        }
-       rcu_read_unlock();
 
        return net;
 }

And then on top of it something like:

fs/proc/proc_net.c +11 -0

index 81825e5819b8..47dc9806395c 100644
@@ -269,6 +269,8 @@ static struct net *get_proc_task_net(struct inode *dir)
 {
        struct task_struct *task;
        struct net *net;
+       struct proc_fs_info *fs_info;
+       const struct cred *cred;
 
        guard(rcu)();
        task = pid_task(proc_pid(dir), PIDTYPE_PID);

@@ -282,6 +284,15 @@ static struct net *get_proc_task_net(struct inode *dir)
                net = get_net(ns->net_ns);
        }
 
+       fs_info = proc_sb_info(dir->i_sb);
+       if (fs_info->pidonly != PROC_PIDONLY_ON)
+               return net;
+
+       cred = rcu_dereference(fs_info->mounter_cred);
+       if (security_capable(cred, net->user_ns, CAP_NET_ADMIN, CAP_OPT_NONE) != 0) {
+               put_net(net);
+               return NULL;
+       }
        return net;
 }
 

fs/proc/root.c +6 -2

index d8ca41d823e4..68397900dab7 100644
@@ -300,11 +300,15 @@ static int proc_reconfigure(struct fs_context *fc)
 {
        struct super_block *sb = fc->root->d_sb;
        struct proc_fs_info *fs_info = proc_sb_info(sb);
+       const struct cred *cred;
 
        sync_filesystem(sb);
 
-       proc_apply_options(fs_info, fc, current_user_ns());
-       return 0;
+       cred = rcu_replace_pointer(fs_info->mounter_cred, get_cred(fc->cred),
+                                  lockdep_is_held(&sb->s_umount));
+       put_cred(cred);
+
+       return proc_apply_options(sb, fc, current_user_ns());
 }
 
 static int proc_get_tree(struct fs_context *fc)

When /proc is mounted with the subset=pid option, all system files from the root of the file system are not accessible in userspace. Only dynamic information about processes is available, which cannot be hidden with overmount.

For this reason, checking for full visibility is not relevant if mounting is performed with the subset=pid option.

Signed-off-by Alexey Gladkov <[email protected]>

---
 fs/namespace.c                 | 29 ++++++++++++++++-------------
 fs/proc/root.c                 | 17 ++++++++++-------
 include/linux/fs/super_types.h |  2 ++
 3 files changed, 28 insertions(+), 20 deletions(-)

fs/namespace.c +2 -1

index c58674a20cad..7daa86315c05 100644
@@ -6116,7 +6116,8 @@ static bool mnt_already_visible(struct mnt_namespace *ns,
 		/* This mount is not fully visible if it's root directory
 		 * is not the root directory of the filesystem.
 		 */
-		if (mnt->mnt.mnt_root != mnt->mnt.mnt_sb->s_root)
+		if (!(sb->s_iflags & SB_I_USERNS_ALLOW_REVEALING) &&
+		    mnt->mnt.mnt_root != mnt->mnt.mnt_sb->s_root)
 			continue;
 
 		/* A local view of the mount flags */

Quoted 30 lines · Alexey Gladkov

When /proc is mounted with the subset=pid option, all system files from the root of the file system are not accessible in userspace. Only dynamic information about processes is available, which cannot be hidden with overmount.

For this reason, checking for full visibility is not relevant if mounting is performed with the subset=pid option.

Signed-off-by Alexey Gladkov <[email protected]>

---
 fs/namespace.c                 | 29 ++++++++++++++++-------------
 fs/proc/root.c                 | 17 ++++++++++-------
 include/linux/fs/super_types.h |  2 ++
 3 files changed, 28 insertions(+), 20 deletions(-)

fs/namespace.c +2 -1

index c58674a20cad..7daa86315c05 100644
@@ -6116,7 +6116,8 @@ static bool mnt_already_visible(struct mnt_namespace *ns,
 		/* This mount is not fully visible if it's root directory
 		 * is not the root directory of the filesystem.
 		 */
-		if (mnt->mnt.mnt_root != mnt->mnt.mnt_sb->s_root)
+		if (!(sb->s_iflags & SB_I_USERNS_ALLOW_REVEALING) &&
+		    mnt->mnt.mnt_root != mnt->mnt.mnt_sb->s_root)
 			continue;
 
 		/* A local view of the mount flags */

fs/namespace.c static bool mnt_already_visible(struct mnt_namespace *ns, · line 6138

Quoted 2 lines

fs/namespace.c +0 -0

@@ -6136,18 +6137,20 @@ static bool mnt_already_visible(struct mnt_namespace *ns,
 		    ((mnt_flags & MNT_ATIME_MASK) != (new_flags & MNT_ATIME_MASK)))
 			continue;

There are a few things that I find problematic here.

Even before your change the mount flags of the first fully visible procfs mount would be picked up. If the caller was unlucky they could stumble upon the most restricted procfs mount in the mount namespace rbtree. Leading to weird scenarios where a user cannot write to the procfs instance they just mounted but could to another one that is also in their namespace.

The other thing is that with this change specifically:

    if (!(sb->s_iflags & SB_I_USERNS_ALLOW_REVEALING) &&
        mnt->mnt.mnt_root != mnt->mnt.mnt_sb->s_root)

we start caring about mount options of even partially exposed procfs mounts. IOW, if someone had a bind-mount of e.g., /proc/pressure somewhere that got inherited via CLONE_NEWNS then we suddenly take the mount options of that into account for a new /proc/<pid>/* only instance. I think we should continue caring only about procfs mounts that are visible from their root.

The the other problem is that it is really annoying that we walk all mounts in a mount namespace just to find procfs and sysfs mounts in there. Currently a lot of workloads still do the CLONE_NEWNS dance meaning they inherit all the crap from the host and then proceed to setup their new rootfs. Busy container workloads that can be a lot.

So let's just be honest about it and treat procfs and sysfs as the snowflakes that they have become and record their instances in a separate per mount namespace hlist as in the (untested) patch below [1].

Also SB_I_USERNS_ALLOW_REVEALING seems unnecessary. The only time we care about that flag is when we setup a new superblock. So this could easily be a struct fs_context bitfield that just exists for the duration of the creation of the new superblock and mount. So maybe pass that down to mount_too_revealing() and further down into the actual helper.

[1]:

Quoted 1 line

From 4bbd41e7a3ef91667dd334f31b1b6bf8caec0599 Mon Sep 17 00:00:00 2001

From: Christian Brauner <[email protected]>
Date: Tue, 17 Feb 2026 12:02:34 +0100
Subject: [PATCH] namespace: record fully visible mounts in list

Instead of wading through all the mounts in the mount namespace rbtree to find fully visible procfs and sysfs mounts, be honest about them being special cruft and record them in a separate per-mount namespace list.

Quoted 77 lines · Christian Brauner

Quoted 30 lines · Alexey Gladkov

When /proc is mounted with the subset=pid option, all system files from the root of the file system are not accessible in userspace. Only dynamic information about processes is available, which cannot be hidden with overmount.

For this reason, checking for full visibility is not relevant if mounting is performed with the subset=pid option.

Signed-off-by Alexey Gladkov <[email protected]>

---
 fs/namespace.c                 | 29 ++++++++++++++++-------------
 fs/proc/root.c                 | 17 ++++++++++-------
 include/linux/fs/super_types.h |  2 ++
 3 files changed, 28 insertions(+), 20 deletions(-)

fs/namespace.c +2 -1

index c58674a20cad..7daa86315c05 100644
@@ -6116,7 +6116,8 @@ static bool mnt_already_visible(struct mnt_namespace *ns,
 		/* This mount is not fully visible if it's root directory
 		 * is not the root directory of the filesystem.
 		 */
-		if (mnt->mnt.mnt_root != mnt->mnt.mnt_sb->s_root)
+		if (!(sb->s_iflags & SB_I_USERNS_ALLOW_REVEALING) &&
+		    mnt->mnt.mnt_root != mnt->mnt.mnt_sb->s_root)
 			continue;
 
 		/* A local view of the mount flags */

fs/namespace.c static bool mnt_already_visible(struct mnt_namespace *ns, · line 6138

Quoted 2 lines

fs/namespace.c +0 -0

@@ -6136,18 +6137,20 @@ static bool mnt_already_visible(struct mnt_namespace *ns,
 		    ((mnt_flags & MNT_ATIME_MASK) != (new_flags & MNT_ATIME_MASK)))
 			continue;

There are a few things that I find problematic here.

Even before your change the mount flags of the first fully visible procfs mount would be picked up. If the caller was unlucky they could stumble upon the most restricted procfs mount in the mount namespace rbtree. Leading to weird scenarios where a user cannot write to the procfs instance they just mounted but could to another one that is also in their namespace.

The other thing is that with this change specifically:

    if (!(sb->s_iflags & SB_I_USERNS_ALLOW_REVEALING) &&
        mnt->mnt.mnt_root != mnt->mnt.mnt_sb->s_root)

we start caring about mount options of even partially exposed procfs mounts. IOW, if someone had a bind-mount of e.g., /proc/pressure somewhere that got inherited via CLONE_NEWNS then we suddenly take the mount options of that into account for a new /proc/<pid>/* only instance. I think we should continue caring only about procfs mounts that are visible from their root.

The the other problem is that it is really annoying that we walk all mounts in a mount namespace just to find procfs and sysfs mounts in there. Currently a lot of workloads still do the CLONE_NEWNS dance meaning they inherit all the crap from the host and then proceed to setup their new rootfs. Busy container workloads that can be a lot.

So let's just be honest about it and treat procfs and sysfs as the snowflakes that they have become and record their instances in a separate per mount namespace hlist as in the (untested) patch below [1].

Also SB_I_USERNS_ALLOW_REVEALING seems unnecessary. The only time we care about that flag is when we setup a new superblock. So this could easily be a struct fs_context bitfield that just exists for the duration of the creation of the new superblock and mount. So maybe pass that down to mount_too_revealing() and further down into the actual helper.

[1]:

Quoted 1 line

From 4bbd41e7a3ef91667dd334f31b1b6bf8caec0599 Mon Sep 17 00:00:00 2001

From: Christian Brauner <[email protected]>
Date: Tue, 17 Feb 2026 12:02:34 +0100
Subject: [PATCH] namespace: record fully visible mounts in list

Quoted 4 lines

Instead of wading through all the mounts in the mount namespace rbtree to find fully visible procfs and sysfs mounts, be honest about them being special cruft and record them in a separate per-mount namespace list.

If you rework this I would expect to take it for v7.3. It's a bit late now...

Quoted 83 lines · Christian Brauner

Quoted 81 lines · Christian Brauner

Quoted 30 lines · Alexey Gladkov

When /proc is mounted with the subset=pid option, all system files from the root of the file system are not accessible in userspace. Only dynamic information about processes is available, which cannot be hidden with overmount.

For this reason, checking for full visibility is not relevant if mounting is performed with the subset=pid option.

Signed-off-by Alexey Gladkov <[email protected]>

---
 fs/namespace.c                 | 29 ++++++++++++++++-------------
 fs/proc/root.c                 | 17 ++++++++++-------
 include/linux/fs/super_types.h |  2 ++
 3 files changed, 28 insertions(+), 20 deletions(-)

fs/namespace.c +2 -1

index c58674a20cad..7daa86315c05 100644
@@ -6116,7 +6116,8 @@ static bool mnt_already_visible(struct mnt_namespace *ns,
 		/* This mount is not fully visible if it's root directory
 		 * is not the root directory of the filesystem.
 		 */
-		if (mnt->mnt.mnt_root != mnt->mnt.mnt_sb->s_root)
+		if (!(sb->s_iflags & SB_I_USERNS_ALLOW_REVEALING) &&
+		    mnt->mnt.mnt_root != mnt->mnt.mnt_sb->s_root)
 			continue;
 
 		/* A local view of the mount flags */

fs/namespace.c static bool mnt_already_visible(struct mnt_namespace *ns, · line 6138

Quoted 2 lines

fs/namespace.c +0 -0

@@ -6136,18 +6137,20 @@ static bool mnt_already_visible(struct mnt_namespace *ns,
 		    ((mnt_flags & MNT_ATIME_MASK) != (new_flags & MNT_ATIME_MASK)))
 			continue;

There are a few things that I find problematic here.

Even before your change the mount flags of the first fully visible procfs mount would be picked up. If the caller was unlucky they could stumble upon the most restricted procfs mount in the mount namespace rbtree. Leading to weird scenarios where a user cannot write to the procfs instance they just mounted but could to another one that is also in their namespace.

The other thing is that with this change specifically:

    if (!(sb->s_iflags & SB_I_USERNS_ALLOW_REVEALING) &&
        mnt->mnt.mnt_root != mnt->mnt.mnt_sb->s_root)

we start caring about mount options of even partially exposed procfs mounts. IOW, if someone had a bind-mount of e.g., /proc/pressure somewhere that got inherited via CLONE_NEWNS then we suddenly take the mount options of that into account for a new /proc/<pid>/* only instance. I think we should continue caring only about procfs mounts that are visible from their root.

The the other problem is that it is really annoying that we walk all mounts in a mount namespace just to find procfs and sysfs mounts in there. Currently a lot of workloads still do the CLONE_NEWNS dance meaning they inherit all the crap from the host and then proceed to setup their new rootfs. Busy container workloads that can be a lot.

So let's just be honest about it and treat procfs and sysfs as the snowflakes that they have become and record their instances in a separate per mount namespace hlist as in the (untested) patch below [1].

Also SB_I_USERNS_ALLOW_REVEALING seems unnecessary. The only time we care about that flag is when we setup a new superblock. So this could easily be a struct fs_context bitfield that just exists for the duration of the creation of the new superblock and mount. So maybe pass that down to mount_too_revealing() and further down into the actual helper.

[1]:

Quoted 1 line

From 4bbd41e7a3ef91667dd334f31b1b6bf8caec0599 Mon Sep 17 00:00:00 2001

From: Christian Brauner <[email protected]>
Date: Tue, 17 Feb 2026 12:02:34 +0100
Subject: [PATCH] namespace: record fully visible mounts in list

Instead of wading through all the mounts in the mount namespace rbtree to find fully visible procfs and sysfs mounts, be honest about them being special cruft and record them in a separate per-mount namespace list.

Quoted 2 lines

If you rework this I would expect to take it for v7.3. It's a bit late now...

No problem. I understand. Sorry it took me so long to get back to you. I was laid off from my job and had to look for a new one quickly.

When procfs is mounted with the subset=pid option, all system files and directories from the root of the filesystem are not accessible in userspace. Only dynamic information about processes is available, which cannot be hidden with overmount.

For this reason, checking for full visibility is not relevant if mounting is performed with the subset=pid option.

Signed-off-by Alexey Gladkov <[email protected]>

---

fs/namespace.c +2 -1

-static bool mount_too_revealing(const struct super_block *sb, int *new_mnt_flags)
+static bool mount_too_revealing(struct fs_context *fc, int *new_mnt_flags)
 {
 	const unsigned long required_iflags = SB_I_NOEXEC | SB_I_NODEV;
 	struct mnt_namespace *ns = current->nsproxy->mnt_ns;
+	const struct super_block *sb = fc->root->d_sb;
 	unsigned long s_iflags;
 
 	if (ns->user_ns == &init_user_ns)

Quoted 21 lines · Alexey Gladkov

When procfs is mounted with the subset=pid option, all system files and directories from the root of the filesystem are not accessible in userspace. Only dynamic information about processes is available, which cannot be hidden with overmount.

For this reason, checking for full visibility is not relevant if mounting is performed with the subset=pid option.

Signed-off-by Alexey Gladkov <[email protected]>

---

fs/namespace.c +2 -1

-static bool mount_too_revealing(const struct super_block *sb, int *new_mnt_flags)
+static bool mount_too_revealing(struct fs_context *fc, int *new_mnt_flags)
 {
 	const unsigned long required_iflags = SB_I_NOEXEC | SB_I_NODEV;
 	struct mnt_namespace *ns = current->nsproxy->mnt_ns;
+	const struct super_block *sb = fc->root->d_sb;
 	unsigned long s_iflags;
 
 	if (ns->user_ns == &init_user_ns)

fs/namespace.c static bool mount_too_revealing(const struct super_block *sb, int *new_mnt_flags · line 6391

Quoted 6 lines

fs/namespace.c +1 -1

@@ -6388,7 +6387,7 @@ static bool mount_too_revealing(const struct super_block *sb, int *new_mnt_flags
 		return true;
 	}
 
-	return !mnt_already_visible(ns, sb, new_mnt_flags);
+	return (!fc->skip_visibility && !mnt_already_visible(ns, sb, new_mnt_flags));
 }

Unless I'm missing something (I haven't tested this locally yet, sorry), this will allow you to bypass mount_too_revealing() even for non-subset=pid mounts because once you create a subset=pid mount then a regular procfs mount will see the subset=pid mount and permit it.

I think the solution is quite simple -- you can also skip super-blocks that have fc->skip_visibility set in mnt_already_visible().

Unfortunately, the fact that both subset=pid and fully-loaded procfs look like the same type (procfs) to mnt_already_visible() is something people have already exploited in userspace. (The k8s workaround from a long time ago used subset=pid from a dead pidns to stop the mount from being useful to an attacker while still bypassing mount_too_revealing(). That being said, that workaround was removed a long time ago and I don't know how widespread this is.)

I'd be happy to give it a shot but if it breaks userspace we might need a new mount option to work around it...

When procfs is mounted with the subset=pid option, all system files and directories from the root of the filesystem are not accessible in userspace. Only dynamic information about processes is available, which cannot be hidden with overmount.

For this reason, checking for full visibility is not relevant if mounting is performed with the subset=pid option.

Signed-off-by Alexey Gladkov <[email protected]>

---
 fs/fs_context.c            |  1 +
 fs/namespace.c             | 15 +++++++--------
 fs/proc/root.c             |  7 +++++++
 include/linux/fs_context.h |  1 +
 4 files changed, 16 insertions(+), 8 deletions(-)

fs/fs_context.c +1 -0

index a37b0a093505..2fd3d6422a38 100644
@@ -545,6 +545,7 @@ void vfs_clean_context(struct fs_context *fc)
 	kfree(fc->source);
 	fc->source = NULL;
 	fc->exclusive = false;
+	fc->skip_visibility = false;
 
 	fc->purpose = FS_CONTEXT_FOR_RECONFIGURE;
 	fc->phase = FS_CONTEXT_AWAITING_RECONF;

fs/namespace.c +7 -8

index 539b74403072..32aaedb020c1 100644
@@ -3755,7 +3755,7 @@ static int do_add_mount(struct mount *newmnt, const struct pinned_mountpoint *mp
 	return graft_tree(newmnt, mp);
 }
 
-static bool mount_too_revealing(const struct super_block *sb, int *new_mnt_flags);
+static bool mount_too_revealing(struct fs_context *fc, int *new_mnt_flags);
 
 /*
  * Create a new mount using a superblock configuration and request it

@@ -3764,19 +3764,17 @@ static bool mount_too_revealing(const struct super_block *sb, int *new_mnt_flags
 static int do_new_mount_fc(struct fs_context *fc, const struct path *mountpoint,
 			   unsigned int mnt_flags)
 {
-	struct super_block *sb;
 	struct vfsmount *mnt __free(mntput) = fc_mount(fc);
 	int error;
 
 	if (IS_ERR(mnt))
 		return PTR_ERR(mnt);
 
-	sb = fc->root->d_sb;
-	error = security_sb_kern_mount(sb);
+	error = security_sb_kern_mount(fc->root->d_sb);
 	if (unlikely(error))
 		return error;
 
-	if (unlikely(mount_too_revealing(sb, &mnt_flags))) {
+	if (unlikely(mount_too_revealing(fc, &mnt_flags))) {
 		errorfcp(fc, "VFS", "Mount too revealing");
 		return -EPERM;
 	}

@@ -4463,7 +4461,7 @@ SYSCALL_DEFINE3(fsmount, int, fs_fd, unsigned int, flags,
 		return ret;
 
 	ret = -EPERM;
-	if (mount_too_revealing(fc->root->d_sb, &mnt_flags)) {
+	if (mount_too_revealing(fc, &mnt_flags)) {
 		errorfcp(fc, "VFS", "Mount too revealing");
 		return ret;
 	}

@@ -6368,10 +6366,11 @@ static bool mnt_already_visible(struct mnt_namespace *ns,
 	return false;
 }
 
-static bool mount_too_revealing(const struct super_block *sb, int *new_mnt_flags)
+static bool mount_too_revealing(struct fs_context *fc, int *new_mnt_flags)
 {
 	const unsigned long required_iflags = SB_I_NOEXEC | SB_I_NODEV;
 	struct mnt_namespace *ns = current->nsproxy->mnt_ns;
+	const struct super_block *sb = fc->root->d_sb;
 	unsigned long s_iflags;
 
 	if (ns->user_ns == &init_user_ns)

@@ -6388,7 +6387,7 @@ static bool mount_too_revealing(const struct super_block *sb, int *new_mnt_flags
 		return true;
 	}
 
-	return !mnt_already_visible(ns, sb, new_mnt_flags);
+	return (!fc->skip_visibility && !mnt_already_visible(ns, sb, new_mnt_flags));
 }
 
 bool mnt_may_suid(struct vfsmount *mnt)

fs/proc/root.c +0 -0

index 05558654df31..6dc870b3061b 100644
@@ -263,6 +263,13 @@ static int proc_fill_super(struct super_block *s, struct fs_context *fc)
 	if (ret)
 		return ret;

Quoted 34 lines · Aleksa Sarai

Quoted 21 lines · Alexey Gladkov

When procfs is mounted with the subset=pid option, all system files and directories from the root of the filesystem are not accessible in userspace. Only dynamic information about processes is available, which cannot be hidden with overmount.

For this reason, checking for full visibility is not relevant if mounting is performed with the subset=pid option.

Signed-off-by Alexey Gladkov <[email protected]>

---

(quoted diff) +2 -1

-static bool mount_too_revealing(const struct super_block *sb, int *new_mnt_flags)
+static bool mount_too_revealing(struct fs_context *fc, int *new_mnt_flags)
 {
 	const unsigned long required_iflags = SB_I_NOEXEC | SB_I_NODEV;
 	struct mnt_namespace *ns = current->nsproxy->mnt_ns;
+	const struct super_block *sb = fc->root->d_sb;
 	unsigned long s_iflags;
 
 	if (ns->user_ns == &init_user_ns)

Review static bool mount_too_revealing(const struct super_block *sb, int *new_mnt_flags · line 6391

Quoted 6 lines

(quoted diff) +1 -1

@@ -6388,7 +6387,7 @@ static bool mount_too_revealing(const struct super_block *sb, int *new_mnt_flags
 		return true;
 	}
 
-	return !mnt_already_visible(ns, sb, new_mnt_flags);
+	return (!fc->skip_visibility && !mnt_already_visible(ns, sb, new_mnt_flags));
 }

Unless I'm missing something (I haven't tested this locally yet, sorry), this will allow you to bypass mount_too_revealing() even for non-subset=pid mounts because once you create a subset=pid mount then a regular procfs mount will see the subset=pid mount and permit it.

Quoted 2 lines

I think the solution is quite simple -- you can also skip super-blocks that have fc->skip_visibility set in mnt_already_visible().

I now see that check was present in v8 but I guess its importance wasn't obvious. I guess this means we will need to reintroduce SB_I_USERNS_ALLOW_REVEALING. :/

Quoted 38 lines · Aleksa Sarai

Quoted 36 lines · Aleksa Sarai

Quoted 21 lines · Alexey Gladkov

When procfs is mounted with the subset=pid option, all system files and directories from the root of the filesystem are not accessible in userspace. Only dynamic information about processes is available, which cannot be hidden with overmount.

For this reason, checking for full visibility is not relevant if mounting is performed with the subset=pid option.

Signed-off-by Alexey Gladkov <[email protected]>

---

(quoted diff) +2 -1

-static bool mount_too_revealing(const struct super_block *sb, int *new_mnt_flags)
+static bool mount_too_revealing(struct fs_context *fc, int *new_mnt_flags)
 {
 	const unsigned long required_iflags = SB_I_NOEXEC | SB_I_NODEV;
 	struct mnt_namespace *ns = current->nsproxy->mnt_ns;
+	const struct super_block *sb = fc->root->d_sb;
 	unsigned long s_iflags;
 
 	if (ns->user_ns == &init_user_ns)

Review static bool mount_too_revealing(const struct super_block *sb, int *new_mnt_flags · line 6391

Quoted 6 lines

(quoted diff) +1 -1

@@ -6388,7 +6387,7 @@ static bool mount_too_revealing(const struct super_block *sb, int *new_mnt_flags
 		return true;
 	}
 
-	return !mnt_already_visible(ns, sb, new_mnt_flags);
+	return (!fc->skip_visibility && !mnt_already_visible(ns, sb, new_mnt_flags));
 }

Unless I'm missing something (I haven't tested this locally yet, sorry), this will allow you to bypass mount_too_revealing() even for non-subset=pid mounts because once you create a subset=pid mount then a regular procfs mount will see the subset=pid mount and permit it.

I think the solution is quite simple -- you can also skip super-blocks that have fc->skip_visibility set in mnt_already_visible().

Quoted 3 lines

I now see that check was present in v8 but I guess its importance wasn't obvious. I guess this means we will need to reintroduce SB_I_USERNS_ALLOW_REVEALING. :/

I've been playing with something else. So first we should move SB_I_USERNS_REVEALING to an fs_type flag. It's not an optional thing and always set and never removed. That also means we can simplify sysfs_get_tree() to just kernfs_get_tree().

And then we raise SB_I_USERNS_RESTRICTED on all procfs mounts with pid_only and disallow using them for calculating mount permissions for unrestricted procfs mounts.

Quoted 39 lines · Christian Brauner

Quoted 38 lines · Aleksa Sarai

Quoted 36 lines · Aleksa Sarai

Quoted 21 lines · Alexey Gladkov

When procfs is mounted with the subset=pid option, all system files and directories from the root of the filesystem are not accessible in userspace. Only dynamic information about processes is available, which cannot be hidden with overmount.

For this reason, checking for full visibility is not relevant if mounting is performed with the subset=pid option.

Signed-off-by Alexey Gladkov <[email protected]>

---

(quoted diff) +2 -1

-static bool mount_too_revealing(const struct super_block *sb, int *new_mnt_flags)
+static bool mount_too_revealing(struct fs_context *fc, int *new_mnt_flags)
 {
 	const unsigned long required_iflags = SB_I_NOEXEC | SB_I_NODEV;
 	struct mnt_namespace *ns = current->nsproxy->mnt_ns;
+	const struct super_block *sb = fc->root->d_sb;
 	unsigned long s_iflags;
 
 	if (ns->user_ns == &init_user_ns)

Review static bool mount_too_revealing(const struct super_block *sb, int *new_mnt_flags · line 6391

Quoted 6 lines

(quoted diff) +1 -1

@@ -6388,7 +6387,7 @@ static bool mount_too_revealing(const struct super_block *sb, int *new_mnt_flags
 		return true;
 	}
 
-	return !mnt_already_visible(ns, sb, new_mnt_flags);
+	return (!fc->skip_visibility && !mnt_already_visible(ns, sb, new_mnt_flags));
 }

Unless I'm missing something (I haven't tested this locally yet, sorry), this will allow you to bypass mount_too_revealing() even for non-subset=pid mounts because once you create a subset=pid mount then a regular procfs mount will see the subset=pid mount and permit it.

I think the solution is quite simple -- you can also skip super-blocks that have fc->skip_visibility set in mnt_already_visible().

Quoted 8 lines

Quoted 3 lines

I now see that check was present in v8 but I guess its importance wasn't obvious. I guess this means we will need to reintroduce SB_I_USERNS_ALLOW_REVEALING. :/

I've been playing with something else. So first we should move SB_I_USERNS_REVEALING to an fs_type flag. It's not an optional thing and always set and never removed. That also means we can simplify sysfs_get_tree() to just kernfs_get_tree().

Seems quite reasonable to me.

Quoted 3 lines

And then we raise SB_I_USERNS_RESTRICTED on all procfs mounts with pid_only and disallow using them for calculating mount permissions for unrestricted procfs mounts.

I think that's fine here and is probably all we need for now (though I think that the name is a little confusing especially given my next comment), but I think there is a bit of a deeper problem here that deserves to be mentioned if only for posterity.

The core issue really is that we actually have two versions of procfs now, and they are effectively distinguished by fs_context flags instead of fs_type. Back when subset=pid was merged there was a discussion about a hypothetical proc2 -- while that got dropped, we are kind of reinventing this split in an ad-hoc way.

Conceptually considering them as two distinct fs_types would be the more semantically correct thing to do, though I think it would be overkill to come up with some framework for that. (I also really hope we don't have any other filesystems where this is also the case.)

This is why I think the name is a little weird, since it isn't an issue about one procfs being restricted in a userns, it's that they are conceptually unrelated filesystem types. A very lightweight version of

this would be making it something like SB_I_RESTRICTED_VARIANT that would be more a little more strict than what you have now -- SB_I_RESTRICTED_VARIANT would not be treated as compatible with anything (even another SB_I_RESTRICTED_VARIANT mount) so that if we add more variants in the future we don't treat them the same either. (Again, I don't think we need this now?) Of course this would still allow subset=pid without a procfs mount because of the other special casing for it in mount_too_revealing().

We are also limited in what semantics we can change here too (and I'm a little worried that even this bit for SB_I_USERNS_RESTRICTED is at risk of breaking something) so maybe that isn't needed today.

Quoted 66 lines · Aleksa Sarai

Quoted 39 lines · Christian Brauner

Quoted 38 lines · Aleksa Sarai

Quoted 36 lines · Aleksa Sarai

Quoted 21 lines · Alexey Gladkov

When procfs is mounted with the subset=pid option, all system files and directories from the root of the filesystem are not accessible in userspace. Only dynamic information about processes is available, which cannot be hidden with overmount.

For this reason, checking for full visibility is not relevant if mounting is performed with the subset=pid option.

Signed-off-by Alexey Gladkov <[email protected]>

---

(quoted diff) +2 -1

-static bool mount_too_revealing(const struct super_block *sb, int *new_mnt_flags)
+static bool mount_too_revealing(struct fs_context *fc, int *new_mnt_flags)
 {
 	const unsigned long required_iflags = SB_I_NOEXEC | SB_I_NODEV;
 	struct mnt_namespace *ns = current->nsproxy->mnt_ns;
+	const struct super_block *sb = fc->root->d_sb;
 	unsigned long s_iflags;
 
 	if (ns->user_ns == &init_user_ns)

Review static bool mount_too_revealing(const struct super_block *sb, int *new_mnt_flags · line 6391

Quoted 6 lines

(quoted diff) +1 -1

@@ -6388,7 +6387,7 @@ static bool mount_too_revealing(const struct super_block *sb, int *new_mnt_flags
 		return true;
 	}
 
-	return !mnt_already_visible(ns, sb, new_mnt_flags);
+	return (!fc->skip_visibility && !mnt_already_visible(ns, sb, new_mnt_flags));
 }

Unless I'm missing something (I haven't tested this locally yet, sorry), this will allow you to bypass mount_too_revealing() even for non-subset=pid mounts because once you create a subset=pid mount then a regular procfs mount will see the subset=pid mount and permit it.

I think the solution is quite simple -- you can also skip super-blocks that have fc->skip_visibility set in mnt_already_visible().

Quoted 8 lines

Quoted 3 lines

I now see that check was present in v8 but I guess its importance wasn't obvious. I guess this means we will need to reintroduce SB_I_USERNS_ALLOW_REVEALING. :/

I've been playing with something else. So first we should move SB_I_USERNS_REVEALING to an fs_type flag. It's not an optional thing and always set and never removed. That also means we can simplify sysfs_get_tree() to just kernfs_get_tree().

Seems quite reasonable to me.

Quoted 3 lines

And then we raise SB_I_USERNS_RESTRICTED on all procfs mounts with pid_only and disallow using them for calculating mount permissions for unrestricted procfs mounts.

I think that's fine here and is probably all we need for now (though I think that the name is a little confusing especially given my next comment), but I think there is a bit of a deeper problem here that deserves to be mentioned if only for posterity.

The core issue really is that we actually have two versions of procfs now, and they are effectively distinguished by fs_context flags instead of fs_type. Back when subset=pid was merged there was a discussion about a hypothetical proc2 -- while that got dropped, we are kind of reinventing this split in an ad-hoc way.

Quoted 4 lines

Conceptually considering them as two distinct fs_types would be the more semantically correct thing to do, though I think it would be overkill to come up with some framework for that. (I also really hope we don't have any other filesystems where this is also the case.)

Doing this is almost trivial. The problem really is that we'd expose "procfs2" or whatever as a new mount option to userspace without actually having fundamentally revamped what we would like procfs2 to do.

IOW, it would elevate a security hack that got added a while ago to a new filesystem type. I think that would just be sad. So I'm not so excited about doing this.

Quoted 3 lines

This is why I think the name is a little weird, since it isn't an issue about one procfs being restricted in a userns, it's that they are conceptually unrelated filesystem types. A very lightweight version of

I somewhat disagree because it's unclear where you draw the line. If you use the hidepid= stuff at its most restrictive where it only shows tasks your user owns you could also argue that it's very close to a separate filesystem type. It's strictly subtractive and so is pidonly. There's just nothing that's better or novel.

Quoted 8 lines

this would be making it something like SB_I_RESTRICTED_VARIANT that would be more a little more strict than what you have now -- SB_I_RESTRICTED_VARIANT would not be treated as compatible with anything (even another SB_I_RESTRICTED_VARIANT mount) so that if we add more variants in the future we don't treat them the same either. (Again, I don't think we need this now?) Of course this would still allow subset=pid without a procfs mount because of the other special casing for it in mount_too_revealing().

It's not a problem today. I think we can simply treat SB_I_RESTRICTED_VARIANT as not allowing other restricted variants. And we'd need a more elaborate mechanism anyway once we'd have "compatible" subsets.

Quoted 3 lines

We are also limited in what semantics we can change here too (and I'm a little worried that even this bit for SB_I_USERNS_RESTRICTED is at risk of breaking something) so maybe that isn't needed today.

So I think yes, SB_I_RESTRICTED_VARIANT is fine by me for now.

Quoted 67 lines · Christian Brauner

Quoted 66 lines · Aleksa Sarai

Quoted 39 lines · Christian Brauner

Quoted 38 lines · Aleksa Sarai

Quoted 36 lines · Aleksa Sarai

Quoted 21 lines · Alexey Gladkov

When procfs is mounted with the subset=pid option, all system files and directories from the root of the filesystem are not accessible in userspace. Only dynamic information about processes is available, which cannot be hidden with overmount.

For this reason, checking for full visibility is not relevant if mounting is performed with the subset=pid option.

Signed-off-by Alexey Gladkov <[email protected]>

---

(quoted diff) +2 -1

-static bool mount_too_revealing(const struct super_block *sb, int *new_mnt_flags)
+static bool mount_too_revealing(struct fs_context *fc, int *new_mnt_flags)
 {
 	const unsigned long required_iflags = SB_I_NOEXEC | SB_I_NODEV;
 	struct mnt_namespace *ns = current->nsproxy->mnt_ns;
+	const struct super_block *sb = fc->root->d_sb;
 	unsigned long s_iflags;
 
 	if (ns->user_ns == &init_user_ns)

Review static bool mount_too_revealing(const struct super_block *sb, int *new_mnt_flags · line 6391

Quoted 6 lines

(quoted diff) +1 -1

@@ -6388,7 +6387,7 @@ static bool mount_too_revealing(const struct super_block *sb, int *new_mnt_flags
 		return true;
 	}
 
-	return !mnt_already_visible(ns, sb, new_mnt_flags);
+	return (!fc->skip_visibility && !mnt_already_visible(ns, sb, new_mnt_flags));
 }

Unless I'm missing something (I haven't tested this locally yet, sorry), this will allow you to bypass mount_too_revealing() even for non-subset=pid mounts because once you create a subset=pid mount then a regular procfs mount will see the subset=pid mount and permit it.

I think the solution is quite simple -- you can also skip super-blocks that have fc->skip_visibility set in mnt_already_visible().

Quoted 8 lines

Quoted 3 lines

I now see that check was present in v8 but I guess its importance wasn't obvious. I guess this means we will need to reintroduce SB_I_USERNS_ALLOW_REVEALING. :/

I've been playing with something else. So first we should move SB_I_USERNS_REVEALING to an fs_type flag. It's not an optional thing and always set and never removed. That also means we can simplify sysfs_get_tree() to just kernfs_get_tree().

Seems quite reasonable to me.

Quoted 3 lines

And then we raise SB_I_USERNS_RESTRICTED on all procfs mounts with pid_only and disallow using them for calculating mount permissions for unrestricted procfs mounts.

I think that's fine here and is probably all we need for now (though I think that the name is a little confusing especially given my next comment), but I think there is a bit of a deeper problem here that deserves to be mentioned if only for posterity.

The core issue really is that we actually have two versions of procfs now, and they are effectively distinguished by fs_context flags instead of fs_type. Back when subset=pid was merged there was a discussion about a hypothetical proc2 -- while that got dropped, we are kind of reinventing this split in an ad-hoc way.

Quoted 8 lines

Quoted 4 lines

Conceptually considering them as two distinct fs_types would be the more semantically correct thing to do, though I think it would be overkill to come up with some framework for that. (I also really hope we don't have any other filesystems where this is also the case.)

Doing this is almost trivial. The problem really is that we'd expose "procfs2" or whatever as a new mount option to userspace without actually having fundamentally revamped what we would like procfs2 to do.

Agree.

The new procfs2 should not have the issues present in procfs.

Quoted 28 lines

IOW, it would elevate a security hack that got added a while ago to a new filesystem type. I think that would just be sad. So I'm not so excited about doing this.

Quoted 3 lines

This is why I think the name is a little weird, since it isn't an issue about one procfs being restricted in a userns, it's that they are conceptually unrelated filesystem types. A very lightweight version of

I somewhat disagree because it's unclear where you draw the line. If you use the hidepid= stuff at its most restrictive where it only shows tasks your user owns you could also argue that it's very close to a separate filesystem type. It's strictly subtractive and so is pidonly. There's just nothing that's better or novel.

Quoted 8 lines

this would be making it something like SB_I_RESTRICTED_VARIANT that would be more a little more strict than what you have now -- SB_I_RESTRICTED_VARIANT would not be treated as compatible with anything (even another SB_I_RESTRICTED_VARIANT mount) so that if we add more variants in the future we don't treat them the same either. (Again, I don't think we need this now?) Of course this would still allow subset=pid without a procfs mount because of the other special casing for it in mount_too_revealing().

It's not a problem today. I think we can simply treat SB_I_RESTRICTED_VARIANT as not allowing other restricted variants. And we'd need a more elaborate mechanism anyway once we'd have "compatible" subsets.

Quoted 5 lines

Quoted 3 lines

We are also limited in what semantics we can change here too (and I'm a little worried that even this bit for SB_I_USERNS_RESTRICTED is at risk of breaking something) so maybe that isn't needed today.

So I think yes, SB_I_RESTRICTED_VARIANT is fine by me for now.

Should I create a new version and add this to my patchset?

Quoted 66 lines · Aleksa Sarai

Quoted 39 lines · Christian Brauner

Quoted 38 lines · Aleksa Sarai

Quoted 36 lines · Aleksa Sarai

Quoted 21 lines · Alexey Gladkov

When procfs is mounted with the subset=pid option, all system files and directories from the root of the filesystem are not accessible in userspace. Only dynamic information about processes is available, which cannot be hidden with overmount.

For this reason, checking for full visibility is not relevant if mounting is performed with the subset=pid option.

Signed-off-by Alexey Gladkov <[email protected]>

---

(quoted diff) +2 -1

-static bool mount_too_revealing(const struct super_block *sb, int *new_mnt_flags)
+static bool mount_too_revealing(struct fs_context *fc, int *new_mnt_flags)
 {
 	const unsigned long required_iflags = SB_I_NOEXEC | SB_I_NODEV;
 	struct mnt_namespace *ns = current->nsproxy->mnt_ns;
+	const struct super_block *sb = fc->root->d_sb;
 	unsigned long s_iflags;
 
 	if (ns->user_ns == &init_user_ns)

Review static bool mount_too_revealing(const struct super_block *sb, int *new_mnt_flags · line 6391

Quoted 6 lines

(quoted diff) +1 -1

@@ -6388,7 +6387,7 @@ static bool mount_too_revealing(const struct super_block *sb, int *new_mnt_flags
 		return true;
 	}
 
-	return !mnt_already_visible(ns, sb, new_mnt_flags);
+	return (!fc->skip_visibility && !mnt_already_visible(ns, sb, new_mnt_flags));
 }

Unless I'm missing something (I haven't tested this locally yet, sorry), this will allow you to bypass mount_too_revealing() even for non-subset=pid mounts because once you create a subset=pid mount then a regular procfs mount will see the subset=pid mount and permit it.

I think the solution is quite simple -- you can also skip super-blocks that have fc->skip_visibility set in mnt_already_visible().

Quoted 8 lines

Quoted 3 lines

I now see that check was present in v8 but I guess its importance wasn't obvious. I guess this means we will need to reintroduce SB_I_USERNS_ALLOW_REVEALING. :/

I've been playing with something else. So first we should move SB_I_USERNS_REVEALING to an fs_type flag. It's not an optional thing and always set and never removed. That also means we can simplify sysfs_get_tree() to just kernfs_get_tree().

Seems quite reasonable to me.

Quoted 3 lines

And then we raise SB_I_USERNS_RESTRICTED on all procfs mounts with pid_only and disallow using them for calculating mount permissions for unrestricted procfs mounts.

I think that's fine here and is probably all we need for now (though I think that the name is a little confusing especially given my next comment), but I think there is a bit of a deeper problem here that deserves to be mentioned if only for posterity.

The core issue really is that we actually have two versions of procfs now, and they are effectively distinguished by fs_context flags instead of fs_type. Back when subset=pid was merged there was a discussion about a hypothetical proc2 -- while that got dropped, we are kind of reinventing this split in an ad-hoc way.

Quoted 4 lines

Conceptually considering them as two distinct fs_types would be the more semantically correct thing to do, though I think it would be overkill to come up with some framework for that. (I also really hope we don't have any other filesystems where this is also the case.)

Doing this is almost trivial. The problem really is that we'd expose "procfs2" or whatever as a new mount option to userspace without actually having fundamentally revamped what we would like procfs2 to do. IOW, it would elevate a security hack that got added a while ago to a new filesystem type. I think that would just be sad. So I'm not so excited about doing this.

Quoted 3 lines

This is why I think the name is a little weird, since it isn't an issue about one procfs being restricted in a userns, it's that they are conceptually unrelated filesystem types. A very lightweight version of

I somewhat disagree because it's unclear where you draw the line. If you use the hidepid= stuff at its most restrictive where it only shows tasks your user owns you could also argue that it's very close to a separate filesystem type. It's strictly subtractive and so is pidonly. There's just nothing that's better or novel.

Quoted 8 lines

this would be making it something like SB_I_RESTRICTED_VARIANT that would be more a little more strict than what you have now -- SB_I_RESTRICTED_VARIANT would not be treated as compatible with anything (even another SB_I_RESTRICTED_VARIANT mount) so that if we add more variants in the future we don't treat them the same either. (Again, I don't think we need this now?) Of course this would still allow subset=pid without a procfs mount because of the other special casing for it in mount_too_revealing().

It's not a problem today. I think we can simply treat SB_I_RESTRICTED_VARIANT as not allowing other restricted variants. And we'd need a more elaborate mechanism anyway once we'd have "compatible" subsets.

Quoted 3 lines

We are also limited in what semantics we can change here too (and I'm a little worried that even this bit for SB_I_USERNS_RESTRICTED is at risk of breaking something) so maybe that isn't needed today.

So I think yes, SB_I_RESTRICTED_VARIANT is fine by me for now.