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

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

On Wed, 2026-03-11 at 21:13 +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=hbRXIRbizwXvlK5bd-6cvolAcmvrKoMLwfPu1dbRTmLihblmLF-pG_-n9blY3iZQ&s=PYtGPY4zEe6GWZtwZHpV27q6S2pbYp7pt4MPLpyrKFg&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=hbRXIRbizwXvlK5bd-6cvolAcmvrKoMLwfPu1dbRTmLihblmLF-pG_-n9blY3iZQ&s=m0spR05lFH2IzlSg01JM1INNQQJftf6373Pu8q1Bz6Q&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>
> ---
> Changes from V3->V4:
> - Remove explicit "do nothing" case statement labels in favor of
>   implicit default
> - Check superblock for bad inode
> 
> 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 | 65 +++++++++++++++++++++++++++++++++++++++-----------
>  fs/hfs/super.c |  2 +-
>  2 files changed, 52 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c
> index 878535db64d6..469ea6401d47 100644
> --- a/fs/hfs/inode.c
> +++ b/fs/hfs/inode.c
> @@ -340,6 +340,31 @@ 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;
> +	default:
> +		/* Invalid reserved CNID */
> +		break;
> +	}
> +
> +	return false;
> +}
> +
>  /*
>   * hfs_read_inode
>   */
> @@ -369,6 +394,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, rec->type))

The rec->type is s8 data type [1]. So, it's OK to use it as it is. However, rec-
>file.FlNum is __be32 data type [2]. So, it needs to use be32_to_cpu() here.

> +			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 +417,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, rec->type))

Ditto.

> +			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 +428,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 inode with cnid %lu\n", inode->i_ino);

I assume that inode->i_ino could be not set up here. Am I right? You need to use
rec->file.FlNum or rec->dir.DirID here.

> +		pr_warn("Volume is probably corrupted, try performing fsck.\n");
> +		fallthrough;
>  	default:
>  		make_bad_inode(inode);
> +		break;
>  	}
>  	return 0;
>  }
> @@ -448,6 +482,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 +499,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;

I am still worried that we do nothing here. I prefer to return some error if
is_valid_catalog_record() failed somehow.

Thanks,
Slava.

>  	}
>  
>  	if (HFS_IS_RSRC(inode))
> diff --git a/fs/hfs/super.c b/fs/hfs/super.c
> index a4f2a2bfa6d3..060421770147 100644
> --- a/fs/hfs/super.c
> +++ b/fs/hfs/super.c
> @@ -369,7 +369,7 @@ static int hfs_fill_super(struct super_block *sb, struct fs_context *fc)
>  	res = -EINVAL;
>  	root_inode = hfs_iget(sb, &fd.search_key->cat, &rec);
>  	hfs_find_exit(&fd);
> -	if (!root_inode)
> +	if (!root_inode || is_bad_inode(root_inode))
>  		goto bail_no_root;
>  
>  	set_default_d_op(sb, &hfs_dentry_operations);

[1]
https://elixir.bootlin.com/linux/v6.19/source/include/linux/hfs_common.h#L443
[2]
https://elixir.bootlin.com/linux/v6.19/source/include/linux/hfs_common.h#L397