| Message ID | 20220520023106.6651-1-zhangxiaoxu5@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nfsd: Fix null-ptr-deref in nfsd_fill_super() | expand |
[ Note well: Updated Bruce's email address. ] > On May 19, 2022, at 10:31 PM, Zhang Xiaoxu <zhangxiaoxu5@huawei.com> wrote: > > KASAN report null-ptr-deref as follows: > > BUG: KASAN: null-ptr-deref in nfsd_fill_super+0xc6/0xe0 [nfsd] > Write of size 8 at addr 000000000000005d by task a.out/852 > > CPU: 7 PID: 852 Comm: a.out Not tainted 5.18.0-rc7-dirty #66 > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.14.0-1.fc33 04/01/2014 > Call Trace: > <TASK> > dump_stack_lvl+0x34/0x44 > kasan_report+0xab/0x120 > ? nfsd_mkdir+0x71/0x1c0 [nfsd] > ? nfsd_fill_super+0xc6/0xe0 [nfsd] > nfsd_fill_super+0xc6/0xe0 [nfsd] > ? nfsd_mkdir+0x1c0/0x1c0 [nfsd] > get_tree_keyed+0x8e/0x100 > vfs_get_tree+0x41/0xf0 > __do_sys_fsconfig+0x590/0x670 > ? fscontext_read+0x180/0x180 > ? anon_inode_getfd+0x4f/0x70 > do_syscall_64+0x35/0x80 > entry_SYSCALL_64_after_hwframe+0x44/0xae > > This can be reproduce by concurrent operations: > 1. fsopen(nfsd)/fsconfig > 2. insmod/rmmod nfsd > > Since the nfsd file system is registered before than nfsd_net allocated, > the caller may get the file_system_type and use the nfsd_net before it > allocated, then null-ptr-deref occured. > > So should allocate the nfsd_net firstly, other than register file system. IIUC, I suggest: "So init_nfsd() should call register_filesystem() last." > Fixes: bd5ae9288d64 ("nfsd: register pernet ops last, unregister first") > Cc: stable@kernel.org > Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com> I think this looks right. Bruce, as author of bd5ae9288d64, any thoughts? I need a v2 of this, though. The current version conflicts with the courteous server patches already in my for-next branch. See: https://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git/log/?h=for-next > --- > fs/nfsd/nfsctl.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c > index 16920e4512bd..e17100e90e19 100644 > --- a/fs/nfsd/nfsctl.c > +++ b/fs/nfsd/nfsctl.c > @@ -1535,20 +1535,20 @@ static int __init init_nfsd(void) > retval = create_proc_exports_entry(); > if (retval) > goto out_free_lockd; > - retval = register_filesystem(&nfsd_fs_type); > - if (retval) > - goto out_free_exports; > retval = register_pernet_subsys(&nfsd_net_ops); > if (retval < 0) > - goto out_free_filesystem; > + goto out_free_exports; > retval = register_cld_notifier(); > + if (retval) > + goto out_free_subsys; > + retval = register_filesystem(&nfsd_fs_type); > if (retval) > goto out_free_all; > return 0; > out_free_all: > + unregister_cld_notifier(); > +out_free_subsys: > unregister_pernet_subsys(&nfsd_net_ops); > -out_free_filesystem: > - unregister_filesystem(&nfsd_fs_type); > out_free_exports: > remove_proc_entry("fs/nfs/exports", NULL); > remove_proc_entry("fs/nfs", NULL); > @@ -1566,6 +1566,7 @@ static int __init init_nfsd(void) > > static void __exit exit_nfsd(void) > { > + unregister_filesystem(&nfsd_fs_type); > unregister_cld_notifier(); > unregister_pernet_subsys(&nfsd_net_ops); > nfsd_drc_slab_free(); > @@ -1575,7 +1576,6 @@ static void __exit exit_nfsd(void) > nfsd_lockd_shutdown(); > nfsd4_free_slabs(); > nfsd4_exit_pnfs(); > - unregister_filesystem(&nfsd_fs_type); > } > > MODULE_AUTHOR("Olaf Kirch <okir@monad.swb.de>"); > -- > 2.31.1 > -- Chuck Lever
On Fri, May 20, 2022 at 03:22:51PM +0000, Chuck Lever III wrote: > [ Note well: Updated Bruce's email address. ] > > > > On May 19, 2022, at 10:31 PM, Zhang Xiaoxu <zhangxiaoxu5@huawei.com> wrote: > > > > KASAN report null-ptr-deref as follows: > > > > BUG: KASAN: null-ptr-deref in nfsd_fill_super+0xc6/0xe0 [nfsd] > > Write of size 8 at addr 000000000000005d by task a.out/852 > > > > CPU: 7 PID: 852 Comm: a.out Not tainted 5.18.0-rc7-dirty #66 > > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.14.0-1.fc33 04/01/2014 > > Call Trace: > > <TASK> > > dump_stack_lvl+0x34/0x44 > > kasan_report+0xab/0x120 > > ? nfsd_mkdir+0x71/0x1c0 [nfsd] > > ? nfsd_fill_super+0xc6/0xe0 [nfsd] > > nfsd_fill_super+0xc6/0xe0 [nfsd] > > ? nfsd_mkdir+0x1c0/0x1c0 [nfsd] > > get_tree_keyed+0x8e/0x100 > > vfs_get_tree+0x41/0xf0 > > __do_sys_fsconfig+0x590/0x670 > > ? fscontext_read+0x180/0x180 > > ? anon_inode_getfd+0x4f/0x70 > > do_syscall_64+0x35/0x80 > > entry_SYSCALL_64_after_hwframe+0x44/0xae > > > > This can be reproduce by concurrent operations: > > 1. fsopen(nfsd)/fsconfig > > 2. insmod/rmmod nfsd > > > > Since the nfsd file system is registered before than nfsd_net allocated, > > the caller may get the file_system_type and use the nfsd_net before it > > allocated, then null-ptr-deref occured. > > > > So should allocate the nfsd_net firstly, other than register file system. > > IIUC, I suggest: "So init_nfsd() should call register_filesystem() last." > > > > Fixes: bd5ae9288d64 ("nfsd: register pernet ops last, unregister first") > > Cc: stable@kernel.org > > Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com> > > I think this looks right. Bruce, as author of bd5ae9288d64, any > thoughts? I'm not seeing any problem with the patch. Reviewed-by: J. Bruce Fields <bfields@fieldses.org> --b. > > I need a v2 of this, though. The current version conflicts with the > courteous server patches already in my for-next branch. See: > > https://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git/log/?h=for-next > > > > --- > > fs/nfsd/nfsctl.c | 14 +++++++------- > > 1 file changed, 7 insertions(+), 7 deletions(-) > > > > diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c > > index 16920e4512bd..e17100e90e19 100644 > > --- a/fs/nfsd/nfsctl.c > > +++ b/fs/nfsd/nfsctl.c > > @@ -1535,20 +1535,20 @@ static int __init init_nfsd(void) > > retval = create_proc_exports_entry(); > > if (retval) > > goto out_free_lockd; > > - retval = register_filesystem(&nfsd_fs_type); > > - if (retval) > > - goto out_free_exports; > > retval = register_pernet_subsys(&nfsd_net_ops); > > if (retval < 0) > > - goto out_free_filesystem; > > + goto out_free_exports; > > retval = register_cld_notifier(); > > + if (retval) > > + goto out_free_subsys; > > + retval = register_filesystem(&nfsd_fs_type); > > if (retval) > > goto out_free_all; > > return 0; > > out_free_all: > > + unregister_cld_notifier(); > > +out_free_subsys: > > unregister_pernet_subsys(&nfsd_net_ops); > > -out_free_filesystem: > > - unregister_filesystem(&nfsd_fs_type); > > out_free_exports: > > remove_proc_entry("fs/nfs/exports", NULL); > > remove_proc_entry("fs/nfs", NULL); > > @@ -1566,6 +1566,7 @@ static int __init init_nfsd(void) > > > > static void __exit exit_nfsd(void) > > { > > + unregister_filesystem(&nfsd_fs_type); > > unregister_cld_notifier(); > > unregister_pernet_subsys(&nfsd_net_ops); > > nfsd_drc_slab_free(); > > @@ -1575,7 +1576,6 @@ static void __exit exit_nfsd(void) > > nfsd_lockd_shutdown(); > > nfsd4_free_slabs(); > > nfsd4_exit_pnfs(); > > - unregister_filesystem(&nfsd_fs_type); > > } > > > > MODULE_AUTHOR("Olaf Kirch <okir@monad.swb.de>"); > > -- > > 2.31.1 > > > > -- > Chuck Lever > >
Thanks for your review. I will send v2 patch to update the commit description and resolve the conflicts in your for-next branch 在 2022/5/21 0:05, Bruce Fields 写道: > On Fri, May 20, 2022 at 03:22:51PM +0000, Chuck Lever III wrote: >> [ Note well: Updated Bruce's email address. ] >> >> >>> On May 19, 2022, at 10:31 PM, Zhang Xiaoxu <zhangxiaoxu5@huawei.com> wrote: >>> >>> KASAN report null-ptr-deref as follows: >>> >>> BUG: KASAN: null-ptr-deref in nfsd_fill_super+0xc6/0xe0 [nfsd] >>> Write of size 8 at addr 000000000000005d by task a.out/852 >>> >>> CPU: 7 PID: 852 Comm: a.out Not tainted 5.18.0-rc7-dirty #66 >>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.14.0-1.fc33 04/01/2014 >>> Call Trace: >>> <TASK> >>> dump_stack_lvl+0x34/0x44 >>> kasan_report+0xab/0x120 >>> ? nfsd_mkdir+0x71/0x1c0 [nfsd] >>> ? nfsd_fill_super+0xc6/0xe0 [nfsd] >>> nfsd_fill_super+0xc6/0xe0 [nfsd] >>> ? nfsd_mkdir+0x1c0/0x1c0 [nfsd] >>> get_tree_keyed+0x8e/0x100 >>> vfs_get_tree+0x41/0xf0 >>> __do_sys_fsconfig+0x590/0x670 >>> ? fscontext_read+0x180/0x180 >>> ? anon_inode_getfd+0x4f/0x70 >>> do_syscall_64+0x35/0x80 >>> entry_SYSCALL_64_after_hwframe+0x44/0xae >>> >>> This can be reproduce by concurrent operations: >>> 1. fsopen(nfsd)/fsconfig >>> 2. insmod/rmmod nfsd >>> >>> Since the nfsd file system is registered before than nfsd_net allocated, >>> the caller may get the file_system_type and use the nfsd_net before it >>> allocated, then null-ptr-deref occured. >>> >>> So should allocate the nfsd_net firstly, other than register file system. >> >> IIUC, I suggest: "So init_nfsd() should call register_filesystem() last." >> >> >>> Fixes: bd5ae9288d64 ("nfsd: register pernet ops last, unregister first") >>> Cc: stable@kernel.org >>> Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com> >> >> I think this looks right. Bruce, as author of bd5ae9288d64, any >> thoughts? > > I'm not seeing any problem with the patch. > > Reviewed-by: J. Bruce Fields <bfields@fieldses.org> > > --b. > >> >> I need a v2 of this, though. The current version conflicts with the >> courteous server patches already in my for-next branch. See: >> >> https://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git/log/?h=for-next >> >> >>> --- >>> fs/nfsd/nfsctl.c | 14 +++++++------- >>> 1 file changed, 7 insertions(+), 7 deletions(-) >>> >>> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c >>> index 16920e4512bd..e17100e90e19 100644 >>> --- a/fs/nfsd/nfsctl.c >>> +++ b/fs/nfsd/nfsctl.c >>> @@ -1535,20 +1535,20 @@ static int __init init_nfsd(void) >>> retval = create_proc_exports_entry(); >>> if (retval) >>> goto out_free_lockd; >>> - retval = register_filesystem(&nfsd_fs_type); >>> - if (retval) >>> - goto out_free_exports; >>> retval = register_pernet_subsys(&nfsd_net_ops); >>> if (retval < 0) >>> - goto out_free_filesystem; >>> + goto out_free_exports; >>> retval = register_cld_notifier(); >>> + if (retval) >>> + goto out_free_subsys; >>> + retval = register_filesystem(&nfsd_fs_type); >>> if (retval) >>> goto out_free_all; >>> return 0; >>> out_free_all: >>> + unregister_cld_notifier(); >>> +out_free_subsys: >>> unregister_pernet_subsys(&nfsd_net_ops); >>> -out_free_filesystem: >>> - unregister_filesystem(&nfsd_fs_type); >>> out_free_exports: >>> remove_proc_entry("fs/nfs/exports", NULL); >>> remove_proc_entry("fs/nfs", NULL); >>> @@ -1566,6 +1566,7 @@ static int __init init_nfsd(void) >>> >>> static void __exit exit_nfsd(void) >>> { >>> + unregister_filesystem(&nfsd_fs_type); >>> unregister_cld_notifier(); >>> unregister_pernet_subsys(&nfsd_net_ops); >>> nfsd_drc_slab_free(); >>> @@ -1575,7 +1576,6 @@ static void __exit exit_nfsd(void) >>> nfsd_lockd_shutdown(); >>> nfsd4_free_slabs(); >>> nfsd4_exit_pnfs(); >>> - unregister_filesystem(&nfsd_fs_type); >>> } >>> >>> MODULE_AUTHOR("Olaf Kirch <okir@monad.swb.de>"); >>> -- >>> 2.31.1 >>> >> >> -- >> Chuck Lever >> >> > . >
diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c index 16920e4512bd..e17100e90e19 100644 --- a/fs/nfsd/nfsctl.c +++ b/fs/nfsd/nfsctl.c @@ -1535,20 +1535,20 @@ static int __init init_nfsd(void) retval = create_proc_exports_entry(); if (retval) goto out_free_lockd; - retval = register_filesystem(&nfsd_fs_type); - if (retval) - goto out_free_exports; retval = register_pernet_subsys(&nfsd_net_ops); if (retval < 0) - goto out_free_filesystem; + goto out_free_exports; retval = register_cld_notifier(); + if (retval) + goto out_free_subsys; + retval = register_filesystem(&nfsd_fs_type); if (retval) goto out_free_all; return 0; out_free_all: + unregister_cld_notifier(); +out_free_subsys: unregister_pernet_subsys(&nfsd_net_ops); -out_free_filesystem: - unregister_filesystem(&nfsd_fs_type); out_free_exports: remove_proc_entry("fs/nfs/exports", NULL); remove_proc_entry("fs/nfs", NULL); @@ -1566,6 +1566,7 @@ static int __init init_nfsd(void) static void __exit exit_nfsd(void) { + unregister_filesystem(&nfsd_fs_type); unregister_cld_notifier(); unregister_pernet_subsys(&nfsd_net_ops); nfsd_drc_slab_free(); @@ -1575,7 +1576,6 @@ static void __exit exit_nfsd(void) nfsd_lockd_shutdown(); nfsd4_free_slabs(); nfsd4_exit_pnfs(); - unregister_filesystem(&nfsd_fs_type); } MODULE_AUTHOR("Olaf Kirch <okir@monad.swb.de>");
KASAN report null-ptr-deref as follows: BUG: KASAN: null-ptr-deref in nfsd_fill_super+0xc6/0xe0 [nfsd] Write of size 8 at addr 000000000000005d by task a.out/852 CPU: 7 PID: 852 Comm: a.out Not tainted 5.18.0-rc7-dirty #66 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.14.0-1.fc33 04/01/2014 Call Trace: <TASK> dump_stack_lvl+0x34/0x44 kasan_report+0xab/0x120 ? nfsd_mkdir+0x71/0x1c0 [nfsd] ? nfsd_fill_super+0xc6/0xe0 [nfsd] nfsd_fill_super+0xc6/0xe0 [nfsd] ? nfsd_mkdir+0x1c0/0x1c0 [nfsd] get_tree_keyed+0x8e/0x100 vfs_get_tree+0x41/0xf0 __do_sys_fsconfig+0x590/0x670 ? fscontext_read+0x180/0x180 ? anon_inode_getfd+0x4f/0x70 do_syscall_64+0x35/0x80 entry_SYSCALL_64_after_hwframe+0x44/0xae This can be reproduce by concurrent operations: 1. fsopen(nfsd)/fsconfig 2. insmod/rmmod nfsd Since the nfsd file system is registered before than nfsd_net allocated, the caller may get the file_system_type and use the nfsd_net before it allocated, then null-ptr-deref occured. So should allocate the nfsd_net firstly, other than register file system. Fixes: bd5ae9288d64 ("nfsd: register pernet ops last, unregister first") Cc: stable@kernel.org Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com> --- fs/nfsd/nfsctl.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)