[syzbot] [btrfs?] KASAN: slab-out-of-bounds Read in getname_kernel (2)

[syzbot] [btrfs?] KASAN: slab-out-of-bounds Read in getname_kernel (2)

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit:    3bd7d7488169 Merge tag 'io_uring-6.7-2023-12-15' of git://..
git tree:       upstream
console+strace: https://syzkaller.appspot.com/x/log.txt?x=13732cc6e80000
kernel config:  https://syzkaller.appspot.com/x/.config?x=53ec3da1d259132f
dashboard link: https://syzkaller.appspot.com/bug?extid=33f23b49ac24f986c9e8
compiler:       Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=16d8ba06e80000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=136fd5b2e80000

Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/99b3f103aa0b/disk-3bd7d748.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/32f8e3e696ce/vmlinux-3bd7d748.xz
kernel image: https://storage.googleapis.com/syzbot-assets/cb20a5445c11/bzImage-3bd7d748.xz
mounted in repro: https://storage.googleapis.com/syzbot-assets/4f5365674997/mount_0.gz

The issue was bisected to:

commit 9974d37ea75f01b47d16072b5dad305bd8d23fcc
Author: Liu Jian <liujian56@huawei.com>
Date:   Tue Jun 28 12:36:16 2022 +0000

    skmsg: Fix invalid last sg check in sk_msg_recvmsg()

bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=172b998ae80000
final oops:     https://syzkaller.appspot.com/x/report.txt?x=14ab998ae80000
console output: https://syzkaller.appspot.com/x/log.txt?x=10ab998ae80000

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+33f23b49ac24f986c9e8@syzkaller.appspotmail.com
Fixes: 9974d37ea75f ("skmsg: Fix invalid last sg check in sk_msg_recvmsg()")

BTRFS info (device loop0): clearing compat-ro feature flag for FREE_SPACE_TREE_VALID (0x2)
==================================================================
BUG: KASAN: slab-out-of-bounds in strlen+0x58/0x70 lib/string.c:418
Read of size 1 at addr ffff88801d7f2a28 by task syz-executor424/5057

CPU: 1 PID: 5057 Comm: syz-executor424 Not tainted 6.7.0-rc5-syzkaller-00200-g3bd7d7488169 #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 11/17/2023
Call Trace:
 <TASK>
 __dump_stack lib/dump_stack.c:88 [inline]
 dump_stack_lvl+0x1e7/0x2d0 lib/dump_stack.c:106
 print_address_description mm/kasan/report.c:364 [inline]
 print_report+0x163/0x540 mm/kasan/report.c:475
 kasan_report+0x142/0x170 mm/kasan/report.c:588
 strlen+0x58/0x70 lib/string.c:418
 getname_kernel+0x1d/0x2e0 fs/namei.c:226
 kern_path+0x1d/0x50 fs/namei.c:2609
 lookup_bdev block/bdev.c:979 [inline]
 bdev_open_by_path+0xd1/0x540 block/bdev.c:901
 btrfs_init_dev_replace_tgtdev fs/btrfs/dev-replace.c:260 [inline]
 btrfs_dev_replace_start fs/btrfs/dev-replace.c:638 [inline]
 btrfs_dev_replace_by_ioctl+0x41b/0x2010 fs/btrfs/dev-replace.c:747
 btrfs_ioctl_dev_replace+0x2c9/0x390 fs/btrfs/ioctl.c:3299
 vfs_ioctl fs/ioctl.c:51 [inline]
 __do_sys_ioctl fs/ioctl.c:871 [inline]
 __se_sys_ioctl+0xf8/0x170 fs/ioctl.c:857
 do_syscall_x64 arch/x86/entry/common.c:52 [inline]
 do_syscall_64+0x45/0x110 arch/x86/entry/common.c:83
 entry_SYSCALL_64_after_hwframe+0x63/0x6b
RIP: 0033:0x7f8e8ffdc079
Code: 28 00 00 00 75 05 48 83 c4 28 c3 e8 61 17 00 00 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 b8 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007ffe15cbe138 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
RAX: ffffffffffffffda RBX: 00007ffe15cbe308 RCX: 00007f8e8ffdc079
RDX: 0000000020000540 RSI: 00000000ca289435 RDI: 0000000000000005
RBP: 00007f8e90054610 R08: 00007ffe15cbe308 R09: 00007ffe15cbe308
R10: 00007ffe15cbe308 R11: 0000000000000246 R12: 0000000000000001
R13: 00007ffe15cbe2f8 R14: 0000000000000001 R15: 0000000000000001
 </TASK>

Allocated by task 5057:
 kasan_save_stack mm/kasan/common.c:45 [inline]
 kasan_set_track+0x4f/0x70 mm/kasan/common.c:52
 ____kasan_kmalloc mm/kasan/common.c:374 [inline]
 __kasan_kmalloc+0x98/0xb0 mm/kasan/common.c:383
 kasan_kmalloc include/linux/kasan.h:198 [inline]
 __do_kmalloc_node mm/slab_common.c:1007 [inline]
 __kmalloc_node_track_caller+0xb1/0x190 mm/slab_common.c:1027
 memdup_user+0x2b/0xc0 mm/util.c:197
 btrfs_ioctl_dev_replace+0xb8/0x390 fs/btrfs/ioctl.c:3286
 vfs_ioctl fs/ioctl.c:51 [inline]
 __do_sys_ioctl fs/ioctl.c:871 [inline]
 __se_sys_ioctl+0xf8/0x170 fs/ioctl.c:857
 do_syscall_x64 arch/x86/entry/common.c:52 [inline]
 do_syscall_64+0x45/0x110 arch/x86/entry/common.c:83
 entry_SYSCALL_64_after_hwframe+0x63/0x6b

The buggy address belongs to the object at ffff88801d7f2000
 which belongs to the cache kmalloc-4k of size 4096
The buggy address is located 0 bytes to the right of
 allocated 2600-byte region [ffff88801d7f2000, ffff88801d7f2a28)

The buggy address belongs to the physical page:
page:ffffea000075fc00 refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x1d7f0
head:ffffea000075fc00 order:3 entire_mapcount:0 nr_pages_mapped:0 pincount:0
flags: 0xfff00000000840(slab|head|node=0|zone=1|lastcpupid=0x7ff)
page_type: 0xffffffff()
raw: 00fff00000000840 ffff888012c42140 ffffea00007b5600 0000000000000002
raw: 0000000000000000 0000000080040004 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 4716, tgid 4716 (ifup), ts 29920494589, free_ts 29905444571
 set_page_owner include/linux/page_owner.h:31 [inline]
 post_alloc_hook+0x1e6/0x210 mm/page_alloc.c:1537
 prep_new_page mm/page_alloc.c:1544 [inline]
 get_page_from_freelist+0x33ea/0x3570 mm/page_alloc.c:3312
 __alloc_pages+0x255/0x680 mm/page_alloc.c:4568
 alloc_pages_mpol+0x3de/0x640 mm/mempolicy.c:2133
 alloc_slab_page+0x6a/0x170 mm/slub.c:1870
 allocate_slab mm/slub.c:2017 [inline]
 new_slab+0x84/0x2f0 mm/slub.c:2070
 ___slab_alloc+0xc8a/0x1330 mm/slub.c:3223
 __slab_alloc mm/slub.c:3322 [inline]
 __slab_alloc_node mm/slub.c:3375 [inline]
 slab_alloc_node mm/slub.c:3468 [inline]
 __kmem_cache_alloc_node+0x21d/0x300 mm/slub.c:3517
 __do_kmalloc_node mm/slab_common.c:1006 [inline]
 __kmalloc+0xa2/0x1a0 mm/slab_common.c:1020
 kmalloc include/linux/slab.h:604 [inline]
 tomoyo_realpath_from_path+0xcf/0x5e0 security/tomoyo/realpath.c:251
 tomoyo_get_realpath security/tomoyo/file.c:151 [inline]
 tomoyo_check_open_permission+0x255/0x500 security/tomoyo/file.c:771
 security_file_open+0x63/0xa0 security/security.c:2836
 do_dentry_open+0x327/0x1590 fs/open.c:935
 do_open fs/namei.c:3622 [inline]
 path_openat+0x2849/0x3290 fs/namei.c:3779
 do_filp_open+0x234/0x490 fs/namei.c:3809
 do_sys_openat2+0x13e/0x1d0 fs/open.c:1440
page last free stack trace:
 reset_page_owner include/linux/page_owner.h:24 [inline]
 free_pages_prepare mm/page_alloc.c:1137 [inline]
 free_unref_page_prepare+0x931/0xa60 mm/page_alloc.c:2347
 free_unref_page+0x37/0x3f0 mm/page_alloc.c:2487
 discard_slab mm/slub.c:2116 [inline]
 __unfreeze_partials+0x1e0/0x220 mm/slub.c:2655
 put_cpu_partial+0x17b/0x250 mm/slub.c:2731
 __slab_free+0x2b6/0x390 mm/slub.c:3679
 qlink_free mm/kasan/quarantine.c:168 [inline]
 qlist_free_all+0x75/0xe0 mm/kasan/quarantine.c:187
 kasan_quarantine_reduce+0x14b/0x160 mm/kasan/quarantine.c:294
 __kasan_slab_alloc+0x23/0x70 mm/kasan/common.c:305
 kasan_slab_alloc include/linux/kasan.h:188 [inline]
 slab_post_alloc_hook+0x6c/0x3c0 mm/slab.h:763
 slab_alloc_node mm/slub.c:3478 [inline]
 __kmem_cache_alloc_node+0x1d0/0x300 mm/slub.c:3517
 __do_kmalloc_node mm/slab_common.c:1006 [inline]
 __kmalloc+0xa2/0x1a0 mm/slab_common.c:1020
 kmalloc include/linux/slab.h:604 [inline]
 tomoyo_realpath_from_path+0xcf/0x5e0 security/tomoyo/realpath.c:251
 tomoyo_get_realpath security/tomoyo/file.c:151 [inline]
 tomoyo_path_perm+0x2b7/0x730 security/tomoyo/file.c:822
 security_inode_getattr+0xd3/0x120 security/security.c:2153
 vfs_getattr+0x46/0x430 fs/stat.c:173
 vfs_fstat fs/stat.c:198 [inline]
 vfs_fstatat+0xd6/0x190 fs/stat.c:295

Memory state around the buggy address:
 ffff88801d7f2900: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
 ffff88801d7f2980: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>ffff88801d7f2a00: 00 00 00 00 00 fc fc fc fc fc fc fc fc fc fc fc
                                  ^
 ffff88801d7f2a80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
 ffff88801d7f2b00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
==================================================================


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

If the report is already addressed, let syzbot know by replying with:
#syz fix: exact-commit-title

If you want syzbot to run the reproducer, reply with:
#syz test: git://repo/address.git branch-or-commit-hash
If you attach or paste a git patch, syzbot will apply it before testing.

If you want to overwrite report's subsystems, reply with:
#syz set subsystems: new-subsystem
(See the list of subsystem names on the web dashboard)

If the report is a duplicate of another one, reply with:
#syz dup: exact-subject-of-another-report

If you want to undo deduplication, reply with:
#syz undup

[PATCH] btrfs: fix oob Read in getname_kernel

[Edward Adam Davis]

If ioctl does not pass in the correct tgtdev_name string, oob will occur because
"\0" cannot be found.

Reported-and-tested-by: syzbot+33f23b49ac24f986c9e8@syzkaller.appspotmail.com
Signed-off-by: Edward Adam Davis <eadavis@qq.com>
---
 fs/btrfs/dev-replace.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index f9544fda38e9..e7e96e57f682 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -730,7 +730,7 @@ static int btrfs_dev_replace_start(struct btrfs_fs_info *fs_info,
 int btrfs_dev_replace_by_ioctl(struct btrfs_fs_info *fs_info,
 			    struct btrfs_ioctl_dev_replace_args *args)
 {
-	int ret;
+	int ret, len;
 
 	switch (args->start.cont_reading_from_srcdev_mode) {
 	case BTRFS_IOCTL_DEV_REPLACE_CONT_READING_FROM_SRCDEV_MODE_ALWAYS:
@@ -740,8 +740,10 @@ int btrfs_dev_replace_by_ioctl(struct btrfs_fs_info *fs_info,
 		return -EINVAL;
 	}
 
+	len = strnlen(args->start.tgtdev_name, BTRFS_DEVICE_PATH_NAME_MAX + 1);
 	if ((args->start.srcdevid == 0 && args->start.srcdev_name[0] == '\0') ||
-	    args->start.tgtdev_name[0] == '\0')
+	    args->start.tgtdev_name[0] == '\0' ||
+	    len == BTRFS_DEVICE_PATH_NAME_MAX + 1)
 		return -EINVAL;
 
 	ret = btrfs_dev_replace_start(fs_info, args->start.tgtdev_name,
-- 
2.43.0

Re: [PATCH] btrfs: fix oob Read in getname_kernel

[David Sterba]

On Tue, Dec 19, 2023 at 06:19:10PM +0800, Edward Adam Davis wrote:
> If ioctl does not pass in the correct tgtdev_name string, oob will occur because
> "\0" cannot be found.
> 
> Reported-and-tested-by: syzbot+33f23b49ac24f986c9e8@syzkaller.appspotmail.com
> Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> ---
>  fs/btrfs/dev-replace.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
> index f9544fda38e9..e7e96e57f682 100644
> --- a/fs/btrfs/dev-replace.c
> +++ b/fs/btrfs/dev-replace.c
> @@ -730,7 +730,7 @@ static int btrfs_dev_replace_start(struct btrfs_fs_info *fs_info,
>  int btrfs_dev_replace_by_ioctl(struct btrfs_fs_info *fs_info,
>  			    struct btrfs_ioctl_dev_replace_args *args)
>  {
> -	int ret;
> +	int ret, len;
>  
>  	switch (args->start.cont_reading_from_srcdev_mode) {
>  	case BTRFS_IOCTL_DEV_REPLACE_CONT_READING_FROM_SRCDEV_MODE_ALWAYS:
> @@ -740,8 +740,10 @@ int btrfs_dev_replace_by_ioctl(struct btrfs_fs_info *fs_info,
>  		return -EINVAL;
>  	}
>  
> +	len = strnlen(args->start.tgtdev_name, BTRFS_DEVICE_PATH_NAME_MAX + 1);
>  	if ((args->start.srcdevid == 0 && args->start.srcdev_name[0] == '\0') ||
> -	    args->start.tgtdev_name[0] == '\0')
> +	    args->start.tgtdev_name[0] == '\0' ||
> +	    len == BTRFS_DEVICE_PATH_NAME_MAX + 1)

I think srcdev_name would have to be checked the same way, but instead
of strnlen I'd do memchr(name, 0, BTRFS_DEVICE_PATH_NAME_MAX). The check
for 0 in [0] is probably pointless, it's just a shortcut for an empty
buffer. We expect a valid 0-terminated string, which could be an invalid
path but that will be found out later when opening the block device.

Re: [PATCH] btrfs: fix oob Read in getname_kernel

[David Sterba]

On Wed, Jan 10, 2024 at 04:55:46PM +0100, David Sterba wrote:
> On Tue, Dec 19, 2023 at 06:19:10PM +0800, Edward Adam Davis wrote:
> > If ioctl does not pass in the correct tgtdev_name string, oob will occur because
> > "\0" cannot be found.
> > 
> > Reported-and-tested-by: syzbot+33f23b49ac24f986c9e8@syzkaller.appspotmail.com
> > Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> > ---
> >  fs/btrfs/dev-replace.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
> > index f9544fda38e9..e7e96e57f682 100644
> > --- a/fs/btrfs/dev-replace.c
> > +++ b/fs/btrfs/dev-replace.c
> > @@ -730,7 +730,7 @@ static int btrfs_dev_replace_start(struct btrfs_fs_info *fs_info,
> >  int btrfs_dev_replace_by_ioctl(struct btrfs_fs_info *fs_info,
> >  			    struct btrfs_ioctl_dev_replace_args *args)
> >  {
> > -	int ret;
> > +	int ret, len;
> >  
> >  	switch (args->start.cont_reading_from_srcdev_mode) {
> >  	case BTRFS_IOCTL_DEV_REPLACE_CONT_READING_FROM_SRCDEV_MODE_ALWAYS:
> > @@ -740,8 +740,10 @@ int btrfs_dev_replace_by_ioctl(struct btrfs_fs_info *fs_info,
> >  		return -EINVAL;
> >  	}
> >  
> > +	len = strnlen(args->start.tgtdev_name, BTRFS_DEVICE_PATH_NAME_MAX + 1);
> >  	if ((args->start.srcdevid == 0 && args->start.srcdev_name[0] == '\0') ||
> > -	    args->start.tgtdev_name[0] == '\0')
> > +	    args->start.tgtdev_name[0] == '\0' ||
> > +	    len == BTRFS_DEVICE_PATH_NAME_MAX + 1)
> 
> I think srcdev_name would have to be checked the same way, but instead
> of strnlen I'd do memchr(name, 0, BTRFS_DEVICE_PATH_NAME_MAX). The check
> for 0 in [0] is probably pointless, it's just a shortcut for an empty
> buffer. We expect a valid 0-terminated string, which could be an invalid
> path but that will be found out later when opening the block device.

Please let me know if you're going to send an updated fix. I'd like to
get this fixed to close the syzbot report but also want to give you the
credit for debugging and fix.

The preferred fix is something like that:

--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -741,6 +741,8 @@ int btrfs_dev_replace_by_ioctl(struct btrfs_fs_info *fs_info,
        if ((args->start.srcdevid == 0 && args->start.srcdev_name[0] == '\0') ||
            args->start.tgtdev_name[0] == '\0')
                return -EINVAL;
+       args->start.srcdev_name[BTRFS_PATH_NAME_MAX] = 0;
+       args->start.tgtdev_name[BTRFS_PATH_NAME_MAX] = 0;
 
        ret = btrfs_dev_replace_start(fs_info, args->start.tgtdev_name,
                                        args->start.srcdevid,

Re: [syzbot] [btrfs?] KASAN: slab-out-of-bounds Read in getname_kernel (2)

[Edward Adam Davis]

On Mon, 15 Jan 2024 20:08:25 +0100, David Sterba wrote:
> > > If ioctl does not pass in the correct tgtdev_name string, oob will occur because
> > > "\0" cannot be found.
> > >
> > > Reported-and-tested-by: syzbot+33f23b49ac24f986c9e8@syzkaller.appspotmail.com
> > > Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> > > ---
> > >  fs/btrfs/dev-replace.c | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
> > > index f9544fda38e9..e7e96e57f682 100644
> > > --- a/fs/btrfs/dev-replace.c
> > > +++ b/fs/btrfs/dev-replace.c
> > > @@ -730,7 +730,7 @@ static int btrfs_dev_replace_start(struct btrfs_fs_info *fs_info,
> > >  int btrfs_dev_replace_by_ioctl(struct btrfs_fs_info *fs_info,
> > >  			    struct btrfs_ioctl_dev_replace_args *args)
> > >  {
> > > -	int ret;
> > > +	int ret, len;
> > >
> > >  	switch (args->start.cont_reading_from_srcdev_mode) {
> > >  	case BTRFS_IOCTL_DEV_REPLACE_CONT_READING_FROM_SRCDEV_MODE_ALWAYS:
> > > @@ -740,8 +740,10 @@ int btrfs_dev_replace_by_ioctl(struct btrfs_fs_info *fs_info,
> > >  		return -EINVAL;
> > >  	}
> > >
> > > +	len = strnlen(args->start.tgtdev_name, BTRFS_DEVICE_PATH_NAME_MAX + 1);
> > >  	if ((args->start.srcdevid == 0 && args->start.srcdev_name[0] == '\0') ||
> > > -	    args->start.tgtdev_name[0] == '\0')
> > > +	    args->start.tgtdev_name[0] == '\0' ||
> > > +	    len == BTRFS_DEVICE_PATH_NAME_MAX + 1)
> >
> > I think srcdev_name would have to be checked the same way, but instead
> > of strnlen I'd do memchr(name, 0, BTRFS_DEVICE_PATH_NAME_MAX). The check
> > for 0 in [0] is probably pointless, it's just a shortcut for an empty
> > buffer. We expect a valid 0-terminated string, which could be an invalid
> > path but that will be found out later when opening the block device.
> 
> Please let me know if you're going to send an updated fix. I'd like to
> get this fixed to close the syzbot report but also want to give you the
> credit for debugging and fix.
> 
> The preferred fix is something like that:
> 
> --- a/fs/btrfs/dev-replace.c
> +++ b/fs/btrfs/dev-replace.c
> @@ -741,6 +741,8 @@ int btrfs_dev_replace_by_ioctl(struct btrfs_fs_info *fs_info,
>         if ((args->start.srcdevid == 0 && args->start.srcdev_name[0] == '\0') ||
>             args->start.tgtdev_name[0] == '\0')
>                 return -EINVAL;
> +       args->start.srcdev_name[BTRFS_PATH_NAME_MAX] = 0;
> +       args->start.tgtdev_name[BTRFS_PATH_NAME_MAX] = 0;
This is not correct,
1. The maximum length of tgtdev_name is BTRFS_DEVICE_PATH_NAME_MAX + 1
2. strnlen should be used to confirm the presence of \0 in tgtdev_name
3. Input values should not be subjectively updated
4. The current issue only involves tgtdev_name
> 
>         ret = btrfs_dev_replace_start(fs_info, args->start.tgtdev_name,
>                                         args->start.srcdevid,

Re: [syzbot] [btrfs?] KASAN: slab-out-of-bounds Read in getname_kernel (2)

[David Sterba]

On Tue, Jan 16, 2024 at 09:09:47AM +0800, Edward Adam Davis wrote:
> On Mon, 15 Jan 2024 20:08:25 +0100, David Sterba wrote:
> > > > If ioctl does not pass in the correct tgtdev_name string, oob will occur because
> > > > "\0" cannot be found.
> > > >
> > > > Reported-and-tested-by: syzbot+33f23b49ac24f986c9e8@syzkaller.appspotmail.com
> > > > Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> > > > ---
> > > >  fs/btrfs/dev-replace.c | 6 ++++--
> > > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
> > > > index f9544fda38e9..e7e96e57f682 100644
> > > > --- a/fs/btrfs/dev-replace.c
> > > > +++ b/fs/btrfs/dev-replace.c
> > > > @@ -730,7 +730,7 @@ static int btrfs_dev_replace_start(struct btrfs_fs_info *fs_info,
> > > >  int btrfs_dev_replace_by_ioctl(struct btrfs_fs_info *fs_info,
> > > >  			    struct btrfs_ioctl_dev_replace_args *args)
> > > >  {
> > > > -	int ret;
> > > > +	int ret, len;
> > > >
> > > >  	switch (args->start.cont_reading_from_srcdev_mode) {
> > > >  	case BTRFS_IOCTL_DEV_REPLACE_CONT_READING_FROM_SRCDEV_MODE_ALWAYS:
> > > > @@ -740,8 +740,10 @@ int btrfs_dev_replace_by_ioctl(struct btrfs_fs_info *fs_info,
> > > >  		return -EINVAL;
> > > >  	}
> > > >
> > > > +	len = strnlen(args->start.tgtdev_name, BTRFS_DEVICE_PATH_NAME_MAX + 1);
> > > >  	if ((args->start.srcdevid == 0 && args->start.srcdev_name[0] == '\0') ||
> > > > -	    args->start.tgtdev_name[0] == '\0')
> > > > +	    args->start.tgtdev_name[0] == '\0' ||
> > > > +	    len == BTRFS_DEVICE_PATH_NAME_MAX + 1)
> > >
> > > I think srcdev_name would have to be checked the same way, but instead
> > > of strnlen I'd do memchr(name, 0, BTRFS_DEVICE_PATH_NAME_MAX). The check
> > > for 0 in [0] is probably pointless, it's just a shortcut for an empty
> > > buffer. We expect a valid 0-terminated string, which could be an invalid
> > > path but that will be found out later when opening the block device.
> > 
> > Please let me know if you're going to send an updated fix. I'd like to
> > get this fixed to close the syzbot report but also want to give you the
> > credit for debugging and fix.
> > 
> > The preferred fix is something like that:
> > 
> > --- a/fs/btrfs/dev-replace.c
> > +++ b/fs/btrfs/dev-replace.c
> > @@ -741,6 +741,8 @@ int btrfs_dev_replace_by_ioctl(struct btrfs_fs_info *fs_info,
> >         if ((args->start.srcdevid == 0 && args->start.srcdev_name[0] == '\0') ||
> >             args->start.tgtdev_name[0] == '\0')
> >                 return -EINVAL;
> > +       args->start.srcdev_name[BTRFS_PATH_NAME_MAX] = 0;
> > +       args->start.tgtdev_name[BTRFS_PATH_NAME_MAX] = 0;
> This is not correct,
> 1. The maximum length of tgtdev_name is BTRFS_DEVICE_PATH_NAME_MAX + 1

Yes, so if the array size is N + 1 then writing to offset N is valid.
It's a bit confusing that the paths using BTRFS_PATH_NAME_MAX are +1
bigger so it's not folling the patterns using N as the limit.

> 2. strnlen should be used to confirm the presence of \0 in tgtdev_name

Yes, this would work, with the slight difference that memchr looks for
the 0 character regardless of any other, while strnlen also looks for
any intermediate 0. memchr is an optimization, for input parameter
validation it does not matter.

> 3. Input values should not be subjectively updated

Yeah, this is indeed subjective, I propsed that because we already do
that for subvolume ioctls. This probably would never show up in
practice, the paths are not that long and even if the real linux limit
is PATH_MAX (4096) and BTRFS_PATH_NAME_MAX was originally set to a lower
value it's still enough for everybody.

From practiacal perspective I don't see any difference between
overwriting the last place of NUL or checking it by strnlen/memchr.

> 4. The current issue only involves tgtdev_name

Right, that needs to be fixed. With bugs like that it's always good to
look around for similar cases or audit everything of similar pattern,
here it's an ioctl taking a user-specified path. If the target path is
fixed, we need the source path fixed too. It can be a separate patch, no
problem.

Re: [syzbot] [btrfs?] KASAN: slab-out-of-bounds Read in getname_kernel (2)

[David Sterba]

On Tue, Jan 16, 2024 at 09:09:47AM +0800, Edward Adam Davis wrote:
> On Mon, 15 Jan 2024 20:08:25 +0100, David Sterba wrote:
> > > > If ioctl does not pass in the correct tgtdev_name string, oob will occur because
> > > > "\0" cannot be found.
> > > >
> > > > Reported-and-tested-by: syzbot+33f23b49ac24f986c9e8@syzkaller.appspotmail.com
> > > > Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> > > > ---
> > > >  fs/btrfs/dev-replace.c | 6 ++++--
> > > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
> > > > index f9544fda38e9..e7e96e57f682 100644
> > > > --- a/fs/btrfs/dev-replace.c
> > > > +++ b/fs/btrfs/dev-replace.c
> > > > @@ -730,7 +730,7 @@ static int btrfs_dev_replace_start(struct btrfs_fs_info *fs_info,
> > > >  int btrfs_dev_replace_by_ioctl(struct btrfs_fs_info *fs_info,
> > > >  			    struct btrfs_ioctl_dev_replace_args *args)
> > > >  {
> > > > -	int ret;
> > > > +	int ret, len;
> > > >
> > > >  	switch (args->start.cont_reading_from_srcdev_mode) {
> > > >  	case BTRFS_IOCTL_DEV_REPLACE_CONT_READING_FROM_SRCDEV_MODE_ALWAYS:
> > > > @@ -740,8 +740,10 @@ int btrfs_dev_replace_by_ioctl(struct btrfs_fs_info *fs_info,
> > > >  		return -EINVAL;
> > > >  	}
> > > >
> > > > +	len = strnlen(args->start.tgtdev_name, BTRFS_DEVICE_PATH_NAME_MAX + 1);
> > > >  	if ((args->start.srcdevid == 0 && args->start.srcdev_name[0] == '\0') ||
> > > > -	    args->start.tgtdev_name[0] == '\0')
> > > > +	    args->start.tgtdev_name[0] == '\0' ||
> > > > +	    len == BTRFS_DEVICE_PATH_NAME_MAX + 1)
> > >
> > > I think srcdev_name would have to be checked the same way, but instead
> > > of strnlen I'd do memchr(name, 0, BTRFS_DEVICE_PATH_NAME_MAX). The check
> > > for 0 in [0] is probably pointless, it's just a shortcut for an empty
> > > buffer. We expect a valid 0-terminated string, which could be an invalid
> > > path but that will be found out later when opening the block device.
> > 
> > Please let me know if you're going to send an updated fix. I'd like to
> > get this fixed to close the syzbot report but also want to give you the
> > credit for debugging and fix.
> > 
> > The preferred fix is something like that:
> > 
> > --- a/fs/btrfs/dev-replace.c
> > +++ b/fs/btrfs/dev-replace.c
> > @@ -741,6 +741,8 @@ int btrfs_dev_replace_by_ioctl(struct btrfs_fs_info *fs_info,
> >         if ((args->start.srcdevid == 0 && args->start.srcdev_name[0] == '\0') ||
> >             args->start.tgtdev_name[0] == '\0')
> >                 return -EINVAL;
> > +       args->start.srcdev_name[BTRFS_PATH_NAME_MAX] = 0;
> > +       args->start.tgtdev_name[BTRFS_PATH_NAME_MAX] = 0;
> This is not correct,
> 1. The maximum length of tgtdev_name is BTRFS_DEVICE_PATH_NAME_MAX + 1
> 2. strnlen should be used to confirm the presence of \0 in tgtdev_name
> 3. Input values should not be subjectively updated

Regarding that point I agree it's not the best handling and could be
confusing. There are multiple instances of that in the ioctl callbacks
so the proper fix is to add a helper doing the validity check (either
strnlen or memchr) and then use it.

The pattern to look for is "vol_args->name[BTRFS_PATH_NAME_MAX] = '\0';"
in ioctl.c (at least).

Let me know if you'd want to implement that.