Re: [PATCH v2 2/2] hfs: refuse catalog records that address the reserved B-tree CNIDs

[Viacheslav Dubeyko]

On Mon, 2026-10-12 at 15:44 +0800, 1sh1ro wrote:
> The extent and catalog B-tree inodes are described by the MDB, not by
> catalog records, but they are hashed by CNID like any other inode and
> hfs_test_inode() compares i_ino only.  A crafted image with a catalog
> record whose FlNum (or DirID) is HFS_EXT_CNID or HFS_CAT_CNID
> therefore
> makes hfs_lookup() hand out the internal B-tree inode as a regular
> dentry,
> bypassing the CNID validation that hfs_read_inode() performs: a read
> exposes the B-tree file's contents, and a write would go through
> hfs_write_inode(), which tries to update a catalog record that does
> not
> exist.
> 
> Refuse such records in hfs_iget() by returning NULL for the reserved
> B-tree CNIDs.  hfs_lookup() reports that as -EACCES and
> hfs_fill_super() as a mount failure, the same as for any other
> unaddressable record.
> 
> This is hardening on top of the file-type fix, not required for the
> panic; it can be dropped if hfs_is_valid_cnid()'s acceptance of
> HFS_CDR_FIL records for CNID 3/4 is intentional.

Please, combine both patches into one. First patch doesn't fix the
issue alone.

> 
> Signed-off-by: 1sh1ro <yangguikun19@xxxxxx>

Please, provide the real name.

> ---
>   fs/hfs/inode.c |  11 +++++++++++
>  1 file changed, 11 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c
> index 4316d75..5190856 100644
> --- a/fs/hfs/inode.c
> +++ b/fs/hfs/inode.c
> @@ -520,6 +520,17 @@ struct inode *hfs_iget(struct super_block *sb,
> struct hfs_cat_key *key, hfs_cat_
>  	default:
>  		return NULL;
>  	}
> +
> +	/*
> +	 * Never let a catalog record address one of the internal B-
> tree
> +	 * objects.  hfs_test_inode() matches on i_ino alone, so
> such a
> +	 * record would make hfs_lookup() hand out the cached B-tree
> inode as
> +	 * a regular dentry, bypassing the CNID validation done by
> +	 * hfs_read_inode().
> +	 */

Please, rework or remove the comment. Currently, it's long and not
informative.

> +	if (cnid == HFS_EXT_CNID || cnid == HFS_CAT_CNID)

I think we can reject all cnid that lower than HFS_FIRSTUSER_CNID.

Thanks,
Slava.

> +		return NULL;
> +
>  	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);