[syzbot] [hfs?] KMSAN: uninit-value in hfs_read_inode

[syzbot] [hfs?] KMSAN: uninit-value in hfs_read_inode

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit:    f66d6acccbc0 Merge tag 'x86_urgent_for_v6.12' of git://git..
git tree:       upstream
console+strace: https://syzkaller.appspot.com/x/log.txt?x=1381b2e8580000
kernel config:  https://syzkaller.appspot.com/x/.config?x=570f86970553dcd2
dashboard link: https://syzkaller.appspot.com/bug?extid=2db3c7526ba68f4ea776
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=1400bb5f980000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=162c8ac0580000

Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/83bb4f67b45d/disk-f66d6acc.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/cb0cedffb310/vmlinux-f66d6acc.xz
kernel image: https://storage.googleapis.com/syzbot-assets/3af8c8949657/bzImage-f66d6acc.xz
mounted in repro: https://storage.googleapis.com/syzbot-assets/bfaab1c46dcf/mount_0.gz

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

loop0: detected capacity change from 0 to 64
=====================================================
BUG: KMSAN: uninit-value in hfs_inode_read_fork fs/hfs/inode.c:287 [inline]
BUG: KMSAN: uninit-value in hfs_read_inode+0x10ae/0x1690 fs/hfs/inode.c:343
 hfs_inode_read_fork fs/hfs/inode.c:287 [inline]
 hfs_read_inode+0x10ae/0x1690 fs/hfs/inode.c:343
 inode_insert5+0x1dd/0x970 fs/inode.c:1281
 iget5_locked+0xfe/0x1d0 fs/inode.c:1338
 hfs_iget+0x121/0x240 fs/hfs/inode.c:403
 hfs_fill_super+0x2098/0x23c0 fs/hfs/super.c:431
 mount_bdev+0x39a/0x520 fs/super.c:1693
 hfs_mount+0x4d/0x60 fs/hfs/super.c:457
 legacy_get_tree+0x114/0x290 fs/fs_context.c:662
 vfs_get_tree+0xb1/0x5a0 fs/super.c:1814
 do_new_mount+0x71f/0x15e0 fs/namespace.c:3507
 path_mount+0x742/0x1f10 fs/namespace.c:3834
 do_mount fs/namespace.c:3847 [inline]
 __do_sys_mount fs/namespace.c:4057 [inline]
 __se_sys_mount+0x722/0x810 fs/namespace.c:4034
 __x64_sys_mount+0xe4/0x150 fs/namespace.c:4034
 x64_sys_call+0x255a/0x3ba0 arch/x86/include/generated/asm/syscalls_64.h:166
 do_syscall_x64 arch/x86/entry/common.c:52 [inline]
 do_syscall_64+0xcd/0x1e0 arch/x86/entry/common.c:83
 entry_SYSCALL_64_after_hwframe+0x77/0x7f

Local variable rec created at:
 hfs_fill_super+0x67/0x23c0 fs/hfs/super.c:383
 mount_bdev+0x39a/0x520 fs/super.c:1693

CPU: 1 UID: 0 PID: 5790 Comm: syz-executor415 Not tainted 6.12.0-rc7-syzkaller-00216-gf66d6acccbc0 #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 10/30/2024
=====================================================


---
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] [hfs?] KMSAN: uninit-value in hfs_read_inode

[Leo Stone]

After the call to hfs_bnode_read on line 356 of fs/hfs/super.c, the
struct hfs_cat_rec, which is supposed to be for the root dir, has
type HFS_CDR_FIL. Only the first 70 bytes of that struct are then
initialized, because the entrylength passed into hfs_bnode_read is
70, corresponding to the size of struct hfs_cat_dir. This causes
uninitialized values to be used later on when the hfs_cat_rec
union is treated as a hfs_cat_file.

Add a check to make sure the retrieved record has the correct type
for the root directory (HFS_CDR_DIR).

#syz test: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master

---
 fs/hfs/super.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/hfs/super.c b/fs/hfs/super.c
index 3bee9b5dba5e..02d78992eefd 100644
--- a/fs/hfs/super.c
+++ b/fs/hfs/super.c
@@ -354,6 +354,8 @@ static int hfs_fill_super(struct super_block *sb, struct fs_context *fc)
 			goto bail_hfs_find;
 		}
 		hfs_bnode_read(fd.bnode, &rec, fd.entryoffset, fd.entrylength);
+		if (rec.type != HFS_CDR_DIR)
+			res = -EIO;
 	}
 	if (res)
 		goto bail_hfs_find;
-- 
2.43.0

Re: [syzbot] [hfs?] KMSAN: uninit-value in hfs_read_inode

[syzbot]

Hello,

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

Reported-by: syzbot+2db3c7526ba68f4ea776@syzkaller.appspotmail.com
Tested-by: syzbot+2db3c7526ba68f4ea776@syzkaller.appspotmail.com

Tested on:

commit:         28eb75e1 Merge tag 'drm-next-2024-11-21' of https://gi..
git tree:       upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=1382175f980000
kernel config:  https://syzkaller.appspot.com/x/.config?x=ef9abe59471e0aee
dashboard link: https://syzkaller.appspot.com/bug?extid=2db3c7526ba68f4ea776
compiler:       Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40
patch:          https://syzkaller.appspot.com/x/patch.diff?x=16088c90580000

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

[PATCH] hfs: Sanity check the root record

[Leo Stone]

In the syzbot reproducer, the hfs_cat_rec for the root dir has type
HFS_CDR_FIL after being read with hfs_bnode_read() in hfs_super_fill().
This indicates it should be used as an hfs_cat_file, which is 102 bytes.
Only the first 70 bytes of that struct are initialized, however,
because the entrylength passed into hfs_bnode_read() is still the length of
a directory record. This causes uninitialized values to be used later on,
when the hfs_cat_rec union is treated as the larger hfs_cat_file struct.

Add a check to make sure the retrieved record has the correct type
for the root directory (HFS_CDR_DIR).

Reported-by: syzbot+2db3c7526ba68f4ea776@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=2db3c7526ba68f4ea776
Tested-by: syzbot+2db3c7526ba68f4ea776@syzkaller.appspotmail.com
Signed-off-by: Leo Stone <leocstone@gmail.com>
---
 fs/hfs/super.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/hfs/super.c b/fs/hfs/super.c
index 3bee9b5dba5e..02d78992eefd 100644
--- a/fs/hfs/super.c
+++ b/fs/hfs/super.c
@@ -354,6 +354,8 @@ static int hfs_fill_super(struct super_block *sb, struct fs_context *fc)
 			goto bail_hfs_find;
 		}
 		hfs_bnode_read(fd.bnode, &rec, fd.entryoffset, fd.entrylength);
+		if (rec.type != HFS_CDR_DIR)
+			res = -EIO;
 	}
 	if (res)
 		goto bail_hfs_find;
-- 
2.43.0

Re: [PATCH] hfs: Sanity check the root record

[Jan Kara]

On Sat 23-11-24 11:49:47, Leo Stone wrote:
> In the syzbot reproducer, the hfs_cat_rec for the root dir has type
> HFS_CDR_FIL after being read with hfs_bnode_read() in hfs_super_fill().
> This indicates it should be used as an hfs_cat_file, which is 102 bytes.
> Only the first 70 bytes of that struct are initialized, however,
> because the entrylength passed into hfs_bnode_read() is still the length of
> a directory record. This causes uninitialized values to be used later on,
> when the hfs_cat_rec union is treated as the larger hfs_cat_file struct.
> 
> Add a check to make sure the retrieved record has the correct type
> for the root directory (HFS_CDR_DIR).

This certainly won't hurt but shouldn't we also add some stricter checks
for entry length so that we know we've loaded enough data to have full info
about the root dir?

								Honza

> 
> Reported-by: syzbot+2db3c7526ba68f4ea776@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=2db3c7526ba68f4ea776
> Tested-by: syzbot+2db3c7526ba68f4ea776@syzkaller.appspotmail.com
> Signed-off-by: Leo Stone <leocstone@gmail.com>
> ---
>  fs/hfs/super.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/hfs/super.c b/fs/hfs/super.c
> index 3bee9b5dba5e..02d78992eefd 100644
> --- a/fs/hfs/super.c
> +++ b/fs/hfs/super.c
> @@ -354,6 +354,8 @@ static int hfs_fill_super(struct super_block *sb, struct fs_context *fc)
>  			goto bail_hfs_find;
>  		}
>  		hfs_bnode_read(fd.bnode, &rec, fd.entryoffset, fd.entrylength);
> +		if (rec.type != HFS_CDR_DIR)
> +			res = -EIO;
>  	}
>  	if (res)
>  		goto bail_hfs_find;
> -- 
> 2.43.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] hfs: Sanity check the root record

[Leo Stone]

Hello,

On Tue, Nov 26, 2024 at 10:33:13AM +0100, Jan Kara wrote:
> 
> This certainly won't hurt but shouldn't we also add some stricter checks
> for entry length so that we know we've loaded enough data to have full info
> about the root dir?

Yes, that would be a good idea. Do we want to keep the existing checks
and just make sure we have at least enough to initialize the struct:

if (fd.entrylength > sizeof(rec) || fd.entrylength < 0 ||
    fd.entrylength < sizeof(rec.dir)) {
	res = -EIO;
	goto bail_hfs_find;
}

Or be even stricter and only accept the exact length:

if (fd.entrylength != sizeof(rec.dir)) {
	res = -EIO;
	goto bail_hfs_find;
}

Thanks for your feedback,
Leo

Re: [PATCH] hfs: Sanity check the root record

[Jan Kara]

On Tue 26-11-24 09:21:50, Leo Stone wrote:
> Hello,
> 
> On Tue, Nov 26, 2024 at 10:33:13AM +0100, Jan Kara wrote:
> > 
> > This certainly won't hurt but shouldn't we also add some stricter checks
> > for entry length so that we know we've loaded enough data to have full info
> > about the root dir?
> 
> Yes, that would be a good idea. Do we want to keep the existing checks
> and just make sure we have at least enough to initialize the struct:
> 
> if (fd.entrylength > sizeof(rec) || fd.entrylength < 0 ||
>     fd.entrylength < sizeof(rec.dir)) {
> 	res = -EIO;
> 	goto bail_hfs_find;
> }
> 
> Or be even stricter and only accept the exact length:
> 
> if (fd.entrylength != sizeof(rec.dir)) {
> 	res = -EIO;
> 	goto bail_hfs_find;
> }

Yes, this strict check would make sense to me. I just don't know HFS good
enough whether this is a safe assumption to make so it would be good to
test with some HFS filesystem.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] hfs: Sanity check the root record

[Leo Stone]

On Tue, Nov 26, 2024 at 10:52:54PM +0100, Jan Kara wrote:
> Yes, this strict check would make sense to me. I just don't know HFS good
> enough whether this is a safe assumption to make so it would be good to
> test with some HFS filesystem.

I tested with the System 7.5.3 install disks, as well as 2 disks I
formatted inside System 7 in an emulator. The root directory always had
entrylength of 70, and it looks like every directory has an entrylength
of 70.

I'll resend as a PATCH v2.

Thanks,
Leo