Re: [PATCH v7] hfs: validate record ID against requested CNID in hfs_cat_find_brec()

[Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>]

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