Received-SPF: pass (google.com: best guess record for domain of linux-ext4-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <1574759039-7429-1-git-send-email-wangshilong1991@gmail.com> <1574759039-7429-2-git-send-email-wangshilong1991@gmail.com>
In-Reply-To: <1574759039-7429-2-git-send-email-wangshilong1991@gmail.com>
From:   Wang Shilong <wangshilong1991@gmail.com>
Date:   Mon, 30 Dec 2019 19:38:33 +0800
Message-ID: <CAP9B-QksOYmoSX=ExH6HO9iuuj64SLp5Av_tKnXjQ8Kije4ZSg@mail.gmail.com>
Subject: Re: [PATCH 2/2] e2fsck: fix use after free in calculate_tree()
To:     Ext4 Developers List <linux-ext4@vger.kernel.org>
Cc:     Andreas Dilger <adilger@dilger.ca>, Li Xi <lixi@ddn.com>,
        Wang Shilong <wshilong@ddn.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-ext4-owner@vger.kernel.org
Precedence: bulk

Ping...

On Tue, Nov 26, 2019 at 5:04 PM Wang Shilong <wangshilong1991@gmail.com> wrote:
>
> From: Wang Shilong <wshilong@ddn.com>
>
> Hit following Seg errors randomly when running f_large_dir test:
>
> +Signal (11) SIGSEGV si_code=SEGV_MAPERR fault addr=0x7f02cfffbc1a
> +../e2fsck/e2fsck[0x43766e]
> +/lib64/libpthread.so.0(+0xf7e0)[0x7f02d8c9a7e0]
> +../e2fsck/e2fsck(e2fsck_rehash_dir+0x10f3)[0x436173]
> +../e2fsck/e2fsck(e2fsck_rehash_directories+0xf4)[0x4362d4]
> +../e2fsck/e2fsck(e2fsck_pass3+0x722)[0x4292c2]
> +../e2fsck/e2fsck(e2fsck_run+0x47)[0x414ef7]
> +../e2fsck/e2fsck(main+0x1c1d)[0x41319d]
> +/lib64/libc.so.6(__libc_start_main+0x100)[0x7f02d8915d20]
> +../e2fsck/e2fsck[0x40fc59]
> +Exit status is 8
>
> gdb output is:
> 0x436173 is in e2fsck_rehash_dir (rehash.c:752).
> warning: Source file is more recent than executable.
> 747                                     dx_ent->hash =
> 748                                             ext2fs_cpu_to_le32(outdir->hashes[i]);
> 749                             dx_ent++;
> 750                             c3--;
> 751                     }
> 752                     int_limit->count = ext2fs_cpu_to_le16(limit->limit - c2);
> 753                     int_limit->limit = ext2fs_cpu_to_le16(limit->limit);
> 754
> 755                     limit->count = ext2fs_cpu_to_le16(limit->limit - c3);
> 756                     limit->limit = ext2fs_cpu_to_le16(limit->limit);
>
> The problem is alloc_blocks() will call get_next_block()
> which might reallocate @outdir->buf, and memory address
> could be changed after this. @int_limit and @root should
> be recalculated based on new start address. Otherwise,
> it will try to access freed memory and cause SEGV_MAPERR
> errors.
>
> Signed-off-by: Wang Shilong <wshilong@ddn.com>
> ---
>  e2fsck/rehash.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/e2fsck/rehash.c b/e2fsck/rehash.c
> index 5250652e..0eb99328 100644
> --- a/e2fsck/rehash.c
> +++ b/e2fsck/rehash.c
> @@ -636,6 +636,9 @@ static int alloc_blocks(ext2_filsys fs,
>         if (retval)
>                 return retval;
>
> +       /* outdir->buf might be reallocated */
> +       *prev_ent = (struct ext2_dx_entry *) (outdir->buf + *prev_offset);
> +
>         *next_ent = set_int_node(fs, block_start);
>         *limit = (struct ext2_dx_countlimit *)(*next_ent);
>         if (next_offset)
> @@ -725,12 +728,18 @@ static errcode_t calculate_tree(ext2_filsys fs,
>                                         return retval;
>                         }
>                         if (c3 == 0) {
> +                               int delta1 = int_offset;;
> +                               int delta2 = (char *)root - outdir->buf;
> +
>                                 retval = alloc_blocks(fs, &limit, &int_ent,
>                                                       &dx_ent, &int_offset,
>                                                       NULL, outdir, i, &c2,
>                                                       &c3);
>                                 if (retval)
>                                         return retval;
> +                               /* outdir->buf might be reallocated */
> +                               int_limit = (struct ext2_dx_countlimit *)(outdir->buf + delta1);
> +                               root = (struct ext2_dx_entry *)(outdir->buf + delta2);
>
>                         }
>                         dx_ent->block = ext2fs_cpu_to_le32(i);
> --
> 2.21.0
>