Re: [PATCH v2 1/2] hfs: create the internal B-tree inodes through the generic iget path

[Viacheslav Dubeyko]

On Mon, 2026-10-12 at 15:44 +0800, 1sh1ro wrote:
> hfs_btree_open() builds the extents and catalog B-tree inodes with a
> raw
> iget_locked() and never assigns them a file type, so both stay cached
> with
> i_mode == 0 after every mount.  A crafted HFS image whose catalog
> contains
> a record addressing one of the reserved B-tree CNIDs (3 or 4) makes
> hfs_lookup() -> hfs_iget() return that cached inode -
> hfs_test_inode()
> matches on i_ino alone - and the typeless inode then reaches the
> generic
> open path, where it trips VFS_BUG_ON_INODE() in may_open() with
> CONFIG_DEBUG_VFS=y:
> 
>   VFS_BUG_ON_INODE(!IS_ANON_FILE(inode)): inode:... fs:hfs mode:0
> opflags:0xc
>   ------------[ cut here ]------------
>   kernel BUG at fs/namei.c:4271!
>   RIP: 0010:may_open+0x2ba/0x480 fs/namei.c:4271
>   Call Trace:
>    do_open fs/namei.c:4698 [inline]
>    path_openat+0x1600/0x3de0 fs/namei.c:4863
>    __x64_sys_openat+0x13f/0x1f0 fs/open.c:1385
> 
> Without CONFIG_DEBUG_VFS the assertion compiles away and the same
> image
> silently yields an openable typeless inode, so this is not merely a
> debug-config nuisance.
> 
> Fix it the way HFS+ handles its system inodes: add
> hfs_system_read_inode(),
> which reads the fork from the MDB (the counterpart of
> hfsplus_system_read_inode()), and hfs_system_iget(), which looks the
> inode
> up through the normal inode-cache path and can fail before the inode
> is
> published.  hfs_btree_open() now uses hfs_system_iget(), so the B-
> tree
> inodes carry a valid S_IFREG type like every other inode.  The
> extent-record validation is unchanged; it just runs after the inode
> has
> been created.
> 
> Found by syzkaller-style fuzzing of v7.2-rc7; still reproducible on
> current mainline.
> 
> Suggested-by: Viacheslav Dubeyko <slava@xxxxxxxxxxx>
> Link:
> https://lore.kernel.org/linux-fsdevel/tencent_24D928451FEFA8A35482EFFA9132A7305307@xxxxxx/
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: 1sh1ro <yangguikun19@xxxxxx>

Please, provide the real name.

> ---
>   fs/hfs/btree.c  |  25 +++++--------------------
>  fs/hfs/hfs_fs.h |   1 +
>  fs/hfs/inode.c  |  74 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 80 insertions(+), 20 deletions(-)
> 
> diff --git a/fs/hfs/btree.c b/fs/hfs/btree.c
> index 41b4e8f..7486707 100644
> --- a/fs/hfs/btree.c
> +++ b/fs/hfs/btree.c
> @@ -155,44 +155,29 @@ 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)
> +	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");

This check should be in hfs_system_read_inode() now.

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

Ditto. This check should be in hfs_system_read_inode() now.

> -			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);
> +
> +	tree->inode->i_mapping->a_ops = &hfs_btree_aops;
>  
>  	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..26505f7 100644
> --- a/fs/hfs/hfs_fs.h
> +++ b/fs/hfs/hfs_fs.h
> @@ -219,6 +219,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..4316d75 100644
> --- a/fs/hfs/inode.c
> +++ b/fs/hfs/inode.c
> @@ -421,6 +421,80 @@ static int hfs_read_inode(struct inode *inode,
> void *data)
>  	return 0;
>  }
>  
> +/*
> + * hfs_system_read_inode

You need to place short abstract of the function:
hfs_system_read_inode() - <short explanation> or simply remove it
because there is no useful info here.

> + *
> + * Initialize one of the internal B-tree inodes.  These are
> described by the
> + * MDB rather than by catalog records, so they need their own read
> helper -
> + * the HFS counterpart of hfsplus_system_read_inode().

Could you please rework comment? It is not useful now.

> + */
> +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);
> +
> +	switch (inode->i_ino) {
> +	case HFS_EXT_CNID:
> +		hfs_inode_read_fork(inode, mdb->drXTExtRec, mdb-
> >drXTFlSize,
> +				    mdb->drXTFlSize,
> be32_to_cpu(mdb->drXTClpSiz));
> +		break;
> +	case HFS_CAT_CNID:
> +		hfs_inode_read_fork(inode, mdb->drCTExtRec, mdb-
> >drCTFlSize,
> +				    mdb->drCTFlSize,
> be32_to_cpu(mdb->drCTClpSiz));
> +		break;
> +	default:
> +		return -EIO;
> +	}
> +
> +	/*
> +	 * Assign a dummy file type, for may_open() requires that an
> inode has
> +	 * a valid file type.  These inodes are hashed by CNID like
> any other
> +	 * inode, so without a type a catalog record of a crafted
> image that
> +	 * aliases CNID 3 or 4 would reach the VFS with i_mode == 0.
> +	 */

Could it be short and clear comment?

> +	inode->i_mode = S_IFREG;
> +
> +	return 0;
> +}
> +
> +/*
> + * hfs_system_iget

Ditto.

> + *
> + * Look up (or create) one of the internal B-tree inodes. 
> hfs_btree_open()
> + * uses this instead of a raw iget_locked() so that the inode is
> completely
> + * initialized before it becomes visible in the inode cache, in the
> same way
> + * hfsplus_iget() handles the system CNIDs for HFS+.

Could you please rework the AI generated comment? Currently, it is not
useful.

Thanks,
Slava.

> + */
> +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);
> +	if (!(inode_state_read_once(inode) & I_NEW))
> +		return inode;
> +
> +	err = hfs_system_read_inode(inode);
> +	if (err) {
> +		iget_failed(inode);
> +		return ERR_PTR(err);
> +	}
> +
> +	unlock_new_inode(inode);
> +	return inode;
> +}
> +
>  /*
>   * __hfs_iget()
>   *