* [PATCH 0/3] ntfs: fix bugs about Attribute
@ 2022-08-31 2:43 Hawkins Jiawei
2022-08-31 2:43 ` [PATCH 1/3] ntfs: fix use-after-free in ntfs_attr_find() Hawkins Jiawei
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Hawkins Jiawei @ 2022-08-31 2:43 UTC (permalink / raw)
To: syzbot+5f8dcabe4a3b2c51c607
Cc: akpm, anton, chenxiaosong2, linux-kernel, linux-ntfs-dev,
syzkaller-bugs, yin31149, 18801353760
This patchset fixes three bugs relative to Attribute in record:
Patch 1 adds a sanity check to ensure that, attrs_offset field in
first mft record loading from disk is within bounds.
Patch 2 moves the ATTR_RECORD's bounds checking earlier, to avoid
dereferencing ATTR_RECORD before checking this ATTR_RECORD is within
bounds.
Patch 3 adds an overflow checking to avoid possible forever loop in
ntfs_attr_find().
Without patch 1 and patch 2, kernel may trigger following problem
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
==================================================================
Although one of patch 1 or patch 2 can fix above bug, we still
need both of them. Because patch 1 fixes the root cause, and
patch 2 not only fixes the direct cause, but also fixes the
potential out-of-bounds bug.
Hawkins Jiawei (3):
ntfs: fix use-after-free in ntfs_attr_find()
ntfs: fix out-of-bounds read in ntfs_attr_find()
ntfs: check overflow when iterates ATTR_RECORDs
fs/ntfs/attrib.c | 23 +++++++++++++++++++----
fs/ntfs/inode.c | 7 +++++++
2 files changed, 26 insertions(+), 4 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH 1/3] ntfs: fix use-after-free in ntfs_attr_find() 2022-08-31 2:43 [PATCH 0/3] ntfs: fix bugs about Attribute Hawkins Jiawei @ 2022-08-31 2:43 ` Hawkins Jiawei 2022-08-31 2:43 ` [PATCH 2/3] ntfs: fix out-of-bounds read " Hawkins Jiawei 2022-08-31 2:48 ` [PATCH 3/3] ntfs: check overflow when iterates ATTR_RECORDs Hawkins Jiawei 2 siblings, 0 replies; 10+ messages in thread From: Hawkins Jiawei @ 2022-08-31 2:43 UTC (permalink / raw) To: syzbot+5f8dcabe4a3b2c51c607, Anton Altaparmakov Cc: akpm, chenxiaosong2, linux-kernel, linux-ntfs-dev, syzkaller-bugs, yin31149, 18801353760 Syzkaller reported use-after-free read as follows: ================================================================== 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 ================================================================== Kernel will loads $MFT/$DATA's first mft record in ntfs_read_inode_mount(). Yet the problem is that after loading, kernel doesn't check whether attrs_offset field is a valid value. To be more specific, if attrs_offset field is larger than bytes_allocated field, then it may trigger the out-of-bounds read bug(reported as use-after-free bug) in ntfs_attr_find(), when kernel tries to access the corresponding mft record's attribute. This patch solves it by adding the sanity check between attrs_offset field and bytes_allocated field, after loading the first mft record. Reported-and-tested-by: syzbot+5f8dcabe4a3b2c51c607@syzkaller.appspotmail.com Signed-off-by: Hawkins Jiawei <yin31149@gmail.com> --- fs/ntfs/inode.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/fs/ntfs/inode.c b/fs/ntfs/inode.c index db0f1995aedd..08c659332e26 100644 --- a/fs/ntfs/inode.c +++ b/fs/ntfs/inode.c @@ -1829,6 +1829,13 @@ int ntfs_read_inode_mount(struct inode *vi) goto err_out; } + /* Sanity check offset to the first attribute */ + if (le16_to_cpu(m->attrs_offset) >= le32_to_cpu(m->bytes_allocated)) { + ntfs_error(sb, "Incorrect mft offset to the first attribute %u in superblock.", + le16_to_cpu(m->attrs_offset)); + goto err_out; + } + /* Need this to sanity check attribute list references to $MFT. */ vi->i_generation = ni->seq_no = le16_to_cpu(m->sequence_number); -- 2.25.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/3] ntfs: fix out-of-bounds read in ntfs_attr_find() 2022-08-31 2:43 [PATCH 0/3] ntfs: fix bugs about Attribute Hawkins Jiawei 2022-08-31 2:43 ` [PATCH 1/3] ntfs: fix use-after-free in ntfs_attr_find() Hawkins Jiawei @ 2022-08-31 2:43 ` Hawkins Jiawei 2022-08-31 11:07 ` Dan Carpenter 2022-08-31 2:48 ` [PATCH 3/3] ntfs: check overflow when iterates ATTR_RECORDs Hawkins Jiawei 2 siblings, 1 reply; 10+ messages in thread From: Hawkins Jiawei @ 2022-08-31 2:43 UTC (permalink / raw) To: syzbot+5f8dcabe4a3b2c51c607, Anton Altaparmakov Cc: akpm, chenxiaosong2, linux-kernel, linux-ntfs-dev, syzkaller-bugs, yin31149, 18801353760, Dan Carpenter 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@syzkaller.appspotmail.com Signed-off-by: chenxiaosong (A) <chenxiaosong2@huawei.com> Link: https://lore.kernel.org/all/1636796c-c85e-7f47-e96f-e074fee3c7d3@huawei.com/ Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> Link: https://groups.google.com/g/syzkaller-bugs/c/t_XdeKPGTR4/m/LECAuIGcBgAJ Signed-off-by: Hawkins Jiawei <yin31149@gmail.com> --- 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) break; + + /* check whether ATTR_RECORD's name is within bounds */ + name_end = (u8 *)a + le16_to_cpu(a->name_offset) + + a->name_length * sizeof(ntfschar); + if (name_end > mrec_end) + break; + ctx->attr = a; if (unlikely(le32_to_cpu(a->type) > le32_to_cpu(type) || a->type == AT_END)) -- 2.25.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] ntfs: fix out-of-bounds read in ntfs_attr_find() 2022-08-31 2:43 ` [PATCH 2/3] ntfs: fix out-of-bounds read " Hawkins Jiawei @ 2022-08-31 11:07 ` Dan Carpenter 2022-08-31 12:03 ` Hawkins Jiawei 0 siblings, 1 reply; 10+ messages in thread From: Dan Carpenter @ 2022-08-31 11:07 UTC (permalink / raw) To: Hawkins Jiawei Cc: syzbot+5f8dcabe4a3b2c51c607, Anton Altaparmakov, akpm, chenxiaosong2, linux-kernel, linux-ntfs-dev, syzkaller-bugs, 18801353760 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@syzkaller.appspotmail.com > Signed-off-by: chenxiaosong (A) <chenxiaosong2@huawei.com> > Link: https://lore.kernel.org/all/1636796c-c85e-7f47-e96f-e074fee3c7d3@huawei.com/ > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > Link: https://groups.google.com/g/syzkaller-bugs/c/t_XdeKPGTR4/m/LECAuIGcBgAJ > Signed-off-by: Hawkins Jiawei <yin31149@gmail.com> > --- > 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) But what you have also works so if you want to go with that then that's fine. regards, dan carpenter > break; > + > + /* check whether ATTR_RECORD's name is within bounds */ > + name_end = (u8 *)a + le16_to_cpu(a->name_offset) + > + a->name_length * sizeof(ntfschar); > + if (name_end > mrec_end) > + break; > + > ctx->attr = a; > if (unlikely(le32_to_cpu(a->type) > le32_to_cpu(type) || > a->type == AT_END)) ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] ntfs: fix out-of-bounds read in ntfs_attr_find() 2022-08-31 11:07 ` Dan Carpenter @ 2022-08-31 12:03 ` Hawkins Jiawei 2022-08-31 12:20 ` Dan Carpenter 0 siblings, 1 reply; 10+ messages in thread From: Hawkins Jiawei @ 2022-08-31 12:03 UTC (permalink / raw) To: dan.carpenter Cc: 18801353760, akpm, anton, chenxiaosong2, linux-kernel, linux-ntfs-dev, syzbot+5f8dcabe4a3b2c51c607, syzkaller-bugs, yin31149 On Wed, 31 Aug 2022 at 19:08, Dan Carpenter <dan.carpenter@oracle.com> 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@syzkaller.appspotmail.com > > Signed-off-by: chenxiaosong (A) <chenxiaosong2@huawei.com> > > Link: https://lore.kernel.org/all/1636796c-c85e-7f47-e96f-e074fee3c7d3@huawei.com/ > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > Link: https://groups.google.com/g/syzkaller-bugs/c/t_XdeKPGTR4/m/LECAuIGcBgAJ > > Signed-off-by: Hawkins Jiawei <yin31149@gmail.com> > > --- > > 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) > But what you have also works so if you want to go with that then that's > fine. > > regards, > dan carpenter > > > break; > > + > > + /* check whether ATTR_RECORD's name is within bounds */ > > + name_end = (u8 *)a + le16_to_cpu(a->name_offset) + > > + a->name_length * sizeof(ntfschar); > > + if (name_end > mrec_end) > > + break; > > + > > ctx->attr = a; > > if (unlikely(le32_to_cpu(a->type) > le32_to_cpu(type) || > > a->type == AT_END)) > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] ntfs: fix out-of-bounds read in ntfs_attr_find() 2022-08-31 12:03 ` Hawkins Jiawei @ 2022-08-31 12:20 ` Dan Carpenter 2022-08-31 12:47 ` Hawkins Jiawei 0 siblings, 1 reply; 10+ messages in thread From: Dan Carpenter @ 2022-08-31 12:20 UTC (permalink / raw) To: Hawkins Jiawei Cc: 18801353760, akpm, anton, chenxiaosong2, linux-kernel, linux-ntfs-dev, syzbot+5f8dcabe4a3b2c51c607, syzkaller-bugs 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@oracle.com> 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@syzkaller.appspotmail.com > > > Signed-off-by: chenxiaosong (A) <chenxiaosong2@huawei.com> > > > Link: https://lore.kernel.org/all/1636796c-c85e-7f47-e96f-e074fee3c7d3@huawei.com/ > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > > Link: https://groups.google.com/g/syzkaller-bugs/c/t_XdeKPGTR4/m/LECAuIGcBgAJ > > > Signed-off-by: Hawkins Jiawei <yin31149@gmail.com> > > > --- > > > 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] ntfs: fix out-of-bounds read in ntfs_attr_find() 2022-08-31 12:20 ` Dan Carpenter @ 2022-08-31 12:47 ` Hawkins Jiawei 0 siblings, 0 replies; 10+ messages in thread From: Hawkins Jiawei @ 2022-08-31 12:47 UTC (permalink / raw) To: dan.carpenter Cc: 18801353760, akpm, anton, chenxiaosong2, linux-kernel, linux-ntfs-dev, syzbot+5f8dcabe4a3b2c51c607, syzkaller-bugs, yin31149 On Wed, 31 Aug 2022 at 20:22, Dan Carpenter <dan.carpenter@oracle.com> wrote: > > 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@oracle.com> 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@syzkaller.appspotmail.com > > > > Signed-off-by: chenxiaosong (A) <chenxiaosong2@huawei.com> > > > > Link: https://lore.kernel.org/all/1636796c-c85e-7f47-e96f-e074fee3c7d3@huawei.com/ > > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > > > Link: https://groups.google.com/g/syzkaller-bugs/c/t_XdeKPGTR4/m/LECAuIGcBgAJ > > > > Signed-off-by: Hawkins Jiawei <yin31149@gmail.com> > > > > --- > > > > 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 Sorry for the lack of clarity. What you analyse is what I want to ask. For there are code below this check, dereferencing "a"(such as a->name_offset). So if there is an integer overflows in this check, then it may leads to the buffer overflows. > 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. OK, it makes sense now. I will refactor this patch as you suggested before. Thanks for your explaination. ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 3/3] ntfs: check overflow when iterates ATTR_RECORDs 2022-08-31 2:43 [PATCH 0/3] ntfs: fix bugs about Attribute Hawkins Jiawei 2022-08-31 2:43 ` [PATCH 1/3] ntfs: fix use-after-free in ntfs_attr_find() Hawkins Jiawei 2022-08-31 2:43 ` [PATCH 2/3] ntfs: fix out-of-bounds read " Hawkins Jiawei @ 2022-08-31 2:48 ` Hawkins Jiawei 2022-08-31 10:12 ` Dan Carpenter 2 siblings, 1 reply; 10+ messages in thread From: Hawkins Jiawei @ 2022-08-31 2:48 UTC (permalink / raw) To: syzbot+5f8dcabe4a3b2c51c607, Anton Altaparmakov Cc: akpm, chenxiaosong2, linux-kernel, linux-ntfs-dev, syzkaller-bugs, yin31149, 18801353760, Dan Carpenter Kernel will iterates over ATTR_RECORDs in mft record in ntfs_attr_find(). Because the ATTR_RECORDs are next to each other, kernel can get the next ATTR_RECORD from end address of current ATTR_RECORD, through current ATTR_RECORD length field. The problem is that during iteration, when kernel calculates the end address of current ATTR_RECORD, kernel may trigger an overflow bug in executing `a = (ATTR_RECORD*)((u8*)a + le32_to_cpu(a->length))`. This may wrap, leading to a forever iteration on 32bit systems. This patch solves it by adding an overflow checking on calculating end address of current ATTR_RECORD during iteration. Suggested-by: Dan Carpenter <dan.carpenter@oracle.com> Link: https://lore.kernel.org/all/20220827105842.GM2030@kadam/ Signed-off-by: Hawkins Jiawei <yin31149@gmail.com> --- fs/ntfs/attrib.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c index 904734e34507..55e618c9e63e 100644 --- a/fs/ntfs/attrib.c +++ b/fs/ntfs/attrib.c @@ -617,6 +617,9 @@ static int ntfs_attr_find(const ATTR_TYPE type, const ntfschar *name, return -ENOENT; if (unlikely(!a->length)) break; + /* check for wrap around */ + if ((u8 *)a + le32_to_cpu(a->length) < (u8 *)a) + break; if (a->type != type) continue; /* -- 2.25.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] ntfs: check overflow when iterates ATTR_RECORDs 2022-08-31 2:48 ` [PATCH 3/3] ntfs: check overflow when iterates ATTR_RECORDs Hawkins Jiawei @ 2022-08-31 10:12 ` Dan Carpenter 2022-08-31 11:47 ` Hawkins Jiawei 0 siblings, 1 reply; 10+ messages in thread From: Dan Carpenter @ 2022-08-31 10:12 UTC (permalink / raw) To: Hawkins Jiawei Cc: syzbot+5f8dcabe4a3b2c51c607, Anton Altaparmakov, akpm, chenxiaosong2, linux-kernel, linux-ntfs-dev, syzkaller-bugs, 18801353760 On Wed, Aug 31, 2022 at 10:48:54AM +0800, Hawkins Jiawei wrote: > Kernel will iterates over ATTR_RECORDs in mft record in ntfs_attr_find(). > Because the ATTR_RECORDs are next to each other, kernel can get the next > ATTR_RECORD from end address of current ATTR_RECORD, through current > ATTR_RECORD length field. > > The problem is that during iteration, when kernel calculates the end address > of current ATTR_RECORD, kernel may trigger an overflow bug in > executing `a = (ATTR_RECORD*)((u8*)a + le32_to_cpu(a->length))`. This > may wrap, leading to a forever iteration on 32bit systems. > > This patch solves it by adding an overflow checking on calculating end address > of current ATTR_RECORD during iteration. > > Suggested-by: Dan Carpenter <dan.carpenter@oracle.com> > Link: https://lore.kernel.org/all/20220827105842.GM2030@kadam/ > Signed-off-by: Hawkins Jiawei <yin31149@gmail.com> > --- > fs/ntfs/attrib.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c > index 904734e34507..55e618c9e63e 100644 > --- a/fs/ntfs/attrib.c > +++ b/fs/ntfs/attrib.c > @@ -617,6 +617,9 @@ static int ntfs_attr_find(const ATTR_TYPE type, const ntfschar *name, > return -ENOENT; > if (unlikely(!a->length)) > break; > + /* check for wrap around */ > + if ((u8 *)a + le32_to_cpu(a->length) < (u8 *)a) > + break; Wouldn't it also be good to check that a + a->length <= mrec_end? It gets checked on the next iteration sure, but it just seems like a reasonable thing to check here. regards, dan carpenter > if (a->type != type) > continue; > /* > -- > 2.25.1 > > -- > You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group. > To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller-bugs+unsubscribe@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/7b8b8633d921665a717734d011a92f713944d0fb.1661875711.git.yin31149%40gmail.com. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] ntfs: check overflow when iterates ATTR_RECORDs 2022-08-31 10:12 ` Dan Carpenter @ 2022-08-31 11:47 ` Hawkins Jiawei 0 siblings, 0 replies; 10+ messages in thread From: Hawkins Jiawei @ 2022-08-31 11:47 UTC (permalink / raw) To: dan.carpenter Cc: 18801353760, akpm, anton, chenxiaosong2, linux-kernel, linux-ntfs-dev, syzbot+5f8dcabe4a3b2c51c607, syzkaller-bugs, yin31149 On Wed, 31 Aug 2022 at 18:13, Dan Carpenter <dan.carpenter@oracle.com> wrote: > > On Wed, Aug 31, 2022 at 10:48:54AM +0800, Hawkins Jiawei wrote: > > Kernel will iterates over ATTR_RECORDs in mft record in ntfs_attr_find(). > > Because the ATTR_RECORDs are next to each other, kernel can get the next > > ATTR_RECORD from end address of current ATTR_RECORD, through current > > ATTR_RECORD length field. > > > > The problem is that during iteration, when kernel calculates the end address > > of current ATTR_RECORD, kernel may trigger an overflow bug in > > executing `a = (ATTR_RECORD*)((u8*)a + le32_to_cpu(a->length))`. This > > may wrap, leading to a forever iteration on 32bit systems. > > > > This patch solves it by adding an overflow checking on calculating end address > > of current ATTR_RECORD during iteration. > > > > Suggested-by: Dan Carpenter <dan.carpenter@oracle.com> > > Link: https://lore.kernel.org/all/20220827105842.GM2030@kadam/ > > Signed-off-by: Hawkins Jiawei <yin31149@gmail.com> > > --- > > fs/ntfs/attrib.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c > > index 904734e34507..55e618c9e63e 100644 > > --- a/fs/ntfs/attrib.c > > +++ b/fs/ntfs/attrib.c > > @@ -617,6 +617,9 @@ static int ntfs_attr_find(const ATTR_TYPE type, const ntfschar *name, > > return -ENOENT; > > if (unlikely(!a->length)) > > break; > > + /* check for wrap around */ > > + if ((u8 *)a + le32_to_cpu(a->length) < (u8 *)a) > > + break; > > Wouldn't it also be good to check that a + a->length <= mrec_end? > It gets checked on the next iteration sure, but it just seems like a > reasonable thing to check here. Hi Dan, Thanks for your suggestion, I will add this check in my v2 patch! ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-08-31 12:48 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-08-31 2:43 [PATCH 0/3] ntfs: fix bugs about Attribute Hawkins Jiawei 2022-08-31 2:43 ` [PATCH 1/3] ntfs: fix use-after-free in ntfs_attr_find() Hawkins Jiawei 2022-08-31 2:43 ` [PATCH 2/3] ntfs: fix out-of-bounds read " Hawkins Jiawei 2022-08-31 11:07 ` Dan Carpenter 2022-08-31 12:03 ` Hawkins Jiawei 2022-08-31 12:20 ` Dan Carpenter 2022-08-31 12:47 ` Hawkins Jiawei 2022-08-31 2:48 ` [PATCH 3/3] ntfs: check overflow when iterates ATTR_RECORDs Hawkins Jiawei 2022-08-31 10:12 ` Dan Carpenter 2022-08-31 11:47 ` Hawkins Jiawei
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).