Re: [PATCH v5] hfs: initialize B-tree inodes and validate catalog CNIDs
From: Viacheslav Dubeyko
Date: Thu Oct 15 2026 - 18:35:14 EST
On Thu, 2026-10-15 at 15:06 +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@xxxxxxxxxxx>
> Link:
> https://lore.kernel.org/linux-fsdevel/tencent_24D928451FEFA8A35482EFFA9132A7305307@xxxxxx/
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: yangguikun <yangguikun19@xxxxxx>
> ---
> Changes in v5:
> - Remove unnecessary comments.
> - Simplify hfs_is_system_reserved_cnid().
> - Move i_mode and a_ops initialization after set_nlink().
> - Use -EIO instead of -EACCES.
>
> fs/hfs/btree.c | 39 ++-----------------------------
> fs/hfs/dir.c | 2 +-
> fs/hfs/hfs_fs.h | 6 +++++
> fs/hfs/inode.c | 71
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 80 insertions(+), 38 deletions(-)
>
> diff --git a/fs/hfs/btree.c b/fs/hfs/btree.c
> index 41b4e8f..1891ad5 100644
> --- a/fs/hfs/btree.c
> +++ b/fs/hfs/btree.c
> @@ -155,44 +155,9 @@ 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");
> - 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/dir.c b/fs/hfs/dir.c
> index e1f1fb3..0a31304 100644
> --- a/fs/hfs/dir.c
> +++ b/fs/hfs/dir.c
> @@ -36,7 +36,7 @@ static struct dentry *hfs_lookup(struct inode *dir,
> struct dentry *dentry,
> } else {
> inode = hfs_iget(dir->i_sb, &fd.search_key->cat,
> &rec);
> if (!inode)
> - inode = ERR_PTR(-EACCES);
> + inode = ERR_PTR(-EIO);
With the patch applied, hfs_iget() can return NULL in these cases:
1. Unknown record type. The default: branch of its own switch (rec-
>type) returns NULL. That's corruption.
2. The new CNID check (!hfs_is_valid_cnid() ||
hfs_is_system_reserved_cnid()). Also corruption.
3. iget5_locked() returns NULL. In fs/inode.c that happens when:
- alloc_inode(sb) fails, meaning the inode slab allocation ran out
of memory. This is not corruption; it's -ENOMEM.
- The set() callback fails. Here that's hfs_read_inode() returning -
EIO for a bad CNID, which is corruption again.
If we want it done properly, ten, maybe, we need to make hfs_iget()
return ERR_PTR() instead of NULL:
- ERR_PTR(-ENOMEM) when the allocation fails
- ERR_PTR(-EIO) for the corruption cases
What do you think?
And, maybe, we need to consider it as dedicated patch.
> }
> hfs_find_exit(&fd);
> return d_splice_alias(inode, dentry);
> diff --git a/fs/hfs/hfs_fs.h b/fs/hfs/hfs_fs.h
> index e250f87..8d52170 100644
> --- a/fs/hfs/hfs_fs.h
> +++ b/fs/hfs/hfs_fs.h
> @@ -163,6 +163,11 @@ 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);
>
> +static inline bool hfs_is_system_reserved_cnid(u32 cnid)
> +{
> + return cnid > HFS_ROOT_CNID && cnid < HFS_FIRSTUSER_CNID;
> +}
> +
> /*
> * Validate the CNID of a catalog record.
> */
> @@ -219,6 +224,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..5a03fca 100644
> --- a/fs/hfs/inode.c
> +++ b/fs/hfs/inode.c
> @@ -421,6 +421,72 @@ static int hfs_read_inode(struct inode *inode,
> void *data)
> return 0;
> }
>
> +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;
I am still not sure that we need it for b-trees' inodes. Do we need it?
> +
> + inode->i_uid = hsb->s_uid;
> + inode->i_gid = hsb->s_gid;
> + set_nlink(inode, 1);
I assume you copied it from hfs_read_inode(). But this method is for b-
trees' inodes and I suspect that we have trouble here.
Before this patch, B-tree inodes had i_nlink == 0, so
inode_generic_drop() returned true and the final iput() evicted them
without writeback. With nlink = 1, the final iput() in
hfs_btree_close() runs during put_super, when SB_ACTIVE is already
clear. iput_final() then takes the !drop branch and calls
write_inode_now(inode, 1) (fs/inode.c, iput_final). If a tree inode is
still dirty at that point:
- Extents tree (closed first): hfs_write_inode() →
hfs_btree_write(ext_tree) re-reads node 0 into a hash that
hfs_btree_close() has just emptied. Then kfree(tree) leaks that node.
- Catalog tree (closed second): hfs_write_inode() →
hfs_ext_write_extent() can touch HFS_SB(sb)->ext_tree, which has
already been freed. That's a use-after-free if the catalog's extent
record is dirty.
Are you sure that we safe here with set_nlink(inode, 1)?
> +
> + /* may_open() requires a file type */
> + 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;
> + }
> +
> + return 0;
> +}
> +
> +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)) {
> + iput(inode);
> + return ERR_PTR(-EEXIST);
> + }
iget_locked() returns a fresh inode with I_NEW set if no inode with
that number exists yet in this superblock's inode cache. If one already
exists, it returns that cached inode without I_NEW. So the -EEXIST
branch fires only if an inode numbered 3 or 4 is already cached for
this superblock before hfs_btree_open() creates it. That can't happen
I suppose.
What's about this then?
/* B-trees are opened before any catalog lookup can run */
if (WARN_ON_ONCE(!(inode_state_read_once(inode) & I_NEW))) {
iput(inode);
return ERR_PTR(-EEXIST);
}
Thanks,
Slava.
> +
> + 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 +512,11 @@ struct inode *hfs_iget(struct super_block *sb,
> struct hfs_cat_key *key, hfs_cat_
> default:
> return NULL;
> }
> +
> + if (!hfs_is_valid_cnid(cnid, rec->type) ||
> + hfs_is_system_reserved_cnid(cnid))
> + 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);