Re: [PATCH v4] hfs: initialize B-tree inodes and validate catalog CNIDs

[Viacheslav Dubeyko <slava@dubeyko.com>]

On Wed, 2026-10-14 at 16:03 +0800, yangguikun wrote:
> hfs_btree_open() leaves the internal B-tree inodes with i_mode == 0.
> A crafted catalog record naming CNID 3 or 4 can return one of these
> cached inodes through hfs_iget(), triggering VFS_BUG_ON_INODE() in
> may_open() with CONFIG_DEBUG_VFS=y.
> 
> Initialize the B-tree inodes with S_IFREG and their address-space
> operations before publishing them. Reject attempts to reuse an
> already-cached B-tree inode.
> 
> Validate catalog CNIDs and record types before the inode-cache
> lookup,
> since cached inodes bypass hfs_read_inode() validation. This also
> prevents a crafted file record from aliasing the root directory
> inode.
> 
> Found by syzkaller-style fuzzing of v7.2-rc7.
> 
> Suggested-by: Viacheslav Dubeyko <slava@dubeyko.com>
> Link:
> https://lore.kernel.org/linux-fsdevel/tencent_24D928451FEFA8A35482EFFA9132A7305307@qq.com/
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@vger.kernel.org
> Signed-off-by: yangguikun <yangguikun19@qq.com>
> ---
> Changes in v4:
>  - set a_ops in hfs_system_read_inode(), before the inode is
> published.
>  - hfs_system_iget(): reject an already-cached inode (the original
>    BUG_ON()).
>  - document why the free_tree error path is correct.
>  - add hfs_is_system_reserved_cnid(), as suggested.
>  - hfs_iget(): also reject records that hfs_read_inode() would reject
>    (CNID 2 typed as a file, for example); an already-cached inode
> skips
>    that check.
> 
>  fs/hfs/btree.c  |  43 ++++-------------------
>  fs/hfs/hfs_fs.h |  13 +++++++
>  fs/hfs/inode.c  | 103
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 122 insertions(+), 37 deletions(-)
> 
> diff --git a/fs/hfs/btree.c b/fs/hfs/btree.c
> index 41b4e8f..454caa1 100644
> --- a/fs/hfs/btree.c
> +++ b/fs/hfs/btree.c
> @@ -155,44 +155,13 @@ struct hfs_btree *hfs_btree_open(struct
> super_block *sb, u32 id, btree_keycmp ke
>  	tree->cnid = id;
>  	tree->keycmp = keycmp;
>  
> -	tree->inode = iget_locked(sb, id);
> -	if (!tree->inode)
> +	/*
> +	 * hfs_system_iget() releases the inode itself when it
> cannot
> +	 * initialize it, so there is nothing to iput here.
> +	 */

Please, remove this comment.

> +	tree->inode = hfs_system_iget(sb, id);
> +	if (IS_ERR(tree->inode))
>  		goto free_tree;
> -	BUG_ON(!(inode_state_read_once(tree->inode) & I_NEW));
> -	{
> -	struct hfs_mdb *mdb = HFS_SB(sb)->mdb;
> -	HFS_I(tree->inode)->flags = 0;
> -	mutex_init(&HFS_I(tree->inode)->extents_lock);
> -	switch (id) {
> -	case HFS_EXT_CNID:
> -		hfs_inode_read_fork(tree->inode, mdb->drXTExtRec,
> mdb->drXTFlSize,
> -				    mdb->drXTFlSize,
> be32_to_cpu(mdb->drXTClpSiz));
> -		if (HFS_I(tree->inode)->alloc_blocks >
> -					HFS_I(tree->inode)-
> >first_blocks) {
> -			pr_err("invalid btree extent records\n");
> -			unlock_new_inode(tree->inode);
> -			goto free_inode;
> -		}
> -
> -		tree->inode->i_mapping->a_ops = &hfs_btree_aops;
> -		break;
> -	case HFS_CAT_CNID:
> -		hfs_inode_read_fork(tree->inode, mdb->drCTExtRec,
> mdb->drCTFlSize,
> -				    mdb->drCTFlSize,
> be32_to_cpu(mdb->drCTClpSiz));
> -
> -		if (!HFS_I(tree->inode)->first_blocks) {
> -			pr_err("invalid btree extent records (0
> size)\n");
> -			unlock_new_inode(tree->inode);
> -			goto free_inode;
> -		}
> -
> -		tree->inode->i_mapping->a_ops = &hfs_btree_aops;
> -		break;
> -	default:
> -		BUG();
> -	}
> -	}
> -	unlock_new_inode(tree->inode);
>  
>  	mapping = tree->inode->i_mapping;
>  	folio = filemap_grab_folio(mapping, 0);
> diff --git a/fs/hfs/hfs_fs.h b/fs/hfs/hfs_fs.h
> index e250f87..ecdcf81 100644
> --- a/fs/hfs/hfs_fs.h
> +++ b/fs/hfs/hfs_fs.h
> @@ -163,6 +163,18 @@ extern int hfs_cat_move(u32 cnid, struct inode
> *src_dir,
>  extern void hfs_cat_build_key(struct super_block *sb, btree_key
> *key,
>  			      u32 parent, const struct qstr *name);
>  
> +/*
> + * A CNID below HFS_FIRSTUSER_CNID is reserved for the system files;
> the
> + * root directory is the one reserved CNID that catalog records
> address.
> + */

The method name is informative enough. Please, remove the comment.

> +static inline bool hfs_is_system_reserved_cnid(u32 cnid)
> +{
> +	if (likely(cnid > HFS_ROOT_CNID && cnid <
> HFS_FIRSTUSER_CNID))
> +		return true;
> +	else
> +		return false;

       return cnid > HFS_ROOT_CNID && cnid < HFS_FIRSTUSER_CNID;

> +}

We don't need anything more here.
> +
>  /*
>   * Validate the CNID of a catalog record.
>   */
> @@ -219,6 +231,7 @@ extern void hfs_inode_read_fork(struct inode
> *inode, struct hfs_extent *ext,
>  				u32 clump_size);
>  extern struct inode *hfs_iget(struct super_block *sb, struct
> hfs_cat_key *key,
>  				hfs_cat_rec *rec);
> +extern struct inode *hfs_system_iget(struct super_block *sb, u32
> cnid);
>  extern void hfs_evict_inode(struct inode *inode);
>  extern void hfs_delete_inode(struct inode *inode);
>  
> diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c
> index 2aef3c3..986a6f9 100644
> --- a/fs/hfs/inode.c
> +++ b/fs/hfs/inode.c
> @@ -421,6 +421,97 @@ static int hfs_read_inode(struct inode *inode,
> void *data)
>  	return 0;
>  }
>  
> +/**
> + * hfs_system_read_inode - read an internal B-tree inode
> + * @inode: inode to initialize, hashed by its CNID
> + *
> + * The extent and catalog B-trees are described by the MDB instead
> of by
> + * catalog records, so they do not go through hfs_read_inode().
> + *
> + * Return: 0 on success, -EIO if the MDB does not describe this
> CNID.
> + */

I am not sure that we need this comment at all. The function name is
good enough.

> +static int hfs_system_read_inode(struct inode *inode)
> +{
> +	struct hfs_sb_info *hsb = HFS_SB(inode->i_sb);
> +	struct hfs_mdb *mdb = hsb->mdb;
> +
> +	HFS_I(inode)->flags = 0;
> +	HFS_I(inode)->rsrc_inode = NULL;
> +	mutex_init(&HFS_I(inode)->extents_lock);
> +	HFS_I(inode)->tz_secondswest = sys_tz.tz_minuteswest * 60;
> +
> +	inode->i_uid = hsb->s_uid;
> +	inode->i_gid = hsb->s_gid;
> +	set_nlink(inode, 1);

Maybe, we need to place it here?

+	inode->i_mode = S_IFREG;
+	inode->i_mapping->a_ops = &hfs_btree_aops;


> +
> +	switch (inode->i_ino) {
> +	case HFS_EXT_CNID:
> +		hfs_inode_read_fork(inode, mdb->drXTExtRec, mdb-
> >drXTFlSize,
> +				    mdb->drXTFlSize,
> be32_to_cpu(mdb->drXTClpSiz));
> +		if (HFS_I(inode)->alloc_blocks > HFS_I(inode)-
> >first_blocks) {
> +			pr_err("invalid btree extent records\n");
> +			return -EIO;
> +		}
> +		break;
> +	case HFS_CAT_CNID:
> +		hfs_inode_read_fork(inode, mdb->drCTExtRec, mdb-
> >drCTFlSize,
> +				    mdb->drCTFlSize,
> be32_to_cpu(mdb->drCTClpSiz));
> +		if (!HFS_I(inode)->first_blocks) {
> +			pr_err("invalid btree extent records (0
> size)\n");
> +			return -EIO;
> +		}
> +		break;
> +	default:
> +		return -EIO;
> +	}
> +
> +	/*
> +	 * may_open() requires a file type, and a crafted catalog
> record can
> +	 * reach this inode by its CNID.
> +	 */

/* may_open() requires a file type */

> +	inode->i_mode = S_IFREG;
> +
> +	/* Set before hfs_system_iget() publishes the inode. */

Please, remove the comment.

> +	inode->i_mapping->a_ops = &hfs_btree_aops;
> +
> +	return 0;
> +}
> +
> +/**
> + * hfs_system_iget - look up an internal B-tree inode by CNID
> + * @sb: super block
> + * @cnid: HFS_EXT_CNID or HFS_CAT_CNID
> + *
> + * Return: the inode, or an ERR_PTR() if it cannot be initialized.
> + */

Ditto.

> +struct inode *hfs_system_iget(struct super_block *sb, u32 cnid)
> +{
> +	struct inode *inode;
> +	int err;
> +
> +	inode = iget_locked(sb, cnid);
> +	if (!inode)
> +		return ERR_PTR(-ENOMEM);
> +
> +	/*
> +	 * The B-tree inodes are created once per mount, so finding
> one
> +	 * already in the cache means the same tree was opened
> twice.
> +	 */

Please, delete the comment.

> +	if (!(inode_state_read_once(inode) & I_NEW)) {
> +		iput(inode);
> +		return ERR_PTR(-EEXIST);
> +	}
> +
> +	err = hfs_system_read_inode(inode);
> +	if (err) {
> +		iget_failed(inode);
> +		return ERR_PTR(err);
> +	}
> +
> +	unlock_new_inode(inode);
> +	return inode;
> +}
> +
>  /*
>   * __hfs_iget()
>   *
> @@ -446,6 +537,18 @@ struct inode *hfs_iget(struct super_block *sb,
> struct hfs_cat_key *key, hfs_cat_
>  	default:
>  		return NULL;
>  	}
> +
> +	/*
> +	 * hfs_test_inode() matches on i_ino alone, so a crafted
> record can
> +	 * alias any cached inode.  hfs_read_inode() rejects an
> invalid CNID,
> +	 * but it does not run when the inode is already cached, and
> a record
> +	 * naming a reserved CNID would hand out a B-tree inode as a
> regular
> +	 * dentry.
> +	 */

Please, delete the comment. Code is informative enough.

> +	if (!hfs_is_valid_cnid(cnid, rec->type) ||
> +	    hfs_is_system_reserved_cnid(cnid))
> +		return NULL;

A NULL return becomes -EACCES in hfs_lookup() (fs/hfs/dir.c:38). For a
corrupted catalog, -EIO/-EUCLEAN fits better. What do you think? Am I
right here?

Thanks,
Slava.

> +
>  	inode = iget5_locked(sb, cnid, hfs_test_inode,
> hfs_read_inode, &data);
>  	if (inode && (inode_state_read_once(inode) & I_NEW))
>  		unlock_new_inode(inode);