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.

Initially, Viacheslav Dubeyko was assuming that we can fix this problem
by adding validation to hfs_read_inode(), and George Anthony Vernon is
proposing a patch that adds validation to hfs_read_inode().

While I am not against adding validation to hfs_read_inode(), treating
an inode which is not the root inode as if the root inode is a logical
error which should be rejected regardless of whether we hit BUG() or not.
And we confirmed that we can't fix this logical error by adding validation
to hfs_read_inode().

Since hfs_brec_read() doesn't receive 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 as function argument, currently
it is impossible for hfs_brec_read() to return 0 only when the retrieved
record is what the caller wants, despite the HFS's on-disk layout includes
type of struct within the retrieved record.

Therefore, validate the inode number within hfs_cat_find_brec(), for
it seems that Viacheslav Dubeyko does not want to touch hfs_fill_super().
Unfortunately, we cannot remove rec.type == HFS_CDR_DIR test from
hfs_fill_super() despite hfs_cat_find_brec() guarantees that it returns 0
only when the retrieved cnid is the requested one, for the retrieved cnid
by chance matches the requested cnid with HFS_CDR_FIL type when when the
caller wants HFS_CDR_DIR. Also, since hfs_cat_find_brec() does not return
which record type it found, hfs_fill_super() needs to call hfs_bnode_read()
again. Not optimal but inevitable if we don't want to touch
hfs_fill_super()...

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>
---
To: Viacheslav Dubeyko

Since hfs_cat_find_brec() is currently called from only hfs_fill_super() with
cnid == HFS_ROOT_CNID, we know that hfs_cat_find_brec() can assume that the caller wants
HFS_CDR_DIR. But since I didn't get response from you on the optimized version
at https://lore.kernel.org/all/9f66743b-70e0-4886-884e-5203f5c02ed8@I-love.SAKURA.ne.jp/ ,
I can't understand what you meant from the "Please, check the HFS's on-disk layout." response
at https://lore.kernel.org/all/c732a6e39e833b4b63499059cd388b753e1b2297.camel@redhat.com/ .
Do you want to add "which type of struct the caller wants" argument to hfs_brec_read()
so that we can validate record type and fd->entrylength inside hfs_brec_read() ?
Do you want to add "which type of struct the caller wants" argument to hfs_cat_find_brec()
so that we can eliminate the need to call hfs_bnode_read() from both functions?
Unless you have a plan to call hfs_cat_find_brec() from other locations, we should avoid
adding unused code path.

 fs/hfs/catalog.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/fs/hfs/catalog.c b/fs/hfs/catalog.c
index 7f5339ee57c1..bac0218a736c 100644
--- a/fs/hfs/catalog.c
+++ b/fs/hfs/catalog.c
@@ -208,7 +208,20 @@ int hfs_cat_find_brec(struct super_block *sb, u32 cnid,
 		return -EIO;
 	}
 	memcpy(fd->search_key->cat.CName.name, rec.thread.CName.name, len);
-	return hfs_brec_find(fd);
+	res = hfs_brec_find(fd);
+	if (res)
+		return res;
+	if (rec.type == HFS_CDR_THD && fd->entrylength == sizeof(rec.dir)) {
+		hfs_bnode_read(fd->bnode, &rec.dir, fd->entryoffset, sizeof(rec.dir));
+		if (rec.type == HFS_CDR_DIR && cnid == be32_to_cpu(rec.dir.DirID))
+			return 0;
+	} else if (rec.type == HFS_CDR_FTH && fd->entrylength == sizeof(rec.file)) {
+		hfs_bnode_read(fd->bnode, &rec.file, fd->entryoffset, sizeof(rec.file));
+		if (rec.type == HFS_CDR_FIL && cnid == be32_to_cpu(rec.file.FlNum))
+			return 0;
+	}
+	pr_err("found corrupted record in catalog\n");
+	return -EIO;
 }
 
 static inline
-- 
2.54.0

On Thu, 2026-05-14 at 16:34 +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.
> 
> Initially, Viacheslav Dubeyko was assuming that we can fix this problem
> by adding validation to hfs_read_inode(), and George Anthony Vernon is
> proposing a patch that adds validation to hfs_read_inode().
> 

We can fix the problem in by adding validation to hfs_read_inode().

> While I am not against adding validation to hfs_read_inode(), treating
> an inode which is not the root inode as if the root inode is a logical
> error which should be rejected regardless of whether we hit BUG() or not.
> And we confirmed that we can't fix this logical error by adding validation
> to hfs_read_inode().
> 

We haven't confirmed it. The issue can be fixed by adding validation to
hfs_read_inode().

> Since hfs_brec_read() doesn't receive 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 as function argument, currently
> it is impossible for hfs_brec_read() to return 0 only when the retrieved
> record is what the caller wants, despite the HFS's on-disk layout includes
> type of struct within the retrieved record.

The hfs_brec_read() is called with arguments [1]:

int hfs_brec_read(struct hfs_find_data *fd, void *rec, u32 rec_len);

So, we have void *rec as a buffer for read. The hfs_brec_read() is called to
extract the b-tree record, for example [2-4]:

int hfs_cat_find_brec(struct super_block *sb, u32 cnid,
		      struct hfs_find_data *fd)
{
	hfs_cat_rec rec;

<skipped>

	res = hfs_brec_read(fd, &rec, sizeof(rec));
	if (res)
		return res;

<skipped>
}

So, the goal of this function is to extract the hfs_cat_rec [5]:

/* 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;

It is possible to see that hfs_cat_rec starts from type field [6]. This type
field can be checked to make a conclusion which type of record was read by
hfs_brec_read(). Technically speaking, we can change void *rec on hfs_cat_rec
*rec:

int hfs_brec_read(struct hfs_find_data *fd, hfs_cat_rec *rec, u32 rec_len);

or we can cast void *rec into hfs_cat_rec *rec. Also, we can introduce
specialized hfs_brec_read() for Catalog File. So, I don't see any problem to
check the type of the record in the hfs_brec_read().

Also, we have struct hfs_find_data as an input argument:

struct hfs_find_data {
	btree_key *key;
	btree_key *search_key;
	struct hfs_btree *tree;
	struct hfs_bnode *bnode;
	int record;
	int keyoffset, keylength;
	int entryoffset, entrylength;
};

We build the key before the search:

int hfs_cat_find_brec(struct super_block *sb, u32 cnid,
		      struct hfs_find_data *fd)
{
	hfs_cat_rec rec;
	int res, len, type;

	hfs_cat_build_key(sb, fd->search_key, cnid, NULL);
	res = hfs_brec_read(fd, &rec, sizeof(rec));
	if (res)
		return res;

	type = rec.type;
	if (type != HFS_CDR_THD && type != HFS_CDR_FTH) {
		pr_err("found bad thread record in catalog\n");
		return -EIO;
	}

	fdsearch_key->cat.ParID = rec.thread.ParID;
	len = fd->search_key->cat.CName.len = rec.thread.CName.len;
	if (len > HFS_NAMELEN) {
		pr_err("bad catalog namelength\n");
		return -EIO;
	}
	memcpy(fd->search_key->cat.CName.name, rec.thread.CName.name, len);
	return hfs_brec_find(fd);
}

void hfs_cat_build_key(struct super_block *sb, btree_key *key, u32 parent, const
struct qstr *name)
{
	key->cat.reserved = 0;
	key->cat.ParID = cpu_to_be32(parent);
	if (name) {
		hfs_asc2mac(sb, &key->cat.CName, name);
		key->key_len = 6 + key->cat.CName.len;
	} else {
		memset(&key->cat.CName, 0, sizeof(struct hfs_name));
		key->key_len = 6;
	}
}

So, we keep CNID in the key->cat.ParID. As far as I can see, we have everything
for check. We have the pointer on tree in struct hfs_find_data and we can make
conclusion which key need to be used based on tree type.

Thanks,
Slava.

[1] https://elixir.bootlin.com/linux/v7.1-rc3/source/fs/hfs/bfind.c#L170
[2] https://elixir.bootlin.com/linux/v7.1-rc3/source/fs/hfs/catalog.c#L187
[3] https://elixir.bootlin.com/linux/v7.1-rc3/source/fs/hfs/dir.c#L20
[4] https://elixir.bootlin.com/linux/v7.1-rc3/source/fs/hfs/inode.c#L544
[5]
https://elixir.bootlin.com/linux/v7.1-rc3/source/include/linux/hfs_common.h#L447
[6]
https://elixir.bootlin.com/linux/v7.1-rc3/source/include/linux/hfs_common.h#L89

On 2026/05/16 6:10, Viacheslav Dubeyko wrote:
> On Thu, 2026-05-14 at 16:34 +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.
>>
>> Initially, Viacheslav Dubeyko was assuming that we can fix this problem
>> by adding validation to hfs_read_inode(), and George Anthony Vernon is
>> proposing a patch that adds validation to hfs_read_inode().
>>
> 
> We can fix the problem in by adding validation to hfs_read_inode().

No, we can't. We can't fix a logical error that hfs_fill_super() by error
accepts an inode which is not the root inode, by adding validation to
hfs_read_inode().

> 
>> While I am not against adding validation to hfs_read_inode(), treating
>> an inode which is not the root inode as if the root inode is a logical
>> error which should be rejected regardless of whether we hit BUG() or not.
>> And we confirmed that we can't fix this logical error by adding validation
>> to hfs_read_inode().
>>
> 
> We haven't confirmed it. The issue can be fixed by adding validation to
> hfs_read_inode().

We already confirmed it, you forgot it.
https://lkml.kernel.org/r/b7318588-33b2-4dc6-9469-e11da855f8ad@I-love.SAKURA.ne.jp

On Sat, 2026-05-16 at 15:17 +0900, Tetsuo Handa wrote:
> On 2026/05/16 6:10, Viacheslav Dubeyko wrote:
> > On Thu, 2026-05-14 at 16:34 +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.
> > > 
> > > Initially, Viacheslav Dubeyko was assuming that we can fix this problem
> > > by adding validation to hfs_read_inode(), and George Anthony Vernon is
> > > proposing a patch that adds validation to hfs_read_inode().
> > > 
> > 
> > We can fix the problem in by adding validation to hfs_read_inode().
> 
> No, we can't. We can't fix a logical error that hfs_fill_super() by error
> accepts an inode which is not the root inode, by adding validation to
> hfs_read_inode().
> 

We can. And I explained the way.

> > 
> > > While I am not against adding validation to hfs_read_inode(), treating
> > > an inode which is not the root inode as if the root inode is a logical
> > > error which should be rejected regardless of whether we hit BUG() or not.
> > > And we confirmed that we can't fix this logical error by adding validation
> > > to hfs_read_inode().
> > > 
> > 
> > We haven't confirmed it. The issue can be fixed by adding validation to
> > hfs_read_inode().
> 
> We already confirmed it, you forgot it.
> https://lkml.kernel.org/r/b7318588-33b2-4dc6-9469-e11da855f8ad@I-love.SAKURA.ne.jp
> 

No. we haven't confirmed it. We can do it.

Thanks,
Slava.