* [PATCH] binfmt_misc: Node could be NULL when evicting inode
@ 2017-10-10 10:06 Eryu Guan
2017-10-10 12:46 ` Oleg Nesterov
0 siblings, 1 reply; 3+ messages in thread
From: Eryu Guan @ 2017-10-10 10:06 UTC (permalink / raw)
To: linux-fsdevel; +Cc: Alexander Viro, Eryu Guan, Oleg Nesterov
inode->i_private is assigned by a Node pointer only after
registering a new binary format, so it could be NULL if we only
mount binfmt_misc but don't register any format, and this results in
NULL pointer dereference at umount time. e.g.
mount -t binfmt_misc binfmt_misc /proc/sys/fs/binfmt_misc
umount /proc/sys/fs/binfmt_misc
[ 9379.678259] BUG: unable to handle kernel NULL pointer dereference at 0000000000000013
[ 9379.985952] IP: bm_evict_inode+0x16/0x40 [binfmt_misc]
...
[ 9380.964911] Call Trace:
[ 9380.977633] evict+0xd3/0x1a0
[ 9380.994449] iput+0x17d/0x1d0
[ 9381.010306] dentry_unlink_inode+0xb9/0xf0
[ 9381.034046] __dentry_kill+0xc7/0x170
[ 9381.055145] shrink_dentry_list+0x122/0x280
[ 9381.078908] shrink_dcache_parent+0x39/0x90
[ 9381.103082] do_one_tree+0x12/0x40
[ 9381.122005] shrink_dcache_for_umount+0x2d/0x90
[ 9381.146517] generic_shutdown_super+0x1f/0x120
[ 9381.171644] kill_litter_super+0x29/0x40
[ 9381.193513] deactivate_locked_super+0x43/0x70
[ 9381.219177] deactivate_super+0x45/0x60
[ 9381.240130] cleanup_mnt+0x3f/0x70
[ 9381.259064] __cleanup_mnt+0x12/0x20
[ 9381.279802] task_work_run+0x86/0xa0
[ 9381.299612] exit_to_usermode_loop+0x6d/0x99
[ 9381.323872] syscall_return_slowpath+0xba/0xf0
[ 9381.350464] entry_SYSCALL_64_fastpath+0xa3/0xa
Fix it by making sure Node (e) is not NULL.
Cc: Oleg Nesterov <oleg@redhat.com>
Fixes: 83f918274e4b ("exec: binfmt_misc: shift filp_close(interp_file) from kill_node() to bm_evict_inode()")
Signed-off-by: Eryu Guan <eguan@redhat.com>
---
fs/binfmt_misc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
index 2a46762def31..a7c5a9861bef 100644
--- a/fs/binfmt_misc.c
+++ b/fs/binfmt_misc.c
@@ -596,7 +596,7 @@ static void bm_evict_inode(struct inode *inode)
{
Node *e = inode->i_private;
- if (e->flags & MISC_FMT_OPEN_FILE)
+ if (e && e->flags & MISC_FMT_OPEN_FILE)
filp_close(e->interp_file, NULL);
clear_inode(inode);
--
2.13.6
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] binfmt_misc: Node could be NULL when evicting inode 2017-10-10 10:06 [PATCH] binfmt_misc: Node could be NULL when evicting inode Eryu Guan @ 2017-10-10 12:46 ` Oleg Nesterov 2017-10-10 16:25 ` Oleg Nesterov 0 siblings, 1 reply; 3+ messages in thread From: Oleg Nesterov @ 2017-10-10 12:46 UTC (permalink / raw) To: Eryu Guan, Andrew Morton Cc: linux-fsdevel, Alexander Viro, Tycho Andersen, chandan, Kees Cook, linux-kernel On 10/10, Eryu Guan wrote: > > inode->i_private is assigned by a Node pointer only after > registering a new binary format, so it could be NULL if we only > mount binfmt_misc but don't register any format, ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Not really, I 'think... I mean, the problem is that e == NULL when inode is root_inode, or "status", or "register", created by bm_fill_super()... Yes, you need to unregister all formats to hit this problem. Oh, and I'm afraid I intoduced another problem, bm_register_write() error paths can call iput() with MISC_FMT_OPEN_FILE set but e->interpreter == NULL. I'll re-check and send another patch today. Thanks a lot! Acked-by: Oleg Nesterov <oleg@redhat.com> > and this results in > NULL pointer dereference at umount time. e.g. > > mount -t binfmt_misc binfmt_misc /proc/sys/fs/binfmt_misc > umount /proc/sys/fs/binfmt_misc > > [ 9379.678259] BUG: unable to handle kernel NULL pointer dereference at 0000000000000013 > [ 9379.985952] IP: bm_evict_inode+0x16/0x40 [binfmt_misc] > ... > [ 9380.964911] Call Trace: > [ 9380.977633] evict+0xd3/0x1a0 > [ 9380.994449] iput+0x17d/0x1d0 > [ 9381.010306] dentry_unlink_inode+0xb9/0xf0 > [ 9381.034046] __dentry_kill+0xc7/0x170 > [ 9381.055145] shrink_dentry_list+0x122/0x280 > [ 9381.078908] shrink_dcache_parent+0x39/0x90 > [ 9381.103082] do_one_tree+0x12/0x40 > [ 9381.122005] shrink_dcache_for_umount+0x2d/0x90 > [ 9381.146517] generic_shutdown_super+0x1f/0x120 > [ 9381.171644] kill_litter_super+0x29/0x40 > [ 9381.193513] deactivate_locked_super+0x43/0x70 > [ 9381.219177] deactivate_super+0x45/0x60 > [ 9381.240130] cleanup_mnt+0x3f/0x70 > [ 9381.259064] __cleanup_mnt+0x12/0x20 > [ 9381.279802] task_work_run+0x86/0xa0 > [ 9381.299612] exit_to_usermode_loop+0x6d/0x99 > [ 9381.323872] syscall_return_slowpath+0xba/0xf0 > [ 9381.350464] entry_SYSCALL_64_fastpath+0xa3/0xa > > Fix it by making sure Node (e) is not NULL. > > Cc: Oleg Nesterov <oleg@redhat.com> > Fixes: 83f918274e4b ("exec: binfmt_misc: shift filp_close(interp_file) from kill_node() to bm_evict_inode()") > Signed-off-by: Eryu Guan <eguan@redhat.com> > --- > fs/binfmt_misc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c > index 2a46762def31..a7c5a9861bef 100644 > --- a/fs/binfmt_misc.c > +++ b/fs/binfmt_misc.c > @@ -596,7 +596,7 @@ static void bm_evict_inode(struct inode *inode) > { > Node *e = inode->i_private; > > - if (e->flags & MISC_FMT_OPEN_FILE) > + if (e && e->flags & MISC_FMT_OPEN_FILE) > filp_close(e->interp_file, NULL); > > clear_inode(inode); > -- > 2.13.6 > ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] binfmt_misc: Node could be NULL when evicting inode 2017-10-10 12:46 ` Oleg Nesterov @ 2017-10-10 16:25 ` Oleg Nesterov 0 siblings, 0 replies; 3+ messages in thread From: Oleg Nesterov @ 2017-10-10 16:25 UTC (permalink / raw) To: Eryu Guan, Andrew Morton Cc: linux-fsdevel, Alexander Viro, Tycho Andersen, chandan, Kees Cook, linux-kernel On 10/10, Oleg Nesterov wrote: > > On 10/10, Eryu Guan wrote: > > > > inode->i_private is assigned by a Node pointer only after > > registering a new binary format, so it could be NULL if we only > > mount binfmt_misc but don't register any format, > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > Not really, I 'think... I mean, the problem is that e == NULL when inode > is root_inode, or "status", or "register", created by bm_fill_super()... > > Yes, you need to unregister all formats to hit this problem. > > > Oh, and I'm afraid I intoduced another problem, bm_register_write() error > paths can call iput() with MISC_FMT_OPEN_FILE set but e->interpreter == NULL. > I'll re-check and send another patch today. Ah, no, I was wrong again... If bm_register_write() fails, bm_evict_inode() will hit the same ->i_private == NULL problem fixed by your patch. So the changelog can be updated to explain that i_private == NULL is possible if inode was created by bm_fill_super(), or iput() was called by the error path in bm_register_write(). But the patch is obviously fine, thanks again. > Thanks a lot! > > > Acked-by: Oleg Nesterov <oleg@redhat.com> > > > > and this results in > > NULL pointer dereference at umount time. e.g. > > > > mount -t binfmt_misc binfmt_misc /proc/sys/fs/binfmt_misc > > umount /proc/sys/fs/binfmt_misc > > > > [ 9379.678259] BUG: unable to handle kernel NULL pointer dereference at 0000000000000013 > > [ 9379.985952] IP: bm_evict_inode+0x16/0x40 [binfmt_misc] > > ... > > [ 9380.964911] Call Trace: > > [ 9380.977633] evict+0xd3/0x1a0 > > [ 9380.994449] iput+0x17d/0x1d0 > > [ 9381.010306] dentry_unlink_inode+0xb9/0xf0 > > [ 9381.034046] __dentry_kill+0xc7/0x170 > > [ 9381.055145] shrink_dentry_list+0x122/0x280 > > [ 9381.078908] shrink_dcache_parent+0x39/0x90 > > [ 9381.103082] do_one_tree+0x12/0x40 > > [ 9381.122005] shrink_dcache_for_umount+0x2d/0x90 > > [ 9381.146517] generic_shutdown_super+0x1f/0x120 > > [ 9381.171644] kill_litter_super+0x29/0x40 > > [ 9381.193513] deactivate_locked_super+0x43/0x70 > > [ 9381.219177] deactivate_super+0x45/0x60 > > [ 9381.240130] cleanup_mnt+0x3f/0x70 > > [ 9381.259064] __cleanup_mnt+0x12/0x20 > > [ 9381.279802] task_work_run+0x86/0xa0 > > [ 9381.299612] exit_to_usermode_loop+0x6d/0x99 > > [ 9381.323872] syscall_return_slowpath+0xba/0xf0 > > [ 9381.350464] entry_SYSCALL_64_fastpath+0xa3/0xa > > > > Fix it by making sure Node (e) is not NULL. > > > > Cc: Oleg Nesterov <oleg@redhat.com> > > Fixes: 83f918274e4b ("exec: binfmt_misc: shift filp_close(interp_file) from kill_node() to bm_evict_inode()") > > Signed-off-by: Eryu Guan <eguan@redhat.com> > > --- > > fs/binfmt_misc.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c > > index 2a46762def31..a7c5a9861bef 100644 > > --- a/fs/binfmt_misc.c > > +++ b/fs/binfmt_misc.c > > @@ -596,7 +596,7 @@ static void bm_evict_inode(struct inode *inode) > > { > > Node *e = inode->i_private; > > > > - if (e->flags & MISC_FMT_OPEN_FILE) > > + if (e && e->flags & MISC_FMT_OPEN_FILE) > > filp_close(e->interp_file, NULL); > > > > clear_inode(inode); > > -- > > 2.13.6 > > ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-10-10 16:25 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-10-10 10:06 [PATCH] binfmt_misc: Node could be NULL when evicting inode Eryu Guan 2017-10-10 12:46 ` Oleg Nesterov 2017-10-10 16:25 ` Oleg Nesterov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).