Hello,
syzbot found the following issue on:
HEAD commit: 7367539ad4b0 Merge tag 'cxl-fixes-6.9-rc7' of git://git.ke..
git tree: upstream
console+strace: https://syzkaller.appspot.com/x/log.txt?x=11711898980000
kernel config: https://syzkaller.appspot.com/x/.config?x=d2f00edef461175
dashboard link: https://syzkaller.appspot.com/bug?extid=c48865e11e7e893ec4ab
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=1043897f180000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=145c078b180000
Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/03bd77f8af70/disk-7367539a.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/eb03a61f9582/vmlinux-7367539a.xz
kernel image: https://storage.googleapis.com/syzbot-assets/e4c5c654b571/bzImage-7367539a.xz
mounted in repro: https://storage.googleapis.com/syzbot-assets/2d31172220b2/mount_0.gz
IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: [email protected]
loop0: detected capacity change from 0 to 32768
==================================================================
BUG: KASAN: slab-out-of-bounds in bch2_sb_clean_to_text+0x17f/0x230 fs/bcachefs/sb-clean.c:298
Read of size 1 at addr ffff888023ef6004 by task syz-executor493/5073
CPU: 0 PID: 5073 Comm: syz-executor493 Not tainted 6.9.0-rc6-syzkaller-00234-g7367539ad4b0 #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 03/27/2024
Call Trace:
<TASK>
__dump_stack lib/dump_stack.c:88 [inline]
dump_stack_lvl+0x241/0x360 lib/dump_stack.c:114
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_sb_clean_to_text+0x17f/0x230 fs/bcachefs/sb-clean.c:298
bch2_sb_field_validate+0x1f7/0x2d0 fs/bcachefs/super-io.c:1211
bch2_sb_validate+0xa79/0xe10 fs/bcachefs/super-io.c:468
__bch2_read_super+0xc9a/0x1460 fs/bcachefs/super-io.c:822
bch2_fs_open+0x246/0xdf0 fs/bcachefs/super.c:2049
bch2_mount+0x71d/0x1320 fs/bcachefs/fs.c:1903
legacy_get_tree+0xee/0x190 fs/fs_context.c:662
vfs_get_tree+0x90/0x2a0 fs/super.c:1779
do_new_mount+0x2be/0xb40 fs/namespace.c:3352
do_mount fs/namespace.c:3692 [inline]
__do_sys_mount fs/namespace.c:3898 [inline]
__se_sys_mount+0x2d9/0x3c0 fs/namespace.c:3875
do_syscall_x64 arch/x86/entry/common.c:52 [inline]
do_syscall_64+0xf5/0x240 arch/x86/entry/common.c:83
entry_SYSCALL_64_after_hwframe+0x77/0x7f
RIP: 0033:0x7f2a126618fa
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:00007ffe44de6bb8 EFLAGS: 00000282 ORIG_RAX: 00000000000000a5
RAX: ffffffffffffffda RBX: 00007ffe44de6bd0 RCX: 00007f2a126618fa
RDX: 0000000020011a00 RSI: 0000000020011a40 RDI: 00007ffe44de6bd0
RBP: 0000000000000004 R08: 00007ffe44de6c10 R09: 00000000000119fe
R10: 0000000003004000 R11: 0000000000000282 R12: 0000000003004000
R13: 00007ffe44de6c10 R14: 0000000000000003 R15: 0000000001000000
</TASK>
Allocated by task 5073:
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:3966 [inline]
__kmalloc_node_track_caller+0x24e/0x4e0 mm/slub.c:3986
__do_krealloc mm/slab_common.c:1192 [inline]
krealloc+0x7d/0x120 mm/slab_common.c:1225
bch2_sb_realloc+0x2fc/0x660 fs/bcachefs/super-io.c:189
read_one_super+0x7d7/0x3a10 fs/bcachefs/super-io.c:659
__bch2_read_super+0x65a/0x1460 fs/bcachefs/super-io.c:750
bch2_fs_open+0x246/0xdf0 fs/bcachefs/super.c:2049
bch2_mount+0x71d/0x1320 fs/bcachefs/fs.c:1903
legacy_get_tree+0xee/0x190 fs/fs_context.c:662
vfs_get_tree+0x90/0x2a0 fs/super.c:1779
do_new_mount+0x2be/0xb40 fs/namespace.c:3352
do_mount fs/namespace.c:3692 [inline]
__do_sys_mount fs/namespace.c:3898 [inline]
__se_sys_mount+0x2d9/0x3c0 fs/namespace.c:3875
do_syscall_x64 arch/x86/entry/common.c:52 [inline]
do_syscall_64+0xf5/0x240 arch/x86/entry/common.c:83
entry_SYSCALL_64_after_hwframe+0x77/0x7f
The buggy address belongs to the object at ffff888023ef4000
which belongs to the cache kmalloc-8k of size 8192
The buggy address is located 4 bytes to the right of
allocated 8192-byte region [ffff888023ef4000, ffff888023ef6000)
The buggy address belongs to the physical page:
page: refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x23ef0
head: order:3 entire_mapcount:0 nr_pages_mapped:0 pincount:0
anon flags: 0xfff00000000840(slab|head|node=0|zone=1|lastcpupid=0x7ff)
page_type: 0xffffffff()
raw: 00fff00000000840 ffff888015042280 ffffea0000910a00 0000000000000005
raw: 0000000000000000 0000000080020002 00000001ffffffff 0000000000000000
head: 00fff00000000840 ffff888015042280 ffffea0000910a00 0000000000000005
head: 0000000000000000 0000000080020002 00000001ffffffff 0000000000000000
head: 00fff00000000003 ffffea00008fbc01 ffffea00008fbc48 00000000ffffffff
head: 0000000800000000 0000000000000000 00000000ffffffff 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 4736, tgid 1480517225 (sh), ts 4736, free_ts 31493460772
set_page_owner include/linux/page_owner.h:32 [inline]
post_alloc_hook+0x1ea/0x210 mm/page_alloc.c:1534
prep_new_page mm/page_alloc.c:1541 [inline]
get_page_from_freelist+0x3410/0x35b0 mm/page_alloc.c:3317
__alloc_pages+0x256/0x6c0 mm/page_alloc.c:4575
__alloc_pages_node include/linux/gfp.h:238 [inline]
alloc_pages_node include/linux/gfp.h:261 [inline]
alloc_slab_page+0x5f/0x160 mm/slub.c:2175
allocate_slab mm/slub.c:2338 [inline]
new_slab+0x84/0x2f0 mm/slub.c:2391
___slab_alloc+0xc73/0x1260 mm/slub.c:3525
__slab_alloc mm/slub.c:3610 [inline]
__slab_alloc_node mm/slub.c:3663 [inline]
slab_alloc_node mm/slub.c:3835 [inline]
kmalloc_trace+0x269/0x360 mm/slub.c:3992
kmalloc include/linux/slab.h:628 [inline]
kzalloc include/linux/slab.h:749 [inline]
tomoyo_print_bprm security/tomoyo/audit.c:26 [inline]
tomoyo_init_log+0x11ce/0x2050 security/tomoyo/audit.c:264
tomoyo_supervisor+0x38a/0x11f0 security/tomoyo/common.c:2089
tomoyo_audit_env_log security/tomoyo/environ.c:36 [inline]
tomoyo_env_perm+0x178/0x210 security/tomoyo/environ.c:63
tomoyo_environ security/tomoyo/domain.c:672 [inline]
tomoyo_find_next_domain+0x1384/0x1cf0 security/tomoyo/domain.c:878
tomoyo_bprm_check_security+0x115/0x180 security/tomoyo/tomoyo.c:102
security_bprm_check+0x65/0x90 security/security.c:1191
search_binary_handler fs/exec.c:1766 [inline]
exec_binprm fs/exec.c:1820 [inline]
bprm_execve+0xa56/0x17c0 fs/exec.c:1872
do_execveat_common+0x553/0x700 fs/exec.c:1979
do_execve fs/exec.c:2053 [inline]
__do_sys_execve fs/exec.c:2129 [inline]
__se_sys_execve fs/exec.c:2124 [inline]
__x64_sys_execve+0x92/0xb0 fs/exec.c:2124
page last free pid 4734 tgid 4734 stack trace:
reset_page_owner include/linux/page_owner.h:25 [inline]
free_pages_prepare mm/page_alloc.c:1141 [inline]
free_unref_page_prepare+0x97b/0xaa0 mm/page_alloc.c:2347
free_unref_page+0x37/0x3f0 mm/page_alloc.c:2487
discard_slab mm/slub.c:2437 [inline]
__put_partials+0xeb/0x130 mm/slub.c:2906
put_cpu_partial+0x17c/0x250 mm/slub.c:2981
__slab_free+0x2ea/0x3d0 mm/slub.c:4151
qlink_free mm/kasan/quarantine.c:163 [inline]
qlist_free_all+0x5e/0xc0 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:3798 [inline]
slab_alloc_node mm/slub.c:3845 [inline]
kmem_cache_alloc+0x174/0x340 mm/slub.c:3852
getname_flags+0xbd/0x4f0 fs/namei.c:139
do_sys_openat2+0xd2/0x1d0 fs/open.c:1400
do_sys_open fs/open.c:1421 [inline]
__do_sys_openat fs/open.c:1437 [inline]
__se_sys_openat fs/open.c:1432 [inline]
__x64_sys_openat+0x247/0x2a0 fs/open.c:1432
do_syscall_x64 arch/x86/entry/common.c:52 [inline]
do_syscall_64+0xf5/0x240 arch/x86/entry/common.c:83
entry_SYSCALL_64_after_hwframe+0x77/0x7f
Memory state around the buggy address:
ffff888023ef5f00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
ffff888023ef5f80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>ffff888023ef6000: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
^
ffff888023ef6080: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
ffff888023ef6100: 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 [email protected].
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
syzbot has bisected this issue to:
commit 03ef80b469d5d83530ce1ce15be78a40e5300f9b
Author: Kent Overstreet <[email protected]>
Date: Sat Sep 23 22:41:51 2023 +0000
bcachefs: Ignore unknown mount options
bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=15e58a70980000
start commit: 7367539ad4b0 Merge tag 'cxl-fixes-6.9-rc7' of git://git.ke..
git tree: upstream
final oops: https://syzkaller.appspot.com/x/report.txt?x=17e58a70980000
console output: https://syzkaller.appspot.com/x/log.txt?x=13e58a70980000
kernel config: https://syzkaller.appspot.com/x/.config?x=d2f00edef461175
dashboard link: https://syzkaller.appspot.com/bug?extid=c48865e11e7e893ec4ab
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1043897f180000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=145c078b180000
Reported-by: [email protected]
Fixes: 03ef80b469d5 ("bcachefs: Ignore unknown mount options")
For information about bisection process see: https://goo.gl/tpsmEJ#bisection
please test oob in bch2_sb_clean_to_text
#syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 5eb4573ea63d
diff --git a/fs/bcachefs/sb-clean.c b/fs/bcachefs/sb-clean.c
index 5980ba2563fe..02101687853e 100644
--- a/fs/bcachefs/sb-clean.c
+++ b/fs/bcachefs/sb-clean.c
@@ -285,7 +285,7 @@ static void bch2_sb_clean_to_text(struct printbuf *out, struct bch_sb *sb,
prt_newline(out);
for (entry = clean->start;
- entry != vstruct_end(&clean->field);
+ entry < vstruct_end(&clean->field);
entry = vstruct_next(entry)) {
if (entry->type == BCH_JSET_ENTRY_btree_keys &&
!entry->u64s)
Hello,
syzbot has tested the proposed patch and the reproducer did not trigger any issue:
Reported-and-tested-by: [email protected]
Tested on:
commit: 5eb4573e Merge tag 'soc-fixes-6.9-2' of git://git.kern..
git tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
console output: https://syzkaller.appspot.com/x/log.txt?x=165031a8980000
kernel config: https://syzkaller.appspot.com/x/.config?x=3d46aa9d7a44f40d
dashboard link: https://syzkaller.appspot.com/bug?extid=c48865e11e7e893ec4ab
compiler: Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40
patch: https://syzkaller.appspot.com/x/patch.diff?x=146b4a04980000
Note: testing is done by a robot and is best-effort only.
When got too small clean field, entry will never equal vstruct_end(&clean->field),
the dead loop resulted in out of bounds access.
Fixes: 12bf93a429c9 ("bcachefs: Add .to_text() methods for all superblock sections")
Fixes: a37ad1a3aba9 ("bcachefs: sb-clean.c")
Reported-and-tested-by: [email protected]
Signed-off-by: Edward Adam Davis <[email protected]>
---
fs/bcachefs/sb-clean.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/bcachefs/sb-clean.c b/fs/bcachefs/sb-clean.c
index 5980ba2563fe..02101687853e 100644
--- a/fs/bcachefs/sb-clean.c
+++ b/fs/bcachefs/sb-clean.c
@@ -285,7 +285,7 @@ static void bch2_sb_clean_to_text(struct printbuf *out, struct bch_sb *sb,
prt_newline(out);
for (entry = clean->start;
- entry != vstruct_end(&clean->field);
+ entry < vstruct_end(&clean->field);
entry = vstruct_next(entry)) {
if (entry->type == BCH_JSET_ENTRY_btree_keys &&
!entry->u64s)
--
2.43.0
On Tue, May 07, 2024 at 05:19:29PM +0800, Edward Adam Davis wrote:
> When got too small clean field, entry will never equal vstruct_end(&clean->field),
> the dead loop resulted in out of bounds access.
>
> Fixes: 12bf93a429c9 ("bcachefs: Add .to_text() methods for all superblock sections")
> Fixes: a37ad1a3aba9 ("bcachefs: sb-clean.c")
> Reported-and-tested-by: [email protected]
> Signed-off-by: Edward Adam Davis <[email protected]>
I've already got a patch up for this - the validation was missing as
well.
commit f39055220f6f98a180e3503fe05bbf9921c425c8
Author: Kent Overstreet <[email protected]>
Date: Sun May 5 22:28:00 2024 -0400
bcachefs: Add missing validation for superblock section clean
We were forgetting to check for jset entries that overrun the end of the
section - both in validate and to_text(); to_text() needs to be safe for
types that fail to validate.
Reported-by: [email protected]
Signed-off-by: Kent Overstreet <[email protected]>
diff --git a/fs/bcachefs/sb-clean.c b/fs/bcachefs/sb-clean.c
index 35ca3f138de6..194e55b11137 100644
--- a/fs/bcachefs/sb-clean.c
+++ b/fs/bcachefs/sb-clean.c
@@ -278,6 +278,17 @@ static int bch2_sb_clean_validate(struct bch_sb *sb,
return -BCH_ERR_invalid_sb_clean;
}
+ for (struct jset_entry *entry = clean->start;
+ entry != vstruct_end(&clean->field);
+ entry = vstruct_next(entry)) {
+ if ((void *) vstruct_next(entry) > vstruct_end(&clean->field)) {
+ prt_str(err, "entry type ");
+ bch2_prt_jset_entry_type(err, le16_to_cpu(entry->type));
+ prt_str(err, " overruns end of section");
+ return -BCH_ERR_invalid_sb_clean;
+ }
+ }
+
return 0;
}
@@ -295,6 +306,9 @@ static void bch2_sb_clean_to_text(struct printbuf *out, struct bch_sb *sb,
for (entry = clean->start;
entry != vstruct_end(&clean->field);
entry = vstruct_next(entry)) {
+ if ((void *) vstruct_next(entry) > vstruct_end(&clean->field))
+ break;
+
if (entry->type == BCH_JSET_ENTRY_btree_keys &&
!entry->u64s)
continue;
Hi Edward,
kernel test robot noticed the following build warnings:
[auto build test WARNING on linus/master]
[also build test WARNING on v6.9-rc7 next-20240507]
[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/Edward-Adam-Davis/bcachefs-fix-oob-in-bch2_sb_clean_to_text/20240507-172635
base: linus/master
patch link: https://lore.kernel.org/r/tencent_816D842DE96C309554E8E2ED9ACC6078120A%40qq.com
patch subject: [PATCH] bcachefs: fix oob in bch2_sb_clean_to_text
config: i386-buildonly-randconfig-005-20240508 (https://download.01.org/0day-ci/archive/20240508/[email protected]/config)
compiler: clang version 18.1.4 (https://github.com/llvm/llvm-project e6c3289804a67ea0bb6a86fadbe454dd93b8d855)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240508/[email protected]/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 <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
>> fs/bcachefs/sb-clean.c:296:13: warning: comparison of distinct pointer types ('struct jset_entry *' and 'void *') [-Wcompare-distinct-pointer-types]
296 | entry < vstruct_end(&clean->field);
| ~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
vim +296 fs/bcachefs/sb-clean.c
283
284 static void bch2_sb_clean_to_text(struct printbuf *out, struct bch_sb *sb,
285 struct bch_sb_field *f)
286 {
287 struct bch_sb_field_clean *clean = field_to_type(f, clean);
288 struct jset_entry *entry;
289
290 prt_printf(out, "flags: %x", le32_to_cpu(clean->flags));
291 prt_newline(out);
292 prt_printf(out, "journal_seq: %llu", le64_to_cpu(clean->journal_seq));
293 prt_newline(out);
294
295 for (entry = clean->start;
> 296 entry < vstruct_end(&clean->field);
297 entry = vstruct_next(entry)) {
298 if (entry->type == BCH_JSET_ENTRY_btree_keys &&
299 !entry->u64s)
300 continue;
301
302 bch2_journal_entry_to_text(out, NULL, entry);
303 prt_newline(out);
304 }
305 }
306
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi Edward,
kernel test robot noticed the following build warnings:
[auto build test WARNING on linus/master]
[also build test WARNING on v6.9-rc7 next-20240507]
[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/Edward-Adam-Davis/bcachefs-fix-oob-in-bch2_sb_clean_to_text/20240507-172635
base: linus/master
patch link: https://lore.kernel.org/r/tencent_816D842DE96C309554E8E2ED9ACC6078120A%40qq.com
patch subject: [PATCH] bcachefs: fix oob in bch2_sb_clean_to_text
config: i386-buildonly-randconfig-004-20240508 (https://download.01.org/0day-ci/archive/20240508/[email protected]/config)
compiler: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240508/[email protected]/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 <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
fs/bcachefs/sb-clean.c: In function 'bch2_sb_clean_to_text':
>> fs/bcachefs/sb-clean.c:296:20: warning: comparison of distinct pointer types lacks a cast
296 | entry < vstruct_end(&clean->field);
| ^
vim +296 fs/bcachefs/sb-clean.c
283
284 static void bch2_sb_clean_to_text(struct printbuf *out, struct bch_sb *sb,
285 struct bch_sb_field *f)
286 {
287 struct bch_sb_field_clean *clean = field_to_type(f, clean);
288 struct jset_entry *entry;
289
290 prt_printf(out, "flags: %x", le32_to_cpu(clean->flags));
291 prt_newline(out);
292 prt_printf(out, "journal_seq: %llu", le64_to_cpu(clean->journal_seq));
293 prt_newline(out);
294
295 for (entry = clean->start;
> 296 entry < vstruct_end(&clean->field);
297 entry = vstruct_next(entry)) {
298 if (entry->type == BCH_JSET_ENTRY_btree_keys &&
299 !entry->u64s)
300 continue;
301
302 bch2_journal_entry_to_text(out, NULL, entry);
303 prt_newline(out);
304 }
305 }
306
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On Tue, 7 May 2024 10:14:22 -0400, Kent Overstreet wrote:
> > When got too small clean field, entry will never equal vstruct_end(&clean->field),
> > the dead loop resulted in out of bounds access.
> >
> > Fixes: 12bf93a429c9 ("bcachefs: Add .to_text() methods for all superblock sections")
> > Fixes: a37ad1a3aba9 ("bcachefs: sb-clean.c")
> > Reported-and-tested-by: [email protected]
> > Signed-off-by: Edward Adam Davis <[email protected]>
>
> I've already got a patch up for this - the validation was missing as
> well.
>
> commit f39055220f6f98a180e3503fe05bbf9921c425c8
> Author: Kent Overstreet <[email protected]>
> Date: Sun May 5 22:28:00 2024 -0400
>
> bcachefs: Add missing validation for superblock section clean
>
> We were forgetting to check for jset entries that overrun the end of the
> section - both in validate and to_text(); to_text() needs to be safe for
> types that fail to validate.
>
> Reported-by: [email protected]
> Signed-off-by: Kent Overstreet <[email protected]>
>
> diff --git a/fs/bcachefs/sb-clean.c b/fs/bcachefs/sb-clean.c
> index 35ca3f138de6..194e55b11137 100644
> --- a/fs/bcachefs/sb-clean.c
> +++ b/fs/bcachefs/sb-clean.c
> @@ -278,6 +278,17 @@ static int bch2_sb_clean_validate(struct bch_sb *sb,
> return -BCH_ERR_invalid_sb_clean;
> }
>
> + for (struct jset_entry *entry = clean->start;
> + entry != vstruct_end(&clean->field);
> + entry = vstruct_next(entry)) {
> + if ((void *) vstruct_next(entry) > vstruct_end(&clean->field)) {
> + prt_str(err, "entry type ");
> + bch2_prt_jset_entry_type(err, le16_to_cpu(entry->type));
> + prt_str(err, " overruns end of section");
> + return -BCH_ERR_invalid_sb_clean;
> + }
> + }
> +
The original judgment here is sufficient, there is no need to add this section of inspection.
> return 0;
> }
>
> @@ -295,6 +306,9 @@ static void bch2_sb_clean_to_text(struct printbuf *out, struct bch_sb *sb,
> for (entry = clean->start;
> entry != vstruct_end(&clean->field);
> entry = vstruct_next(entry)) {
> + if ((void *) vstruct_next(entry) > vstruct_end(&clean->field))
> + break;
> +
The same check has already been done in bch2_sb_clean_validate(), so it is unnecessary to redo it here.
> if (entry->type == BCH_JSET_ENTRY_btree_keys &&
> !entry->u64s)
> continue;
On Wed, May 08, 2024 at 08:49:39AM +0800, Edward Adam Davis wrote:
> On Tue, 7 May 2024 10:14:22 -0400, Kent Overstreet wrote:
> > > When got too small clean field, entry will never equal vstruct_end(&clean->field),
> > > the dead loop resulted in out of bounds access.
> > >
> > > Fixes: 12bf93a429c9 ("bcachefs: Add .to_text() methods for all superblock sections")
> > > Fixes: a37ad1a3aba9 ("bcachefs: sb-clean.c")
> > > Reported-and-tested-by: [email protected]
> > > Signed-off-by: Edward Adam Davis <[email protected]>
> >
> > I've already got a patch up for this - the validation was missing as
> > well.
> >
> > commit f39055220f6f98a180e3503fe05bbf9921c425c8
> > Author: Kent Overstreet <[email protected]>
> > Date: Sun May 5 22:28:00 2024 -0400
> >
> > bcachefs: Add missing validation for superblock section clean
> >
> > We were forgetting to check for jset entries that overrun the end of the
> > section - both in validate and to_text(); to_text() needs to be safe for
> > types that fail to validate.
> >
> > Reported-by: [email protected]
> > Signed-off-by: Kent Overstreet <[email protected]>
> >
> > diff --git a/fs/bcachefs/sb-clean.c b/fs/bcachefs/sb-clean.c
> > index 35ca3f138de6..194e55b11137 100644
> > --- a/fs/bcachefs/sb-clean.c
> > +++ b/fs/bcachefs/sb-clean.c
> > @@ -278,6 +278,17 @@ static int bch2_sb_clean_validate(struct bch_sb *sb,
> > return -BCH_ERR_invalid_sb_clean;
> > }
> >
> > + for (struct jset_entry *entry = clean->start;
> > + entry != vstruct_end(&clean->field);
> > + entry = vstruct_next(entry)) {
> > + if ((void *) vstruct_next(entry) > vstruct_end(&clean->field)) {
> > + prt_str(err, "entry type ");
> > + bch2_prt_jset_entry_type(err, le16_to_cpu(entry->type));
> > + prt_str(err, " overruns end of section");
> > + return -BCH_ERR_invalid_sb_clean;
> > + }
> > + }
> > +
> The original judgment here is sufficient, there is no need to add this section of inspection.
No, we need to be able to print things that failed to validate so that
we see what went wrong.
Hi Edward,
kernel test robot noticed the following build warnings:
[auto build test WARNING on linus/master]
[also build test WARNING on v6.9-rc7 next-20240507]
[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/Edward-Adam-Davis/bcachefs-fix-oob-in-bch2_sb_clean_to_text/20240507-172635
base: linus/master
patch link: https://lore.kernel.org/r/tencent_816D842DE96C309554E8E2ED9ACC6078120A%40qq.com
patch subject: [PATCH] bcachefs: fix oob in bch2_sb_clean_to_text
config: x86_64-randconfig-121-20240508 (https://download.01.org/0day-ci/archive/20240508/[email protected]/config)
compiler: clang version 18.1.4 (https://github.com/llvm/llvm-project e6c3289804a67ea0bb6a86fadbe454dd93b8d855)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240508/[email protected]/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 <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
sparse warnings: (new ones prefixed by >>)
fs/bcachefs/sb-clean.c: note: in included file:
fs/bcachefs/bcachefs.h:1027:9: sparse: sparse: array of flexible structures
fs/bcachefs/sb-clean.c: note: in included file (through fs/bcachefs/bcachefs.h):
fs/bcachefs/bcachefs_format.h:794:38: sparse: sparse: array of flexible structures
fs/bcachefs/bcachefs_format.h:1453:38: sparse: sparse: array of flexible structures
>> fs/bcachefs/sb-clean.c:296:20: sparse: sparse: incompatible types in comparison expression (different base types):
fs/bcachefs/sb-clean.c:296:20: sparse: struct jset_entry *
fs/bcachefs/sb-clean.c:296:20: sparse: void *
vim +296 fs/bcachefs/sb-clean.c
283
284 static void bch2_sb_clean_to_text(struct printbuf *out, struct bch_sb *sb,
285 struct bch_sb_field *f)
286 {
287 struct bch_sb_field_clean *clean = field_to_type(f, clean);
288 struct jset_entry *entry;
289
290 prt_printf(out, "flags: %x", le32_to_cpu(clean->flags));
291 prt_newline(out);
292 prt_printf(out, "journal_seq: %llu", le64_to_cpu(clean->journal_seq));
293 prt_newline(out);
294
295 for (entry = clean->start;
> 296 entry < vstruct_end(&clean->field);
297 entry = vstruct_next(entry)) {
298 if (entry->type == BCH_JSET_ENTRY_btree_keys &&
299 !entry->u64s)
300 continue;
301
302 bch2_journal_entry_to_text(out, NULL, entry);
303 prt_newline(out);
304 }
305 }
306
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On Tue, 7 May 2024 20:59:14 -0400, Kent Overstreet wrote:
> > > diff --git a/fs/bcachefs/sb-clean.c b/fs/bcachefs/sb-clean.c
> > > index 35ca3f138de6..194e55b11137 100644
> > > --- a/fs/bcachefs/sb-clean.c
> > > +++ b/fs/bcachefs/sb-clean.c
> > > @@ -278,6 +278,17 @@ static int bch2_sb_clean_validate(struct bch_sb *sb,
> > > return -BCH_ERR_invalid_sb_clean;
> > > }
> > >
> > > + for (struct jset_entry *entry = clean->start;
> > > + entry != vstruct_end(&clean->field);
> > > + entry = vstruct_next(entry)) {
> > > + if ((void *) vstruct_next(entry) > vstruct_end(&clean->field)) {
> > > + prt_str(err, "entry type ");
> > > + bch2_prt_jset_entry_type(err, le16_to_cpu(entry->type));
> > > + prt_str(err, " overruns end of section");
> > > + return -BCH_ERR_invalid_sb_clean;
> > > + }
> > > + }
> > > +
> > The original judgment here is sufficient, there is no need to add this section of inspection.
>
> No, we need to be able to print things that failed to validate so that
> we see what went wrong.
The follow check work fine, why add above check ?
1 if (vstruct_bytes(&clean->field) < sizeof(*clean)) {
268 prt_printf(err, "wrong size (got %zu should be %zu)",
1 vstruct_bytes(&clean->field), sizeof(*clean));
On Wed, May 08, 2024 at 09:11:37AM +0800, Edward Adam Davis wrote:
> On Tue, 7 May 2024 20:59:14 -0400, Kent Overstreet wrote:
> > > > diff --git a/fs/bcachefs/sb-clean.c b/fs/bcachefs/sb-clean.c
> > > > index 35ca3f138de6..194e55b11137 100644
> > > > --- a/fs/bcachefs/sb-clean.c
> > > > +++ b/fs/bcachefs/sb-clean.c
> > > > @@ -278,6 +278,17 @@ static int bch2_sb_clean_validate(struct bch_sb *sb,
> > > > return -BCH_ERR_invalid_sb_clean;
> > > > }
> > > >
> > > > + for (struct jset_entry *entry = clean->start;
> > > > + entry != vstruct_end(&clean->field);
> > > > + entry = vstruct_next(entry)) {
> > > > + if ((void *) vstruct_next(entry) > vstruct_end(&clean->field)) {
> > > > + prt_str(err, "entry type ");
> > > > + bch2_prt_jset_entry_type(err, le16_to_cpu(entry->type));
> > > > + prt_str(err, " overruns end of section");
> > > > + return -BCH_ERR_invalid_sb_clean;
> > > > + }
> > > > + }
> > > > +
> > > The original judgment here is sufficient, there is no need to add this section of inspection.
> >
> > No, we need to be able to print things that failed to validate so that
> > we see what went wrong.
> The follow check work fine, why add above check ?
> 1 if (vstruct_bytes(&clean->field) < sizeof(*clean)) {
> 268 prt_printf(err, "wrong size (got %zu should be %zu)",
> 1 vstruct_bytes(&clean->field), sizeof(*clean));
>
You sure you're not inebriated?
On Tue, 7 May 2024 21:21:16 -0400, Kent Overstreet wrote:
> > > > > diff --git a/fs/bcachefs/sb-clean.c b/fs/bcachefs/sb-clean.c
> > > > > index 35ca3f138de6..194e55b11137 100644
> > > > > --- a/fs/bcachefs/sb-clean.c
> > > > > +++ b/fs/bcachefs/sb-clean.c
> > > > > @@ -278,6 +278,17 @@ static int bch2_sb_clean_validate(struct bch_sb *sb,
> > > > > return -BCH_ERR_invalid_sb_clean;
> > > > > }
> > > > >
> > > > > + for (struct jset_entry *entry = clean->start;
> > > > > + entry != vstruct_end(&clean->field);
> > > > > + entry = vstruct_next(entry)) {
> > > > > + if ((void *) vstruct_next(entry) > vstruct_end(&clean->field)) {
> > > > > + prt_str(err, "entry type ");
> > > > > + bch2_prt_jset_entry_type(err, le16_to_cpu(entry->type));
> > > > > + prt_str(err, " overruns end of section");
> > > > > + return -BCH_ERR_invalid_sb_clean;
> > > > > + }
> > > > > + }
> > > > > +
> > > > The original judgment here is sufficient, there is no need to add this section of inspection.
> > >
> > > No, we need to be able to print things that failed to validate so that
> > > we see what went wrong.
> > The follow check work fine, why add above check ?
> > 1 if (vstruct_bytes(&clean->field) < sizeof(*clean)) {
> > 268 prt_printf(err, "wrong size (got %zu should be %zu)",
> > 1 vstruct_bytes(&clean->field), sizeof(*clean));
> >
>
> You sure you're not inebriated?
Here, is my test log, according to it, I can confirm what went wrong.
[ 129.350671][ T7772] bcachefs (/dev/loop0): error validating superblock: Invalid superblock section clean: wrong size (got 8 should be 24)
[ 129.350671][ T7772] clean (size 8):
[ 129.350671][ T7772] flags: 0
[ 129.350671][ T7772] journal_seq: 0