KASAN: use-after-free Read in ntfs_attr_find

Hello,

syzbot found the following issue on:

HEAD commit:    c40e8341e3b3 Merge tag 'cgroup-for-6.0-rc2-fixes' of git:/..
git tree:       upstream
console+strace: https://syzkaller.appspot.com/x/log.txt?x=161114c7080000
kernel config:  https://syzkaller.appspot.com/x/.config?x=911efaff115942bb
dashboard link: https://syzkaller.appspot.com/bug?extid=5f8dcabe4a3b2c51c607
compiler:       gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=14dd8265080000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=11f30033080000

The issue was bisected to:

commit 38c9c22a85aeed28d0831f230136e9cf6fa2ed44

Author: ChenXiaoSong [email protected]>
Date:   Thu Jul 7 10:53:29 2022 +0000

ntfs: fix use-after-free in ntfs_ucsncmp()

bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=16f6cd8d080000
final oops:     https://syzkaller.appspot.com/x/report.txt?x=15f6cd8d080000
console output: https://syzkaller.appspot.com/x/log.txt?x=11f6cd8d080000

IMPORTANT: if you fix the issue, please add the following tag to the commit:

Reported-by [email protected]

Fixes 38c9c22a85ae ("ntfs: fix use-after-free in ntfs_ucsncmp()")

This is incorrect. The issue is caused by:
9b75450d6c58 ("fs/ntfs3: Fix memory leak if fill_super failed")

syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master

Looks like it is improper check order that causes this bug.

Quoted 3 lines

syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master

Looks like it is improper check order that causes this bug.

Sorry for wrong command.
#syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master

fs/ntfs/attrib.c +0 -0

index 52615e6090e1..6480cd2d371d 100644

Quoted 7 lines · Hawkins Jiawei

Quoted 3 lines

syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master

Looks like it is improper check order that causes this bug.

Sorry for wrong command.
#syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master

Quoted 10 lines

fs/ntfs/attrib.c +0 -0

index 52615e6090e1..6480cd2d371d 100644
@@ -594,10 +594,11 @@ static int ntfs_attr_find(const ATTR_TYPE type, const ntfschar *name,
       for (;; a = (ATTR_RECORD*)((u8*)a + le32_to_cpu(a->length))) {

              u8 *mrec_end = (u8 *)ctx->mrec + le32_to_cpu(ctx->mrec->bytes_allocated);
+             if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end)
+                     break;

This definitely seems like a bug.  But your code won't build.  Syzbot must have -Werror turned off?

Btw, this was in the original code, but those casts are ugly.  Ideally there would be some way to get rid of them.  But otherwise at least put a space after the u8.  "(u8 *)a < (u8 *)ctx->mrec".

Quoted 6 lines

              u8 *name_end = (u8 *)a + le16_to_cpu(a->name_offset) + a->name_length * sizeof(ntfschar);
-             if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end ||
-                 name_end > mrec_end)
+             if (name_end > mrec_end)
                      break;

regards,
dan carpenter

Quoted 13 lines

Btw, this was in the original code, but those casts are ugly. Ideally there would be some way to get rid of them. But otherwise at least put a space after the u8. "(u8 *)a < (u8 *)ctx->mrec".

Quoted 6 lines

              u8 *name_end = (u8 *)a + le16_to_cpu(a->name_offset) +
                             a->name_length * sizeof(ntfschar);

fs/ntfs/attrib.c +1 -2

-             if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end ||
-                 name_end > mrec_end)
+             if (name_end > mrec_end)
                      break;

Quoted 2 lines

regards,
dan carpenter

So maybe I can try to refactor these codes. But I wonder if this can be done in a seperate bug

Quoted 9 lines · Dan Carpenter

Quoted 7 lines · Hawkins Jiawei

Quoted 3 lines

syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master

Looks like it is improper check order that causes this bug.

Sorry for wrong command.
#syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master

Quoted 13 lines

Quoted 10 lines

fs/ntfs/attrib.c +0 -0

index 52615e6090e1..6480cd2d371d 100644
@@ -594,10 +594,11 @@ static int ntfs_attr_find(const ATTR_TYPE type, const ntfschar *name,
       for (;; a = (ATTR_RECORD*)((u8*)a + le32_to_cpu(a->length))) {

              u8 *mrec_end = (u8 *)ctx->mrec + le32_to_cpu(ctx->mrec->bytes_allocated);
+             if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end)
+                     break;

This definitely seems like a bug.  But your code won't build.  Syzbot must have -Werror turned off?

Hi Dan,
Did you mean we should put the variable declares at the beginning of the function? (Correct me if I understand anything wrong)

Quoted 13 lines

Btw, this was in the original code, but those casts are ugly.  Ideally there would be some way to get rid of them.  But otherwise at least put a space after the u8.  "(u8 *)a < (u8 *)ctx->mrec".

Quoted 6 lines

              u8 *name_end = (u8 *)a + le16_to_cpu(a->name_offset) + a->name_length * sizeof(ntfschar);
-             if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end ||
-                 name_end > mrec_end)
+             if (name_end > mrec_end)
                      break;

Quoted 2 lines

regards,
dan carpenter

So maybe I can try to refactor these codes. But I wonder if this can be done in a seperate bug

Quoted 23 lines · Hawkins Jiawei

Quoted 22 lines · Dan Carpenter

Quoted 12 lines · Hawkins Jiawei

Quoted 3 lines

syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master

Looks like it is improper check order that causes this bug.

Sorry for wrong command.
#syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master

fs/ntfs/attrib.c +0 -0

index 52615e6090e1..6480cd2d371d 100644

fs/ntfs/attrib.c static int ntfs_attr_find(const ATTR_TYPE type, const ntfschar *name, · line 598

Quoted 5 lines

fs/ntfs/attrib.c +2 -0

@@ -594,10 +594,11 @@ static int ntfs_attr_find(const ATTR_TYPE type, const ntfschar *name,
      for (;; a = (ATTR_RECORD*)((u8*)a + le32_to_cpu(a->length))) {
              u8 *mrec_end = (u8 *)ctx->mrec +
                             le32_to_cpu(ctx->mrec->bytes_allocated);
+             if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end)
+                     break;

This definitely seems like a bug. But your code won't build. Syzbot must have -Werror turned off?

Quoted 3 lines

Hi Dan,
Did you mean we should put the variable declares at the beginning of the function? (Correct me if I understand anything wrong)

You can declare it at the beginning of the block.

Quoted 14 lines

Quoted 14 lines

Btw, this was in the original code, but those casts are ugly. Ideally there would be some way to get rid of them. But otherwise at least put a space after the u8. "(u8 *)a < (u8 *)ctx->mrec".

Quoted 6 lines

fs/ntfs/attrib.c +1 -2

              u8 *name_end = (u8 *)a + le16_to_cpu(a->name_offset) +
                             a->name_length * sizeof(ntfschar);
-             if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end ||
-                 name_end > mrec_end)
+             if (name_end > mrec_end)
                      break;

Quoted 4 lines

Quoted 2 lines

regards,
dan carpenter

So maybe I can try to refactor these codes. But I wonder if this can be done in a seperate bug

The kernel has a strict "one thing per patch rule". Those rules are for reviewers and easier backporting. So the trick is to write the commit message to persuade the reviewer that the way you've written the patch is the easiest way to review it. So here is how I would write the commit message:

[PATCH] ntfs: fix out of bounds read in ntfs_attr_find()

This code deferences "a" to calculate "name_end" and then it checks to ensure that "a" is within bounds. Move the bounds checks earlier and add some comments to make it more clear what they're doing. Then calculate "name_end" and check that.

(Btw, are the wrap around checks really sufficient? It seems like it could wrap to something still within the ->mrec buffer but before the current entry so it would end up in a forever loop or something?)

fs/ntfs/attrib.c +7 -4

index 52615e6090e1..90d567acb2a3 100644
@@ -594,11 +594,20 @@ 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 for wrap around */
+               if ((u8 *)a < (u8 *)ctx->mrec)
+                       break;
+               /* check for overflow */
+               if ((u8 *)a > mrec_end)

Quoted 25 lines · Dan Carpenter

Quoted 23 lines · Hawkins Jiawei

Quoted 22 lines · Dan Carpenter

Quoted 12 lines · Hawkins Jiawei

Quoted 3 lines

syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master

Looks like it is improper check order that causes this bug.

Sorry for wrong command.
#syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master

fs/ntfs/attrib.c +0 -0

index 52615e6090e1..6480cd2d371d 100644

fs/ntfs/attrib.c static int ntfs_attr_find(const ATTR_TYPE type, const ntfschar *name, · line 598

Quoted 5 lines

fs/ntfs/attrib.c +2 -0

@@ -594,10 +594,11 @@ static int ntfs_attr_find(const ATTR_TYPE type, const ntfschar *name,
      for (;; a = (ATTR_RECORD*)((u8*)a + le32_to_cpu(a->length))) {
              u8 *mrec_end = (u8 *)ctx->mrec +
                             le32_to_cpu(ctx->mrec->bytes_allocated);
+             if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end)
+                     break;

This definitely seems like a bug. But your code won't build. Syzbot must have -Werror turned off?

Quoted 5 lines

Quoted 3 lines

Hi Dan,
Did you mean we should put the variable declares at the beginning of the function? (Correct me if I understand anything wrong)

You can declare it at the beginning of the block.

OK, I will do like that.

Quoted 32 lines

Quoted 14 lines

Quoted 14 lines

Btw, this was in the original code, but those casts are ugly. Ideally there would be some way to get rid of them. But otherwise at least put a space after the u8. "(u8 *)a < (u8 *)ctx->mrec".

Quoted 6 lines

fs/ntfs/attrib.c +1 -2

              u8 *name_end = (u8 *)a + le16_to_cpu(a->name_offset) +
                             a->name_length * sizeof(ntfschar);
-             if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end ||
-                 name_end > mrec_end)
+             if (name_end > mrec_end)
                      break;

Quoted 4 lines

Quoted 2 lines

regards,
dan carpenter

So maybe I can try to refactor these codes. But I wonder if this can be done in a seperate bug

The kernel has a strict "one thing per patch rule". Those rules are for reviewers and easier backporting. So the trick is to write the commit message to persuade the reviewer that the way you've written the patch is the easiest way to review it. So here is how I would write the commit message:

[PATCH] ntfs: fix out of bounds read in ntfs_attr_find()

This code deferences "a" to calculate "name_end" and then it checks to ensure that "a" is within bounds. Move the bounds checks earlier and add some comments to make it more clear what they're doing. Then calculate "name_end" and check that.

Quoted 3 lines

(Btw, are the wrap around checks really sufficient? It seems like it could wrap to something still within the ->mrec buffer but before the current entry so it would end up in a forever loop or something?)

I am not for sure, but it seems that it is OK before.
As for the forever loop, there is a break when a->length is 0 in the loop, So I think it probably would not end up in a forever loop?(Correct me if I am wrong)

Quoted 65 lines · Hawkins Jiawei

Quoted 25 lines · Dan Carpenter

Quoted 23 lines · Hawkins Jiawei

Quoted 22 lines · Dan Carpenter

Quoted 13 lines · Hawkins Jiawei

Quoted 3 lines

syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git     master

Looks like it is improper check order that causes this bug.

Sorry for wrong command.
#syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git     master

fs/ntfs/attrib.c +0 -0

index 52615e6090e1..6480cd2d371d 100644
@@ -594,10 +594,11 @@ static int ntfs_attr_find(const ATTR_TYPE type, const ntfschar *name,
       for (;; a = (ATTR_RECORD*)((u8*)a + le32_to_cpu(a->length))) {

Quoted 4 lines

              u8 *mrec_end = (u8 *)ctx->mrec + le32_to_cpu(ctx->mrec->bytes_allocated);
+             if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end)
+                     break;

This definitely seems like a bug.  But your code won't build.  Syzbot must have -Werror turned off?

Quoted 5 lines

Quoted 3 lines

Hi Dan,
Did you mean we should put the variable declares at the beginning of the function? (Correct me if I understand anything wrong)

You can declare it at the beginning of the block.

OK, I will do like that.

Quoted 32 lines

Quoted 14 lines

Quoted 14 lines

Btw, this was in the original code, but those casts are ugly.  Ideally there would be some way to get rid of them.  But otherwise at least put a space after the u8.  "(u8 *)a < (u8 *)ctx->mrec".

Quoted 6 lines

              u8 *name_end = (u8 *)a + le16_to_cpu(a->name_offset) + a->name_length * sizeof(ntfschar);
-             if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end ||
-                 name_end > mrec_end)
+             if (name_end > mrec_end)
                      break;

Quoted 4 lines

Quoted 2 lines

regards,
dan carpenter

So maybe I can try to refactor these codes. But I wonder if this can be done in a seperate bug

The kernel has a strict "one thing per patch rule".  Those rules are for reviewers and easier backporting.  So the trick is to write the commit message to persuade the reviewer that the way you've written the patch is the easiest way to review it.  So here is how I would write the commit message:

[PATCH] ntfs: fix out of bounds read in ntfs_attr_find()

This code deferences "a" to calculate "name_end" and then it checks to ensure that "a" is within bounds.  Move the bounds checks earlier and add some comments to make it more clear what they're doing.  Then calculate "name_end" and check that.

Quoted 8 lines

Quoted 3 lines

(Btw, are the wrap around checks really sufficient?  It seems like it could wrap to something still within the ->mrec buffer but before the current entry so it would end up in a forever loop or something?)

I am not for sure, but it seems that it is OK before.
As for the forever loop, there is a break when a->length is 0 in the loop, So I think it probably would not end up in a forever loop?(Correct me if I am wrong)

Checking for zero is not sufficient because it can wrap on 32 bit systems.  It needs something like:

                        return -ENOENT; len = le32_to_cpu(a->length); if (!len ||
                    (void *)a + len < (void *)a || (void *)a + len > mrec_end) break;
                if (a->type != type) continue;

Sort of ugly, but hopefully it gives the idea of what I'm saying.

regards,
dan carpenter

Quoted 3 lines

syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master

Looks like it is improper check order that causes this bug.

Sorry for wrong command.
#syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master

Quoted 3 lines · On Fri, 26 Aug 2022 20:32:57 +0800 Hawkins Jiawei <[email protected]> wrote:

Quoted 3 lines

syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master

Looks like it is improper check order that causes this bug.

um, what bug?

Quoted 2 lines

Sorry for wrong command.
#syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master

Please prepare a full changelog for the next version? Describe the user-visible runtime effects of the bug, describe what the code does wrong and how the patch repairs it.

syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master

Looks like it is improper check order that causes this bug.

Signed-off-by Hawkins Jiawei <[email protected]>

---
  fs/ntfs/attrib.c | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)

fs/ntfs/attrib.c +0 -0

index 52615e6090e1..6480cd2d371d 100644
@@ -594,10 +594,11 @@ static int ntfs_attr_find(const ATTR_TYPE type, const ntfschar *name,
  	for (;;	a = (ATTR_RECORD*)((u8*)a + le32_to_cpu(a->length))) {
  		u8 *mrec_end = (u8 *)ctx->mrec +
  		               le32_to_cpu(ctx->mrec->bytes_allocated);

Quoted 18 lines · Hawkins Jiawei

syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master

Looks like it is improper check order that causes this bug.

Signed-off-by Hawkins Jiawei <[email protected]>

---
  fs/ntfs/attrib.c | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)

fs/ntfs/attrib.c +0 -0

index 52615e6090e1..6480cd2d371d 100644
@@ -594,10 +594,11 @@ static int ntfs_attr_find(const ATTR_TYPE type, const ntfschar *name,
      for (;; a = (ATTR_RECORD*)((u8*)a + le32_to_cpu(a->length))) {
              u8 *mrec_end = (u8 *)ctx->mrec +
                             le32_to_cpu(ctx->mrec->bytes_allocated);

fs/ntfs/attrib.c static int ntfs_attr_find(const ATTR_TYPE type, const ntfschar *name, · line 604

Quoted 10 lines

fs/ntfs/attrib.c +3 -2

+             if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end)
+                     break;
              u8 *name_end = (u8 *)a + le16_to_cpu(a->name_offset) +
                             a->name_length * sizeof(ntfschar);
-             if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end ||
-                 name_end > mrec_end)
+             if (name_end > mrec_end)
                      break;
              ctx->attr = a;
              if (unlikely(le32_to_cpu(a->type) > le32_to_cpu(type) ||

The reason is that a->length is 0, it will occur uaf when deref any field of ATTR_RECORD.

It seems that changing check order will not fix root cause, if the condition "if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end)" is false, uaf will still occur.

Do you have any thoughts on this ?

static int ntfs_attr_find(const ATTR_TYPE type, const ntfschar *name,
const u32 name_len, const IGNORE_CASE_BOOL ic, const u8 *val, const u32 val_len, ntfs_attr_search_ctx *ctx)

{
	...

	for (;;	a = (ATTR_RECORD*)((u8*)a + le32_to_cpu(a->length))) {
		u8 *mrec_end = (u8 *)ctx->mrec +
		               le32_to_cpu(ctx->mrec->bytes_allocated);
		u8 *name_end = (u8 *)a + le16_to_cpu(a->name_offset) +
			       a->name_length * sizeof(ntfschar);
		if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end ||
		    name_end > mrec_end)
			break;

		ctx->attr = a;
		if (unlikely(le32_to_cpu(a->type) > le32_to_cpu(type) ||
				a->type == AT_END))
			return -ENOENT;
		if (unlikely(!a->length))
			break;
	...
}

==================================================================

BUG: KASAN: use-after-free in ntfs_attr_find+0xc02/0xce0 fs/ntfs/attrib.c:597
Read of size 2 at addr ffff88807e352009 by task syz-executor153/3607
Call Trace:
 <TASK>
 __dump_stack lib/dump_stack.c:88 [inline]
 dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106
 print_address_description mm/kasan/report.c:317 [inline]
 print_report.cold+0x2ba/0x719 mm/kasan/report.c:433
 kasan_report+0xb1/0x1e0 mm/kasan/report.c:495
 ntfs_attr_find+0xc02/0xce0 fs/ntfs/attrib.c:597
 ntfs_attr_lookup+0x1056/0x2070 fs/ntfs/attrib.c:1193
 ntfs_read_inode_mount+0x89a/0x2580 fs/ntfs/inode.c:1845
 ntfs_fill_super+0x1799/0x9320 fs/ntfs/super.c:2854
 mount_bdev+0x34d/0x410 fs/super.c:1400
 legacy_get_tree+0x105/0x220 fs/fs_context.c:610
 vfs_get_tree+0x89/0x2f0 fs/super.c:1530
 do_new_mount fs/namespace.c:3040 [inline]
 path_mount+0x1326/0x1e20 fs/namespace.c:3370
 do_mount fs/namespace.c:3383 [inline]
 __do_sys_mount fs/namespace.c:3591 [inline]
 __se_sys_mount fs/namespace.c:3568 [inline]
 __x64_sys_mount+0x27f/0x300 fs/namespace.c:3568
 do_syscall_x64 arch/x86/entry/common.c:50 [inline]
 do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
 entry_SYSCALL_64_after_hwframe+0x63/0xcd
 [...]
 </TASK>

Memory state around the buggy address:
 ffff88807e351f00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
 ffff88807e351f80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>ffff88807e352000: fa fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
                      ^
 ffff88807e352080: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
 ffff88807e352100: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
==================================================================

Quoted 34 lines · chenxiaosong (A)

Quoted 19 lines · Hawkins Jiawei

syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master

Looks like it is improper check order that causes this bug.

Signed-off-by Hawkins Jiawei <[email protected]>

---
  fs/ntfs/attrib.c | 5 +++--

  1 file changed, 3 insertions(+), 2 deletions(-)

fs/ntfs/attrib.c +0 -0

index 52615e6090e1..6480cd2d371d 100644
@@ -594,10 +594,11 @@ static int ntfs_attr_find(const ATTR_TYPE type, const ntfschar *name,
       for (;; a = (ATTR_RECORD*)((u8*)a + le32_to_cpu(a->length))) {

              u8 *mrec_end = (u8 *)ctx->mrec + le32_to_cpu(ctx->mrec->bytes_allocated);
+             if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end)
+                     break;

Quoted 9 lines

              u8 *name_end = (u8 *)a + le16_to_cpu(a->name_offset) + a->name_length * sizeof(ntfschar);
-             if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end ||
-                 name_end > mrec_end)
+             if (name_end > mrec_end)
                      break;
              ctx->attr = a;
              if (unlikely(le32_to_cpu(a->type) > le32_to_cpu(type) ||

The reason is that a->length is 0, it will occur uaf when deref any field of ATTR_RECORD.

Quoted 5 lines

It seems that changing check order will not fix root cause, if the condition "if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end)" is false, uaf will still occur.

Do you have any thoughts on this ?

Hi, chenxiaosong

I think changing the check order is able to fix this bug. But we may need to check whether mft record header is out of bounds, maybe "if ((u8*)a < (u8*)ctx->mrec || (u8*)a + sizeof(MFT_RECORD) > mrec_end)"

Because we just need to check if this ATTR_RECORD is in valid addresss. As for situation that a->length is 0, there seems already a check in the loop (Correct me if I am wrong):

Quoted 6 lines

static int ntfs_attr_find(const ATTR_TYPE type, const ntfschar *name, const u32 name_len, const IGNORE_CASE_BOOL ic, const u8 *val, const u32 val_len, ntfs_attr_search_ctx *ctx) {
      ...

Quoted 16 lines

      for (;; a = (ATTR_RECORD*)((u8*)a + le32_to_cpu(a->length))) { u8 *mrec_end = (u8 *)ctx->mrec + le32_to_cpu(ctx->mrec->bytes_allocated); u8 *name_end = (u8 *)a + le16_to_cpu(a->name_offset) + a->name_length * sizeof(ntfschar); if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end || name_end > mrec_end)
                      break;
              ctx->attr = a;
              if (unlikely(le32_to_cpu(a->type) > le32_to_cpu(type) || a->type == AT_END))
                      return -ENOENT;
              if (unlikely(!a->length))
                      break;
      ...
}

And as for the root cause for use-after-free read, I think it is the ctx->attr->length to be blamed.

To be more specific, when kernel loads the struct MFT_RECORD from disk in ntfs_read_inode_mount(),  m's attrs_offset field should less than m's bytes_allocated field, or it may out of the bounds:

int ntfs_read_inode_mount(struct inode *vi)
{
        ...
        MFT_RECORD *m = NULL;
        i = vol->mft_record_size;

        ...
        m = (MFT_RECORD*)ntfs_malloc_nofs(i);

        /* Determine the first block of the $MFT/$DATA attribute. */ block = vol->mft_lcn << vol->cluster_size_bits >> sb->s_blocksize_bits;
        nr_blocks = vol->mft_record_size >> sb->s_blocksize_bits;

        ...

        /* Load $MFT/$DATA's first mft record. */
        for (i = 0; i < nr_blocks; i++) {
                bh = sb_bread(sb, block++);
                if (!bh) {
                        ntfs_error(sb, "Device read failed."); goto err_out;
                }
                memcpy((char*)m + (i << sb->s_blocksize_bits), bh->b_data, sb->s_blocksize);
                brelse(bh);
        }

        if (le32_to_cpu(m->bytes_allocated) != vol->mft_record_size) { ntfs_error(sb, "Incorrect mft record size %u in superblock, should be %u.", le32_to_cpu(m->bytes_allocated), vol->mft_record_size); goto err_out;
        }

        ...

        ctx = ntfs_attr_get_search_ctx(ni, m);
        if (!ctx) {
                err = -ENOMEM;
                goto err_out;
        }

        /* Find the attribute list attribute if present. */
        err = ntfs_attr_lookup(AT_ATTRIBUTE_LIST, NULL, 0, 0, 0, NULL, 0, ctx);

        ...
}

Quoted 32 lines

==================================================================

BUG: KASAN: use-after-free in ntfs_attr_find+0xc02/0xce0 fs/ntfs/attrib.c:597
Read of size 2 at addr ffff88807e352009 by task syz-executor153/3607
Call Trace:
 <TASK>
 __dump_stack lib/dump_stack.c:88 [inline]
 dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106
 print_address_description mm/kasan/report.c:317 [inline]
 print_report.cold+0x2ba/0x719 mm/kasan/report.c:433
 kasan_report+0xb1/0x1e0 mm/kasan/report.c:495
 ntfs_attr_find+0xc02/0xce0 fs/ntfs/attrib.c:597
 ntfs_attr_lookup+0x1056/0x2070 fs/ntfs/attrib.c:1193
 ntfs_read_inode_mount+0x89a/0x2580 fs/ntfs/inode.c:1845
 ntfs_fill_super+0x1799/0x9320 fs/ntfs/super.c:2854
 mount_bdev+0x34d/0x410 fs/super.c:1400
 legacy_get_tree+0x105/0x220 fs/fs_context.c:610
 vfs_get_tree+0x89/0x2f0 fs/super.c:1530
 do_new_mount fs/namespace.c:3040 [inline]
 path_mount+0x1326/0x1e20 fs/namespace.c:3370
 do_mount fs/namespace.c:3383 [inline]
 __do_sys_mount fs/namespace.c:3591 [inline]
 __se_sys_mount fs/namespace.c:3568 [inline]
 __x64_sys_mount+0x27f/0x300 fs/namespace.c:3568
 do_syscall_x64 arch/x86/entry/common.c:50 [inline]
 do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
 entry_SYSCALL_64_after_hwframe+0x63/0xcd
 [...]
 </TASK>
Memory state around the buggy address:
 ffff88807e351f00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
 ffff88807e351f80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>ffff88807e352000: fa fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb

Quoted 4 lines

                      ^
 ffff88807e352080: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
 ffff88807e352100: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
==================================================================

So check these fields as follow should fix the root cause for this use-after-free bug(Correct me if I am wrong). I test this patch locally, it doesn't trigger any issue.

fs/ntfs/inode.c +5 -0

index db0f1995aedd..6ba99e109ca9 100644
@@ -1822,6 +1822,11 @@ int ntfs_read_inode_mount(struct inode *vi)
                 goto err_out;
         }
 
+       if (m->attrs_offset >= le32_to_cpu(m->bytes_allocated)) {
+               ntfs_error(sb, "Incorrect mft record attrs_offset %u", m->attrs_offset);
+               goto err_out;
+       }
+
         /* Apply the mst fixups. */
         if (post_read_mst_fixup((NTFS_RECORD*)m, vol->mft_record_size)) {
                 /* FIXME: Try to use the $MFTMirr now. */

What's your opinion?

Quoted 3 lines

And as for the root cause for use-after-free read, I think it is the ctx->attr->length to be blamed.

Apologize for my typing error, it is the ctx->attr should be blamed: