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

[Dongliang Mu]

On Wed, Nov 3, 2021 at 10:22 PM Dongliang Mu <mudongliangabcd@xxxxxxxxx> 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@xxxxxxxxx>
> ---
>  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
>