Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Hawkins Jiawei <yin31149@gmail.com>
To:     yin31149@gmail.com
Cc:     akpm@linux-foundation.org, anton@tuxera.com,
        chenxiaosong2@huawei.com, linux-kernel@vger.kernel.org,
        linux-ntfs-dev@lists.sourceforge.net,
        syzbot+5f8dcabe4a3b2c51c607@syzkaller.appspotmail.com,
        syzkaller-bugs@googlegroups.com
Subject: Re: [PATCH] ntfs: change check order in ntfs_attr_find
Date:   Sat, 27 Aug 2022 22:49:44 +0800
Message-Id: <20220827144943.5290-1-yin31149@gmail.com>
In-Reply-To: <20220827075143.61311-1-yin31149@gmail.com>
References: <20220827075143.61311-1-yin31149@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

On Sat, 27 Aug 2022 at 15:59, Hawkins Jiawei <yin31149@gmail.com> wrote:
>
> On Sat, 27 Aug 2022 at 09:29, chenxiaosong (A) <chenxiaosong2@huawei.com> wrote:
> >
> > 在 2022/8/26 20:27, Hawkins Jiawei 写道:
> > > syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> > >
> > > Looks like it is improper check order that causes this bug.
> > >
> > > Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>
> > > ---
> > >   fs/ntfs/attrib.c | 5 +++--
> > >   1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
> > > index 52615e6090e1..6480cd2d371d 100644
> > > --- a/fs/ntfs/attrib.c
> > > +++ b/fs/ntfs/attrib.c
> > > @@ -594,10 +594,11 @@ static int ntfs_attr_find(const ATTR_TYPE type, const ntfschar *name,
> > >       for (;; a = (ATTR_RECORD*)((u8*)a + le32_to_cpu(a->length))) {
> > >               u8 *mrec_end = (u8 *)ctx->mrec +
> > >                              le32_to_cpu(ctx->mrec->bytes_allocated);
> > > +             if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end)
> > > +                     break;
> > >               u8 *name_end = (u8 *)a + le16_to_cpu(a->name_offset) +
> > >                              a->name_length * sizeof(ntfschar);
> > > -             if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end ||
> > > -                 name_end > mrec_end)
> > > +             if (name_end > mrec_end)
> > >                       break;
> > >               ctx->attr = a;
> > >               if (unlikely(le32_to_cpu(a->type) > le32_to_cpu(type) ||
> > >
> >
> > The reason is that a->length is 0, it will occur uaf when deref any
> > field of ATTR_RECORD.
> >
> > It seems that changing check order will not fix root cause, if the
> > condition "if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end)" is false,
> > uaf will still occur.
> >
> > Do you have any thoughts on this ?
> Hi, chenxiaosong
>
> I think changing the check order is able to fix this bug. But we may need
> to check whether mft record header is out of bounds, maybe
> "if ((u8*)a < (u8*)ctx->mrec || (u8*)a + sizeof(MFT_RECORD) > mrec_end)"
>
> Because we just need to check if this ATTR_RECORD is in valid addresss. As for
> situation that a->length is 0, there seems already a check in the loop
> (Correct me if I am wrong):
> > static int ntfs_attr_find(const ATTR_TYPE type, const ntfschar *name,
> >               const u32 name_len, const IGNORE_CASE_BOOL ic,
> >               const u8 *val, const u32 val_len, ntfs_attr_search_ctx *ctx)
> > {
> >       ...
> >
> >       for (;; a = (ATTR_RECORD*)((u8*)a + le32_to_cpu(a->length))) {
> >               u8 *mrec_end = (u8 *)ctx->mrec +
> >                              le32_to_cpu(ctx->mrec->bytes_allocated);
> >               u8 *name_end = (u8 *)a + le16_to_cpu(a->name_offset) +
> >                              a->name_length * sizeof(ntfschar);
> >               if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end ||
> >                   name_end > mrec_end)
> >                       break;
> >               ctx->attr = a;
> >               if (unlikely(le32_to_cpu(a->type) > le32_to_cpu(type) ||
> >                               a->type == AT_END))
> >                       return -ENOENT;
> >               if (unlikely(!a->length))
> >                       break;
> >       ...
> > }
>
> And as for the root cause for use-after-free read, I think it is the
> ctx->attr->length to be blamed.
Apologize for my typing error, it is the ctx->attr should be blamed:

static int ntfs_attr_find(const ATTR_TYPE type, const ntfschar *name,
		const u32 name_len, const IGNORE_CASE_BOOL ic,
		const u8 *val, const u32 val_len, ntfs_attr_search_ctx *ctx)
{
	ATTR_RECORD *a;
	ntfs_volume *vol = ctx->ntfs_ino->vol;
	ntfschar *upcase = vol->upcase;
	u32 upcase_len = vol->upcase_len;

	/*
	 * Iterate over attributes in mft record starting at @ctx->attr, or the
	 * attribute following that, if @ctx->is_first is 'true'.
	 */
	if (ctx->is_first) {
		a = ctx->attr;
		ctx->is_first = false;
	} else
		a = (ATTR_RECORD*)((u8*)ctx->attr +
				le32_to_cpu(ctx->attr->length));
	...
}

The "ctx->attr" is not invalid, then the pointer "a" will probably also
points to an invalid memory, which triggers a use-after-free read.
This also probably explains why the memory state around the buggy address are
all invalid in KASAN, instead of at the bounder between valid memory
and invalid memory

Yet the analysis below is still correct. ctx->attr is assigned with
"((u8*)mrec + le16_to_cpu(mrec->attrs_offset))" in
ntfs_attr_init_search_ctx()
>
> To be more specific, when kernel loads the struct MFT_RECORD from disk
> in ntfs_read_inode_mount(),  m's attrs_offset field should less than
> m's bytes_allocated field, or it may out of the bounds:
>
> int ntfs_read_inode_mount(struct inode *vi)
> {
>         ...
>         MFT_RECORD *m = NULL;
>         i = vol->mft_record_size;
>
>         ...
>         m = (MFT_RECORD*)ntfs_malloc_nofs(i);
>
>         /* Determine the first block of the $MFT/$DATA attribute. */
>         block = vol->mft_lcn << vol->cluster_size_bits >>
>                         sb->s_blocksize_bits;
>         nr_blocks = vol->mft_record_size >> sb->s_blocksize_bits;
>
>         ...
>
>         /* Load $MFT/$DATA's first mft record. */
>         for (i = 0; i < nr_blocks; i++) {
>                 bh = sb_bread(sb, block++);
>                 if (!bh) {
>                         ntfs_error(sb, "Device read failed.");
>                         goto err_out;
>                 }
>                 memcpy((char*)m + (i << sb->s_blocksize_bits), bh->b_data,
>                                 sb->s_blocksize);
>                 brelse(bh);
>         }
>
>         if (le32_to_cpu(m->bytes_allocated) != vol->mft_record_size) {
>                 ntfs_error(sb, "Incorrect mft record size %u in superblock, should be %u.",
>                                 le32_to_cpu(m->bytes_allocated), vol->mft_record_size);
>                 goto err_out;
>         }
>
>         ...
>
>         ctx = ntfs_attr_get_search_ctx(ni, m);
>         if (!ctx) {
>                 err = -ENOMEM;
>                 goto err_out;
>         }
>
>         /* Find the attribute list attribute if present. */
>         err = ntfs_attr_lookup(AT_ATTRIBUTE_LIST, NULL, 0, 0, 0, NULL, 0, ctx);
>
>         ...
> }
>
> > ==================================================================
> > BUG: KASAN: use-after-free in ntfs_attr_find+0xc02/0xce0 fs/ntfs/attrib.c:597
> > Read of size 2 at addr ffff88807e352009 by task syz-executor153/3607
> > Call Trace:
> >  <TASK>
> >  __dump_stack lib/dump_stack.c:88 [inline]
> >  dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106
> >  print_address_description mm/kasan/report.c:317 [inline]
> >  print_report.cold+0x2ba/0x719 mm/kasan/report.c:433
> >  kasan_report+0xb1/0x1e0 mm/kasan/report.c:495
> >  ntfs_attr_find+0xc02/0xce0 fs/ntfs/attrib.c:597
> >  ntfs_attr_lookup+0x1056/0x2070 fs/ntfs/attrib.c:1193
> >  ntfs_read_inode_mount+0x89a/0x2580 fs/ntfs/inode.c:1845
> >  ntfs_fill_super+0x1799/0x9320 fs/ntfs/super.c:2854
> >  mount_bdev+0x34d/0x410 fs/super.c:1400
> >  legacy_get_tree+0x105/0x220 fs/fs_context.c:610
> >  vfs_get_tree+0x89/0x2f0 fs/super.c:1530
> >  do_new_mount fs/namespace.c:3040 [inline]
> >  path_mount+0x1326/0x1e20 fs/namespace.c:3370
> >  do_mount fs/namespace.c:3383 [inline]
> >  __do_sys_mount fs/namespace.c:3591 [inline]
> >  __se_sys_mount fs/namespace.c:3568 [inline]
> >  __x64_sys_mount+0x27f/0x300 fs/namespace.c:3568
> >  do_syscall_x64 arch/x86/entry/common.c:50 [inline]
> >  do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
> >  entry_SYSCALL_64_after_hwframe+0x63/0xcd
> >  [...]
> >  </TASK>
> > Memory state around the buggy address:
> >  ffff88807e351f00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> >  ffff88807e351f80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> > >ffff88807e352000: fa fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> >                       ^
> >  ffff88807e352080: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> >  ffff88807e352100: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> > ==================================================================
> So check these fields as follow should fix the root cause for this
> use-after-free bug(Correct me if I am wrong). I test this patch locally,
> it doesn't trigger any issue.
>
> diff --git a/fs/ntfs/inode.c b/fs/ntfs/inode.c
> index db0f1995aedd..6ba99e109ca9 100644
> --- a/fs/ntfs/inode.c
> +++ b/fs/ntfs/inode.c
> @@ -1822,6 +1822,11 @@ int ntfs_read_inode_mount(struct inode *vi)
>                 goto err_out;
>         }
>
> +       if (m->attrs_offset >= le32_to_cpu(m->bytes_allocated)) {
> +               ntfs_error(sb, "Incorrect mft record attrs_offset %u", m->attrs_offset);
> +               goto err_out;
> +       }
> +
>         /* Apply the mst fixups. */
>         if (post_read_mst_fixup((NTFS_RECORD*)m, vol->mft_record_size)) {
>                 /* FIXME: Try to use the $MFTMirr now. */
>
>
> What's your opinion?