Re: [PATCH 2/3] ntfs: fix out-of-bounds read in ntfs_attr_find()
From: Dan Carpenter
Date: Wed Aug 31 2022 - 08:22:40 EST
On Wed, Aug 31, 2022 at 08:03:25PM +0800, Hawkins Jiawei wrote:
> On Wed, 31 Aug 2022 at 19:08, Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
> >
> > On Wed, Aug 31, 2022 at 10:43:36AM +0800, Hawkins Jiawei wrote:
> > > Kernel will iterates over ATTR_RECORDs in mft record in ntfs_attr_find().
> > > To ensure access on these ATTR_RECORDs are within bounds, kernel will
> > > do some checking during iteration.
> > >
> > > The problem is that during checking whether ATTR_RECORD's name is within
> > > bounds, kernel will dereferences the ATTR_RECORD name_offset field,
> > > before checking this ATTR_RECORD strcture is within bounds. This problem
> > > may result out-of-bounds read in ntfs_attr_find(), reported by
> > > Syzkaller:
> > >
> > > ==================================================================
> > > 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>
> > >
> > > The buggy address belongs to the physical page:
> > > page:ffffea0001f8d400 refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x7e350
> > > head:ffffea0001f8d400 order:3 compound_mapcount:0 compound_pincount:0
> > > flags: 0xfff00000010200(slab|head|node=0|zone=1|lastcpupid=0x7ff)
> > > raw: 00fff00000010200 0000000000000000 dead000000000122 ffff888011842140
> > > raw: 0000000000000000 0000000000040004 00000001ffffffff 0000000000000000
> > > page dumped because: kasan: bad access detected
> > > 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
> > > ==================================================================
> > >
> > > This patch solves it by moving the ATTR_RECORD strcture's bounds
> > > checking earlier, then checking whether ATTR_RECORD's name
> > > is within bounds. What's more, this patch also add some comments
> > > to improve its maintainability.
> > >
> > > Reported-and-tested-by: syzbot+5f8dcabe4a3b2c51c607@xxxxxxxxxxxxxxxxxxxxxxxxx
> > > Signed-off-by: chenxiaosong (A) <chenxiaosong2@xxxxxxxxxx>
> > > Link: https://lore.kernel.org/all/1636796c-c85e-7f47-e96f-e074fee3c7d3@xxxxxxxxxx/
> > > Signed-off-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> > > Link: https://groups.google.com/g/syzkaller-bugs/c/t_XdeKPGTR4/m/LECAuIGcBgAJ
> > > Signed-off-by: Hawkins Jiawei <yin31149@xxxxxxxxx>
> > > ---
> > > fs/ntfs/attrib.c | 20 ++++++++++++++++----
> > > 1 file changed, 16 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
> > > index 52615e6090e1..904734e34507 100644
> > > --- a/fs/ntfs/attrib.c
> > > +++ b/fs/ntfs/attrib.c
> > > @@ -594,11 +594,23 @@ 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);
> > > - 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)
> > > + u8 *name_end, *arec_head_end;
> > > +
> > > + /* check for wrap around */
> > > + if ((u8 *)a < (u8 *)ctx->mrec)
> > > + break;
> > > +
> > > + /* check whether Attribute Record Header is within bounds */
> > > + arec_head_end = (u8 *)a + sizeof(ATTR_RECORD);
> > > + if (arec_head_end < (u8 *)a || arec_head_end > mrec_end)
> >
> > This works but I feel like it would be more natural to just check if
> > a was valid and if a + sizeof(ATTR_RECORD) was also valid.
> >
> > if (a > mrec_end || (u8 *)a + sizeof(ATTR_RECORD) > mrec_end)
> Hi Dan,
> Thanks for your suggestion.
> This looks more natural than original patch, yet I wonder if there may
> be an overflow?
>
> To be more specific, if "a" and "mrec_end" is large enough, it seems that
> some fields of "a" may be out-of-bounds and also bypass this check because
> of the overflow.(Please correct me if I am wrong)
Are we talking buffer overflows or integer overflows? There is no
buffer overflow until we dereference "a". The checks are just pointer
math and not dereferences.
For integer overflows if "a" is valid then "a + sizeof(ATTR_RECORD)"
will not have an integer. I do not know exactly how memory is laid out
in the kernel and it also depends on the arch. But the last page is
always error pointer values so you can always add a page to any valid
pointer without an integer overflow.
regards,
dan carpenter