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
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 <[email protected]>
Link: https://lore.kernel.org/all/20220827105842.GM2030@kadam/
Signed-off-by: Hawkins Jiawei <[email protected]>
---
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
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: [email protected]
Signed-off-by: chenxiaosong (A) <[email protected]>
Link: https://lore.kernel.org/all/[email protected]/
Signed-off-by: Dan Carpenter <[email protected]>
Link: https://groups.google.com/g/syzkaller-bugs/c/t_XdeKPGTR4/m/LECAuIGcBgAJ
Signed-off-by: Hawkins Jiawei <[email protected]>
---
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
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: [email protected]
Signed-off-by: Hawkins Jiawei <[email protected]>
---
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
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 <[email protected]>
> Link: https://lore.kernel.org/all/20220827105842.GM2030@kadam/
> Signed-off-by: Hawkins Jiawei <[email protected]>
> ---
> 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 [email protected].
> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/7b8b8633d921665a717734d011a92f713944d0fb.1661875711.git.yin31149%40gmail.com.
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: [email protected]
> Signed-off-by: chenxiaosong (A) <[email protected]>
> Link: https://lore.kernel.org/all/[email protected]/
> Signed-off-by: Dan Carpenter <[email protected]>
> Link: https://groups.google.com/g/syzkaller-bugs/c/t_XdeKPGTR4/m/LECAuIGcBgAJ
> Signed-off-by: Hawkins Jiawei <[email protected]>
> ---
> 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))
On Wed, 31 Aug 2022 at 18:13, Dan Carpenter <[email protected]> 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 <[email protected]>
> > Link: https://lore.kernel.org/all/20220827105842.GM2030@kadam/
> > Signed-off-by: Hawkins Jiawei <[email protected]>
> > ---
> > 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!
On Wed, 31 Aug 2022 at 19:08, Dan Carpenter <[email protected]> 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: [email protected]
> > Signed-off-by: chenxiaosong (A) <[email protected]>
> > Link: https://lore.kernel.org/all/[email protected]/
> > Signed-off-by: Dan Carpenter <[email protected]>
> > Link: https://groups.google.com/g/syzkaller-bugs/c/t_XdeKPGTR4/m/LECAuIGcBgAJ
> > Signed-off-by: Hawkins Jiawei <[email protected]>
> > ---
> > 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))
>
On Wed, Aug 31, 2022 at 08:03:25PM +0800, Hawkins Jiawei wrote:
> On Wed, 31 Aug 2022 at 19:08, Dan Carpenter <[email protected]> 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: [email protected]
> > > Signed-off-by: chenxiaosong (A) <[email protected]>
> > > Link: https://lore.kernel.org/all/[email protected]/
> > > Signed-off-by: Dan Carpenter <[email protected]>
> > > Link: https://groups.google.com/g/syzkaller-bugs/c/t_XdeKPGTR4/m/LECAuIGcBgAJ
> > > Signed-off-by: Hawkins Jiawei <[email protected]>
> > > ---
> > > ?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
On Wed, 31 Aug 2022 at 20:22, Dan Carpenter <[email protected]> wrote:
>
> On Wed, Aug 31, 2022 at 08:03:25PM +0800, Hawkins Jiawei wrote:
> > On Wed, 31 Aug 2022 at 19:08, Dan Carpenter <[email protected]> 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: [email protected]
> > > > Signed-off-by: chenxiaosong (A) <[email protected]>
> > > > Link: https://lore.kernel.org/all/[email protected]/
> > > > Signed-off-by: Dan Carpenter <[email protected]>
> > > > Link: https://groups.google.com/g/syzkaller-bugs/c/t_XdeKPGTR4/m/LECAuIGcBgAJ
> > > > Signed-off-by: Hawkins Jiawei <[email protected]>
> > > > ---
> > > > 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.