[syzbot] KASAN: use-after-free Read in ntfs_attr_find

[syzbot] KASAN: use-after-free Read in ntfs_attr_find

[syzbot]

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 <chenxiaosong2@huawei.com>
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: syzbot+5f8dcabe4a3b2c51c607@syzkaller.appspotmail.com
Fixes: 38c9c22a85ae ("ntfs: fix use-after-free in ntfs_ucsncmp()")

loop0: detected capacity change from 0 to 64
ntfs: (device loop0): is_boot_sector_ntfs(): Invalid end of sector marker.
==================================================================
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

CPU: 1 PID: 3607 Comm: syz-executor153 Not tainted 6.0.0-rc2-syzkaller-00054-gc40e8341e3b3 #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 07/22/2022
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
RIP: 0033:0x7f7916be3b4a
Code: 48 c7 c2 c0 ff ff ff f7 d8 64 89 02 b8 ff ff ff ff eb d2 e8 f8 03 00 00 0f 1f 84 00 00 00 00 00 49 89 ca b8 a5 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 c0 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007ffd20250468 EFLAGS: 00000286 ORIG_RAX: 00000000000000a5
RAX: ffffffffffffffda RBX: 00007ffd202504c0 RCX: 00007f7916be3b4a
RDX: 0000000020000000 RSI: 0000000020000100 RDI: 00007ffd20250480
RBP: 00007ffd20250480 R08: 00007ffd202504c0 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000286 R12: 0000000020000230
R13: 0000000000000003 R14: 0000000000000004 R15: 0000000000000002
 </TASK>

Allocated by task 3311:
 kasan_save_stack+0x1e/0x40 mm/kasan/common.c:38
 kasan_set_track mm/kasan/common.c:45 [inline]
 set_alloc_info mm/kasan/common.c:437 [inline]
 ____kasan_kmalloc mm/kasan/common.c:516 [inline]
 ____kasan_kmalloc mm/kasan/common.c:475 [inline]
 __kasan_kmalloc+0xa9/0xd0 mm/kasan/common.c:525
 kmalloc include/linux/slab.h:605 [inline]
 tomoyo_realpath_from_path+0xc3/0x620 security/tomoyo/realpath.c:254
 tomoyo_get_realpath security/tomoyo/file.c:151 [inline]
 tomoyo_check_open_permission+0x272/0x380 security/tomoyo/file.c:771
 tomoyo_file_open security/tomoyo/tomoyo.c:320 [inline]
 tomoyo_file_open+0x9d/0xc0 security/tomoyo/tomoyo.c:315
 security_file_open+0x45/0xb0 security/security.c:1646
 do_dentry_open+0x349/0x13a0 fs/open.c:865
 do_open fs/namei.c:3557 [inline]
 path_openat+0x1c92/0x28f0 fs/namei.c:3691
 do_filp_open+0x1b6/0x400 fs/namei.c:3718
 do_sys_openat2+0x16d/0x4c0 fs/open.c:1311
 do_sys_open fs/open.c:1327 [inline]
 __do_sys_openat fs/open.c:1343 [inline]
 __se_sys_openat fs/open.c:1338 [inline]
 __x64_sys_openat+0x13f/0x1f0 fs/open.c:1338
 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

Freed by task 3311:
 kasan_save_stack+0x1e/0x40 mm/kasan/common.c:38
 kasan_set_track+0x21/0x30 mm/kasan/common.c:45
 kasan_set_free_info+0x20/0x30 mm/kasan/generic.c:370
 ____kasan_slab_free mm/kasan/common.c:367 [inline]
 ____kasan_slab_free+0x166/0x1c0 mm/kasan/common.c:329
 kasan_slab_free include/linux/kasan.h:200 [inline]
 slab_free_hook mm/slub.c:1754 [inline]
 slab_free_freelist_hook+0x8b/0x1c0 mm/slub.c:1780
 slab_free mm/slub.c:3534 [inline]
 kfree+0xe2/0x580 mm/slub.c:4562
 tomoyo_realpath_from_path+0x191/0x620 security/tomoyo/realpath.c:291
 tomoyo_get_realpath security/tomoyo/file.c:151 [inline]
 tomoyo_check_open_permission+0x272/0x380 security/tomoyo/file.c:771
 tomoyo_file_open security/tomoyo/tomoyo.c:320 [inline]
 tomoyo_file_open+0x9d/0xc0 security/tomoyo/tomoyo.c:315
 security_file_open+0x45/0xb0 security/security.c:1646
 do_dentry_open+0x349/0x13a0 fs/open.c:865
 do_open fs/namei.c:3557 [inline]
 path_openat+0x1c92/0x28f0 fs/namei.c:3691
 do_filp_open+0x1b6/0x400 fs/namei.c:3718
 do_sys_openat2+0x16d/0x4c0 fs/open.c:1311
 do_sys_open fs/open.c:1327 [inline]
 __do_sys_openat fs/open.c:1343 [inline]
 __se_sys_openat fs/open.c:1338 [inline]
 __x64_sys_openat+0x13f/0x1f0 fs/open.c:1338
 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

The buggy address belongs to the object at ffff88807e352000
 which belongs to the cache kmalloc-4k of size 4096
The buggy address is located 9 bytes inside of
 4096-byte region [ffff88807e352000, ffff88807e353000)

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
page_owner tracks the page as allocated
page last allocated via order 3, migratetype Unmovable, gfp_mask 0xd2040(__GFP_IO|__GFP_NOWARN|__GFP_NORETRY|__GFP_COMP|__GFP_NOMEMALLOC), pid 3311, tgid 3311 (cmp), ts 23752313648, free_ts 23694935443
 prep_new_page mm/page_alloc.c:2532 [inline]
 get_page_from_freelist+0x109b/0x2ce0 mm/page_alloc.c:4283
 __alloc_pages+0x1c7/0x510 mm/page_alloc.c:5515
 alloc_pages+0x1a6/0x270 mm/mempolicy.c:2270
 alloc_slab_page mm/slub.c:1824 [inline]
 allocate_slab+0x27e/0x3d0 mm/slub.c:1969
 new_slab mm/slub.c:2029 [inline]
 ___slab_alloc+0x7f1/0xe10 mm/slub.c:3031
 __slab_alloc.constprop.0+0x4d/0xa0 mm/slub.c:3118
 slab_alloc_node mm/slub.c:3209 [inline]
 slab_alloc mm/slub.c:3251 [inline]
 __kmalloc+0x32b/0x340 mm/slub.c:4420
 kmalloc include/linux/slab.h:605 [inline]
 tomoyo_realpath_from_path+0xc3/0x620 security/tomoyo/realpath.c:254
 tomoyo_get_realpath security/tomoyo/file.c:151 [inline]
 tomoyo_path_perm+0x21b/0x400 security/tomoyo/file.c:822
 security_inode_getattr+0xcf/0x140 security/security.c:1345
 vfs_getattr fs/stat.c:157 [inline]
 vfs_statx+0x16a/0x390 fs/stat.c:232
 vfs_fstatat+0x8c/0xb0 fs/stat.c:255
 __do_sys_newfstatat+0x91/0x110 fs/stat.c:425
 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
page last free stack trace:
 reset_page_owner include/linux/page_owner.h:24 [inline]
 free_pages_prepare mm/page_alloc.c:1449 [inline]
 free_pcp_prepare+0x5e4/0xd20 mm/page_alloc.c:1499
 free_unref_page_prepare mm/page_alloc.c:3380 [inline]
 free_unref_page+0x19/0x4d0 mm/page_alloc.c:3476
 qlink_free mm/kasan/quarantine.c:168 [inline]
 qlist_free_all+0x6a/0x170 mm/kasan/quarantine.c:187
 kasan_quarantine_reduce+0x180/0x200 mm/kasan/quarantine.c:294
 __kasan_slab_alloc+0xa2/0xc0 mm/kasan/common.c:447
 kasan_slab_alloc include/linux/kasan.h:224 [inline]
 slab_post_alloc_hook mm/slab.h:727 [inline]
 slab_alloc_node mm/slub.c:3243 [inline]
 kmem_cache_alloc_node+0x2b1/0x3f0 mm/slub.c:3293
 __alloc_skb+0x210/0x2f0 net/core/skbuff.c:418
 alloc_skb include/linux/skbuff.h:1257 [inline]
 netlink_alloc_large_skb net/netlink/af_netlink.c:1191 [inline]
 netlink_sendmsg+0x9a2/0xe10 net/netlink/af_netlink.c:1896
 sock_sendmsg_nosec net/socket.c:714 [inline]
 sock_sendmsg+0xcf/0x120 net/socket.c:734
 __sys_sendto+0x236/0x340 net/socket.c:2117
 __do_sys_sendto net/socket.c:2129 [inline]
 __se_sys_sendto net/socket.c:2125 [inline]
 __x64_sys_sendto+0xdd/0x1b0 net/socket.c:2125
 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

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 report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.

syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
For information about bisection process see: https://goo.gl/tpsmEJ#bisection
syzbot can test patches for this issue, for details see:
https://goo.gl/tpsmEJ#testing-patches

Re: [syzbot] KASAN: use-after-free Read in ntfs_attr_find

[Siddh Raman Pant]

On Thu, 25 Aug 2022 23:55:35 +0530  syzbot  wrote:
> 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 chenxiaosong2@huawei.com>
> 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: syzbot+5f8dcabe4a3b2c51c607@syzkaller.appspotmail.com
> 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")

The fix for this can be seen by the two different patches tested by syzbot, which
are listed on the dashboard.

Thanks,
Siddh

Re: [syzbot] KASAN: use-after-free Read in ntfs_attr_find

[Siddh Raman Pant]

On Fri, 26 Aug 2022 00:02:07 +0530  Siddh Raman Pant  wrote:
> This is incorrect. The issue is caused by:
> 9b75450d6c58 ("fs/ntfs3: Fix memory leak if fill_super failed")
>

Please ignore. I mistook this for another thread.

Extremely sorry,
Siddh

[PATCH] ntfs: change check order in ntfs_attr_find

[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 <yin31149@gmail.com>
---
 fs/ntfs/attrib.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
index 52615e6090e1..6480cd2d371d 100644
--- a/fs/ntfs/attrib.c
+++ b/fs/ntfs/attrib.c
@@ -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;
 		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) ||
-- 
2.25.1

[PATCH] ntfs: change check order in ntfs_attr_find

[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.

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

diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
index 52615e6090e1..6480cd2d371d 100644
--- a/fs/ntfs/attrib.c
+++ b/fs/ntfs/attrib.c
@@ -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;
 		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) ||
-- 
2.25.1

Re: [syzbot] KASAN: use-after-free Read in ntfs_attr_find

[syzbot]

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger any issue:

Reported-and-tested-by: syzbot+5f8dcabe4a3b2c51c607@syzkaller.appspotmail.com

Tested on:

commit:         4c612826 Merge tag 'net-6.0-rc3' of git://git.kernel.o..
git tree:       https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
console output: https://syzkaller.appspot.com/x/log.txt?x=115a8b13080000
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
patch:          https://syzkaller.appspot.com/x/patch.diff?x=1229de4d080000

Note: testing is done by a robot and is best-effort only.

Re: [PATCH] ntfs: change check order in ntfs_attr_find

[Dan Carpenter]

On Fri, Aug 26, 2022 at 08:32:57PM +0800, Hawkins Jiawei wrote:
> > 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
> 
> diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
> index 52615e6090e1..6480cd2d371d 100644
> --- a/fs/ntfs/attrib.c
> +++ b/fs/ntfs/attrib.c
> @@ -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".

>  		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

Re: [PATCH] ntfs: change check order in ntfs_attr_find

[Hawkins Jiawei]

On Fri, 26 Aug 2022 at 23:15, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Fri, Aug 26, 2022 at 08:32:57PM +0800, Hawkins Jiawei wrote:
> > > 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
> >
> > diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
> > index 52615e6090e1..6480cd2d371d 100644
> > --- a/fs/ntfs/attrib.c
> > +++ b/fs/ntfs/attrib.c
> > @@ -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)

>
> 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".
>
> >               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
So maybe I can try to refactor these codes. But I wonder if this can be
done in a seperate bug

Re: [PATCH] ntfs: change check order in ntfs_attr_find

[Hawkins Jiawei]

On Fri, 26 Aug 2022 at 23:49, Hawkins Jiawei <yin31149@gmail.com> wrote:
>
> >
> > 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".
> >
> > >               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
> So maybe I can try to refactor these codes. But I wonder if this can be
> done in a seperate bug
Sorry for the typing error. I mean refactoring these code
in another seperate patch seems better.

Re: [PATCH] ntfs: change check order in ntfs_attr_find

[chenxiaosong (A)]

在 2022/8/26 20:27, 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 <yin31149@gmail.com>
> ---
>   fs/ntfs/attrib.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
> index 52615e6090e1..6480cd2d371d 100644
> --- a/fs/ntfs/attrib.c
> +++ b/fs/ntfs/attrib.c
> @@ -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;
>   		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 ?

Re: [PATCH] ntfs: change check order in ntfs_attr_find

[Andrew Morton]

On Fri, 26 Aug 2022 20:32:57 +0800 Hawkins Jiawei <yin31149@gmail.com> wrote:

> > 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?

> 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.

Re: [PATCH] ntfs: change check order in ntfs_attr_find

[Dan Carpenter]

On Fri, Aug 26, 2022 at 11:42:32PM +0800, Hawkins Jiawei wrote:
> On Fri, 26 Aug 2022 at 23:15, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > On Fri, Aug 26, 2022 at 08:32:57PM +0800, Hawkins Jiawei wrote:
> > > > 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
> > >
> > > diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
> > > index 52615e6090e1..6480cd2d371d 100644
> > > --- a/fs/ntfs/attrib.c
> > > +++ b/fs/ntfs/attrib.c
> > > @@ -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)

You can declare it at the beginning of the block.

> 
> >
> > 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".
> >
> > >               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
> 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?)

diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
index 52615e6090e1..90d567acb2a3 100644
--- a/fs/ntfs/attrib.c
+++ b/fs/ntfs/attrib.c
@@ -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)
 			break;
+
+		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))

Re: [PATCH] ntfs: change check order in ntfs_attr_find

[Hawkins Jiawei]

On Sat, 27 Aug 2022 at 09:29, chenxiaosong (A) <chenxiaosong2@huawei.com> wrote:
>
> 在 2022/8/26 20:27, 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 <yin31149@gmail.com>
> > ---
> >   fs/ntfs/attrib.c | 5 +++--
> >   1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
> > index 52615e6090e1..6480cd2d371d 100644
> > --- a/fs/ntfs/attrib.c
> > +++ b/fs/ntfs/attrib.c
> > @@ -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;
> >               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 ?
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):
> 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;
> 	...
> }

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);

	...
}

> ==================================================================
> 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
> ==================================================================
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.

diff --git a/fs/ntfs/inode.c b/fs/ntfs/inode.c
index db0f1995aedd..6ba99e109ca9 100644
--- a/fs/ntfs/inode.c
+++ b/fs/ntfs/inode.c
@@ -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?

Re: [PATCH] ntfs: change check order in ntfs_attr_find

[Hawkins Jiawei]

On Sat, 27 Aug 2022 at 10:42, Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Fri, 26 Aug 2022 20:32:57 +0800 Hawkins Jiawei <yin31149@gmail.com> wrote:
>
> > > 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?
>
> > 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.
>
I am sorry for that improper email.

I send that email just wants to confirm whether my guess is right by syzbot.
That is not an official patch email.

Thanks for your remind, I will take care next time!

Re: [PATCH] ntfs: change check order in ntfs_attr_find

[Hawkins Jiawei]

On Sat, 27 Aug 2022 at 14:42, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Fri, Aug 26, 2022 at 11:42:32PM +0800, Hawkins Jiawei wrote:
> > On Fri, 26 Aug 2022 at 23:15, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > >
> > > On Fri, Aug 26, 2022 at 08:32:57PM +0800, Hawkins Jiawei wrote:
> > > > > 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
> > > >
> > > > diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
> > > > index 52615e6090e1..6480cd2d371d 100644
> > > > --- a/fs/ntfs/attrib.c
> > > > +++ b/fs/ntfs/attrib.c
> > > > @@ -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)
>
> You can declare it at the beginning of the block.
OK, I will do like that.

>
> >
> > >
> > > 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".
> > >
> > > >               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
> > 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?)
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)

>
> diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
> index 52615e6090e1..90d567acb2a3 100644
> --- a/fs/ntfs/attrib.c
> +++ b/fs/ntfs/attrib.c
> @@ -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)
>                         break;
> +
> +               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))
Thanks for your suggestion, I will refactor these codes in this way.

Re: [PATCH] ntfs: change check order in ntfs_attr_find

[Dan Carpenter]

On Sat, Aug 27, 2022 at 05:02:31PM +0800, Hawkins Jiawei wrote:
> On Sat, 27 Aug 2022 at 14:42, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > On Fri, Aug 26, 2022 at 11:42:32PM +0800, Hawkins Jiawei wrote:
> > > On Fri, 26 Aug 2022 at 23:15, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > >
> > > > On Fri, Aug 26, 2022 at 08:32:57PM +0800, Hawkins Jiawei wrote:
> > > > > > 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
> > > > >
> > > > > diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
> > > > > index 52615e6090e1..6480cd2d371d 100644
> > > > > --- a/fs/ntfs/attrib.c
> > > > > +++ b/fs/ntfs/attrib.c
> > > > > @@ -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)
> >
> > You can declare it at the beginning of the block.
> OK, I will do like that.
> 
> >
> > >
> > > >
> > > > 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".
> > > >
> > > > >               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
> > > 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?)
> 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

Re: [PATCH] ntfs: change check order in ntfs_attr_find

[Hawkins Jiawei]

On Sat, 27 Aug 2022 at 15:59, Hawkins Jiawei <yin31149@gmail.com> wrote:
>
> On Sat, 27 Aug 2022 at 09:29, chenxiaosong (A) <chenxiaosong2@huawei.com> wrote:
> >
> > 在 2022/8/26 20:27, 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 <yin31149@gmail.com>
> > > ---
> > >   fs/ntfs/attrib.c | 5 +++--
> > >   1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
> > > index 52615e6090e1..6480cd2d371d 100644
> > > --- a/fs/ntfs/attrib.c
> > > +++ b/fs/ntfs/attrib.c
> > > @@ -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;
> > >               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 ?
> 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):
> > 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;
> >       ...
> > }
>
> 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:

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)
{
	ATTR_RECORD *a;
	ntfs_volume *vol = ctx->ntfs_ino->vol;
	ntfschar *upcase = vol->upcase;
	u32 upcase_len = vol->upcase_len;

	/*
	 * Iterate over attributes in mft record starting at @ctx->attr, or the
	 * attribute following that, if @ctx->is_first is 'true'.
	 */
	if (ctx->is_first) {
		a = ctx->attr;
		ctx->is_first = false;
	} else
		a = (ATTR_RECORD*)((u8*)ctx->attr +
				le32_to_cpu(ctx->attr->length));
	...
}

The "ctx->attr" is not invalid, then the pointer "a" will probably also
points to an invalid memory, which triggers a use-after-free read.
This also probably explains why the memory state around the buggy address are
all invalid in KASAN, instead of at the bounder between valid memory
and invalid memory

Yet the analysis below is still correct. ctx->attr is assigned with
"((u8*)mrec + le16_to_cpu(mrec->attrs_offset))" in
ntfs_attr_init_search_ctx()
>
> 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);
>
>         ...
> }
>
> > ==================================================================
> > 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
> > ==================================================================
> 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.
>
> diff --git a/fs/ntfs/inode.c b/fs/ntfs/inode.c
> index db0f1995aedd..6ba99e109ca9 100644
> --- a/fs/ntfs/inode.c
> +++ b/fs/ntfs/inode.c
> @@ -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?

Re: [PATCH] ntfs: change check order in ntfs_attr_find

[Hawkins Jiawei]

On Sat, 27 Aug 2022 at 18:59, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Sat, Aug 27, 2022 at 05:02:31PM +0800, Hawkins Jiawei wrote:
> > On Sat, 27 Aug 2022 at 14:42, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > >
> > > On Fri, Aug 26, 2022 at 11:42:32PM +0800, Hawkins Jiawei wrote:
> > > > On Fri, 26 Aug 2022 at 23:15, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > > >
> > > > > On Fri, Aug 26, 2022 at 08:32:57PM +0800, Hawkins Jiawei wrote:
> > > > > > > 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
> > > > > >
> > > > > > diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
> > > > > > index 52615e6090e1..6480cd2d371d 100644
> > > > > > --- a/fs/ntfs/attrib.c
> > > > > > +++ b/fs/ntfs/attrib.c
> > > > > > @@ -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)
> > >
> > > You can declare it at the beginning of the block.
> > OK, I will do like that.
> >
> > >
> > > >
> > > > >
> > > > > 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".
> > > > >
> > > > > >               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
> > > > 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?)
> > 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
Hi, Dan
Do you mean there may be an overflow on 32bit systems?
It seems that it is, so it may end up in a forever loop
as you point out before.

I will try to add an overflow checking helper to fix it.

Re: [PATCH] ntfs: change check order in ntfs_attr_find

[Dan Carpenter]

On Sat, Aug 27, 2022 at 10:49:44PM +0800, Hawkins Jiawei wrote:
> >
> > 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:
> 

Yep.  Both patches are required.

regards,
dan carpenter