linux-kernel - Re: [PATCH v2] binfmt_misc: Fix possible deadlock in bm_register

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1dae5800-32f3-5527-083a-88334142057a@gmx.de>
Date:   Mon, 1 Mar 2021 21:24:33 +0100
From:   Helge Deller <deller@....de>
To:     Lior Ribak <liorribak@...il.com>, akpm@...ux-foundation.org,
        viro@...iv.linux.org.uk
Cc:     linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH v2] binfmt_misc: Fix possible deadlock in
 bm_register_write

On 2/28/21 11:44 PM, Lior Ribak wrote:
> There is a deadlock in bm_register_write:
> First, in the beggining of the function, a lock is taken on the
> binfmt_misc root inode with inode_lock(d_inode(root))
> Then, if the user used the MISC_FMT_OPEN_FILE flag, the function will
> call open_exec on the user-provided interpreter.
> open_exec will call a path lookup, and if the path lookup process
> includes the root of binfmt_misc, it will try to take a shared lock
> on its inode again, but it is already locked, and the code will
> get stuck in a deadlock
>
> To reproduce the bug:
> $ echo ":iiiii:E::ii::/proc/sys/fs/binfmt_misc/bla:F" > /proc/sys/fs/binfmt_misc/register

Yes, it's easily reproduceable with this command.


> backtrace of where the lock occurs (#5):
> 0  schedule () at ./arch/x86/include/asm/current.h:15
> 1  0xffffffff81b51237 in rwsem_down_read_slowpath (sem=0xffff888003b202e0, count=<optimized out>, state=state@...ry=2) at kernel/locking/rwsem.c:992
> 2  0xffffffff81b5150a in __down_read_common (state=2, sem=<optimized out>) at kernel/locking/rwsem.c:1213
> 3  __down_read (sem=<optimized out>) at kernel/locking/rwsem.c:1222
> 4  down_read (sem=<optimized out>) at kernel/locking/rwsem.c:1355
> 5  0xffffffff811ee22a in inode_lock_shared (inode=<optimized out>) at ./include/linux/fs.h:783
> 6  open_last_lookups (op=0xffffc9000022fe34, file=0xffff888004098600, nd=0xffffc9000022fd10) at fs/namei.c:3177
> 7  path_openat (nd=nd@...ry=0xffffc9000022fd10, op=op@...ry=0xffffc9000022fe34, flags=flags@...ry=65) at fs/namei.c:3366
> 8  0xffffffff811efe1c in do_filp_open (dfd=<optimized out>, pathname=pathname@...ry=0xffff8880031b9000, op=op@...ry=0xffffc9000022fe34) at fs/namei.c:3396
> 9  0xffffffff811e493f in do_open_execat (fd=fd@...ry=-100, name=name@...ry=0xffff8880031b9000, flags=<optimized out>, flags@...ry=0) at fs/exec.c:913
> 10 0xffffffff811e4a92 in open_exec (name=<optimized out>) at fs/exec.c:948
> 11 0xffffffff8124aa84 in bm_register_write (file=<optimized out>, buffer=<optimized out>, count=19, ppos=<optimized out>) at fs/binfmt_misc.c:682
> 12 0xffffffff811decd2 in vfs_write (file=file@...ry=0xffff888004098500, buf=buf@...ry=0xa758d0 ":iiiii:E::ii::i:CF\n", count=count@...ry=19, pos=pos@...ry=0xffffc9000022ff10) at fs/read_write.c:603
> 13 0xffffffff811defda in ksys_write (fd=<optimized out>, buf=0xa758d0 ":iiiii:E::ii::i:CF\n", count=19) at fs/read_write.c:658
> 14 0xffffffff81b49813 in do_syscall_64 (nr=<optimized out>, regs=0xffffc9000022ff58) at arch/x86/entry/common.c:46
> 15 0xffffffff81c0007c in entry_SYSCALL_64 () at arch/x86/entry/entry_64.S:120
>
> To solve the issue, the open_exec call is moved to before the write
> lock is taken by bm_register_write
>
> Signed-off-by: Lior Ribak <liorribak@...il.com>


Acked-by: Helge Deller <deller@....de>

Thanks!
Helge

> ---
> v2: Added "kfree(e)" above "return PTR_ERR(f)"
>
>   fs/binfmt_misc.c | 29 ++++++++++++++---------------
>   1 file changed, 14 insertions(+), 15 deletions(-)
>
> diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
> index c457334de43f..e1eae7ea823a 100644
> --- a/fs/binfmt_misc.c
> +++ b/fs/binfmt_misc.c
> @@ -649,12 +649,24 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer,
>   	struct super_block *sb = file_inode(file)->i_sb;
>   	struct dentry *root = sb->s_root, *dentry;
>   	int err = 0;
> +	struct file *f = NULL;
>
>   	e = create_entry(buffer, count);
>
>   	if (IS_ERR(e))
>   		return PTR_ERR(e);
>
> +	if (e->flags & MISC_FMT_OPEN_FILE) {
> +		f = open_exec(e->interpreter);
> +		if (IS_ERR(f)) {
> +			pr_notice("register: failed to install interpreter file %s\n",
> +				 e->interpreter);
> +			kfree(e);
> +			return PTR_ERR(f);
> +		}
> +		e->interp_file = f;
> +	}
> +
>   	inode_lock(d_inode(root));
>   	dentry = lookup_one_len(e->name, root, strlen(e->name));
>   	err = PTR_ERR(dentry);
> @@ -678,21 +690,6 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer,
>   		goto out2;
>   	}
>
> -	if (e->flags & MISC_FMT_OPEN_FILE) {
> -		struct file *f;
> -
> -		f = open_exec(e->interpreter);
> -		if (IS_ERR(f)) {
> -			err = PTR_ERR(f);
> -			pr_notice("register: failed to install interpreter file %s\n", e->interpreter);
> -			simple_release_fs(&bm_mnt, &entry_count);
> -			iput(inode);
> -			inode = NULL;
> -			goto out2;
> -		}
> -		e->interp_file = f;
> -	}
> -
>   	e->dentry = dget(dentry);
>   	inode->i_private = e;
>   	inode->i_fop = &bm_entry_operations;
> @@ -709,6 +706,8 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer,
>   	inode_unlock(d_inode(root));
>
>   	if (err) {
> +		if (f)
> +			filp_close(f, NULL);
>   		kfree(e);
>   		return err;
>   	}
>