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

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

[Tetsuo Handa]

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
commit b905bafdea21 ("hfs: Sanity check the root record") checked
the record size and the record type but did not check the inode number.

Viacheslav Dubeyko considers that the fix should be in hfs_read_inode()
but Viacheslav has no time for proposing the fix [1]. Also, we can't
guarantee that the inode number of the record retrieved as a result of
hfs_cat_find_brec(HFS_ROOT_CNID) is HFS_ROOT_CNID if we validate only in
hfs_read_inode(). Therefore, while what Viacheslav would propose might
partially overwrap with my proposal, let's fix an 1000+ days old bug by
adding a sanity check in hfs_fill_super().

Reported-by: syzbot+97e301b4b82ae803d21b@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=97e301b4b82ae803d21b
Link: https://lkml.kernel.org/r/a3d1464ee40df7f072ea1c19e1ccf533e34554ca.camel@ibm.com [1]
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 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 388a318297ec..ae6dbc4bb813 100644
--- a/fs/hfs/super.c
+++ b/fs/hfs/super.c
@@ -354,7 +354,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))
 			res = -EIO;
 	}
 	if (res)
-- 
2.51.0

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

[Viacheslav Dubeyko]

On Fri, 2025-09-12 at 23:59 +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
> commit b905bafdea21 ("hfs: Sanity check the root record") checked
> the record size and the record type but did not check the inode number.
> 
> Viacheslav Dubeyko considers that the fix should be in hfs_read_inode()
> but Viacheslav has no time for proposing the fix [1]. Also, we can't
> guarantee that the inode number of the record retrieved as a result of
> hfs_cat_find_brec(HFS_ROOT_CNID) is HFS_ROOT_CNID if we validate only in
> hfs_read_inode(). Therefore, while what Viacheslav would propose might
> partially overwrap with my proposal, let's fix an 1000+ days old bug by
> adding a sanity check in hfs_fill_super().
> 

I cannot accept any fix with such comment. The commit message should explain the
issue and fix nature.

We are not in a hurry. We should fix the reason of the issue even if it is
"1000+ days old bug".

Thanks,
Slava.

> Reported-by: syzbot+97e301b4b82ae803d21b@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=97e301b4b82ae803d21b  
> Link: https://lkml.kernel.org/r/a3d1464ee40df7f072ea1c19e1ccf533e34554ca.camel@ibm.com   [1]
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
>  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 388a318297ec..ae6dbc4bb813 100644
> --- a/fs/hfs/super.c
> +++ b/fs/hfs/super.c
> @@ -354,7 +354,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))
>  			res = -EIO;
>  	}
>  	if (res)

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

[Tetsuo Handa]

On 2025/09/16 7:14, Viacheslav Dubeyko wrote:
> On Fri, 2025-09-12 at 23:59 +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
>> commit b905bafdea21 ("hfs: Sanity check the root record") checked
>> the record size and the record type but did not check the inode number.
>>
>> Viacheslav Dubeyko considers that the fix should be in hfs_read_inode()
>> but Viacheslav has no time for proposing the fix [1]. Also, we can't
>> guarantee that the inode number of the record retrieved as a result of
>> hfs_cat_find_brec(HFS_ROOT_CNID) is HFS_ROOT_CNID if we validate only in
>> hfs_read_inode(). Therefore, while what Viacheslav would propose might
>> partially overwrap with my proposal, let's fix an 1000+ days old bug by
>> adding a sanity check in hfs_fill_super().
>>
> 
> I cannot accept any fix with such comment. The commit message should explain the
> issue and fix nature.

Then, see v4 at https://lkml.kernel.org/r/427fcb57-8424-4e52-9f21-7041b2c4ae5b@I-love.SAKURA.ne.jp

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

[Tetsuo Handa]

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@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=97e301b4b82ae803d21b
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
George Anthony Vernon is trying to fix this bug without my patch
( https://lkml.kernel.org/r/20260311211309.27856-1-contact@gvernon.com ).
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.

 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))
 			res = -EIO;
 	}
 	if (res)
-- 
2.53.0

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

[George Anthony Vernon]

On Sat, Mar 28, 2026 at 04:12:07PM +0900, Tetsuo Handa wrote:
> George Anthony Vernon is trying to fix this bug without my patch
> ( https://lkml.kernel.org/r/20260311211309.27856-1-contact@gvernon.com ).
> 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.
Thanks for the CC. Slava (Viacheslav) got in contact with me directly on
Tuesday 24th to say he missed some emails and acknowledged not being
completely correct about the thread record logic which we were talking
about. He indicated he would take a look at the next patch version when
he received it.

I will follow this thread with interest. In the meantime I will follow
up my patch with a new version, thanks for your review of it.

Your patch validates the superblock lookup, however my patch which you
linked validates reserved CNIDs retrieved from a bad cat record on disk,
which is also a reachable logical path which hits the BUG() on
writeback. I can consistently reproduce that BUG() with your patch
applied.

Both patches are therefore necessary.

Hope this helps,

George

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@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=97e301b4b82ae803d21b
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
> George Anthony Vernon is trying to fix this bug without my patch
> ( https://lkml.kernel.org/r/20260311211309.27856-1-contact@gvernon.com ).
> 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

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

[Tetsuo Handa]

On 2026/03/31 6:45, Viacheslav Dubeyko wrote:
> 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);

What we are talking about is not hfsplus but hfs.
I can't catch why you are talking about hfsplus function.

> 
> 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.

hfs_brec_read() indeed calls hfs_brec_find(). But I can't interpret how to extract
CNID as of returning from hfs_brec_find().

  int hfs_brec_read(struct hfs_find_data *fd, void *rec, u32 rec_len)
  {
  	int res;
  
  	res = hfs_brec_find(fd);
  	if (res)
  		return res;
  	if (fd->entrylength > rec_len)
  		return -EINVAL;
  	hfs_bnode_read(fd->bnode, rec, fd->entryoffset, fd->entrylength);
  	return 0;
  }

Since hfs_brec_read() doesn't know which type of struct (one of "struct hfs_cat_file",
"struct hfs_cat_dir" or "struct hfs_cat_thread") does the caller of hfs_brec_read()
want to read, I don't think hfs_brec_read() can tell whether the CNID is correct.

I am waiting for your response on
https://lkml.kernel.org/r/9f66743b-70e0-4886-884e-5203f5c02ed8@I-love.SAKURA.ne.jp
where the caller of hfs_brec_read() can tell whether the CNID is correct. But such
change is a matter of preference.

You can respond with your patch which will be much faster.