[syzbot] [hfs?] KASAN: slab-out-of-bounds Write in hfsplus_bnode_read_key

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit: 076d56d74f17 Add linux-next specific files for 20240202
git tree: linux-next
console+strace: https://syzkaller.appspot.com/x/log.txt?x=1773d5d3e80000
kernel config: https://syzkaller.appspot.com/x/.config?x=428086ff1c010d9f
dashboard link: https://syzkaller.appspot.com/bug?extid=57028366b9825d8e8ad0
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=1692c160180000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=13bab004180000

Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/dece45d1a4b5/disk-076d56d7.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/4921e269b178/vmlinux-076d56d7.xz
kernel image: https://storage.googleapis.com/syzbot-assets/2a9156da9091/bzImage-076d56d7.xz
mounted in repro: https://storage.googleapis.com/syzbot-assets/a4851316d70a/mount_0.gz

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

loop0: detected capacity change from 0 to 1024
==================================================================
BUG: KASAN: slab-out-of-bounds in memcpy_from_page include/linux/highmem.h:417 [inline]
BUG: KASAN: slab-out-of-bounds in hfsplus_bnode_read fs/hfsplus/bnode.c:32 [inline]
BUG: KASAN: slab-out-of-bounds in hfsplus_bnode_read_key+0x394/0x610 fs/hfsplus/bnode.c:70
Write of size 3970 at addr ffff88802a197800 by task syz-executor339/5066

CPU: 0 PID: 5066 Comm: syz-executor339 Not tainted 6.8.0-rc2-next-20240202-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/25/2024
Call Trace:
<TASK>
__dump_stack lib/dump_stack.c:88 [inline]
dump_stack_lvl+0x1e7/0x2e0 lib/dump_stack.c:106
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
kasan_check_range+0x282/0x290 mm/kasan/generic.c:189
__asan_memcpy+0x40/0x70 mm/kasan/shadow.c:106
memcpy_from_page include/linux/highmem.h:417 [inline]
hfsplus_bnode_read fs/hfsplus/bnode.c:32 [inline]
hfsplus_bnode_read_key+0x394/0x610 fs/hfsplus/bnode.c:70
hfsplus_brec_insert+0x6ea/0xde0 fs/hfsplus/brec.c:141
hfsplus_create_attr+0x4a2/0x640 fs/hfsplus/attributes.c:252
__hfsplus_setxattr+0x6fe/0x22d0 fs/hfsplus/xattr.c:354
hfsplus_setxattr+0xb0/0xe0 fs/hfsplus/xattr.c:434
hfsplus_security_setxattr+0x40/0x60 fs/hfsplus/xattr_security.c:31
__vfs_setxattr+0x468/0x4a0 fs/xattr.c:201
__vfs_setxattr_noperm+0x12e/0x5e0 fs/xattr.c:235
vfs_setxattr+0x221/0x430 fs/xattr.c:322
do_setxattr fs/xattr.c:630 [inline]
setxattr+0x25d/0x2f0 fs/xattr.c:653
path_setxattr+0x1c0/0x2a0 fs/xattr.c:672
__do_sys_setxattr fs/xattr.c:688 [inline]
__se_sys_setxattr fs/xattr.c:684 [inline]
__x64_sys_setxattr+0xbb/0xd0 fs/xattr.c:684
do_syscall_64+0xfb/0x240
entry_SYSCALL_64_after_hwframe+0x6d/0x75
RIP: 0033:0x7fc180ad2639
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:00007ffc6080f8c8 EFLAGS: 00000246 ORIG_RAX: 00000000000000bc
RAX: ffffffffffffffda RBX: 0031656c69662f2e RCX: 00007fc180ad2639
RDX: 0000000000000000 RSI: 0000000020000140 RDI: 0000000020000240
RBP: 00007fc180b45610 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000001
R13: 00007ffc6080fa98 R14: 0000000000000001 R15: 0000000000000001
</TASK>

Allocated by task 5066:
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:3982 [inline]
__kmalloc+0x231/0x4a0 mm/slub.c:3995
kmalloc include/linux/slab.h:594 [inline]
hfsplus_find_init+0x85/0x1c0 fs/hfsplus/bfind.c:21
hfsplus_create_attr+0x161/0x640 fs/hfsplus/attributes.c:216
__hfsplus_setxattr+0x6fe/0x22d0 fs/hfsplus/xattr.c:354
hfsplus_setxattr+0xb0/0xe0 fs/hfsplus/xattr.c:434
hfsplus_security_setxattr+0x40/0x60 fs/hfsplus/xattr_security.c:31
__vfs_setxattr+0x468/0x4a0 fs/xattr.c:201
__vfs_setxattr_noperm+0x12e/0x5e0 fs/xattr.c:235
vfs_setxattr+0x221/0x430 fs/xattr.c:322
do_setxattr fs/xattr.c:630 [inline]
setxattr+0x25d/0x2f0 fs/xattr.c:653
path_setxattr+0x1c0/0x2a0 fs/xattr.c:672
__do_sys_setxattr fs/xattr.c:688 [inline]
__se_sys_setxattr fs/xattr.c:684 [inline]
__x64_sys_setxattr+0xbb/0xd0 fs/xattr.c:684
do_syscall_64+0xfb/0x240
entry_SYSCALL_64_after_hwframe+0x6d/0x75

The buggy address belongs to the object at ffff88802a197800
which belongs to the cache kmalloc-1k of size 1024
The buggy address is located 0 bytes inside of
allocated 536-byte region [ffff88802a197800, ffff88802a197a18)

The buggy address belongs to the physical page:
page:ffffea0000a86400 refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x2a190
head:ffffea0000a86400 order:3 entire_mapcount:0 nr_pages_mapped:0 pincount:0
anon flags: 0xfff80000000840(slab|head|node=0|zone=1|lastcpupid=0xfff)
page_type: 0xffffffff()
raw: 00fff80000000840 ffff888014c41dc0 0000000000000000 dead000000000001
raw: 0000000000000000 0000000080100010 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 0xd20c0(__GFP_IO|__GFP_FS|__GFP_NOWARN|__GFP_NORETRY|__GFP_COMP|__GFP_NOMEMALLOC), pid 1, tgid 1 (swapper/0), ts 10670801188, free_ts 0
set_page_owner include/linux/page_owner.h:31 [inline]
post_alloc_hook+0x1ea/0x210 mm/page_alloc.c:1539
prep_new_page mm/page_alloc.c:1546 [inline]
get_page_from_freelist+0x34eb/0x3680 mm/page_alloc.c:3353
__alloc_pages+0x256/0x680 mm/page_alloc.c:4609
__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:2191
allocate_slab mm/slub.c:2354 [inline]
new_slab+0x84/0x2f0 mm/slub.c:2407
___slab_alloc+0xc73/0x1260 mm/slub.c:3541
__slab_alloc mm/slub.c:3626 [inline]
__slab_alloc_node mm/slub.c:3679 [inline]
slab_alloc_node mm/slub.c:3851 [inline]
__do_kmalloc_node mm/slub.c:3981 [inline]
__kmalloc+0x2e3/0x4a0 mm/slub.c:3995
kmalloc include/linux/slab.h:594 [inline]
kzalloc include/linux/slab.h:711 [inline]
alloc_workqueue+0x19b/0x1f40 kernel/workqueue.c:5196
nf_flow_table_offload_init+0x3c/0xb0 net/netfilter/nf_flow_table_offload.c:1217
nf_flow_table_module_init+0x2b/0x70 net/netfilter/nf_flow_table_core.c:662
do_one_initcall+0x238/0x830 init/main.c:1233
do_initcall_level+0x157/0x210 init/main.c:1295
do_initcalls+0x3f/0x80 init/main.c:1311
kernel_init_freeable+0x430/0x5d0 init/main.c:1542
kernel_init+0x1d/0x2b0 init/main.c:1432
ret_from_fork+0x4b/0x80 arch/x86/kernel/process.c:147
page_owner free stack trace missing

Memory state around the buggy address:
ffff88802a197900: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
ffff88802a197980: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>ffff88802a197a00: 00 00 00 fc fc fc fc fc fc fc fc fc fc fc fc fc
^
ffff88802a197a80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
ffff88802a197b00: 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 syzk...@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

[Edward Adam Davis]

please test oob in hfsplus_bnode_read_key

#syz test https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master

diff --git a/fs/hfsplus/brec.c b/fs/hfsplus/brec.c
index 1918544a7871..9e0e0c1f15a5 100644
--- a/fs/hfsplus/brec.c
+++ b/fs/hfsplus/brec.c
@@ -138,7 +138,8 @@ int hfs_brec_insert(struct hfs_find_data *fd, void *entry, int entry_len)
* at the start of the node and it is not the new node
*/
if (!rec && new_node != node) {
- hfs_bnode_read_key(node, fd->search_key, data_off + size);
+ hfs_bnode_read_key(node, fd->search_key, data_off +
+ (idx_rec_off == data_rec_off ? 0 : size));
hfs_brec_update_parent(fd);
}

--
2.43.0

[syzbot]

Hello,

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

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

Tested on:

commit: 076d56d7 Add linux-next specific files for 20240202
git tree: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master
console output: https://syzkaller.appspot.com/x/log.txt?x=15872d7be80000

kernel config: https://syzkaller.appspot.com/x/.config?x=428086ff1c010d9f
dashboard link: https://syzkaller.appspot.com/bug?extid=57028366b9825d8e8ad0
compiler: Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40

patch: https://syzkaller.appspot.com/x/patch.diff?x=17d0b18fe80000

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

[Edward Adam Davis]

In hfs_brec_insert(), if data has not been moved to "data_off + size", the size
should not be added when reading search_key from node->page.

Reported-and-tested-by: syzbot+570283...@syzkaller.appspotmail.com
Signed-off-by: Edward Adam Davis <ead...@qq.com>
---
fs/hfsplus/brec.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

[Viacheslav Dubeyko]

> On 4 Feb 2024, at 14:51, Edward Adam Davis <ead...@qq.com> wrote:
>
> In hfs_brec_insert(), if data has not been moved to "data_off + size", the size
> should not be added when reading search_key from node->page.
>
> Reported-and-tested-by: syzbot+570283...@syzkaller.appspotmail.com
> Signed-off-by: Edward Adam Davis <ead...@qq.com>
> ---
> fs/hfsplus/brec.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/fs/hfsplus/brec.c b/fs/hfsplus/brec.c
> index 1918544a7871..9e0e0c1f15a5 100644
> --- a/fs/hfsplus/brec.c
> +++ b/fs/hfsplus/brec.c
> @@ -138,7 +138,8 @@ int hfs_brec_insert(struct hfs_find_data *fd, void *entry, int entry_len)
> * at the start of the node and it is not the new node
> */
> if (!rec && new_node != node) {
> - hfs_bnode_read_key(node, fd->search_key, data_off + size);

As far as I can see, likewise pattern 'data_off + size’ is used multiple times in hfs_brec_insert().
It’s real source of potential bugs, for my taste. Could we introduce a special variable (like offset)
that can keep calculated value?

> + hfs_bnode_read_key(node, fd->search_key, data_off +
> + (idx_rec_off == data_rec_off ? 0 : size));

I believe the code of hfs_brec_insert() is complicated enough.
It will be great to rework this code and to add comments with
reasonable explanation of the essence of modification. It’s not so easy
to follow how moving is related to read the key operation.

What do you think?

Thanks,
Slava.

> hfs_brec_update_parent(fd);
> }
>
> --
> 2.43.0
>
>

[Edward Adam Davis]

On Tue, 6 Feb 2024 15:05:23 +0300, Viacheslav Dubeyko <sl...@dubeyko.com> wrote:
> > In hfs_brec_insert(), if data has not been moved to "data_off + size", the size
> > should not be added when reading search_key from node->page.
> >
> > Reported-and-tested-by: syzbot+570283...@syzkaller.appspotmail.com
> > Signed-off-by: Edward Adam Davis <ead...@qq.com>
> > ---
> > fs/hfsplus/brec.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/hfsplus/brec.c b/fs/hfsplus/brec.c
> > index 1918544a7871..9e0e0c1f15a5 100644
> > --- a/fs/hfsplus/brec.c
> > +++ b/fs/hfsplus/brec.c
> > @@ -138,7 +138,8 @@ int hfs_brec_insert(struct hfs_find_data *fd, void *entry, int entry_len)
> > * at the start of the node and it is not the new node
> > */
> > if (!rec && new_node != node) {
> > - hfs_bnode_read_key(node, fd->search_key, data_off + size);
>
> As far as I can see, likewise pattern 'data_off + size’ is used multiple times in hfs_brec_insert().
> It’s real source of potential bugs, for my taste. Could we introduce a special variable (like offset)
> that can keep calculated value?

The code after "skip:" only adds size at this point, so currently there is no
need to add variables for separate management.

>
> > + hfs_bnode_read_key(node, fd->search_key, data_off +
> > + (idx_rec_off == data_rec_off ? 0 : size));
>
> I believe the code of hfs_brec_insert() is complicated enough.
> It will be great to rework this code and to add comments with
> reasonable explanation of the essence of modification. It’s not so easy
> to follow how moving is related to read the key operation.

As the case may be, other code is just complex but no issues have been reported.
It is not recommended to make unfounded optimizations.

>
> What do you think?
Thanks,

Edward.