[f2fs-dev] [PATCH] fs: f2fs: fix UAF in f2fs_available_free_memory

[f2fs-dev] [PATCH] fs: f2fs: fix UAF in f2fs_available_free_memory

[Dongliang Mu]

f2fs_fill_super
-> f2fs_build_segment_manager
   -> create_discard_cmd_control
      -> f2fs_start_discard_thread

It invokes kthread_run to create a thread and run issue_discard_thread.

However, if f2fs_build_node_manager fails, the control flow goes to
free_nm and calls f2fs_destroy_node_manager. This function will free
sbi->nm_info. However, if issue_discard_thread accesses sbi->nm_info
after the deallocation, but before the f2fs_stop_discard_thread, it will
cause UAF(Use-after-free).

-> f2fs_destroy_segment_manager
   -> destroy_discard_cmd_control
      -> f2fs_stop_discard_thread

Fix this by switching the order of f2fs_build_segment_manager
and f2fs_build_node_manager.

Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
---
 fs/f2fs/super.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 78ebc306ee2b..1a23b64cfb74 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -4135,18 +4135,18 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 	}
 
 	/* setup f2fs internal modules */
-	err = f2fs_build_segment_manager(sbi);
-	if (err) {
-		f2fs_err(sbi, "Failed to initialize F2FS segment manager (%d)",
-			 err);
-		goto free_sm;
-	}
 	err = f2fs_build_node_manager(sbi);
 	if (err) {
 		f2fs_err(sbi, "Failed to initialize F2FS node manager (%d)",
 			 err);
 		goto free_nm;
 	}
+	err = f2fs_build_segment_manager(sbi);
+	if (err) {
+		f2fs_err(sbi, "Failed to initialize F2FS segment manager (%d)",
+			 err);
+		goto free_sm;
+	}
 
 	/* For write statistics */
 	sbi->sectors_written_start = f2fs_get_sectors_written(sbi);
@@ -4351,10 +4351,10 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 	sbi->node_inode = NULL;
 free_stats:
 	f2fs_destroy_stats(sbi);
-free_nm:
-	f2fs_destroy_node_manager(sbi);
 free_sm:
 	f2fs_destroy_segment_manager(sbi);
+free_nm:
+	f2fs_destroy_node_manager(sbi);
 	f2fs_destroy_post_read_wq(sbi);
 stop_ckpt_thread:
 	f2fs_stop_ckpt_thread(sbi);
-- 
2.25.1



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH] fs: f2fs: fix UAF in f2fs_available_free_memory

[Dongliang Mu]

On Wed, Nov 3, 2021 at 10:22 PM Dongliang Mu <mudongliangabcd@gmail.com> wrote:
>
> f2fs_fill_super
> -> f2fs_build_segment_manager
>    -> create_discard_cmd_control
>       -> f2fs_start_discard_thread
>
> It invokes kthread_run to create a thread and run issue_discard_thread.
>
> However, if f2fs_build_node_manager fails, the control flow goes to
> free_nm and calls f2fs_destroy_node_manager. This function will free
> sbi->nm_info. However, if issue_discard_thread accesses sbi->nm_info
> after the deallocation, but before the f2fs_stop_discard_thread, it will
> cause UAF(Use-after-free).

This UAF only can be triggered in a small time window. Even if
syzkaller generates a reproducer, it is hard to reproduce.

>
> -> f2fs_destroy_segment_manager
>    -> destroy_discard_cmd_control
>       -> f2fs_stop_discard_thread
>
> Fix this by switching the order of f2fs_build_segment_manager
> and f2fs_build_node_manager.
>
> Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
> ---
>  fs/f2fs/super.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index 78ebc306ee2b..1a23b64cfb74 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -4135,18 +4135,18 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
>         }
>
>         /* setup f2fs internal modules */
> -       err = f2fs_build_segment_manager(sbi);
> -       if (err) {
> -               f2fs_err(sbi, "Failed to initialize F2FS segment manager (%d)",
> -                        err);
> -               goto free_sm;
> -       }
>         err = f2fs_build_node_manager(sbi);
>         if (err) {
>                 f2fs_err(sbi, "Failed to initialize F2FS node manager (%d)",
>                          err);
>                 goto free_nm;
>         }
> +       err = f2fs_build_segment_manager(sbi);
> +       if (err) {
> +               f2fs_err(sbi, "Failed to initialize F2FS segment manager (%d)",
> +                        err);
> +               goto free_sm;
> +       }

I am not very familiar with this filesystem. If the building of node
manager is based on segment manager,
we can ignore this patch and try to think up other solutions to fix this bug.

>
>         /* For write statistics */
>         sbi->sectors_written_start = f2fs_get_sectors_written(sbi);
> @@ -4351,10 +4351,10 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
>         sbi->node_inode = NULL;
>  free_stats:
>         f2fs_destroy_stats(sbi);
> -free_nm:
> -       f2fs_destroy_node_manager(sbi);
>  free_sm:
>         f2fs_destroy_segment_manager(sbi);
> +free_nm:
> +       f2fs_destroy_node_manager(sbi);
>         f2fs_destroy_post_read_wq(sbi);
>  stop_ckpt_thread:
>         f2fs_stop_ckpt_thread(sbi);
> --
> 2.25.1
>


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH] fs: f2fs: fix UAF in f2fs_available_free_memory

[Chao Yu]

On 2021/11/3 22:22, Dongliang Mu wrote:
> f2fs_fill_super
> -> f2fs_build_segment_manager
>     -> create_discard_cmd_control
>        -> f2fs_start_discard_thread
> 
> It invokes kthread_run to create a thread and run issue_discard_thread.
> 
> However, if f2fs_build_node_manager fails, the control flow goes to
> free_nm and calls f2fs_destroy_node_manager. This function will free
> sbi->nm_info. However, if issue_discard_thread accesses sbi->nm_info
> after the deallocation, but before the f2fs_stop_discard_thread, it will
> cause UAF(Use-after-free).
> 
> -> f2fs_destroy_segment_manager
>     -> destroy_discard_cmd_control
>        -> f2fs_stop_discard_thread
> 
> Fix this by switching the order of f2fs_build_segment_manager
> and f2fs_build_node_manager.
> 
> Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
> ---
>   fs/f2fs/super.c | 16 ++++++++--------
>   1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index 78ebc306ee2b..1a23b64cfb74 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -4135,18 +4135,18 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
>   	}
>   
>   	/* setup f2fs internal modules */
> -	err = f2fs_build_segment_manager(sbi);
> -	if (err) {
> -		f2fs_err(sbi, "Failed to initialize F2FS segment manager (%d)",
> -			 err);
> -		goto free_sm;
> -	}
>   	err = f2fs_build_node_manager(sbi);
>   	if (err) {
>   		f2fs_err(sbi, "Failed to initialize F2FS node manager (%d)",
>   			 err);
>   		goto free_nm;
>   	}
> +	err = f2fs_build_segment_manager(sbi);
> +	if (err) {
> +		f2fs_err(sbi, "Failed to initialize F2FS segment manager (%d)",
> +			 err);
> +		goto free_sm;
> +	}
>   
>   	/* For write statistics */
>   	sbi->sectors_written_start = f2fs_get_sectors_written(sbi);
> @@ -4351,10 +4351,10 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
>   	sbi->node_inode = NULL;
>   free_stats:
>   	f2fs_destroy_stats(sbi);
> -free_nm:
> -	f2fs_destroy_node_manager(sbi);
>   free_sm:
>   	f2fs_destroy_segment_manager(sbi);
> +free_nm:
> +	f2fs_destroy_node_manager(sbi);

IIRC, above two functions shouldn't not be called reversely due to some
resource dependency, Jaegeuk, please help to confirm this.

So I suggest to call destroy_discard_cmd_control() before
f2fs_destroy_node_manager(), is it fine to you?

Thanks,

>   	f2fs_destroy_post_read_wq(sbi);
>   stop_ckpt_thread:
>   	f2fs_stop_ckpt_thread(sbi);
> 


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH] fs: f2fs: fix UAF in f2fs_available_free_memory

[Dongliang Mu]

On Thu, Nov 4, 2021 at 3:16 PM Chao Yu <chao@kernel.org> wrote:
>
> On 2021/11/3 22:22, Dongliang Mu wrote:
> > f2fs_fill_super
> > -> f2fs_build_segment_manager
> >     -> create_discard_cmd_control
> >        -> f2fs_start_discard_thread
> >
> > It invokes kthread_run to create a thread and run issue_discard_thread.
> >
> > However, if f2fs_build_node_manager fails, the control flow goes to
> > free_nm and calls f2fs_destroy_node_manager. This function will free
> > sbi->nm_info. However, if issue_discard_thread accesses sbi->nm_info
> > after the deallocation, but before the f2fs_stop_discard_thread, it will
> > cause UAF(Use-after-free).
> >
> > -> f2fs_destroy_segment_manager
> >     -> destroy_discard_cmd_control
> >        -> f2fs_stop_discard_thread
> >
> > Fix this by switching the order of f2fs_build_segment_manager
> > and f2fs_build_node_manager.
> >
> > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
> > ---
> >   fs/f2fs/super.c | 16 ++++++++--------
> >   1 file changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> > index 78ebc306ee2b..1a23b64cfb74 100644
> > --- a/fs/f2fs/super.c
> > +++ b/fs/f2fs/super.c
> > @@ -4135,18 +4135,18 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
> >       }
> >
> >       /* setup f2fs internal modules */
> > -     err = f2fs_build_segment_manager(sbi);
> > -     if (err) {
> > -             f2fs_err(sbi, "Failed to initialize F2FS segment manager (%d)",
> > -                      err);
> > -             goto free_sm;
> > -     }
> >       err = f2fs_build_node_manager(sbi);
> >       if (err) {
> >               f2fs_err(sbi, "Failed to initialize F2FS node manager (%d)",
> >                        err);
> >               goto free_nm;
> >       }
> > +     err = f2fs_build_segment_manager(sbi);
> > +     if (err) {
> > +             f2fs_err(sbi, "Failed to initialize F2FS segment manager (%d)",
> > +                      err);
> > +             goto free_sm;
> > +     }
> >
> >       /* For write statistics */
> >       sbi->sectors_written_start = f2fs_get_sectors_written(sbi);
> > @@ -4351,10 +4351,10 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
> >       sbi->node_inode = NULL;
> >   free_stats:
> >       f2fs_destroy_stats(sbi);
> > -free_nm:
> > -     f2fs_destroy_node_manager(sbi);
> >   free_sm:
> >       f2fs_destroy_segment_manager(sbi);
> > +free_nm:
> > +     f2fs_destroy_node_manager(sbi);
>
> IIRC, above two functions shouldn't not be called reversely due to some
> resource dependency, Jaegeuk, please help to confirm this.
>
> So I suggest to call destroy_discard_cmd_control() before
> f2fs_destroy_node_manager(), is it fine to you?

Maybe f2fs_stop_discard_thread is better than
destroy_discard_cmd_control. It only stops the kthread, leading to
fewer side effects.

How do you think?

>
> Thanks,
>
> >       f2fs_destroy_post_read_wq(sbi);
> >   stop_ckpt_thread:
> >       f2fs_stop_ckpt_thread(sbi);
> >


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH] fs: f2fs: fix UAF in f2fs_available_free_memory

[Chao Yu]

On 2021/11/4 15:35, Dongliang Mu wrote:
> On Thu, Nov 4, 2021 at 3:16 PM Chao Yu <chao@kernel.org> wrote:
>>
>> On 2021/11/3 22:22, Dongliang Mu wrote:
>>> f2fs_fill_super
>>> -> f2fs_build_segment_manager
>>>      -> create_discard_cmd_control
>>>         -> f2fs_start_discard_thread
>>>
>>> It invokes kthread_run to create a thread and run issue_discard_thread.
>>>
>>> However, if f2fs_build_node_manager fails, the control flow goes to
>>> free_nm and calls f2fs_destroy_node_manager. This function will free
>>> sbi->nm_info. However, if issue_discard_thread accesses sbi->nm_info
>>> after the deallocation, but before the f2fs_stop_discard_thread, it will
>>> cause UAF(Use-after-free).
>>>
>>> -> f2fs_destroy_segment_manager
>>>      -> destroy_discard_cmd_control
>>>         -> f2fs_stop_discard_thread
>>>
>>> Fix this by switching the order of f2fs_build_segment_manager
>>> and f2fs_build_node_manager.
>>>
>>> Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
>>> ---
>>>    fs/f2fs/super.c | 16 ++++++++--------
>>>    1 file changed, 8 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
>>> index 78ebc306ee2b..1a23b64cfb74 100644
>>> --- a/fs/f2fs/super.c
>>> +++ b/fs/f2fs/super.c
>>> @@ -4135,18 +4135,18 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
>>>        }
>>>
>>>        /* setup f2fs internal modules */
>>> -     err = f2fs_build_segment_manager(sbi);
>>> -     if (err) {
>>> -             f2fs_err(sbi, "Failed to initialize F2FS segment manager (%d)",
>>> -                      err);
>>> -             goto free_sm;
>>> -     }
>>>        err = f2fs_build_node_manager(sbi);
>>>        if (err) {
>>>                f2fs_err(sbi, "Failed to initialize F2FS node manager (%d)",
>>>                         err);
>>>                goto free_nm;
>>>        }
>>> +     err = f2fs_build_segment_manager(sbi);
>>> +     if (err) {
>>> +             f2fs_err(sbi, "Failed to initialize F2FS segment manager (%d)",
>>> +                      err);
>>> +             goto free_sm;
>>> +     }
>>>
>>>        /* For write statistics */
>>>        sbi->sectors_written_start = f2fs_get_sectors_written(sbi);
>>> @@ -4351,10 +4351,10 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
>>>        sbi->node_inode = NULL;
>>>    free_stats:
>>>        f2fs_destroy_stats(sbi);
>>> -free_nm:
>>> -     f2fs_destroy_node_manager(sbi);
>>>    free_sm:
>>>        f2fs_destroy_segment_manager(sbi);
>>> +free_nm:
>>> +     f2fs_destroy_node_manager(sbi);
>>
>> IIRC, above two functions shouldn't not be called reversely due to some
>> resource dependency, Jaegeuk, please help to confirm this.
>>
>> So I suggest to call destroy_discard_cmd_control() before
>> f2fs_destroy_node_manager(), is it fine to you?
> 
> Maybe f2fs_stop_discard_thread is better than
> destroy_discard_cmd_control. It only stops the kthread, leading to
> fewer side effects.

Fine to me. :)

Thanks,

> 
> How do you think?
> 
>>
>> Thanks,
>>
>>>        f2fs_destroy_post_read_wq(sbi);
>>>    stop_ckpt_thread:
>>>        f2fs_stop_ckpt_thread(sbi);
>>>


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel