Re: [PATCH v5] hfs: update sanity check of the root record

[Viacheslav Dubeyko]

On Sat, 2026-03-28 at 16:12 +0900, Tetsuo Handa wrote:
> syzbot is reporting that BUG() in hfs_write_inode() fires upon unmount
> operation when the inode number of the record retrieved as a result of
> hfs_cat_find_brec(HFS_ROOT_CNID) is not HFS_ROOT_CNID, for the inode
> number is assigned using 32bits integer retrieved from relevant offset of
> the file system image.
> 
> Since commit b905bafdea21 ("hfs: Sanity check the root record") checks only
> the record size and the record type, let's also check the inode number.
> 
> Since hfs_cat_find_brec() is called by only hfs_fill_super() with cnid ==
> HFS_ROOT_CNID, we could move hfs_bnode_read() and related validations to
> inside hfs_cat_find_brec(). But such change is a matter of preference
> while this 3+ years old bug is effectively the top crasher for syzbot.
> Let me immediately stop wasting syzbot's computation resource.
> 
> Reported-by: syzbot+97e301b4b82ae803d21b@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=97e301b4b82ae803d21b
> Signed-off-by: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx>
> ---
> George Anthony Vernon is trying to fix this bug without my patch
> ( https://lkml.kernel.org/r/20260311211309.27856-1-contact@xxxxxxxxxxx ).
> But it was proven that George's patch cannot fix this bug. Therefore,
> I believe that we came to a conclusion that applying my patch is
> the way to go. We can apply George's patch as a further improvement.
> But unfortunately Viacheslav Dubeyko (the maintainer of HFS) is not
> responding. Therefore, I'm sending this patch to more developers who
> might accept this patch, instead of waiting for Viacheslav.

It's completely not correct. I am reviewing patches in time when I have enough
free time for it. 

> 
>  fs/hfs/super.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/hfs/super.c b/fs/hfs/super.c
> index a4f2a2bfa6d3..2e52acf282b0 100644
> --- a/fs/hfs/super.c
> +++ b/fs/hfs/super.c
> @@ -361,7 +361,7 @@ 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)
> +		if (rec.type != HFS_CDR_DIR || rec.dir.DirID != cpu_to_be32(HFS_ROOT_CNID))

I've already reviewed this patch. And I am not agree with this suggestion.

We prepare the key with HFSPLUS_ROOT_CNID [1]:

err = hfsplus_cat_build_key(sb, fd.search_key, HFSPLUS_ROOT_CNID, &str);

The hfs_brec_read() executes the search of the record [2]:

	res = hfs_brec_find(fd);
	if (res)
		return res;

The hfs_brec_find() should found the record for requested key. And if the found
thread record contains not correct CNID, then we can check the found thread
record and return error as the result of the search.

Thanks,
Slava.

[1] https://elixir.bootlin.com/linux/v7.0-rc6/source/fs/hfsplus/super.c#L571
[2] https://elixir.bootlin.com/linux/v7.0-rc6/source/fs/hfs/bfind.c#L174