[PATCH 0/3] ntfs: fix bugs about Attribute

[Hawkins Jiawei]

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

[Hawkins Jiawei]

Syzkaller reported use-after-free read as follows:

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+5f8dca...@syzkaller.appspotmail.com
Signed-off-by: Hawkins Jiawei <yin3...@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

[Hawkins Jiawei]

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

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+5f8dca...@syzkaller.appspotmail.com
Signed-off-by: chenxiaosong (A) <chenxi...@huawei.com>
Link: https://lore.kernel.org/all/1636796c-c85e-7f47...@huawei.com/
Signed-off-by: Dan Carpenter <dan.ca...@oracle.com>
Link: https://groups.google.com/g/syzkaller-bugs/c/t_XdeKPGTR4/m/LECAuIGcBgAJ

Signed-off-by: Hawkins Jiawei <yin3...@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

[Hawkins Jiawei]

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.ca...@oracle.com>
Link: https://lore.kernel.org/all/20220827105842.GM2030@kadam/

Signed-off-by: Hawkins Jiawei <yin3...@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

[Dan Carpenter]

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-bug...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/7b8b8633d921665a717734d011a92f713944d0fb.1661875711.git.yin31149%40gmail.com.

[Dan Carpenter]

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

[Hawkins Jiawei]

Hi Dan,
Thanks for your suggestion, I will add this check in my v2 patch!

[Hawkins Jiawei]

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)

[Dan Carpenter]

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

[Hawkins Jiawei]

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.

[Hawkins Jiawei]

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

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.

v1->v2:
* Refactor check on Attribute Record Header
suggested by Dan Carpenter.
* Add check bounds on ATTR_RECORD's length suggested
by Dan Carpenter.

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 | 28 ++++++++++++++++++++++++----
fs/ntfs/inode.c | 7 +++++++
2 files changed, 31 insertions(+), 4 deletions(-)

--
2.25.1

[Hawkins Jiawei]

Syzkaller reported use-after-free read as follows:

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.

Signed-off-by: Hawkins Jiawei <yin3...@gmail.com>
---

v1->v2:
* No change.

[Hawkins Jiawei]

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

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+5f8dca...@syzkaller.appspotmail.com
Signed-off-by: chenxiaosong (A) <chenxi...@huawei.com>
Link: https://lore.kernel.org/all/1636796c-c85e-7f47...@huawei.com/
Signed-off-by: Dan Carpenter <dan.ca...@oracle.com>
Link: https://groups.google.com/g/syzkaller-bugs/c/t_XdeKPGTR4/m/LECAuIGcBgAJ

Signed-off-by: Hawkins Jiawei <yin3...@gmail.com>
---

v1->v2:
* Refactor check on Attribute Record Header
suggested by Dan Carpenter.

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..cec4be2a2d23 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;
+
+ /* check whether ATTR_RECORD wrap */

+ if ((u8 *)a < (u8 *)ctx->mrec)
+ break;
+
+ /* check whether Attribute Record Header is within bounds */

+ if ((u8 *)a > mrec_end ||
+ (u8 *)a + sizeof(ATTR_RECORD) > 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

[Hawkins Jiawei]

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 integer 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 some checks on calculating end address

of current ATTR_RECORD during iteration.

Suggested-by: Dan Carpenter <dan.ca...@oracle.com>
Link: https://lore.kernel.org/all/20220827105842.GM2030@kadam/

Signed-off-by: Hawkins Jiawei <yin3...@gmail.com>
---
v1->v2:

* Add check bounds on ATTR_RECORD's length suggested
by Dan Carpenter.

fs/ntfs/attrib.c | 8 ++++++++
1 file changed, 8 insertions(+)

diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
index cec4be2a2d23..a3865bc4a0c6 100644
--- a/fs/ntfs/attrib.c
+++ b/fs/ntfs/attrib.c
@@ -617,6 +617,14 @@ static int ntfs_attr_find(const ATTR_TYPE type, const ntfschar *name,

return -ENOENT;
if (unlikely(!a->length))

break;
+
+ /* check whether ATTR_RECORD's length wrap */

+ if ((u8 *)a + le32_to_cpu(a->length) < (u8 *)a)
+ break;

+ /* check whether ATTR_RECORD's length is within bounds */
+ if ((u8 *)a + le32_to_cpu(a->length) > mrec_end)
+ break;
+