[PATCH v3] hfs: Validate CNIDs in hfs_read_inode

[PATCH v3] hfs: Validate CNIDs in hfs_read_inode

[George Anthony Vernon]

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

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

[Tetsuo Handa]

On 2026/03/10 9:08, George Anthony Vernon wrote:
> - Remove Tetsuo's additional superblock check from patch series

Why do we want to make mount() succeed for CNIDs other than HFS_ROOT_CNID ?

  0 => EINVAL
  HFS_EXT_CNID => ENOTDIR
  HFS_CAT_CNID => ENOTDIR
  All others => succeed

Fuzzing against unreadable root directory is not useful.

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

[kernel test robot]

Hi George,

kernel test robot noticed the following build warnings:

[auto build test WARNING on brauner-vfs/vfs.all]
[also build test WARNING on linus/master v7.0-rc3 next-20260309]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/George-Anthony-Vernon/hfs-Validate-CNIDs-in-hfs_read_inode/20260310-081836
base:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git vfs.all
patch link:    https://lore.kernel.org/r/20260310000826.242674-1-contact%40gvernon.com
patch subject: [PATCH v3] hfs: Validate CNIDs in hfs_read_inode
config: microblaze-randconfig-r132-20260310 (https://download.01.org/0day-ci/archive/20260310/202603102229.zVuUXq6v-lkp@intel.com/config)
compiler: microblaze-linux-gcc (GCC) 9.5.0
sparse: v0.6.5-rc1
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260310/202603102229.zVuUXq6v-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202603102229.zVuUXq6v-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> fs/hfs/inode.c:408:55: sparse: sparse: incorrect type in argument 1 (different base types) @@     expected unsigned int [usertype] cnid @@     got restricted __be32 [usertype] FlNum @@
   fs/hfs/inode.c:408:55: sparse:     expected unsigned int [usertype] cnid
   fs/hfs/inode.c:408:55: sparse:     got restricted __be32 [usertype] FlNum
>> fs/hfs/inode.c:431:54: sparse: sparse: incorrect type in argument 1 (different base types) @@     expected unsigned int [usertype] cnid @@     got restricted __be32 [usertype] DirID @@
   fs/hfs/inode.c:431:54: sparse:     expected unsigned int [usertype] cnid
   fs/hfs/inode.c:431:54: sparse:     got restricted __be32 [usertype] DirID

vim +408 fs/hfs/inode.c

   378	
   379	/*
   380	 * hfs_read_inode
   381	 */
   382	static int hfs_read_inode(struct inode *inode, void *data)
   383	{
   384		struct hfs_iget_data *idata = data;
   385		struct hfs_sb_info *hsb = HFS_SB(inode->i_sb);
   386		hfs_cat_rec *rec;
   387	
   388		HFS_I(inode)->flags = 0;
   389		HFS_I(inode)->rsrc_inode = NULL;
   390		mutex_init(&HFS_I(inode)->extents_lock);
   391		INIT_LIST_HEAD(&HFS_I(inode)->open_dir_list);
   392		spin_lock_init(&HFS_I(inode)->open_dir_lock);
   393	
   394		/* Initialize the inode */
   395		inode->i_uid = hsb->s_uid;
   396		inode->i_gid = hsb->s_gid;
   397		set_nlink(inode, 1);
   398	
   399		if (idata->key)
   400			HFS_I(inode)->cat_key = *idata->key;
   401		else
   402			HFS_I(inode)->flags |= HFS_FLG_RSRC;
   403		HFS_I(inode)->tz_secondswest = sys_tz.tz_minuteswest * 60;
   404	
   405		rec = idata->rec;
   406		switch (rec->type) {
   407		case HFS_CDR_FIL:
 > 408			if (!is_valid_catalog_record(rec->file.FlNum, HFS_CDR_FIL))
   409				goto make_bad_inode;
   410			if (!HFS_IS_RSRC(inode)) {
   411				hfs_inode_read_fork(inode, rec->file.ExtRec, rec->file.LgLen,
   412						    rec->file.PyLen, be16_to_cpu(rec->file.ClpSize));
   413			} else {
   414				hfs_inode_read_fork(inode, rec->file.RExtRec, rec->file.RLgLen,
   415						    rec->file.RPyLen, be16_to_cpu(rec->file.ClpSize));
   416			}
   417	
   418			inode->i_ino = be32_to_cpu(rec->file.FlNum);
   419			inode->i_mode = S_IRUGO | S_IXUGO;
   420			if (!(rec->file.Flags & HFS_FIL_LOCK))
   421				inode->i_mode |= S_IWUGO;
   422			inode->i_mode &= ~hsb->s_file_umask;
   423			inode->i_mode |= S_IFREG;
   424			inode_set_mtime_to_ts(inode,
   425					      inode_set_atime_to_ts(inode, inode_set_ctime_to_ts(inode, hfs_m_to_utime(rec->file.MdDat))));
   426			inode->i_op = &hfs_file_inode_operations;
   427			inode->i_fop = &hfs_file_operations;
   428			inode->i_mapping->a_ops = &hfs_aops;
   429			break;
   430		case HFS_CDR_DIR:
 > 431			if (!is_valid_catalog_record(rec->dir.DirID, HFS_CDR_DIR))
   432				goto make_bad_inode;
   433			inode->i_ino = be32_to_cpu(rec->dir.DirID);
   434			inode->i_size = be16_to_cpu(rec->dir.Val) + 2;
   435			HFS_I(inode)->fs_blocks = 0;
   436			inode->i_mode = S_IFDIR | (S_IRWXUGO & ~hsb->s_dir_umask);
   437			inode_set_mtime_to_ts(inode,
   438					      inode_set_atime_to_ts(inode, inode_set_ctime_to_ts(inode, hfs_m_to_utime(rec->dir.MdDat))));
   439			inode->i_op = &hfs_dir_inode_operations;
   440			inode->i_fop = &hfs_dir_operations;
   441			break;
   442		make_bad_inode:
   443			pr_warn("Invalid cnid %lu\n", inode->i_ino);
   444			pr_warn("Volume is probably corrupted, try performing fsck.\n");
   445			fallthrough;
   446		default:
   447			make_bad_inode(inode);
   448			break;
   449		}
   450		return 0;
   451	}
   452	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Viacheslav Dubeyko]

On Tue, 2026-03-10 at 00:08 +0000, George Anthony Vernon wrote:
> 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://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_all_427fcb57-2D8424-2D4e52-2D9f21-2D7041b2c4ae5b-40&d=DwIDAg&c=BSDicqBQBDjDI9RkVyTcHQ&r=q5bIm4AXMzc8NJu1_RGmnQ2fMWKq4Y4RAkElvUgSs00&m=AzLgd6KhZfr8AykzDg76T-sDUALdUDSC68BUfheUxxhq-KIglGTX6mtAci6dTupM&s=nuXDj9Z8fvUpvnJIyDsfPIi-YuBzvbXxQUhypXeaheU&e= 
> I-love.SAKURA.ne.jp/T/
> Reported-by: syzbot+97e301b4b82ae803d21b@syzkaller.appspotmail.com
> Closes: https://urldefense.proofpoint.com/v2/url?u=https-3A__syzkaller.appspot.com_bug-3Fextid-3D97e301b4b82ae803d21b&d=DwIDAg&c=BSDicqBQBDjDI9RkVyTcHQ&r=q5bIm4AXMzc8NJu1_RGmnQ2fMWKq4Y4RAkElvUgSs00&m=AzLgd6KhZfr8AykzDg76T-sDUALdUDSC68BUfheUxxhq-KIglGTX6mtAci6dTupM&s=lGn13lU4-Np727qrFQB17Y-qYKD4fRkJ3gSkQYQH8cg&e= 
> 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:

If we don't process this ID, then default case will be completely enough. We
don't need to introduce this as a special case.

> +		/* 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".
> +		 */

Yes, HFS driver (bad block sparing algorithm) will not create this file because
this file could be created by mkfs tool. And the bad block sparing algorithm
could simply read this file and add new items (bad sectors). This CNID could be
in Catalog File because it could be created by mkfs tool.

Anyway, I think that probability to have the Bad block file is really low. If
you really insist not to process this CNID, then I suggest completely remove
this case and comments.

> +		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;

I think to use the rec->type is better here than HFS_CDR_FIL.

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

Ditto. Let's use rec->type.

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

Why this has been removed:

-		default:
-			BUG();
-			return -EIO;
-		}


>  	}
>  
>  	if (HFS_IS_RSRC(inode))


You completely missed the hfs_fill_super() logic:

	root_inode = hfs_iget(sb, &fd.search_key->cat, &rec);
	hfs_find_exit(&fd);
	if (!root_inode)
		goto bail_no_root;

We need to process it specially now because we've created bad inode.

Thanks,
Slava.

RE: [PATCH v3] hfs: Validate CNIDs in hfs_read_inode

[Viacheslav Dubeyko]

On Tue, 2026-03-10 at 19:41 +0900, Tetsuo Handa wrote:
> On 2026/03/10 9:08, George Anthony Vernon wrote:
> > - Remove Tetsuo's additional superblock check from patch series
> 
> Why do we want to make mount() succeed for CNIDs other than HFS_ROOT_CNID ?
> 
>   0 => EINVAL
>   HFS_EXT_CNID => ENOTDIR
>   HFS_CAT_CNID => ENOTDIR
>   All others => succeed
> 
> Fuzzing against unreadable root directory is not useful.
> 

The hfs_fill_super() has not been modified as it was discussed.

Thanks,
Slava.

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

[George Anthony Vernon]

On Tue, Mar 10, 2026 at 09:28:53PM +0000, Viacheslav Dubeyko wrote:
> On Tue, 2026-03-10 at 00:08 +0000, George Anthony Vernon wrote:
> > 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://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_all_427fcb57-2D8424-2D4e52-2D9f21-2D7041b2c4ae5b-40&d=DwIDAg&c=BSDicqBQBDjDI9RkVyTcHQ&r=q5bIm4AXMzc8NJu1_RGmnQ2fMWKq4Y4RAkElvUgSs00&m=AzLgd6KhZfr8AykzDg76T-sDUALdUDSC68BUfheUxxhq-KIglGTX6mtAci6dTupM&s=nuXDj9Z8fvUpvnJIyDsfPIi-YuBzvbXxQUhypXeaheU&e= 
> > I-love.SAKURA.ne.jp/T/
> > Reported-by: syzbot+97e301b4b82ae803d21b@syzkaller.appspotmail.com
> > Closes: https://urldefense.proofpoint.com/v2/url?u=https-3A__syzkaller.appspot.com_bug-3Fextid-3D97e301b4b82ae803d21b&d=DwIDAg&c=BSDicqBQBDjDI9RkVyTcHQ&r=q5bIm4AXMzc8NJu1_RGmnQ2fMWKq4Y4RAkElvUgSs00&m=AzLgd6KhZfr8AykzDg76T-sDUALdUDSC68BUfheUxxhq-KIglGTX6mtAci6dTupM&s=lGn13lU4-Np727qrFQB17Y-qYKD4fRkJ3gSkQYQH8cg&e= 
> > 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:
> 
> If we don't process this ID, then default case will be completely enough. We
> don't need to introduce this as a special case.
> 
Okay, I've removed it. It was an intentional style choice, the purpose
was to provide some explanation.

> > +		/* 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".
> > +		 */
> 
> Yes, HFS driver (bad block sparing algorithm) will not create this file because
> this file could be created by mkfs tool. And the bad block sparing algorithm
> could simply read this file and add new items (bad sectors). This CNID could be
> in Catalog File because it could be created by mkfs tool.
> 
> Anyway, I think that probability to have the Bad block file is really low. If
> you really insist not to process this CNID, then I suggest completely remove
> this case and comments.
> 
Ditto here, I thought the comments were useful explanation, I've removed
them. I really think it is correct not to parse this CNID. I think mkfs
would be violating the HFS standard if it created a CNID with that
inode.
> > +		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;
> 
> I think to use the rec->type is better here than HFS_CDR_FIL.
> 
Thanks for pointing this out, after rewriting it the way you suggested I saw a further improvement to let the helper take a single hfs_cat_rec pointer.

> 
> >  		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;
> 
> Why this has been removed:
> 
> -		default:
> -			BUG();
> -			return -EIO;
> -		}
> 
> 
It hasn't been totally removed, the same logic has been absorbed into
is_valid_catalog_record(). The difference is now we just return -EIO
instead of throwing BUG() like you previously suggested, which I now
agree with.
> >  	}
> >  
> >  	if (HFS_IS_RSRC(inode))
> 
> 
> You completely missed the hfs_fill_super() logic:
> 
> 	root_inode = hfs_iget(sb, &fd.search_key->cat, &rec);
> 	hfs_find_exit(&fd);
> 	if (!root_inode)
> 		goto bail_no_root;
> 
> We need to process it specially now because we've created bad inode.
> 
> Thanks,
> Slava.

Sorry I did miss this. I agree that this change will be sufficient to
check the superblock, it's functionally equivalent to Tetsuo's patch.

Will follow up with new patch shortly.

Thanks again,

George