[syzbot] [bcachefs?] KASAN: slab-out-of-bounds Write in bch2_dev_journal_init

[syzbot] [bcachefs?] KASAN: slab-out-of-bounds Write in bch2_dev_journal_init

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit:    c3f2d783a459 Merge tag 'mm-hotfixes-stable-2024-08-17-19-3..
git tree:       upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=100aeafd980000
kernel config:  https://syzkaller.appspot.com/x/.config?x=7229118d88b4a71b
dashboard link: https://syzkaller.appspot.com/bug?extid=47ecc948aadfb2ab3efc
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=15b1ba05980000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=1315f9f5980000

Downloadable assets:
disk image (non-bootable): https://storage.googleapis.com/syzbot-assets/7bc7510fe41f/non_bootable_disk-c3f2d783.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/4d927f7c3cfd/vmlinux-c3f2d783.xz
kernel image: https://storage.googleapis.com/syzbot-assets/ea54bdfad24b/bzImage-c3f2d783.xz
mounted in repro: https://storage.googleapis.com/syzbot-assets/c595598cced9/mount_0.gz

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+47ecc948aadfb2ab3efc@syzkaller.appspotmail.com

loop0: detected capacity change from 0 to 32768
==================================================================
BUG: KASAN: slab-out-of-bounds in bch2_dev_journal_init+0x7a1/0xb20 fs/bcachefs/journal.c:1344
Write of size 8 at addr ffff888037acd1b0 by task syz-executor526/5092

CPU: 0 UID: 0 PID: 5092 Comm: syz-executor526 Not tainted 6.11.0-rc3-syzkaller-00338-gc3f2d783a459 #0
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.3-debian-1.16.3-2~bpo12+1 04/01/2014
Call Trace:
 <TASK>
 __dump_stack lib/dump_stack.c:93 [inline]
 dump_stack_lvl+0x241/0x360 lib/dump_stack.c:119
 print_address_description mm/kasan/report.c:377 [inline]
 print_report+0x169/0x550 mm/kasan/report.c:488
 kasan_report+0x143/0x180 mm/kasan/report.c:601
 bch2_dev_journal_init+0x7a1/0xb20 fs/bcachefs/journal.c:1344
 __bch2_dev_attach_bdev+0x217/0x340 fs/bcachefs/super.c:1390
 bch2_dev_attach_bdev+0x2a8/0x6f0 fs/bcachefs/super.c:1420
 bch2_fs_open+0x97c/0xdf0 fs/bcachefs/super.c:2122
 bch2_fs_get_tree+0x731/0x1700 fs/bcachefs/fs.c:1933
 vfs_get_tree+0x90/0x2a0 fs/super.c:1800
 do_new_mount+0x2be/0xb40 fs/namespace.c:3472
 do_mount fs/namespace.c:3812 [inline]
 __do_sys_mount fs/namespace.c:4020 [inline]
 __se_sys_mount+0x2d6/0x3c0 fs/namespace.c:3997
 do_syscall_x64 arch/x86/entry/common.c:52 [inline]
 do_syscall_64+0xf3/0x230 arch/x86/entry/common.c:83
 entry_SYSCALL_64_after_hwframe+0x77/0x7f
RIP: 0033:0x7fbd7d15ddea
Code: d8 64 89 02 48 c7 c0 ff ff ff ff eb a6 e8 5e 04 00 00 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 49 89 ca b8 a5 00 00 00 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:00007ffefd8bb078 EFLAGS: 00000282 ORIG_RAX: 00000000000000a5
RAX: ffffffffffffffda RBX: 00007ffefd8bb090 RCX: 00007fbd7d15ddea
RDX: 00000000200058c0 RSI: 0000000020005900 RDI: 00007ffefd8bb090
RBP: 0000000000000004 R08: 00007ffefd8bb0d0 R09: 00000000000058a7
R10: 0000000000000000 R11: 0000000000000282 R12: 0000000000000000
R13: 00007ffefd8bb0d0 R14: 0000000000000003 R15: 0000000001000000
 </TASK>

Allocated by task 5092:
 kasan_save_stack mm/kasan/common.c:47 [inline]
 kasan_save_track+0x3f/0x80 mm/kasan/common.c:68
 poison_kmalloc_redzone mm/kasan/common.c:370 [inline]
 __kasan_kmalloc+0x98/0xb0 mm/kasan/common.c:387
 kasan_kmalloc include/linux/kasan.h:211 [inline]
 __do_kmalloc_node mm/slub.c:4158 [inline]
 __kmalloc_noprof+0x1fc/0x400 mm/slub.c:4170
 kmalloc_noprof include/linux/slab.h:685 [inline]
 kmalloc_array_noprof include/linux/slab.h:726 [inline]
 bch2_dev_journal_init+0x647/0xb20 fs/bcachefs/journal.c:1334
 __bch2_dev_attach_bdev+0x217/0x340 fs/bcachefs/super.c:1390
 bch2_dev_attach_bdev+0x2a8/0x6f0 fs/bcachefs/super.c:1420
 bch2_fs_open+0x97c/0xdf0 fs/bcachefs/super.c:2122
 bch2_fs_get_tree+0x731/0x1700 fs/bcachefs/fs.c:1933
 vfs_get_tree+0x90/0x2a0 fs/super.c:1800
 do_new_mount+0x2be/0xb40 fs/namespace.c:3472
 do_mount fs/namespace.c:3812 [inline]
 __do_sys_mount fs/namespace.c:4020 [inline]
 __se_sys_mount+0x2d6/0x3c0 fs/namespace.c:3997
 do_syscall_x64 arch/x86/entry/common.c:52 [inline]
 do_syscall_64+0xf3/0x230 arch/x86/entry/common.c:83
 entry_SYSCALL_64_after_hwframe+0x77/0x7f

The buggy address belongs to the object at ffff888037acd180
 which belongs to the cache kmalloc-64 of size 64
The buggy address is located 0 bytes to the right of
 allocated 48-byte region [ffff888037acd180, ffff888037acd1b0)

The buggy address belongs to the physical page:
page: refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x37acd
anon flags: 0x4fff00000000000(node=1|zone=1|lastcpupid=0x7ff)
page_type: 0xfdffffff(slab)
raw: 04fff00000000000 ffff8880158418c0 ffffea0000e1eb00 dead000000000005
raw: 0000000000000000 0000000080200020 00000001fdffffff 0000000000000000
page dumped because: kasan: bad access detected
page_owner tracks the page as allocated
page last allocated via order 0, migratetype Unmovable, gfp_mask 0x52c40(GFP_NOFS|__GFP_NOWARN|__GFP_NORETRY|__GFP_COMP), pid 4863, tgid 4863 (cmp), ts 54291412922, free_ts 54229723596
 set_page_owner include/linux/page_owner.h:32 [inline]
 post_alloc_hook+0x1f3/0x230 mm/page_alloc.c:1493
 prep_new_page mm/page_alloc.c:1501 [inline]
 get_page_from_freelist+0x2e4c/0x2f10 mm/page_alloc.c:3439
 __alloc_pages_noprof+0x256/0x6c0 mm/page_alloc.c:4695
 __alloc_pages_node_noprof include/linux/gfp.h:269 [inline]
 alloc_pages_node_noprof include/linux/gfp.h:296 [inline]
 alloc_slab_page+0x5f/0x120 mm/slub.c:2321
 allocate_slab+0x5a/0x2f0 mm/slub.c:2484
 new_slab mm/slub.c:2537 [inline]
 ___slab_alloc+0xcd1/0x14b0 mm/slub.c:3723
 __slab_alloc+0x58/0xa0 mm/slub.c:3813
 __slab_alloc_node mm/slub.c:3866 [inline]
 slab_alloc_node mm/slub.c:4025 [inline]
 __do_kmalloc_node mm/slub.c:4157 [inline]
 __kmalloc_noprof+0x25a/0x400 mm/slub.c:4170
 kmalloc_noprof include/linux/slab.h:685 [inline]
 tomoyo_add_entry security/tomoyo/common.c:2023 [inline]
 tomoyo_supervisor+0xe0d/0x11f0 security/tomoyo/common.c:2095
 tomoyo_audit_path_log security/tomoyo/file.c:168 [inline]
 tomoyo_path_permission+0x243/0x360 security/tomoyo/file.c:587
 tomoyo_check_open_permission+0x2fb/0x500 security/tomoyo/file.c:777
 security_file_open+0x6a/0x750 security/security.c:2988
 do_dentry_open+0x38e/0x1440 fs/open.c:946
 vfs_open+0x3e/0x330 fs/open.c:1089
 do_open fs/namei.c:3727 [inline]
 path_openat+0x2b3e/0x3470 fs/namei.c:3886
 do_filp_open+0x235/0x490 fs/namei.c:3913
page last free pid 4862 tgid 4862 stack trace:
 reset_page_owner include/linux/page_owner.h:25 [inline]
 free_pages_prepare mm/page_alloc.c:1094 [inline]
 free_unref_page+0xd22/0xea0 mm/page_alloc.c:2612
 discard_slab mm/slub.c:2583 [inline]
 __put_partials+0xeb/0x130 mm/slub.c:3051
 put_cpu_partial+0x17c/0x250 mm/slub.c:3126
 __slab_free+0x2ea/0x3d0 mm/slub.c:4343
 qlink_free mm/kasan/quarantine.c:163 [inline]
 qlist_free_all+0x9e/0x140 mm/kasan/quarantine.c:179
 kasan_quarantine_reduce+0x14f/0x170 mm/kasan/quarantine.c:286
 __kasan_slab_alloc+0x23/0x80 mm/kasan/common.c:322
 kasan_slab_alloc include/linux/kasan.h:201 [inline]
 slab_post_alloc_hook mm/slub.c:3988 [inline]
 slab_alloc_node mm/slub.c:4037 [inline]
 kmem_cache_alloc_noprof+0x135/0x2a0 mm/slub.c:4044
 getname_flags+0xb7/0x540 fs/namei.c:139
 vfs_fstatat+0x12c/0x190 fs/stat.c:340
 __do_sys_newfstatat fs/stat.c:505 [inline]
 __se_sys_newfstatat fs/stat.c:499 [inline]
 __x64_sys_newfstatat+0x11d/0x1a0 fs/stat.c:499
 do_syscall_x64 arch/x86/entry/common.c:52 [inline]
 do_syscall_64+0xf3/0x230 arch/x86/entry/common.c:83
 entry_SYSCALL_64_after_hwframe+0x77/0x7f

Memory state around the buggy address:
 ffff888037acd080: 00 00 00 00 00 00 fc fc fc fc fc fc fc fc fc fc
 ffff888037acd100: fa fb fb fb fb fb fb fb fc fc fc fc fc fc fc fc
>ffff888037acd180: 00 00 00 00 00 00 fc fc fc fc fc fc fc fc fc fc
                                     ^
 ffff888037acd200: fa fb fb fb fb fb fb fb fc fc fc fc fc fc fc fc
 ffff888037acd280: fa fb fb fb fb fb fb fb 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.

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

Re: [syzbot] Re: [syzbot] [bcachefs?] KASAN: slab-out-of-bounds Write in bch2_dev_journal_init

[syzbot]

For archival purposes, forwarding an incoming command email to
linux-kernel@vger.kernel.org.

***

Subject: Re: [syzbot] [bcachefs?] KASAN: slab-out-of-bounds Write in bch2_dev_journal_init
Author: lizhi.xu@windriver.com

unsigned may overflow

#syz test: upstream c3f2d783a459

diff --git a/fs/bcachefs/journal_types.h b/fs/bcachefs/journal_types.h
index 19183fcf7ad7..311a62a0f6c1 100644
--- a/fs/bcachefs/journal_types.h
+++ b/fs/bcachefs/journal_types.h
@@ -323,7 +323,7 @@ struct journal_device {
 	unsigned		dirty_idx_ondisk;
 	unsigned		dirty_idx;
 	unsigned		cur_idx;		/* Journal bucket we're currently writing to */
-	unsigned		nr;
+	u64			nr;
 
 	u64			*buckets;
 

Re: [syzbot] Re: [syzbot] [bcachefs?] KASAN: slab-out-of-bounds Write in bch2_dev_journal_init

[syzbot]

For archival purposes, forwarding an incoming command email to
linux-kernel@vger.kernel.org.

***

Subject: Re: [syzbot] [bcachefs?] KASAN: slab-out-of-bounds Write in bch2_dev_journal_init
Author: lizhi.xu@windriver.com

two obj are null ?

#syz test: upstream c3f2d783a459

diff --git a/fs/bcachefs/journal.c b/fs/bcachefs/journal.c
index 13669dd0e375..1219f921690a 100644
--- a/fs/bcachefs/journal.c
+++ b/fs/bcachefs/journal.c
@@ -1304,6 +1304,10 @@ int bch2_dev_journal_init(struct bch_dev *ca, struct bch_sb *sb)
 
 	ja->nr = 0;
 
+	struct printbuf buf = PRINTBUF;
+	prt_printf(&buf, "ja nr: %u, %s\n", ja->nr, journal_buckets_v2, journal_buckets, __func__);
+	printbuf_exit(&buf);
+
 	if (journal_buckets_v2) {
 		unsigned nr = bch2_sb_field_journal_v2_nr_entries(journal_buckets_v2);
 
@@ -1311,7 +1315,8 @@ int bch2_dev_journal_init(struct bch_dev *ca, struct bch_sb *sb)
 			ja->nr += le64_to_cpu(journal_buckets_v2->d[i].nr);
 	} else if (journal_buckets) {
 		ja->nr = bch2_nr_journal_buckets(journal_buckets);
-	}
+	} else
+		return -EINVAL;
 
 	ja->bucket_seq = kcalloc(ja->nr, sizeof(u64), GFP_KERNEL);
 	if (!ja->bucket_seq)

Re: [syzbot] Re: [syzbot] [bcachefs?] KASAN: slab-out-of-bounds Write in bch2_dev_journal_init

[syzbot]

For archival purposes, forwarding an incoming command email to
linux-kernel@vger.kernel.org.

***

Subject: Re: [syzbot] [bcachefs?] KASAN: slab-out-of-bounds Write in bch2_dev_journal_init
Author: lizhi.xu@windriver.com

two obj are null ?

#syz test: upstream c3f2d783a459

diff --git a/fs/bcachefs/journal.c b/fs/bcachefs/journal.c
index 13669dd0e375..d6970d834991 100644
--- a/fs/bcachefs/journal.c
+++ b/fs/bcachefs/journal.c
@@ -1307,8 +1307,18 @@ int bch2_dev_journal_init(struct bch_dev *ca, struct bch_sb *sb)
 	if (journal_buckets_v2) {
 		unsigned nr = bch2_sb_field_journal_v2_nr_entries(journal_buckets_v2);
 
-		for (unsigned i = 0; i < nr; i++)
+		for (unsigned i = 0; i < nr; i++) {
 			ja->nr += le64_to_cpu(journal_buckets_v2->d[i].nr);
+			if (le64_to_cpu(journal_buckets_v2->d[i].nr) > UINT_MAX) {
+				struct bch_fs *c = ca->fs;
+				struct printbuf buf = PRINTBUF;
+				prt_printf(&buf, "v2d[%u]: %lu overflow!\n", i,
+					le64_to_cpu(journal_buckets_v2->d[i].nr));
+				bch_info(c, "%s", buf.buf);
+				printbuf_exit(&buf);
+				return -BCH_ERR_ENOMEM_dev_journal_init;
+			}
+		}
 	} else if (journal_buckets) {
 		ja->nr = bch2_nr_journal_buckets(journal_buckets);
 	}

[PATCH] bcachefs: Fix oob in bch2_dev_journal_init

[Lizhi Xu]

When journal v2 entry nr overflow, it will cause the value of ja->nr to
be incorrect, this will result in the allocated memory to ja->buckets
being too small, leading to out of bounds access.

Reported-by: syzbot+47ecc948aadfb2ab3efc@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=47ecc948aadfb2ab3efc
Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
---
 fs/bcachefs/journal.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/fs/bcachefs/journal.c b/fs/bcachefs/journal.c
index 13669dd0e375..d4fb5a23b3f6 100644
--- a/fs/bcachefs/journal.c
+++ b/fs/bcachefs/journal.c
@@ -1307,8 +1307,18 @@ int bch2_dev_journal_init(struct bch_dev *ca, struct bch_sb *sb)
 	if (journal_buckets_v2) {
 		unsigned nr = bch2_sb_field_journal_v2_nr_entries(journal_buckets_v2);
 
-		for (unsigned i = 0; i < nr; i++)
+		for (unsigned i = 0; i < nr; i++) {
 			ja->nr += le64_to_cpu(journal_buckets_v2->d[i].nr);
+			if (le64_to_cpu(journal_buckets_v2->d[i].nr) > UINT_MAX) {
+				struct bch_fs *c = ca->fs;
+				struct printbuf buf = PRINTBUF;
+				prt_printf(&buf, "journal v2 entry d[%u].nr %lu overflow!\n", i,
+					le64_to_cpu(journal_buckets_v2->d[i].nr));
+				bch_info(c, "%s", buf.buf);
+				printbuf_exit(&buf);
+				return -BCH_ERR_ENOMEM_dev_journal_init;
+			}
+		}
 	} else if (journal_buckets) {
 		ja->nr = bch2_nr_journal_buckets(journal_buckets);
 	}
-- 
2.43.0

Re: [PATCH] bcachefs: Fix oob in bch2_dev_journal_init

[Kent Overstreet]

On Mon, Aug 19, 2024 at 02:47:54PM GMT, Lizhi Xu wrote:
> When journal v2 entry nr overflow, it will cause the value of ja->nr to
> be incorrect, this will result in the allocated memory to ja->buckets
> being too small, leading to out of bounds access.
> 
> Reported-by: syzbot+47ecc948aadfb2ab3efc@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=47ecc948aadfb2ab3efc
> Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
> ---
>  fs/bcachefs/journal.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/bcachefs/journal.c b/fs/bcachefs/journal.c
> index 13669dd0e375..d4fb5a23b3f6 100644
> --- a/fs/bcachefs/journal.c
> +++ b/fs/bcachefs/journal.c
> @@ -1307,8 +1307,18 @@ int bch2_dev_journal_init(struct bch_dev *ca, struct bch_sb *sb)
>  	if (journal_buckets_v2) {
>  		unsigned nr = bch2_sb_field_journal_v2_nr_entries(journal_buckets_v2);
>  
> -		for (unsigned i = 0; i < nr; i++)
> +		for (unsigned i = 0; i < nr; i++) {
>  			ja->nr += le64_to_cpu(journal_buckets_v2->d[i].nr);
> +			if (le64_to_cpu(journal_buckets_v2->d[i].nr) > UINT_MAX) {
> +				struct bch_fs *c = ca->fs;
> +				struct printbuf buf = PRINTBUF;
> +				prt_printf(&buf, "journal v2 entry d[%u].nr %lu overflow!\n", i,
> +					le64_to_cpu(journal_buckets_v2->d[i].nr));
> +				bch_info(c, "%s", buf.buf);
> +				printbuf_exit(&buf);
> +				return -BCH_ERR_ENOMEM_dev_journal_init;
> +			}
> +		}

this should be done in the validation code

Re: [PATCH] bcachefs: Fix oob in bch2_dev_journal_init

[kernel test robot]

Hi Lizhi,

kernel test robot noticed the following build warnings:

[auto build test WARNING on linus/master]
[also build test WARNING on v6.11-rc4 next-20240819]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Lizhi-Xu/bcachefs-Fix-oob-in-bch2_dev_journal_init/20240819-145031
base:   linus/master
patch link:    https://lore.kernel.org/r/20240819064754.35606-1-lizhi.xu%40windriver.com
patch subject: [PATCH] bcachefs: Fix oob in bch2_dev_journal_init
config: arm-randconfig-003-20240819 (https://download.01.org/0day-ci/archive/20240819/202408192244.CGhqCzQ3-lkp@intel.com/config)
compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240819/202408192244.CGhqCzQ3-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408192244.CGhqCzQ3-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> fs/bcachefs/journal.c:1317:6: warning: format specifies type 'unsigned long' but the argument has type '__u64' (aka 'unsigned long long') [-Wformat]
    1316 |                                 prt_printf(&buf, "journal v2 entry d[%u].nr %lu overflow!\n", i,
         |                                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
         |                                                                             %llu
    1317 |                                         le64_to_cpu(journal_buckets_v2->d[i].nr));
         |                                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/byteorder/generic.h:87:21: note: expanded from macro 'le64_to_cpu'
      87 | #define le64_to_cpu __le64_to_cpu
         |                     ^
   include/uapi/linux/byteorder/little_endian.h:33:26: note: expanded from macro '__le64_to_cpu'
      33 | #define __le64_to_cpu(x) ((__force __u64)(__le64)(x))
         |                          ^
   fs/bcachefs/util.h:78:54: note: expanded from macro 'prt_printf'
      78 | #define prt_printf(_out, ...)           bch2_prt_printf(_out, __VA_ARGS__)
         |                                                               ^~~~~~~~~~~
   1 warning generated.


vim +1317 fs/bcachefs/journal.c

  1297	
  1298	int bch2_dev_journal_init(struct bch_dev *ca, struct bch_sb *sb)
  1299	{
  1300		struct journal_device *ja = &ca->journal;
  1301		struct bch_sb_field_journal *journal_buckets =
  1302			bch2_sb_field_get(sb, journal);
  1303		struct bch_sb_field_journal_v2 *journal_buckets_v2 =
  1304			bch2_sb_field_get(sb, journal_v2);
  1305	
  1306		ja->nr = 0;
  1307	
  1308		if (journal_buckets_v2) {
  1309			unsigned nr = bch2_sb_field_journal_v2_nr_entries(journal_buckets_v2);
  1310	
  1311			for (unsigned i = 0; i < nr; i++) {
  1312				ja->nr += le64_to_cpu(journal_buckets_v2->d[i].nr);
  1313				if (le64_to_cpu(journal_buckets_v2->d[i].nr) > UINT_MAX) {
  1314					struct bch_fs *c = ca->fs;
  1315					struct printbuf buf = PRINTBUF;
  1316					prt_printf(&buf, "journal v2 entry d[%u].nr %lu overflow!\n", i,
> 1317						le64_to_cpu(journal_buckets_v2->d[i].nr));
  1318					bch_info(c, "%s", buf.buf);
  1319					printbuf_exit(&buf);
  1320					return -BCH_ERR_ENOMEM_dev_journal_init;
  1321				}
  1322			}
  1323		} else if (journal_buckets) {
  1324			ja->nr = bch2_nr_journal_buckets(journal_buckets);
  1325		}
  1326	
  1327		ja->bucket_seq = kcalloc(ja->nr, sizeof(u64), GFP_KERNEL);
  1328		if (!ja->bucket_seq)
  1329			return -BCH_ERR_ENOMEM_dev_journal_init;
  1330	
  1331		unsigned nr_bvecs = DIV_ROUND_UP(JOURNAL_ENTRY_SIZE_MAX, PAGE_SIZE);
  1332	
  1333		for (unsigned i = 0; i < ARRAY_SIZE(ja->bio); i++) {
  1334			ja->bio[i] = kmalloc(struct_size(ja->bio[i], bio.bi_inline_vecs,
  1335					     nr_bvecs), GFP_KERNEL);
  1336			if (!ja->bio[i])
  1337				return -BCH_ERR_ENOMEM_dev_journal_init;
  1338	
  1339			ja->bio[i]->ca = ca;
  1340			ja->bio[i]->buf_idx = i;
  1341			bio_init(&ja->bio[i]->bio, NULL, ja->bio[i]->bio.bi_inline_vecs, nr_bvecs, 0);
  1342		}
  1343	
  1344		ja->buckets = kcalloc(ja->nr, sizeof(u64), GFP_KERNEL);
  1345		if (!ja->buckets)
  1346			return -BCH_ERR_ENOMEM_dev_journal_init;
  1347	
  1348		if (journal_buckets_v2) {
  1349			unsigned nr = bch2_sb_field_journal_v2_nr_entries(journal_buckets_v2);
  1350			unsigned dst = 0;
  1351	
  1352			for (unsigned i = 0; i < nr; i++)
  1353				for (unsigned j = 0; j < le64_to_cpu(journal_buckets_v2->d[i].nr); j++)
  1354					ja->buckets[dst++] =
  1355						le64_to_cpu(journal_buckets_v2->d[i].start) + j;
  1356		} else if (journal_buckets) {
  1357			for (unsigned i = 0; i < ja->nr; i++)
  1358				ja->buckets[i] = le64_to_cpu(journal_buckets->buckets[i]);
  1359		}
  1360	
  1361		return 0;
  1362	}
  1363	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH] bcachefs: Fix oob in bch2_dev_journal_init

[kernel test robot]

Hi Lizhi,

kernel test robot noticed the following build warnings:

[auto build test WARNING on linus/master]
[also build test WARNING on v6.11-rc4 next-20240819]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Lizhi-Xu/bcachefs-Fix-oob-in-bch2_dev_journal_init/20240819-145031
base:   linus/master
patch link:    https://lore.kernel.org/r/20240819064754.35606-1-lizhi.xu%40windriver.com
patch subject: [PATCH] bcachefs: Fix oob in bch2_dev_journal_init
config: arc-randconfig-001-20240819 (https://download.01.org/0day-ci/archive/20240820/202408200353.I1MmR4S5-lkp@intel.com/config)
compiler: arceb-elf-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240820/202408200353.I1MmR4S5-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408200353.I1MmR4S5-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from fs/bcachefs/vstructs.h:5,
                    from fs/bcachefs/bcachefs_format.h:80,
                    from fs/bcachefs/bcachefs.h:207,
                    from fs/bcachefs/journal.c:8:
   fs/bcachefs/journal.c: In function 'bch2_dev_journal_init':
>> fs/bcachefs/journal.c:1316:50: warning: format '%lu' expects argument of type 'long unsigned int', but argument 4 has type 'long long unsigned int' [-Wformat=]
    1316 |                                 prt_printf(&buf, "journal v2 entry d[%u].nr %lu overflow!\n", i,
         |                                                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   fs/bcachefs/util.h:78:63: note: in definition of macro 'prt_printf'
      78 | #define prt_printf(_out, ...)           bch2_prt_printf(_out, __VA_ARGS__)
         |                                                               ^~~~~~~~~~~
   fs/bcachefs/journal.c:1316:79: note: format string is defined here
    1316 |                                 prt_printf(&buf, "journal v2 entry d[%u].nr %lu overflow!\n", i,
         |                                                                             ~~^
         |                                                                               |
         |                                                                               long unsigned int
         |                                                                             %llu


vim +1316 fs/bcachefs/journal.c

  1297	
  1298	int bch2_dev_journal_init(struct bch_dev *ca, struct bch_sb *sb)
  1299	{
  1300		struct journal_device *ja = &ca->journal;
  1301		struct bch_sb_field_journal *journal_buckets =
  1302			bch2_sb_field_get(sb, journal);
  1303		struct bch_sb_field_journal_v2 *journal_buckets_v2 =
  1304			bch2_sb_field_get(sb, journal_v2);
  1305	
  1306		ja->nr = 0;
  1307	
  1308		if (journal_buckets_v2) {
  1309			unsigned nr = bch2_sb_field_journal_v2_nr_entries(journal_buckets_v2);
  1310	
  1311			for (unsigned i = 0; i < nr; i++) {
  1312				ja->nr += le64_to_cpu(journal_buckets_v2->d[i].nr);
  1313				if (le64_to_cpu(journal_buckets_v2->d[i].nr) > UINT_MAX) {
  1314					struct bch_fs *c = ca->fs;
  1315					struct printbuf buf = PRINTBUF;
> 1316					prt_printf(&buf, "journal v2 entry d[%u].nr %lu overflow!\n", i,
  1317						le64_to_cpu(journal_buckets_v2->d[i].nr));
  1318					bch_info(c, "%s", buf.buf);
  1319					printbuf_exit(&buf);
  1320					return -BCH_ERR_ENOMEM_dev_journal_init;
  1321				}
  1322			}
  1323		} else if (journal_buckets) {
  1324			ja->nr = bch2_nr_journal_buckets(journal_buckets);
  1325		}
  1326	
  1327		ja->bucket_seq = kcalloc(ja->nr, sizeof(u64), GFP_KERNEL);
  1328		if (!ja->bucket_seq)
  1329			return -BCH_ERR_ENOMEM_dev_journal_init;
  1330	
  1331		unsigned nr_bvecs = DIV_ROUND_UP(JOURNAL_ENTRY_SIZE_MAX, PAGE_SIZE);
  1332	
  1333		for (unsigned i = 0; i < ARRAY_SIZE(ja->bio); i++) {
  1334			ja->bio[i] = kmalloc(struct_size(ja->bio[i], bio.bi_inline_vecs,
  1335					     nr_bvecs), GFP_KERNEL);
  1336			if (!ja->bio[i])
  1337				return -BCH_ERR_ENOMEM_dev_journal_init;
  1338	
  1339			ja->bio[i]->ca = ca;
  1340			ja->bio[i]->buf_idx = i;
  1341			bio_init(&ja->bio[i]->bio, NULL, ja->bio[i]->bio.bi_inline_vecs, nr_bvecs, 0);
  1342		}
  1343	
  1344		ja->buckets = kcalloc(ja->nr, sizeof(u64), GFP_KERNEL);
  1345		if (!ja->buckets)
  1346			return -BCH_ERR_ENOMEM_dev_journal_init;
  1347	
  1348		if (journal_buckets_v2) {
  1349			unsigned nr = bch2_sb_field_journal_v2_nr_entries(journal_buckets_v2);
  1350			unsigned dst = 0;
  1351	
  1352			for (unsigned i = 0; i < nr; i++)
  1353				for (unsigned j = 0; j < le64_to_cpu(journal_buckets_v2->d[i].nr); j++)
  1354					ja->buckets[dst++] =
  1355						le64_to_cpu(journal_buckets_v2->d[i].start) + j;
  1356		} else if (journal_buckets) {
  1357			for (unsigned i = 0; i < ja->nr; i++)
  1358				ja->buckets[i] = le64_to_cpu(journal_buckets->buckets[i]);
  1359		}
  1360	
  1361		return 0;
  1362	}
  1363	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH V2] bcachefs: Add journal v2 entry nr value check

[Lizhi Xu]

When journal v2 entry nr overflow, it will cause the value of ja->nr to
be incorrect, this will result in the allocated memory to ja->buckets
being too small, leading to out of bounds access in bch2_dev_journal_init.

Reported-by: syzbot+47ecc948aadfb2ab3efc@syzkaller.appspotmail.com
Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
---
 fs/bcachefs/journal_sb.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/fs/bcachefs/journal_sb.c b/fs/bcachefs/journal_sb.c
index db80e506e3ab..db2b2100e4e5 100644
--- a/fs/bcachefs/journal_sb.c
+++ b/fs/bcachefs/journal_sb.c
@@ -119,6 +119,11 @@ static int bch2_sb_journal_v2_validate(struct bch_sb *sb, struct bch_sb_field *f
 	for (i = 0; i < nr; i++) {
 		b[i].start = le64_to_cpu(journal->d[i].start);
 		b[i].end = b[i].start + le64_to_cpu(journal->d[i].nr);
+		if (le64_to_cpu(journal->d[i].nr) > UINT_MAX) {
+			prt_printf(err, "journal v2 entry d[%u].nr %llu overflow\n",
+				i, le64_to_cpu(journal->d[i].nr));
+			goto err;
+		}
 	}
 
 	sort(b, nr, sizeof(*b), u64_range_cmp, NULL);
-- 
2.43.0

Re: [PATCH V2] bcachefs: Add journal v2 entry nr value check

[Kent Overstreet]

On Tue, Aug 20, 2024 at 03:11:45PM GMT, Lizhi Xu wrote:
> When journal v2 entry nr overflow, it will cause the value of ja->nr to
> be incorrect, this will result in the allocated memory to ja->buckets
> being too small, leading to out of bounds access in bch2_dev_journal_init.
> 
> Reported-by: syzbot+47ecc948aadfb2ab3efc@syzkaller.appspotmail.com
> Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
> ---
>  fs/bcachefs/journal_sb.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/fs/bcachefs/journal_sb.c b/fs/bcachefs/journal_sb.c
> index db80e506e3ab..db2b2100e4e5 100644
> --- a/fs/bcachefs/journal_sb.c
> +++ b/fs/bcachefs/journal_sb.c
> @@ -119,6 +119,11 @@ static int bch2_sb_journal_v2_validate(struct bch_sb *sb, struct bch_sb_field *f
>  	for (i = 0; i < nr; i++) {
>  		b[i].start = le64_to_cpu(journal->d[i].start);
>  		b[i].end = b[i].start + le64_to_cpu(journal->d[i].nr);
> +		if (le64_to_cpu(journal->d[i].nr) > UINT_MAX) {
> +			prt_printf(err, "journal v2 entry d[%u].nr %llu overflow\n",
> +				i, le64_to_cpu(journal->d[i].nr));
> +			goto err;
> +		}

no, you need to sum up _all_ the entries and verify the total doesn't
overflow UINT_MAX

Re: [PATCH V2] bcachefs: Add journal v2 entry nr value check

[Lizhi Xu]

On Tue, 20 Aug 2024 19:34:18 -0400, Kent Overstreet wrote:
> > When journal v2 entry nr overflow, it will cause the value of ja->nr to
> > be incorrect, this will result in the allocated memory to ja->buckets
> > being too small, leading to out of bounds access in bch2_dev_journal_init.
> >
> > Reported-by: syzbot+47ecc948aadfb2ab3efc@syzkaller.appspotmail.com
> > Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
> > ---
> >  fs/bcachefs/journal_sb.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/fs/bcachefs/journal_sb.c b/fs/bcachefs/journal_sb.c
> > index db80e506e3ab..db2b2100e4e5 100644
> > --- a/fs/bcachefs/journal_sb.c
> > +++ b/fs/bcachefs/journal_sb.c
> > @@ -119,6 +119,11 @@ static int bch2_sb_journal_v2_validate(struct bch_sb *sb, struct bch_sb_field *f
> >  	for (i = 0; i < nr; i++) {
> >  		b[i].start = le64_to_cpu(journal->d[i].start);
> >  		b[i].end = b[i].start + le64_to_cpu(journal->d[i].nr);
> > +		if (le64_to_cpu(journal->d[i].nr) > UINT_MAX) {
> > +			prt_printf(err, "journal v2 entry d[%u].nr %llu overflow\n",
> > +				i, le64_to_cpu(journal->d[i].nr));
> > +			goto err;
> > +		}
> 
> no, you need to sum up _all_ the entries and verify the total doesn't
> overflow UINT_MAX
The overflow value of le64_to_cpu(journal->d[i].nr) is 18446744073709551615(for u64),
or in other words, it is -1 for s64.

Therefore, the existing check for single entry is retained, and an overflow
check for the total value of all entry is will added.

BR,
Lizhi

Re: [PATCH V2] bcachefs: Add journal v2 entry nr value check

[Kent Overstreet]

On Wed, Aug 21, 2024 at 10:33:55AM GMT, Lizhi Xu wrote:
> On Tue, 20 Aug 2024 19:34:18 -0400, Kent Overstreet wrote:
> > > When journal v2 entry nr overflow, it will cause the value of ja->nr to
> > > be incorrect, this will result in the allocated memory to ja->buckets
> > > being too small, leading to out of bounds access in bch2_dev_journal_init.
> > >
> > > Reported-by: syzbot+47ecc948aadfb2ab3efc@syzkaller.appspotmail.com
> > > Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
> > > ---
> > >  fs/bcachefs/journal_sb.c | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > >
> > > diff --git a/fs/bcachefs/journal_sb.c b/fs/bcachefs/journal_sb.c
> > > index db80e506e3ab..db2b2100e4e5 100644
> > > --- a/fs/bcachefs/journal_sb.c
> > > +++ b/fs/bcachefs/journal_sb.c
> > > @@ -119,6 +119,11 @@ static int bch2_sb_journal_v2_validate(struct bch_sb *sb, struct bch_sb_field *f
> > >  	for (i = 0; i < nr; i++) {
> > >  		b[i].start = le64_to_cpu(journal->d[i].start);
> > >  		b[i].end = b[i].start + le64_to_cpu(journal->d[i].nr);
> > > +		if (le64_to_cpu(journal->d[i].nr) > UINT_MAX) {
> > > +			prt_printf(err, "journal v2 entry d[%u].nr %llu overflow\n",
> > > +				i, le64_to_cpu(journal->d[i].nr));
> > > +			goto err;
> > > +		}
> > 
> > no, you need to sum up _all_ the entries and verify the total doesn't
> > overflow UINT_MAX
> The overflow value of le64_to_cpu(journal->d[i].nr) is 18446744073709551615(for u64),
> or in other words, it is -1 for s64.
> 
> Therefore, the existing check for single entry is retained, and an overflow
> check for the total value of all entry is will added.

No, this is completely broken.

[PATCH V3] bcachefs: Add journal v2 entry nr value check

[Lizhi Xu]

When the nr value of a signle entry or their sum overflows, it will
cause the value of ja->nr to be incorrect, this will result in the
allocated memory to ja->buckets being too small, leading to out of
bounds access in bch2_dev_journal_init.

Reported-by: syzbot+47ecc948aadfb2ab3efc@syzkaller.appspotmail.com
Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
---
 fs/bcachefs/journal_sb.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/fs/bcachefs/journal_sb.c b/fs/bcachefs/journal_sb.c
index db80e506e3ab..230ed99130e4 100644
--- a/fs/bcachefs/journal_sb.c
+++ b/fs/bcachefs/journal_sb.c
@@ -107,6 +107,7 @@ static int bch2_sb_journal_v2_validate(struct bch_sb *sb, struct bch_sb_field *f
 	unsigned nr;
 	unsigned i;
 	struct u64_range *b;
+	u64 total_nr = 0, entry_nr;
 
 	nr = bch2_sb_field_journal_v2_nr_entries(journal);
 	if (!nr)
@@ -117,8 +118,21 @@ static int bch2_sb_journal_v2_validate(struct bch_sb *sb, struct bch_sb_field *f
 		return -BCH_ERR_ENOMEM_sb_journal_v2_validate;
 
 	for (i = 0; i < nr; i++) {
+		entry_nr = le64_to_cpu(journal->d[i].nr);
+		if (entry_nr > UINT_MAX) {
+			prt_printf(err, "Journal v2 entry d[%u] nr %llu overflow\n",
+				i, entry_nr);
+			goto err;
+		}
+		total_nr += entry_nr;
 		b[i].start = le64_to_cpu(journal->d[i].start);
-		b[i].end = b[i].start + le64_to_cpu(journal->d[i].nr);
+		b[i].end = b[i].start + entry_nr;
+	}
+
+	if (total_nr > UINT_MAX) {
+		prt_printf(err, "Sum of journal v2 entries nr %llu overflow\n",
+				total_nr);
+		goto err;
 	}
 
 	sort(b, nr, sizeof(*b), u64_range_cmp, NULL);
-- 
2.43.0

Re: [PATCH V3] bcachefs: Add journal v2 entry nr value check

[Kent Overstreet]

On Wed, Aug 21, 2024 at 10:57:37AM GMT, Lizhi Xu wrote:
> When the nr value of a signle entry or their sum overflows, it will
> cause the value of ja->nr to be incorrect, this will result in the
> allocated memory to ja->buckets being too small, leading to out of
> bounds access in bch2_dev_journal_init.
> 
> Reported-by: syzbot+47ecc948aadfb2ab3efc@syzkaller.appspotmail.com
> Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
> ---
>  fs/bcachefs/journal_sb.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/bcachefs/journal_sb.c b/fs/bcachefs/journal_sb.c
> index db80e506e3ab..230ed99130e4 100644
> --- a/fs/bcachefs/journal_sb.c
> +++ b/fs/bcachefs/journal_sb.c
> @@ -107,6 +107,7 @@ static int bch2_sb_journal_v2_validate(struct bch_sb *sb, struct bch_sb_field *f
>  	unsigned nr;
>  	unsigned i;
>  	struct u64_range *b;
> +	u64 total_nr = 0, entry_nr;
>  
>  	nr = bch2_sb_field_journal_v2_nr_entries(journal);
>  	if (!nr)
> @@ -117,8 +118,21 @@ static int bch2_sb_journal_v2_validate(struct bch_sb *sb, struct bch_sb_field *f
>  		return -BCH_ERR_ENOMEM_sb_journal_v2_validate;
>  
>  	for (i = 0; i < nr; i++) {
> +		entry_nr = le64_to_cpu(journal->d[i].nr);
> +		if (entry_nr > UINT_MAX) {
> +			prt_printf(err, "Journal v2 entry d[%u] nr %llu overflow\n",
> +				i, entry_nr);
> +			goto err;
> +		}

This check is unnecessary; we know the sum can't overflow a u64 because
we're also checking that the entries are nonoverlapping.

> +		total_nr += entry_nr;
>  		b[i].start = le64_to_cpu(journal->d[i].start);
> -		b[i].end = b[i].start + le64_to_cpu(journal->d[i].nr);
> +		b[i].end = b[i].start + entry_nr;
> +	}
> +
> +	if (total_nr > UINT_MAX) {
> +		prt_printf(err, "Sum of journal v2 entries nr %llu overflow\n",
> +				total_nr);
> +		goto err;
>  	}
>  
>  	sort(b, nr, sizeof(*b), u64_range_cmp, NULL);
> -- 
> 2.43.0
> 

Re: [PATCH V3] bcachefs: Add journal v2 entry nr value check

[Lizhi Xu]

On Tue, 20 Aug 2024 23:00:05 -0400, Kent Overstreet wrote:
> > When the nr value of a signle entry or their sum overflows, it will
> > cause the value of ja->nr to be incorrect, this will result in the
> > allocated memory to ja->buckets being too small, leading to out of
> > bounds access in bch2_dev_journal_init.
> > 
> > Reported-by: syzbot+47ecc948aadfb2ab3efc@syzkaller.appspotmail.com
> > Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
> > ---
> >  fs/bcachefs/journal_sb.c | 16 +++++++++++++++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/bcachefs/journal_sb.c b/fs/bcachefs/journal_sb.c
> > index db80e506e3ab..230ed99130e4 100644
> > --- a/fs/bcachefs/journal_sb.c
> > +++ b/fs/bcachefs/journal_sb.c
> > @@ -107,6 +107,7 @@ static int bch2_sb_journal_v2_validate(struct bch_sb *sb, struct bch_sb_field *f
> >  	unsigned nr;
> >  	unsigned i;
> >  	struct u64_range *b;
> > +	u64 total_nr = 0, entry_nr;
> >  
> >  	nr = bch2_sb_field_journal_v2_nr_entries(journal);
> >  	if (!nr)
> > @@ -117,8 +118,21 @@ static int bch2_sb_journal_v2_validate(struct bch_sb *sb, struct bch_sb_field *f
> >  		return -BCH_ERR_ENOMEM_sb_journal_v2_validate;
> >  
> >  	for (i = 0; i < nr; i++) {
> > +		entry_nr = le64_to_cpu(journal->d[i].nr);
> > +		if (entry_nr > UINT_MAX) {
> > +			prt_printf(err, "Journal v2 entry d[%u] nr %llu overflow\n",
> > +				i, entry_nr);
> > +			goto err;
> > +		}
> 
> This check is unnecessary; we know the sum can't overflow a u64 because
> we're also checking that the entries are nonoverlapping.
You didn't read my previous email carefully.
In this issue, journal->d[0] is 7, journal->d[1] is 18446744073709551615,
so the sum of their u64 type values will definitely overflow.
> 
> > +		total_nr += entry_nr;
> >  		b[i].start = le64_to_cpu(journal->d[i].start);
> > -		b[i].end = b[i].start + le64_to_cpu(journal->d[i].nr);
> > +		b[i].end = b[i].start + entry_nr;
> > +	}
> > +
> > +	if (total_nr > UINT_MAX) {
> > +		prt_printf(err, "Sum of journal v2 entries nr %llu overflow\n",
> > +				total_nr);
> > +		goto err;
> >  	}
> >  
> >  	sort(b, nr, sizeof(*b), u64_range_cmp, NULL);
> > -- 

BR,
Lizhi

Re: [PATCH V3] bcachefs: Add journal v2 entry nr value check

[Kent Overstreet]

On Wed, Aug 21, 2024 at 11:10:00AM GMT, Lizhi Xu wrote:
> On Tue, 20 Aug 2024 23:00:05 -0400, Kent Overstreet wrote:
> > > When the nr value of a signle entry or their sum overflows, it will
> > > cause the value of ja->nr to be incorrect, this will result in the
> > > allocated memory to ja->buckets being too small, leading to out of
> > > bounds access in bch2_dev_journal_init.
> > > 
> > > Reported-by: syzbot+47ecc948aadfb2ab3efc@syzkaller.appspotmail.com
> > > Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
> > > ---
> > >  fs/bcachefs/journal_sb.c | 16 +++++++++++++++-
> > >  1 file changed, 15 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/fs/bcachefs/journal_sb.c b/fs/bcachefs/journal_sb.c
> > > index db80e506e3ab..230ed99130e4 100644
> > > --- a/fs/bcachefs/journal_sb.c
> > > +++ b/fs/bcachefs/journal_sb.c
> > > @@ -107,6 +107,7 @@ static int bch2_sb_journal_v2_validate(struct bch_sb *sb, struct bch_sb_field *f
> > >  	unsigned nr;
> > >  	unsigned i;
> > >  	struct u64_range *b;
> > > +	u64 total_nr = 0, entry_nr;
> > >  
> > >  	nr = bch2_sb_field_journal_v2_nr_entries(journal);
> > >  	if (!nr)
> > > @@ -117,8 +118,21 @@ static int bch2_sb_journal_v2_validate(struct bch_sb *sb, struct bch_sb_field *f
> > >  		return -BCH_ERR_ENOMEM_sb_journal_v2_validate;
> > >  
> > >  	for (i = 0; i < nr; i++) {
> > > +		entry_nr = le64_to_cpu(journal->d[i].nr);
> > > +		if (entry_nr > UINT_MAX) {
> > > +			prt_printf(err, "Journal v2 entry d[%u] nr %llu overflow\n",
> > > +				i, entry_nr);
> > > +			goto err;
> > > +		}
> > 
> > This check is unnecessary; we know the sum can't overflow a u64 because
> > we're also checking that the entries are nonoverlapping.
> You didn't read my previous email carefully.
> In this issue, journal->d[0] is 7, journal->d[1] is 18446744073709551615,
> so the sum of their u64 type values will definitely overflow.

It doesn't matter. We're already checking that the entries are
nonoverlapping, and within the range of [1, nbuckets), so total_nr can't
overflow nbuckets, much less an s64 (not that that matters).

Re: [PATCH V3] bcachefs: Add journal v2 entry nr value check

[Kent Overstreet]

On Tue, Aug 20, 2024 at 11:16:55PM GMT, Kent Overstreet wrote:
> On Wed, Aug 21, 2024 at 11:10:00AM GMT, Lizhi Xu wrote:
> > On Tue, 20 Aug 2024 23:00:05 -0400, Kent Overstreet wrote:
> > > > When the nr value of a signle entry or their sum overflows, it will
> > > > cause the value of ja->nr to be incorrect, this will result in the
> > > > allocated memory to ja->buckets being too small, leading to out of
> > > > bounds access in bch2_dev_journal_init.
> > > > 
> > > > Reported-by: syzbot+47ecc948aadfb2ab3efc@syzkaller.appspotmail.com
> > > > Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
> > > > ---
> > > >  fs/bcachefs/journal_sb.c | 16 +++++++++++++++-
> > > >  1 file changed, 15 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/fs/bcachefs/journal_sb.c b/fs/bcachefs/journal_sb.c
> > > > index db80e506e3ab..230ed99130e4 100644
> > > > --- a/fs/bcachefs/journal_sb.c
> > > > +++ b/fs/bcachefs/journal_sb.c
> > > > @@ -107,6 +107,7 @@ static int bch2_sb_journal_v2_validate(struct bch_sb *sb, struct bch_sb_field *f
> > > >  	unsigned nr;
> > > >  	unsigned i;
> > > >  	struct u64_range *b;
> > > > +	u64 total_nr = 0, entry_nr;
> > > >  
> > > >  	nr = bch2_sb_field_journal_v2_nr_entries(journal);
> > > >  	if (!nr)
> > > > @@ -117,8 +118,21 @@ static int bch2_sb_journal_v2_validate(struct bch_sb *sb, struct bch_sb_field *f
> > > >  		return -BCH_ERR_ENOMEM_sb_journal_v2_validate;
> > > >  
> > > >  	for (i = 0; i < nr; i++) {
> > > > +		entry_nr = le64_to_cpu(journal->d[i].nr);
> > > > +		if (entry_nr > UINT_MAX) {
> > > > +			prt_printf(err, "Journal v2 entry d[%u] nr %llu overflow\n",
> > > > +				i, entry_nr);
> > > > +			goto err;
> > > > +		}
> > > 
> > > This check is unnecessary; we know the sum can't overflow a u64 because
> > > we're also checking that the entries are nonoverlapping.
> > You didn't read my previous email carefully.
> > In this issue, journal->d[0] is 7, journal->d[1] is 18446744073709551615,
> > so the sum of their u64 type values will definitely overflow.
> 
> It doesn't matter. We're already checking that the entries are
> nonoverlapping, and within the range of [1, nbuckets), so total_nr can't
> overflow nbuckets, much less an s64 (not that that matters).

The check that's missing is that start + nr doesn't overflow, when we
convert to u64_ranges.

Re: [PATCH V3] bcachefs: Add journal v2 entry nr value check

[Lizhi Xu]

On Tue, 20 Aug 2024 23:16:50 -0400, Lizhi Xu wrote:
> > On Tue, 20 Aug 2024 23:00:05 -0400, Kent Overstreet wrote:
> > > > When the nr value of a signle entry or their sum overflows, it will
> > > > cause the value of ja->nr to be incorrect, this will result in the
> > > > allocated memory to ja->buckets being too small, leading to out of
> > > > bounds access in bch2_dev_journal_init.
> > > >
> > > > Reported-by: syzbot+47ecc948aadfb2ab3efc@syzkaller.appspotmail.com
> > > > Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
> > > > ---
> > > >  fs/bcachefs/journal_sb.c | 16 +++++++++++++++-
> > > >  1 file changed, 15 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/fs/bcachefs/journal_sb.c b/fs/bcachefs/journal_sb.c
> > > > index db80e506e3ab..230ed99130e4 100644
> > > > --- a/fs/bcachefs/journal_sb.c
> > > > +++ b/fs/bcachefs/journal_sb.c
> > > > @@ -107,6 +107,7 @@ static int bch2_sb_journal_v2_validate(struct bch_sb *sb, struct bch_sb_field *f
> > > >  	unsigned nr;
> > > >  	unsigned i;
> > > >  	struct u64_range *b;
> > > > +	u64 total_nr = 0, entry_nr;
> > > >
> > > >  	nr = bch2_sb_field_journal_v2_nr_entries(journal);
> > > >  	if (!nr)
> > > > @@ -117,8 +118,21 @@ static int bch2_sb_journal_v2_validate(struct bch_sb *sb, struct bch_sb_field *f
> > > >  		return -BCH_ERR_ENOMEM_sb_journal_v2_validate;
> > > >
> > > >  	for (i = 0; i < nr; i++) {
> > > > +		entry_nr = le64_to_cpu(journal->d[i].nr);
> > > > +		if (entry_nr > UINT_MAX) {
> > > > +			prt_printf(err, "Journal v2 entry d[%u] nr %llu overflow\n",
> > > > +				i, entry_nr);
> > > > +			goto err;
> > > > +		}
> > >
> > > This check is unnecessary; we know the sum can't overflow a u64 because
> > > we're also checking that the entries are nonoverlapping.
> > You didn't read my previous email carefully.
> > In this issue, journal->d[0] is 7, journal->d[1] is 18446744073709551615,
> > so the sum of their u64 type values will definitely overflow.
> 
> It doesn't matter. We're already checking that the entries are
> nonoverlapping, and within the range of [1, nbuckets), so total_nr can't
> overflow nbuckets, much less an s64 (not that that matters).
Are you sure? Or did I not express myself clearly? See the actual running results below:
le64_to_cpu(journal->d[1].nr) + le64_to_cpu(journal->d[0].nr) = 7 + 18446744073709551615 will overflow.

When a u64 overflow occurs, total_nr will not be greater than UINT_MAX,
so it is not enough to only calculate the total nr of entry to determine.

Note: u64 contains 0 ~ 18446744073709551616.

BR,
Lizhi

Re: [syzbot] [bcachefs?] KASAN: slab-out-of-bounds Write in bch2_dev_journal_init

[Kent Overstreet]

#syz fix: bcachefs: Fix missing validation in bch2_sb_journal_v2_validate()