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@xxxxxxxxxxxxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxxxxxx>
> Tested-by: syzbot+97e301b4b82ae803d21b@xxxxxxxxxxxxxxxxxxxxxxxxx
> Signed-off-by: George Anthony Vernon <contact@xxxxxxxxxxx>
> ---
> 
> 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.