Re: [PATCH v4] hfs: Validate CNIDs in hfs_read_inode

[Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>]

On 2026/03/19 21:26, George Vernon wrote:
> March 19, 2026 at 9:57 AM, "Tetsuo Handa" <penguin-kernel@i-love.sakura.ne.jp mailto:penguin-kernel@i-love.sakura.ne.jp?to=%22Tetsuo%20Handa%22%20%3Cpenguin-kernel%40i-love.sakura.ne.jp%3E > wrote:
> 
> 
>>
>> On 2026/03/19 7:49, George Anthony Vernon wrote:
>>
>>>
>>> Tetsuo, what do you think about doing your check inside
>>>  hfs_cat_find_brec? Something a bit like this:
>>>
>> Currently hfs_cat_find_brec() is called by only hfs_fill_super()
>> with cnid == HFS_ROOT_CNID. Are you planning to add more callers?
>>
>> If no, my patch
>>
>>  - if (rec.type != HFS_CDR_DIR)
>>  + if (rec.type != HFS_CDR_DIR || rec.dir.DirID != cpu_to_be32(HFS_ROOT_CNID))
>>
>> is sufficient.
> 
> I'm trying to write a patch Slava will accept, maybe I didn't fully understand
> what he wanted but I think he wanted to fix hfs_cat_find_brec() so it should not
> return an incorrect cat record.

Neither do I understand. Is the diff below what Viacheslav Dubeyko wants?

 fs/hfs/catalog.c |   26 ++++++++++++++++----------
 fs/hfs/hfs_fs.h  |    4 ++--
 fs/hfs/super.c   |   11 +----------
 3 files changed, 19 insertions(+), 22 deletions(-)

diff --git a/fs/hfs/catalog.c b/fs/hfs/catalog.c
index 7f5339ee57c1..e24fe49d5850 100644
--- a/fs/hfs/catalog.c
+++ b/fs/hfs/catalog.c
@@ -184,31 +184,37 @@ int hfs_cat_keycmp(const btree_key *key1, const btree_key *key2)
 
 /* Try to get a catalog entry for given catalog id */
 // move to read_super???
-int hfs_cat_find_brec(struct super_block *sb, u32 cnid,
-		      struct hfs_find_data *fd)
+int hfs_cat_find_root_brec(struct super_block *sb, struct hfs_find_data *fd, hfs_cat_rec *rec)
 {
-	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));
+	hfs_cat_build_key(sb, fd->search_key, HFS_ROOT_CNID, NULL);
+	res = hfs_brec_read(fd, rec, sizeof(*rec));
 	if (res)
 		return res;
 
-	type = rec.type;
+	type = rec->type;
 	if (type != HFS_CDR_THD && type != HFS_CDR_FTH) {
 		pr_err("found bad thread record in catalog\n");
 		return -EIO;
 	}
 
-	fd->search_key->cat.ParID = rec.thread.ParID;
-	len = fd->search_key->cat.CName.len = rec.thread.CName.len;
+	fd->search_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);
+	memcpy(fd->search_key->cat.CName.name, rec->thread.CName.name, len);
+	res = hfs_brec_find(fd);
+	if (res)
+		return res;
+	if (fd->entrylength != sizeof(rec->dir))
+		return -EIO;
+	hfs_bnode_read(fd->bnode, rec, fd->entryoffset, sizeof(rec->dir));
+	if (rec->type != HFS_CDR_DIR || rec->dir.DirID != cpu_to_be32(HFS_ROOT_CNID))
+		return -EIO;
+	return 0;
 }
 
 static inline
diff --git a/fs/hfs/hfs_fs.h b/fs/hfs/hfs_fs.h
index ac0e83f77a0f..46641b60913f 100644
--- a/fs/hfs/hfs_fs.h
+++ b/fs/hfs/hfs_fs.h
@@ -145,8 +145,8 @@ extern int hfs_clear_vbm_bits(struct super_block *sb, u16 start, u16 count);
 /* catalog.c */
 extern int hfs_cat_keycmp(const btree_key *key1, const btree_key *key2);
 struct hfs_find_data;
-extern int hfs_cat_find_brec(struct super_block *sb, u32 cnid,
-			     struct hfs_find_data *fd);
+extern int hfs_cat_find_root_brec(struct super_block *sb, struct hfs_find_data *fd,
+				  hfs_cat_rec *rec);
 extern int hfs_cat_create(u32 cnid, struct inode *dir,
 			  const struct qstr *str, struct inode *inode);
 extern int hfs_cat_delete(u32 cnid, struct inode *dir, const struct qstr *str);
diff --git a/fs/hfs/super.c b/fs/hfs/super.c
index 2e52acf282b0..c0be872d4079 100644
--- a/fs/hfs/super.c
+++ b/fs/hfs/super.c
@@ -354,16 +354,7 @@ static int hfs_fill_super(struct super_block *sb, struct fs_context *fc)
 	res = hfs_find_init(HFS_SB(sb)->cat_tree, &fd);
 	if (res)
 		goto bail_no_root;
-	res = hfs_cat_find_brec(sb, HFS_ROOT_CNID, &fd);
-	if (!res) {
-		if (fd.entrylength != sizeof(rec.dir)) {
-			res =  -EIO;
-			goto bail_hfs_find;
-		}
-		hfs_bnode_read(fd.bnode, &rec, fd.entryoffset, fd.entrylength);
-		if (rec.type != HFS_CDR_DIR || rec.dir.DirID != cpu_to_be32(HFS_ROOT_CNID))
-			res = -EIO;
-	}
+	res = hfs_cat_find_root_brec(sb, &fd, &rec);
 	if (res)
 		goto bail_hfs_find;
 	res = -EINVAL;