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

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

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

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

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