[PATCH] nfsd: Fix null-ptr-deref in nfsd_fill_super()

[PATCH] nfsd: Fix null-ptr-deref in nfsd_fill_super()

[Zhang Xiaoxu]

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(-)

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

Re: [PATCH] nfsd: Fix null-ptr-deref in nfsd_fill_super()

[Chuck Lever III]

[ 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

Re: [PATCH] nfsd: Fix null-ptr-deref in nfsd_fill_super()

[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
> 
> 

Re: [PATCH] nfsd: Fix null-ptr-deref in nfsd_fill_super()

[zhangxiaoxu (A)]

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
>>
>>
> .
>