[PATCH 0/2] binfmt_misc: don't let an 'F' entry pin its own instance

[PATCH 0/2] binfmt_misc: don't let an 'F' entry pin its own instance

[Christian Brauner]

An entry registered with 'F' opens its interpreter at registration time
and holds that file until the entry is freed. Any entry nobody removes
by hand only gets closed once the binfmt_misc superblock is shut down.
If the interpreter lives on a mount that keeps that superblock alive the
two pin each other:

    binfmt_misc sb -> inode -> entry -> interp_file -> vfsmount -> binfmt_misc sb

TL;DR the file is never closed. Once the mount namespace is gone there
is nothing left to unregister through either.

There are two ways to trigger this bug:

- Point the interpreter at the instance itself. Its files are regular
  files owned by the mounter and both bm_get_inode() and
  simple_fill_super() leave i_op at empty_iops. So notify_change() falls
  back to simple_setattr() and chmod +x works. We never set SB_I_NOEXEC
  and so open_exec() accepts it.

- Use the instance as an overlayfs lower layer. The overlay superblock
  holds a clone_private_mount() of every layer until it is destroyed and
  that clone is in no namespace. So umount_tree() never reaches it.

That's a DoS. And it isn't only the superblock that leaks. It pins the
user namespace it was mounted in, so every iteration permanently eats
one of the caller's user namespace charges.

So let's just do the sane thing. SB_I_NOEXEC makes open_exec() fail on
the instance's own files and s_stack_depth makes overlayfs reject the
layer before it ever takes a clone. That also covers the ecryptfs and
fuse passthrough variants. What 'F' promises is unchanged.

The stable tag is narrower than the Fixes tags on purpose. Before
sandboxed mounts this needed global root against the single instance
everyone shares, and the change doesn't apply to those trees anyway.

Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
Christian Brauner (2):
      binfmt_misc: don't let an 'F' entry pin its own instance
      selftests/exec: check that a binfmt_misc instance cannot be pinned

 fs/binfmt_misc.c                                   |   4 +
 tools/testing/selftests/exec/Makefile              |  10 ++
 tools/testing/selftests/exec/binfmt_misc_selfpin.c | 158 +++++++++++++++++++++
 tools/testing/selftests/exec/config                |   4 +
 4 files changed, 176 insertions(+)
---
base-commit: 744c5dd7c3c92dc7cac84a028934855cd9536a3c
change-id: 20260728-work-binfmt_misc-selfpin-d55b1794f0fe

[PATCH 1/2] binfmt_misc: don't let an 'F' entry pin its own instance

[Christian Brauner]

An entry registered with 'F' opens its interpreter at registration time
and holds that file until the entry is freed. Any entry nobody removes
by hand only gets closed once the binfmt_misc superblock is shut down.
If the interpreter lives on a mount that keeps that superblock alive the
two pin each other:

    binfmt_misc sb -> inode -> entry -> interp_file -> vfsmount -> binfmt_misc sb

TL;DR the file is never closed. Once the mount namespace is gone there
is nothing left to unregister through either.

There are two ways to trigger this bug:

- Point the interpreter at the instance itself. Its files are regular
  files owned by the mounter and both bm_get_inode() and
  simple_fill_super() leave i_op at empty_iops. So notify_change() falls
  back to simple_setattr() and chmod +x works. We never set SB_I_NOEXEC
  and so open_exec() accepts it.

- Use the instance as an overlayfs lower layer. The overlay superblock
  holds a clone_private_mount() of every layer until it is destroyed and
  that clone is in no namespace. So umount_tree() never reaches it.

That's a DoS. And it isn't only the superblock that leaks. It pins the
user namespace it was mounted in, so every iteration permanently eats
one of the caller's user namespace charges.

So let's just do the sane thing. SB_I_NOEXEC makes open_exec() fail on
the instance's own files and s_stack_depth makes overlayfs reject the
layer before it ever takes a clone. That also covers the ecryptfs and
fuse passthrough variants. What 'F' promises is unchanged.

The stable tag is narrower than the Fixes tags on purpose. Before
sandboxed mounts this needed global root against the single instance
everyone shares, and the change doesn't apply to those trees anyway.

Note that SB_I_NODEV is implicitly raised for userns mounts but raise it
explicitly here as well.

Fixes: 948b701a607f ("binfmt_misc: add persistent opened binary handler for containers")
Fixes: 21ca59b365c0 ("binfmt_misc: enable sandboxed mounts")
Cc: stable@vger.kernel.org # v6.7+
Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 fs/binfmt_misc.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
index 5de615ca7a75..47aeb2b68d3e 100644
--- a/fs/binfmt_misc.c
+++ b/fs/binfmt_misc.c
@@ -937,6 +937,10 @@ static int bm_fill_super(struct super_block *sb, struct fs_context *fc)
 	if (WARN_ON(user_ns != current_user_ns()))
 		return -EINVAL;
 
+	/* Never exec off this instance and never let anything stack on it. */
+	sb->s_iflags |= SB_I_NOEXEC | SB_I_NODEV;
+	sb->s_stack_depth = FILESYSTEM_MAX_STACK_DEPTH;
+
 	/*
 	 * Lazily allocate a new binfmt_misc instance for this namespace, i.e.
 	 * do it here during the first mount of binfmt_misc. We don't need to

-- 
2.53.0

Re: [PATCH 1/2] binfmt_misc: don't let an 'F' entry pin its own instance

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The user_ns reference obtained in `bm_get_tree` and transferred to `sb->s_fs_info` by `sget_fc` is permanently leaked if `bm_fill_super` fails.
- [High] Deleting a `binfmt_misc` entry registered with the `F` (MISC_FMT_OPEN_FILE) flag leaks the negative `i_writecount` of the interpreter file, permanently preventing it from being opened for writing until reboot.
--

--- Patch [1]: [PATCH 1/2] binfmt_misc: don't let an 'F' entry pin its own instance ---
commit 69b9018135a8cbdf7775e3461daaa6ac7c2e2eb2
Author: Christian Brauner <brauner@kernel.org>

binfmt_misc: don't let an 'F' entry pin its own instance
    
An entry registered with 'F' opens its interpreter at registration time
and holds that file until the entry is freed. Any entry nobody removes
by hand only gets closed once the binfmt_misc superblock is shut down.
If the interpreter lives on a mount that keeps that superblock alive the
two pin each other. This patch ensures SB_I_NOEXEC and s_stack_depth are
set to prevent this circular dependency and pinning.
    
Cc: stable@vger.kernel.org # v6.7+

> diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
> index 84349fcb93f13..659a9f215db2c 100644
> --- a/fs/binfmt_misc.c
> +++ b/fs/binfmt_misc.c
> @@ -938,6 +938,10 @@ static int bm_fill_super(struct super_block *sb, struct fs_context *fc)
>  	if (WARN_ON(user_ns != current_user_ns()))
>  		return -EINVAL;
>  
> +	/* Never exec off this instance and never let anything stack on it. */
> +	sb->s_iflags |= SB_I_NOEXEC | SB_I_NODEV;
> +	sb->s_stack_depth = FILESYSTEM_MAX_STACK_DEPTH;
> +
>  	/*
>  	 * Lazily allocate a new binfmt_misc instance for this namespace, i.e.

[ ... ]

Looking at the rest of bm_fill_super() in fs/binfmt_misc.c:

	err = simple_fill_super(sb, BINFMTFS_MAGIC, bm_files);
	if (!err)
		sb->s_op = &s_ops;
	return err;
}

[Severity: High]
This is a pre-existing issue, but does this error path permanently leak the 
user_namespace reference?

If simple_fill_super() fails, or if a memory allocation failure occurs 
earlier in bm_fill_super(), sb->s_root remains NULL. When vfs_get_super() 
handles the error, generic_shutdown_super() will skip calling 
s_op->put_super(sb). 

Since fc->s_fs_info was already cleared during sget_fc(), does this mean 
the user_namespace reference in sb->s_fs_info is never released?

Could an unprivileged local user trigger this by intentionally failing 
userns mounts (e.g., via memory exhaustion) to cause resource exhaustion?


On a related note regarding pre-existing issues, looking at 
put_binfmt_handler() in fs/binfmt_misc.c:

static void put_binfmt_handler(struct binfmt_misc *e)
{
        ...
        if (e->flags & MISC_FMT_OPEN_FILE)
                filp_close(e->interp_file, NULL);
        kfree(e);
}

[Severity: High]
This is a pre-existing issue, but does this teardown path leak the negative 
i_writecount of the interpreter file?

When an entry is registered with the 'F' flag, bm_register_write() uses 
open_exec(), which internally decrements the inode's i_writecount to prevent 
concurrent writes. While the error path during registration calls 
exe_file_allow_write_access(), put_binfmt_handler() only calls filp_close(). 

Since filp_close() does not automatically restore executable write access, 
would deleting the binfmt_misc entry permanently leave the binary unwritable 
(returning -ETXTBSY) until reboot?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728-work-binfmt_misc-selfpin-v1-0-74df5daeca5b@kernel.org?part=1

Re: [PATCH 1/2] binfmt_misc: don't let an 'F' entry pin its own instance

[Christian Brauner]

> [ ... ]
> 
> Looking at the rest of bm_fill_super() in fs/binfmt_misc.c:
> 
> 	err = simple_fill_super(sb, BINFMTFS_MAGIC, bm_files);
> 	if (!err)
> 		sb->s_op = &s_ops;
> 	return err;
> }

This _seems_ legit but I need to double-check.

> [Severity: High]
> This is a pre-existing issue, but does this error path permanently leak the 
> user_namespace reference?
> 
> If simple_fill_super() fails, or if a memory allocation failure occurs 
> earlier in bm_fill_super(), sb->s_root remains NULL. When vfs_get_super() 
> handles the error, generic_shutdown_super() will skip calling 
> s_op->put_super(sb). 
> 
> Since fc->s_fs_info was already cleared during sget_fc(), does this mean 
> the user_namespace reference in sb->s_fs_info is never released?
> 
> Could an unprivileged local user trigger this by intentionally failing 
> userns mounts (e.g., via memory exhaustion) to cause resource exhaustion?
> 
> 
> On a related note regarding pre-existing issues, looking at 
> put_binfmt_handler() in fs/binfmt_misc.c:
> 
> static void put_binfmt_handler(struct binfmt_misc *e)
> {
>         ...
>         if (e->flags & MISC_FMT_OPEN_FILE)
>                 filp_close(e->interp_file, NULL);
>         kfree(e);
> }
> 
> [Severity: High]
> This is a pre-existing issue, but does this teardown path leak the negative 
> i_writecount of the interpreter file?
> 
> When an entry is registered with the 'F' flag, bm_register_write() uses 
> open_exec(), which internally decrements the inode's i_writecount to prevent 
> concurrent writes. While the error path during registration calls 
> exe_file_allow_write_access(), put_binfmt_handler() only calls filp_close(). 
> 
> Since filp_close() does not automatically restore executable write access, 
> would deleting the binfmt_misc entry permanently leave the binary unwritable 
> (returning -ETXTBSY) until reboot?

This is fixed in another patch series of mine.

[PATCH 2/2] selftests/exec: check that a binfmt_misc instance cannot be pinned

[Christian Brauner]

An 'F' entry whose interpreter keeps the binfmt_misc superblock alive
pins the instance that owns it forever. Cover both ways to build that:

- an interpreter on the instance's own files, control file and entry
  file alike

- and an instance used as an overlayfs lower layer.

Check that an ordinary 'F' registration still succeeds so the fix stays
honest about not changing what 'F' promises.

Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 tools/testing/selftests/exec/Makefile              |  10 ++
 tools/testing/selftests/exec/binfmt_misc_selfpin.c | 158 +++++++++++++++++++++
 tools/testing/selftests/exec/config                |   4 +
 3 files changed, 172 insertions(+)

diff --git a/tools/testing/selftests/exec/Makefile b/tools/testing/selftests/exec/Makefile
index 45a3cfc435cf..b15505f5e0bc 100644
--- a/tools/testing/selftests/exec/Makefile
+++ b/tools/testing/selftests/exec/Makefile
@@ -21,6 +21,10 @@ TEST_GEN_PROGS += recursion-depth
 TEST_GEN_PROGS += null-argv
 TEST_GEN_PROGS += check-exec
 
+# binfmt_misc must not be reachable as an exec source or as a stacking layer,
+# or an 'F' entry can pin the instance that owns it. Unprivileged, no bpf.
+TEST_GEN_PROGS += binfmt_misc_selfpin
+
 EXTRA_CLEAN := $(OUTPUT)/subdir.moved $(OUTPUT)/execveat.moved $(OUTPUT)/xxxxx*	\
 	       $(OUTPUT)/S_I*.test
 
@@ -55,3 +59,9 @@ $(OUTPUT)/script-exec.inc: $(CHECK_EXEC_SAMPLES)/script-exec.inc
 	cp $< $@
 $(OUTPUT)/script-noexec.inc: $(CHECK_EXEC_SAMPLES)/script-noexec.inc
 	cp $< $@
+
+# Reuses setup_userns()/write_file() from the filesystems selftests. Their
+# wrappers.h wants the uapi headers, so ask for them here rather than widening
+# CFLAGS for every program in this directory.
+$(OUTPUT)/binfmt_misc_selfpin: CFLAGS += $(TOOLS_INCLUDES)
+$(OUTPUT)/binfmt_misc_selfpin: ../filesystems/utils.c
diff --git a/tools/testing/selftests/exec/binfmt_misc_selfpin.c b/tools/testing/selftests/exec/binfmt_misc_selfpin.c
new file mode 100644
index 000000000000..5286b0604eed
--- /dev/null
+++ b/tools/testing/selftests/exec/binfmt_misc_selfpin.c
@@ -0,0 +1,158 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * An 'F' entry keeps its interpreter open for as long as the entry exists,
+ * and the entry only goes away when the binfmt_misc superblock is destroyed.
+ * An interpreter that lives on a mount which in turn keeps that superblock
+ * alive therefore pins the instance that owns it, and nothing can break the
+ * cycle. Check the two ways userspace could arrange for that: an interpreter
+ * on the binfmt_misc instance itself, and one on a filesystem stacked on it.
+ *
+ * Runs unprivileged in a user namespace; binfmt_misc is FS_USERNS_MOUNT.
+ */
+#define _GNU_SOURCE
+#include <fcntl.h>
+#include <limits.h>
+#include <sched.h>
+#include <sys/mount.h>
+#include <sys/stat.h>
+
+#include "../filesystems/utils.h"
+#include "kselftest_harness.h"
+
+#define MNT		"/tmp/binfmt_selfpin"
+#define BACKING		"/tmp/binfmt_selfpin_back"
+#define LOWER		BACKING "/lower"
+#define MERGED		"/tmp/binfmt_selfpin_merged"
+
+#define MAGIC		"\\xde\\xad"
+#define RULE(interp)	":selfpin:M::" MAGIC "::" interp ":F"
+/* Not on the instance, and unlike /bin/true it always exists. */
+#define INTERP		"/proc/self/exe"
+
+#define OPTS_MAX	(3 * PATH_MAX + 64)
+
+static int ensure_dir(const char *path)
+{
+	if (mkdir(path, 0755) && errno != EEXIST)
+		return -1;
+	return 0;
+}
+
+/* Write @rule to this instance's register file, preserving write(2)'s errno. */
+static int register_at(struct __test_metadata *_metadata, const char *rule)
+{
+	int fd, saved;
+	ssize_t n;
+
+	fd = open(MNT "/register", O_WRONLY);
+	ASSERT_GE(fd, 0);
+	n = write(fd, rule, strlen(rule));
+	saved = errno;
+	close(fd);
+	errno = saved;
+	return n < 0 ? -1 : 0;
+}
+
+/*
+ * Mount an overlay over @lower using a private upper/work pair, so the two
+ * mounts this test performs cannot interfere with each other and neither
+ * overlaps the lower layer.
+ */
+static int mount_overlay(const char *lower, int nr)
+{
+	char opts[OPTS_MAX], upper[PATH_MAX], work[PATH_MAX];
+
+	snprintf(upper, sizeof(upper), "%s/upper%d", BACKING, nr);
+	snprintf(work, sizeof(work), "%s/work%d", BACKING, nr);
+	if (mkdir(upper, 0755) || mkdir(work, 0755))
+		return -1;
+
+	snprintf(opts, sizeof(opts), "lowerdir=%s,upperdir=%s,workdir=%s",
+		 lower, upper, work);
+	return mount("ovl", MERGED, "overlay", 0, opts);
+}
+
+FIXTURE(selfpin) {
+};
+
+FIXTURE_SETUP(selfpin)
+{
+	/* setup_userns() exits rather than returns if this is not there. */
+	if (access("/proc/self/ns/user", F_OK))
+		SKIP(return, "kernel without user namespaces");
+	ASSERT_EQ(setup_userns(), 0);
+
+	ASSERT_EQ(ensure_dir(MNT), 0);
+	if (mount("binfmt_misc", MNT, "binfmt_misc", 0, NULL)) {
+		int saved = errno;
+
+		/* Teardown doesn't run when setup skips, so clean up here. */
+		rmdir(MNT);
+		SKIP(return, "no binfmt_misc: %s", strerror(saved));
+	}
+}
+
+FIXTURE_TEARDOWN(selfpin)
+{
+	/* The namespaces go with the process; just don't litter /tmp. */
+	umount2(MERGED, MNT_DETACH);
+	umount2(BACKING, MNT_DETACH);
+	umount2(MNT, MNT_DETACH);
+	rmdir(MERGED);
+	rmdir(BACKING);
+	rmdir(MNT);
+}
+
+/*
+ * The instance's own files are regular files the mounter owns, so they can be
+ * made executable. Opening one for exec still has to fail, otherwise the entry
+ * pins the very superblock it lives in.
+ */
+TEST_F(selfpin, interpreter_on_the_instance)
+{
+	ASSERT_EQ(chmod(MNT "/status", 0755), 0);
+
+	ASSERT_NE(register_at(_metadata, RULE(MNT "/status")), 0);
+	EXPECT_EQ(errno, EACCES);
+}
+
+/* Same for an entry file rather than one of the control files. */
+TEST_F(selfpin, interpreter_on_an_entry)
+{
+	ASSERT_EQ(register_at(_metadata, ":victim:M::" MAGIC "::" INTERP ":"), 0);
+	ASSERT_EQ(chmod(MNT "/victim", 0755), 0);
+
+	ASSERT_NE(register_at(_metadata, RULE(MNT "/victim")), 0);
+	EXPECT_EQ(errno, EACCES);
+}
+
+/*
+ * A stacking filesystem holds a private clone of each layer for its whole
+ * lifetime, so an instance used as a layer can be pinned by an interpreter
+ * that does not live on it at all. Refuse to be a layer.
+ */
+TEST_F(selfpin, refuses_to_be_stacked_on)
+{
+	ASSERT_EQ(ensure_dir(BACKING), 0);
+	ASSERT_EQ(mount("tmpfs", BACKING, "tmpfs", 0, NULL), 0);
+	ASSERT_EQ(mkdir(LOWER, 0755), 0);
+	ASSERT_EQ(ensure_dir(MERGED), 0);
+
+	/* Nothing to prove unless overlayfs works here at all. */
+	if (mount_overlay(LOWER, 1)) {
+		if (errno == ENODEV || errno == EPERM)
+			SKIP(return, "no unprivileged overlayfs");
+		SKIP(return, "overlayfs unusable here: %s", strerror(errno));
+	}
+	ASSERT_EQ(umount(MERGED), 0);
+
+	EXPECT_NE(mount_overlay(MNT, 2), 0);
+}
+
+/* An ordinary interpreter still registers with 'F'. */
+TEST_F(selfpin, ordinary_interpreter_still_works)
+{
+	EXPECT_EQ(register_at(_metadata, RULE(INTERP)), 0);
+}
+
+TEST_HARNESS_MAIN
diff --git a/tools/testing/selftests/exec/config b/tools/testing/selftests/exec/config
index c308079867b3..c03d52d8abee 100644
--- a/tools/testing/selftests/exec/config
+++ b/tools/testing/selftests/exec/config
@@ -1,2 +1,6 @@
 CONFIG_BLK_DEV=y
 CONFIG_BLK_DEV_LOOP=y
+CONFIG_BINFMT_MISC=y
+CONFIG_OVERLAY_FS=y
+CONFIG_TMPFS=y
+CONFIG_USER_NS=y

-- 
2.53.0