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

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

[Viacheslav Dubeyko]

On Tue, 2026-03-31 at 10:12 +0900, Tetsuo Handa wrote:
> 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.

There are a lot of similarity between HFS and HFS+ b-trees functionality. Even
we can have a common code in the form of library shared between HFS and HFS+
code. HFS and HFS+ have pretty the same function names in b-tree
implementations.

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

/* The catalog record for a file */
struct hfs_cat_file {
	s8 type;			/* The type of entry */
	u8 reserved;
	u8 Flags;			/* Flags such as read-only */
	s8 Typ;				/* file version number = 0 */
	struct hfs_finfo UsrWds;	/* data used by the Finder */
	__be32 FlNum;			/* The CNID */
	__be16 StBlk;			/* obsolete */
	__be32 LgLen;			/* The logical EOF of the data fork*/
	__be32 PyLen;			/* The physical EOF of the data fork */
	__be16 RStBlk;			/* obsolete */
	__be32 RLgLen;			/* The logical EOF of the rsrc fork */
	__be32 RPyLen;			/* The physical EOF of the rsrc fork */
	__be32 CrDat;			/* The creation date */
	__be32 MdDat;			/* The modified date */
	__be32 BkDat;			/* The last backup date */
	struct hfs_fxinfo FndrInfo;	/* more data for the Finder */
	__be16 ClpSize;			/* number of bytes to allocate
					   when extending files */
	hfs_extent_rec ExtRec;		/* first extent record
					   for the data fork */
	hfs_extent_rec RExtRec;		/* first extent record
					   for the resource fork */
	u32 Resrv;			/* reserved by Apple */
} __packed;

/* the catalog record for a directory */
struct hfs_cat_dir {
	s8 type;			/* The type of entry */
	u8 reserved;
	__be16 Flags;			/* flags */
	__be16 Val;			/* Valence: number of files and
					   dirs in the directory */
	__be32 DirID;			/* The CNID */
	__be32 CrDat;			/* The creation date */
	__be32 MdDat;			/* The modification date */
	__be32 BkDat;			/* The last backup date */
	struct hfs_dinfo UsrInfo;	/* data used by the Finder */
	struct hfs_dxinfo FndrInfo;	/* more data used by Finder */
	u8 Resrv[16];			/* reserved by Apple */
} __packed;

/* the catalog record for a thread */
struct hfs_cat_thread {
	s8 type;			/* The type of entry */
	u8 reserved[9];			/* reserved by Apple */
	__be32 ParID;			/* CNID of parent directory */
	struct hfs_name CName;		/* The name of this entry */
}  __packed;

/* A catalog tree record */
typedef union hfs_cat_rec {
	s8 type;			/* The type of entry */
	struct hfs_cat_file file;
	struct hfs_cat_dir dir;
	struct hfs_cat_thread thread;
} hfs_cat_rec;

Every record starts with type. And anyone can easily retrieve the type of
record. And, then, you can extract the CNID.

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

Please, check the HFS's on-disk layout.

> 
> I am waiting for your response on
> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.kernel.org_r_9f66743b-2D70e0-2D4886-2D884e-2D5203f5c02ed8-40I-2Dlove.SAKURA.ne.jp&d=DwICaQ&c=BSDicqBQBDjDI9RkVyTcHQ&r=q5bIm4AXMzc8NJu1_RGmnQ2fMWKq4Y4RAkElvUgSs00&m=V9I6gMC1If2D6irQzVVC1M0wKQW-kbErffC9npM7z-fyjkutPS0YOARfqxRsyUdc&s=MAn71bGJ4-F3OEEhjru0azzQc62bUcvfCsk1VndcjC4&e= 
> 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.
> 

Sorry, I am busy with other HFS/HFS+ issues.

Thanks,
Slava.