public inbox for linux-fsdevel@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3] hfs: Validate CNIDs in hfs_read_inode
@ 2026-03-10  0:08 George Anthony Vernon
  2026-03-10 10:41 ` Tetsuo Handa
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: George Anthony Vernon @ 2026-03-10  0:08 UTC (permalink / raw)
  To: slava, glaubitz, frank.li
  Cc: George Anthony Vernon, linux-fsdevel, linux-kernel,
	syzbot+97e301b4b82ae803d21b, Tetsuo Handa

hfs_read_inode previously did not validate CNIDs read from disk, thereby
allowing inodes to be constructed with disallowed CNIDs and placed on
the dirty list, eventually hitting a bug on writeback.

Validate reserved CNIDs as specified for HFS according to
"Inside Macintosh: Files."

This issue was discussed at length on LKML previously, the discussion
is linked below.

Syzbot tested this patch on mainline and the bug did not replicate.
This patch was regression tested by issuing various system calls on a
mounted HFS filesystem and validating that file creation, deletion,
reads and writes all work.

Link: https://lore.kernel.org/all/427fcb57-8424-4e52-9f21-7041b2c4ae5b@
I-love.SAKURA.ne.jp/T/
Reported-by: syzbot+97e301b4b82ae803d21b@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=97e301b4b82ae803d21b
Cc: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Tested-by: syzbot+97e301b4b82ae803d21b@syzkaller.appspotmail.com
Signed-off-by: George Anthony Vernon <contact@gvernon.com>
---

Sorry there was a long wait for V3! I have now reviewed the feedback
given in response to V2, which I very greatly appreciate.

Most of the changes here are directly implementing changes requested. I
disagree that CNID 5 should be considered valid and have added some
comments referencing the documentation. This is linked to the change
from is_valid_cnid() -> is_valid_catalog_record(). I believe it is now
semantically correct, because CNID 5 is a valid CNID, but it can not
belong to a catalog record.

Thanks,

George

Changes from V2->V3:
- Use HFS-specific references in preference to TN1150
- Remove Tetsuo's additional superblock check from patch series
- Rename is_valid_cnid() -> is_valid_catalog_record()
- Add static inline hfs_inode_type() helper function
- Do not BUG() on detected bad inode, use pr_warn()

Changes from V1->V2:
- is_valid_cnid prototype now takes u32 and u8 types
- Add fsck advice in dmesg
- Replace make_bad_inode calls in hfs_read_inode with gotos
- Reuse same check in hfs_write_inode
- Disallow HFS_POR_CNID, HFS_BAD_CNID, and HFS_EXCH_CNID
- Add Tetsuo's patch to mine and make it a patch series

 fs/hfs/inode.c | 76 ++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 62 insertions(+), 14 deletions(-)

diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c
index 878535db64d6..db31b9840371 100644
--- a/fs/hfs/inode.c
+++ b/fs/hfs/inode.c
@@ -340,6 +340,42 @@ static int hfs_test_inode(struct inode *inode, void *data)
 	}
 }
 
+/*
+ * is_valid_catalog_record
+ *
+ * Validate the CNID of a catalog record
+ */
+static inline
+bool is_valid_catalog_record(u32 cnid, u8 type)
+{
+	if (likely(cnid >= HFS_FIRSTUSER_CNID))
+		return true;
+
+	switch (cnid) {
+	case HFS_ROOT_CNID:
+		return type == HFS_CDR_DIR;
+	case HFS_EXT_CNID:
+	case HFS_CAT_CNID:
+		return type == HFS_CDR_FIL;
+	case HFS_POR_CNID:
+		/* No valid record with this CNID */
+		break;
+	case HFS_BAD_CNID:
+		/*
+		 * Bad block file "doesn't have a file record in the catalog" as per TN1150 (HFS+).
+		 * Inside Macintosh: Files chapter 5-8 tells us for plain old HFS:
+		 * "... the bad block sparing algorithm does not create any new
+		 * entries in the volume's catalog file".
+		 */
+		break;
+	default:
+		/* Invalid reserved CNID */
+		break;
+	}
+
+	return false;
+}
+
 /*
  * hfs_read_inode
  */
@@ -369,6 +405,8 @@ static int hfs_read_inode(struct inode *inode, void *data)
 	rec = idata->rec;
 	switch (rec->type) {
 	case HFS_CDR_FIL:
+		if (!is_valid_catalog_record(rec->file.FlNum, HFS_CDR_FIL))
+			goto make_bad_inode;
 		if (!HFS_IS_RSRC(inode)) {
 			hfs_inode_read_fork(inode, rec->file.ExtRec, rec->file.LgLen,
 					    rec->file.PyLen, be16_to_cpu(rec->file.ClpSize));
@@ -390,6 +428,8 @@ static int hfs_read_inode(struct inode *inode, void *data)
 		inode->i_mapping->a_ops = &hfs_aops;
 		break;
 	case HFS_CDR_DIR:
+		if (!is_valid_catalog_record(rec->dir.DirID, HFS_CDR_DIR))
+			goto make_bad_inode;
 		inode->i_ino = be32_to_cpu(rec->dir.DirID);
 		inode->i_size = be16_to_cpu(rec->dir.Val) + 2;
 		HFS_I(inode)->fs_blocks = 0;
@@ -399,8 +439,13 @@ static int hfs_read_inode(struct inode *inode, void *data)
 		inode->i_op = &hfs_dir_inode_operations;
 		inode->i_fop = &hfs_dir_operations;
 		break;
+	make_bad_inode:
+		pr_warn("Invalid cnid %lu\n", inode->i_ino);
+		pr_warn("Volume is probably corrupted, try performing fsck.\n");
+		fallthrough;
 	default:
 		make_bad_inode(inode);
+		break;
 	}
 	return 0;
 }
@@ -448,6 +493,11 @@ void hfs_inode_write_fork(struct inode *inode, struct hfs_extent *ext,
 					 HFS_SB(inode->i_sb)->alloc_blksz);
 }
 
+static inline u8 hfs_inode_type(struct inode *inode)
+{
+	return S_ISDIR(inode->i_mode) ? HFS_CDR_DIR : HFS_CDR_FIL;
+}
+
 int hfs_write_inode(struct inode *inode, struct writeback_control *wbc)
 {
 	struct inode *main_inode = inode;
@@ -460,20 +510,18 @@ int hfs_write_inode(struct inode *inode, struct writeback_control *wbc)
 	if (res)
 		return res;
 
-	if (inode->i_ino < HFS_FIRSTUSER_CNID) {
-		switch (inode->i_ino) {
-		case HFS_ROOT_CNID:
-			break;
-		case HFS_EXT_CNID:
-			hfs_btree_write(HFS_SB(inode->i_sb)->ext_tree);
-			return 0;
-		case HFS_CAT_CNID:
-			hfs_btree_write(HFS_SB(inode->i_sb)->cat_tree);
-			return 0;
-		default:
-			BUG();
-			return -EIO;
-		}
+	if (!is_valid_catalog_record(inode->i_ino, hfs_inode_type(inode)))
+		return -EIO;
+
+	switch (inode->i_ino) {
+	case HFS_EXT_CNID:
+		hfs_btree_write(HFS_SB(inode->i_sb)->ext_tree);
+		return 0;
+	case HFS_CAT_CNID:
+		hfs_btree_write(HFS_SB(inode->i_sb)->cat_tree);
+		return 0;
+	default:
+		break;
 	}
 
 	if (HFS_IS_RSRC(inode))
-- 
2.53.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2026-03-11 20:32 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-03-10  0:08 [PATCH v3] hfs: Validate CNIDs in hfs_read_inode George Anthony Vernon
2026-03-10 10:41 ` Tetsuo Handa
2026-03-10 21:29   ` Viacheslav Dubeyko
2026-03-10 14:39 ` kernel test robot
2026-03-10 21:28 ` Viacheslav Dubeyko
2026-03-11 20:31   ` George Anthony Vernon

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox