Messages in this thread Patch in this message | | | From | Hawkins Jiawei <> | Subject | Re: [PATCH] ntfs: change check order in ntfs_attr_find | Date | Sat, 27 Aug 2022 15:51:44 +0800 |
| |
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.
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?
| |