| Message ID | 20210228224414.95962-1-liorribak@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] binfmt_misc: Fix possible deadlock in bm_register_write | expand |
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@entry=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@entry=0xffffc9000022fd10, op=op@entry=0xffffc9000022fe34, flags=flags@entry=65) at fs/namei.c:3366 > 8 0xffffffff811efe1c in do_filp_open (dfd=<optimized out>, pathname=pathname@entry=0xffff8880031b9000, op=op@entry=0xffffc9000022fe34) at fs/namei.c:3396 > 9 0xffffffff811e493f in do_open_execat (fd=fd@entry=-100, name=name@entry=0xffff8880031b9000, flags=<optimized out>, flags@entry=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@entry=0xffff888004098500, buf=buf@entry=0xa758d0 ":iiiii:E::ii::i:CF\n", count=count@entry=19, pos=pos@entry=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@gmail.com> Acked-by: Helge Deller <deller@gmx.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; > } >
On Sun, 28 Feb 2021 14:44:14 -0800 Lior Ribak <liorribak@gmail.com> 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 > > 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@entry=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@entry=0xffffc9000022fd10, op=op@entry=0xffffc9000022fe34, flags=flags@entry=65) at fs/namei.c:3366 > 8 0xffffffff811efe1c in do_filp_open (dfd=<optimized out>, pathname=pathname@entry=0xffff8880031b9000, op=op@entry=0xffffc9000022fe34) at fs/namei.c:3396 > 9 0xffffffff811e493f in do_open_execat (fd=fd@entry=-100, name=name@entry=0xffff8880031b9000, flags=<optimized out>, flags@entry=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@entry=0xffff888004098500, buf=buf@entry=0xa758d0 ":iiiii:E::ii::i:CF\n", count=count@entry=19, pos=pos@entry=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 > Looks good to me. I assume this is an ancient bug and that a backport to -stable trees (with a cc:stable) is warranted?
> Looks good to me. > > I assume this is an ancient bug and that a backport to -stable trees > (with a cc:stable) is warranted? Yep, it exists since the "persistent opened binary handler" commit 948b701a607f123df92ed29084413e5dd8cda2ed was introduced in version 4.8, and yes i think the patch can be backported to the stable trees
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; }
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 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@entry=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@entry=0xffffc9000022fd10, op=op@entry=0xffffc9000022fe34, flags=flags@entry=65) at fs/namei.c:3366 8 0xffffffff811efe1c in do_filp_open (dfd=<optimized out>, pathname=pathname@entry=0xffff8880031b9000, op=op@entry=0xffffc9000022fe34) at fs/namei.c:3396 9 0xffffffff811e493f in do_open_execat (fd=fd@entry=-100, name=name@entry=0xffff8880031b9000, flags=<optimized out>, flags@entry=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@entry=0xffff888004098500, buf=buf@entry=0xa758d0 ":iiiii:E::ii::i:CF\n", count=count@entry=19, pos=pos@entry=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@gmail.com> --- v2: Added "kfree(e)" above "return PTR_ERR(f)" fs/binfmt_misc.c | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-)