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

[sashiko-bot@kernel.org]

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