[syzbot] WARNING in hfsplus_cat_read_inode

[syzbot] WARNING in hfsplus_cat_read_inode

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit:    cdb931b58ff5 Merge branch 'for-next/core' into for-kernelci
git tree:       git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-kernelci
console output: https://syzkaller.appspot.com/x/log.txt?x=1672f7fd880000
kernel config:  https://syzkaller.appspot.com/x/.config?x=ec7118319bfb771e
dashboard link: https://syzkaller.appspot.com/bug?extid=e2787430e752a92b8750
compiler:       Debian clang version 13.0.1-++20220126092033+75e33f71c2da-1~exp1~20220126212112.63, GNU ld (GNU Binutils for Debian) 2.35.2
userspace arch: arm64
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=120b96a7880000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=116da6bd880000

Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/07e4eae17e60/disk-cdb931b5.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/fc4815dd00c0/vmlinux-cdb931b5.xz
kernel image: https://storage.googleapis.com/syzbot-assets/0f46b40f30e1/Image-cdb931b5.gz.xz
mounted in repro: https://storage.googleapis.com/syzbot-assets/3082309c63cc/mount_0.gz

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

loop0: detected capacity change from 0 to 1024
------------[ cut here ]------------
WARNING: CPU: 0 PID: 3073 at fs/hfsplus/inode.c:534 hfsplus_cat_read_inode+0x32c/0x338 fs/hfsplus/inode.c:534
Modules linked in:
CPU: 0 PID: 3073 Comm: syz-executor278 Not tainted 6.1.0-rc7-syzkaller-33054-gcdb931b58ff5 #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 09/30/2022
pstate: 80400005 (Nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
pc : hfsplus_cat_read_inode+0x32c/0x338 fs/hfsplus/inode.c:534
lr : hfsplus_cat_read_inode+0x32c/0x338 fs/hfsplus/inode.c:534
sp : ffff80000ff23640
x29: ffff80000ff23850 x28: ffff0000c23a0000 x27: 000000000000000b
x26: ffff0000c9e92000 x25: 00000000000000ff x24: ffff0000cbd81530
x23: ffff0000c7bffdf0 x22: ffff0000c7bffd30 x21: 0000000000000058
x20: ffff80000ff23880 x19: ffff0000cbd81bb0 x18: 00000000000000c0
x17: ffff80000dda8198 x16: 0000000000000000 x15: 0000000000000000
x14: 0000000000000000 x13: 0000000000000002 x12: ffff80000d514f80
x11: ff808000088e828c x10: 0000000000000000 x9 : ffff8000088e828c
x8 : ffff0000c23a0000 x7 : 0000000000000000 x6 : 0000000000000000
x5 : ffff80000ff235f6 x4 : ffff0001803f7028 x3 : 0000000000000000
x2 : 0000000000000002 x1 : 0000000000000058 x0 : 00000000000000f8
Call trace:
 hfsplus_cat_read_inode+0x32c/0x338 fs/hfsplus/inode.c:534
 hfsplus_iget+0x244/0x2ac fs/hfsplus/super.c:84
 hfsplus_fill_super+0x480/0x864 fs/hfsplus/super.c:503
 mount_bdev+0x1b8/0x210 fs/super.c:1401
 hfsplus_mount+0x44/0x58 fs/hfsplus/super.c:641
 legacy_get_tree+0x30/0x74 fs/fs_context.c:610
 vfs_get_tree+0x40/0x140 fs/super.c:1531
 do_new_mount+0x1dc/0x4e4 fs/namespace.c:3040
 path_mount+0x358/0x890 fs/namespace.c:3370
 do_mount fs/namespace.c:3383 [inline]
 __do_sys_mount fs/namespace.c:3591 [inline]
 __se_sys_mount fs/namespace.c:3568 [inline]
 __arm64_sys_mount+0x2c4/0x3c4 fs/namespace.c:3568
 __invoke_syscall arch/arm64/kernel/syscall.c:38 [inline]
 invoke_syscall arch/arm64/kernel/syscall.c:52 [inline]
 el0_svc_common+0x138/0x220 arch/arm64/kernel/syscall.c:142
 do_el0_svc+0x48/0x140 arch/arm64/kernel/syscall.c:197
 el0_svc+0x58/0x150 arch/arm64/kernel/entry-common.c:637
 el0t_64_sync_handler+0x84/0xf0 arch/arm64/kernel/entry-common.c:655
 el0t_64_sync+0x190/0x194 arch/arm64/kernel/entry.S:584
irq event stamp: 15818
hardirqs last  enabled at (15817): [<ffff80000c0963d4>] __raw_spin_unlock_irqrestore include/linux/spinlock_api_smp.h:151 [inline]
hardirqs last  enabled at (15817): [<ffff80000c0963d4>] _raw_spin_unlock_irqrestore+0x48/0x8c kernel/locking/spinlock.c:194
hardirqs last disabled at (15818): [<ffff80000c083704>] el1_dbg+0x24/0x80 arch/arm64/kernel/entry-common.c:405
softirqs last  enabled at (15354): [<ffff8000080102e4>] _stext+0x2e4/0x37c
softirqs last disabled at (15241): [<ffff800008017c88>] ____do_softirq+0x14/0x20 arch/arm64/kernel/irq.c:80
---[ end trace 0000000000000000 ]---


---
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.
syzbot can test patches for this issue, for details see:
https://goo.gl/tpsmEJ#testing-patches

[PATCH] fs: hfsplus: remove WARN_ON() from hfsplus_cat_{read,write}_inode()

[Tetsuo Handa]

syzbot is hitting WARN_ON() in hfsplus_cat_{read,write}_inode(), for
crafted filesystem image can contain bogus length. There conditions are
not kernel bugs that can justify kernel to panic.

Reported-by: syzbot <syzbot+e2787430e752a92b8750@syzkaller.appspotmail.com>
Link: https://syzkaller.appspot.com/bug?extid=e2787430e752a92b8750
Reported-by: syzbot <syzbot+4913dca2ea6e4d43f3f1@syzkaller.appspotmail.com>
Link: https://syzkaller.appspot.com/bug?extid=4913dca2ea6e4d43f3f1
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 fs/hfsplus/inode.c | 28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/fs/hfsplus/inode.c b/fs/hfsplus/inode.c
index abb91f5fae92..b21660475ac1 100644
--- a/fs/hfsplus/inode.c
+++ b/fs/hfsplus/inode.c
@@ -511,7 +511,11 @@ int hfsplus_cat_read_inode(struct inode *inode, struct hfs_find_data *fd)
 	if (type == HFSPLUS_FOLDER) {
 		struct hfsplus_cat_folder *folder = &entry.folder;
 
-		WARN_ON(fd->entrylength < sizeof(struct hfsplus_cat_folder));
+		if (fd->entrylength < sizeof(struct hfsplus_cat_folder)) {
+			pr_err("bad catalog folder entry\n");
+			res = -EIO;
+			goto out;
+		}
 		hfs_bnode_read(fd->bnode, &entry, fd->entryoffset,
 					sizeof(struct hfsplus_cat_folder));
 		hfsplus_get_perms(inode, &folder->permissions, 1);
@@ -531,7 +535,11 @@ int hfsplus_cat_read_inode(struct inode *inode, struct hfs_find_data *fd)
 	} else if (type == HFSPLUS_FILE) {
 		struct hfsplus_cat_file *file = &entry.file;
 
-		WARN_ON(fd->entrylength < sizeof(struct hfsplus_cat_file));
+		if (fd->entrylength < sizeof(struct hfsplus_cat_file)) {
+			pr_err("bad catalog file entry\n");
+			res = -EIO;
+			goto out;
+		}
 		hfs_bnode_read(fd->bnode, &entry, fd->entryoffset,
 					sizeof(struct hfsplus_cat_file));
 
@@ -562,6 +570,7 @@ int hfsplus_cat_read_inode(struct inode *inode, struct hfs_find_data *fd)
 		pr_err("bad catalog entry used to create inode\n");
 		res = -EIO;
 	}
+out:
 	return res;
 }
 
@@ -570,6 +579,7 @@ int hfsplus_cat_write_inode(struct inode *inode)
 	struct inode *main_inode = inode;
 	struct hfs_find_data fd;
 	hfsplus_cat_entry entry;
+	int res = 0;
 
 	if (HFSPLUS_IS_RSRC(inode))
 		main_inode = HFSPLUS_I(inode)->rsrc_inode;
@@ -588,7 +598,11 @@ int hfsplus_cat_write_inode(struct inode *inode)
 	if (S_ISDIR(main_inode->i_mode)) {
 		struct hfsplus_cat_folder *folder = &entry.folder;
 
-		WARN_ON(fd.entrylength < sizeof(struct hfsplus_cat_folder));
+		if (fd.entrylength < sizeof(struct hfsplus_cat_folder)) {
+			pr_err("bad catalog folder entry\n");
+			res = -EIO;
+			goto out;
+		}
 		hfs_bnode_read(fd.bnode, &entry, fd.entryoffset,
 					sizeof(struct hfsplus_cat_folder));
 		/* simple node checks? */
@@ -613,7 +627,11 @@ int hfsplus_cat_write_inode(struct inode *inode)
 	} else {
 		struct hfsplus_cat_file *file = &entry.file;
 
-		WARN_ON(fd.entrylength < sizeof(struct hfsplus_cat_file));
+		if (fd.entrylength < sizeof(struct hfsplus_cat_file)) {
+			pr_err("bad catalog file entry\n");
+			res = -EIO;
+			goto out;
+		}
 		hfs_bnode_read(fd.bnode, &entry, fd.entryoffset,
 					sizeof(struct hfsplus_cat_file));
 		hfsplus_inode_write_fork(inode, &file->data_fork);
@@ -634,7 +652,7 @@ int hfsplus_cat_write_inode(struct inode *inode)
 	set_bit(HFSPLUS_I_CAT_DIRTY, &HFSPLUS_I(inode)->flags);
 out:
 	hfs_find_exit(&fd);
-	return 0;
+	return res;
 }
 
 int hfsplus_fileattr_get(struct dentry *dentry, struct fileattr *fa)
-- 
2.34.1

Re: [PATCH] hfsplus: remove WARN_ON() from hfsplus_cat_{read,write}_inode()

[Viacheslav Dubeyko]

> On Apr 11, 2023, at 3:57 AM, Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> wrote:
> 
> syzbot is hitting WARN_ON() in hfsplus_cat_{read,write}_inode(), for
> crafted filesystem image can contain bogus length. There conditions are
> not kernel bugs that can justify kernel to panic.
> 
> Reported-by: syzbot <syzbot+e2787430e752a92b8750@syzkaller.appspotmail.com>
> Link: https://syzkaller.appspot.com/bug?extid=e2787430e752a92b8750
> Reported-by: syzbot <syzbot+4913dca2ea6e4d43f3f1@syzkaller.appspotmail.com>
> Link: https://syzkaller.appspot.com/bug?extid=4913dca2ea6e4d43f3f1
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
> fs/hfsplus/inode.c | 28 +++++++++++++++++++++++-----
> 1 file changed, 23 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/hfsplus/inode.c b/fs/hfsplus/inode.c
> index abb91f5fae92..b21660475ac1 100644
> --- a/fs/hfsplus/inode.c
> +++ b/fs/hfsplus/inode.c
> @@ -511,7 +511,11 @@ int hfsplus_cat_read_inode(struct inode *inode, struct hfs_find_data *fd)
> 	if (type == HFSPLUS_FOLDER) {
> 		struct hfsplus_cat_folder *folder = &entry.folder;
> 
> -		WARN_ON(fd->entrylength < sizeof(struct hfsplus_cat_folder));
> +		if (fd->entrylength < sizeof(struct hfsplus_cat_folder)) {
> +			pr_err("bad catalog folder entry\n");
> +			res = -EIO;
> +			goto out;
> +		}
> 		hfs_bnode_read(fd->bnode, &entry, fd->entryoffset,
> 					sizeof(struct hfsplus_cat_folder));
> 		hfsplus_get_perms(inode, &folder->permissions, 1);
> @@ -531,7 +535,11 @@ int hfsplus_cat_read_inode(struct inode *inode, struct hfs_find_data *fd)
> 	} else if (type == HFSPLUS_FILE) {
> 		struct hfsplus_cat_file *file = &entry.file;
> 
> -		WARN_ON(fd->entrylength < sizeof(struct hfsplus_cat_file));
> +		if (fd->entrylength < sizeof(struct hfsplus_cat_file)) {
> +			pr_err("bad catalog file entry\n");
> +			res = -EIO;
> +			goto out;
> +		}
> 		hfs_bnode_read(fd->bnode, &entry, fd->entryoffset,
> 					sizeof(struct hfsplus_cat_file));
> 
> @@ -562,6 +570,7 @@ int hfsplus_cat_read_inode(struct inode *inode, struct hfs_find_data *fd)
> 		pr_err("bad catalog entry used to create inode\n");
> 		res = -EIO;
> 	}
> +out:
> 	return res;
> }
> 
> @@ -570,6 +579,7 @@ int hfsplus_cat_write_inode(struct inode *inode)
> 	struct inode *main_inode = inode;
> 	struct hfs_find_data fd;
> 	hfsplus_cat_entry entry;
> +	int res = 0;
> 
> 	if (HFSPLUS_IS_RSRC(inode))
> 		main_inode = HFSPLUS_I(inode)->rsrc_inode;
> @@ -588,7 +598,11 @@ int hfsplus_cat_write_inode(struct inode *inode)
> 	if (S_ISDIR(main_inode->i_mode)) {
> 		struct hfsplus_cat_folder *folder = &entry.folder;
> 
> -		WARN_ON(fd.entrylength < sizeof(struct hfsplus_cat_folder));
> +		if (fd.entrylength < sizeof(struct hfsplus_cat_folder)) {
> +			pr_err("bad catalog folder entry\n");
> +			res = -EIO;
> +			goto out;
> +		}
> 		hfs_bnode_read(fd.bnode, &entry, fd.entryoffset,
> 					sizeof(struct hfsplus_cat_folder));
> 		/* simple node checks? */
> @@ -613,7 +627,11 @@ int hfsplus_cat_write_inode(struct inode *inode)
> 	} else {
> 		struct hfsplus_cat_file *file = &entry.file;
> 
> -		WARN_ON(fd.entrylength < sizeof(struct hfsplus_cat_file));
> +		if (fd.entrylength < sizeof(struct hfsplus_cat_file)) {
> +			pr_err("bad catalog file entry\n");
> +			res = -EIO;
> +			goto out;
> +		}
> 		hfs_bnode_read(fd.bnode, &entry, fd.entryoffset,
> 					sizeof(struct hfsplus_cat_file));
> 		hfsplus_inode_write_fork(inode, &file->data_fork);
> @@ -634,7 +652,7 @@ int hfsplus_cat_write_inode(struct inode *inode)
> 	set_bit(HFSPLUS_I_CAT_DIRTY, &HFSPLUS_I(inode)->flags);
> out:
> 	hfs_find_exit(&fd);
> -	return 0;
> +	return res;
> }
> 

Looks reasonable to me. Maybe, WARN_ON() provided the opportunity to see
the call stack of the issue. It could be useful for the issue’s environment
analysis. But returning error from the function(s) makes the execution flow
much better.

Reviewed-by: Viacheslav Dubeyko <slava@dubeyko.com>

Thanks,
Slava.


> int hfsplus_fileattr_get(struct dentry *dentry, struct fileattr *fa)
> -- 
> 2.34.1
> 

Re: [PATCH] fs: hfsplus: remove WARN_ON() from hfsplus_cat_{read,write}_inode()

[Christian Brauner]

On Tue, 11 Apr 2023 19:57:33 +0900, Tetsuo Handa wrote:
> syzbot is hitting WARN_ON() in hfsplus_cat_{read,write}_inode(), for
> crafted filesystem image can contain bogus length. There conditions are
> not kernel bugs that can justify kernel to panic.
> 
> 

Seems fine as a follow-up fix to Arnd's in
55d1cbbbb29e ("hfs/hfsplus: use WARN_ON for sanity check").
Filesystems is orphaned so taking this through one of our vfs trees,

branch: fs.misc
[1/1] fs: hfsplus: remove WARN_ON() from hfsplus_cat_{read,write}_inode()
      commit: 81b21c0f0138ff5a499eafc3eb0578ad2a99622c