[PATCH 1/1] hfs: don't let internal B-tree inodes leak into the VFS open path

[PATCH 1/1] hfs: don't let internal B-tree inodes leak into the VFS open path

[1sh1ro]

hfs_btree_open() creates the extents and catalog B-tree inodes with
iget_locked() and never assigns them a file type, so both stay cached
with i_mode == 0 after mount.

A crafted HFS image whose catalog contains a record whose DirID/FlNum
aliases HFS_EXT_CNID (3) or HFS_CAT_CNID (4) makes hfs_lookup() ->
hfs_iget() -> iget5_locked() return that already-cached internal inode
(hfs_test_inode() only compares i_ino), without running
hfs_read_inode().  The typeless inode then reaches the generic open
path and, with CONFIG_DEBUG_VFS=y, deterministically panics:

  VFS_BUG_ON_INODE(!IS_ANON_FILE(inode)): inode:... fs:hfs mode:0 opflags:0xc flags:0x0 state:0x0 count:2
  ------------[ 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

Fix it in two steps:

 - give the B-tree inodes an S_IFREG type in hfs_btree_open(), so the
   "every cached inode has a valid S_IFMT type" VFS invariant holds
   regardless of what is on disk;

 - refuse, in hfs_iget(), catalog records that address the reserved
   B-tree CNIDs so a malformed image cannot alias internal objects
   into dentries at all.  Returning NULL follows the existing
   convention for unaddressable records: hfs_fill_super() turns it
   into a mount failure and hfs_lookup() into -EACCES.

Without CONFIG_DEBUG_VFS the assertion compiles away and the same
crafted image silently makes a typeless inode openable, so this is not
merely a debug-config nuisance.

Found by syzkaller-style fuzzing of v7.2-rc7; still reproducible on
current mainline.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: 1sh1ro <yangguikun19@qq.com>
---
 fs/hfs/btree.c | 8 ++++++++
 fs/hfs/inode.c | 11 +++++++++++
 2 files changed, 19 insertions(+)

diff --git a/fs/hfs/btree.c b/fs/hfs/btree.c
index 2eb37a2f64e8..bd69ace8f309 100644
--- a/fs/hfs/btree.c
+++ b/fs/hfs/btree.c
@@ -43,6 +43,14 @@ struct hfs_btree *hfs_btree_open(struct super_block *sb, u32 id, btree_keycmp ke
    if (!tree->inode)
       goto free_tree;
    BUG_ON(!(inode_state_read_once(tree->inode) & I_NEW));
+
+   /* B-tree inodes are internal objects that must never be exposed
+    * through directory entries.  Give them a REG type so the VFS
+    * invariant "every cached inode has a valid S_IFMT type" holds even
+    * when a corrupted image contains catalog records aliasing these
+    * reserved CNIDs.
+    */
+   tree->inode->i_mode = S_IFREG;
    {
    struct hfs_mdb *mdb = HFS_SB(sb)->mdb;
    HFS_I(tree->inode)->flags = 0;
diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c
index ac4a9055c5c0..bb759345f1fc 100644
--- a/fs/hfs/inode.c
+++ b/fs/hfs/inode.c
@@ -432,6 +432,17 @@ struct inode *hfs_iget(struct super_block *sb, struct hfs_cat_key *key, hfs_cat_
    default:
       return NULL;
    }
+
+   /* Refuse catalog records that address the internal B-tree objects:
+    * a crafted image can otherwise make hfs_lookup() return the cached
+    * typeless B-tree inode, which then reaches may_open() with
+    * i_mode == 0 and trips VFS_BUG_ON_INODE() there.  NULL follows the
+    * existing convention for unaddressable records (mount failure /
+    * -EACCES).
+    */
+   if (cnid == HFS_EXT_CNID || cnid == HFS_CAT_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);
--
2.39.5

Re: [PATCH 1/1] hfs: don't let internal B-tree inodes leak into the VFS open path

[Viacheslav Dubeyko]

On Wed, 2026-09-16 at 17:09 +0800, 1sh1ro wrote:
> hfs_btree_open() creates the extents and catalog B-tree inodes with
> iget_locked() and never assigns them a file type, so both stay cached
> with i_mode == 0 after mount.
> 
> A crafted HFS image whose catalog contains a record whose DirID/FlNum
> aliases HFS_EXT_CNID (3) or HFS_CAT_CNID (4) makes hfs_lookup() ->
> hfs_iget() -> iget5_locked() return that already-cached internal
> inode
> (hfs_test_inode() only compares i_ino), without running
> hfs_read_inode().  The typeless inode then reaches the generic open
> path and, with CONFIG_DEBUG_VFS=y, deterministically panics:
> 
>   VFS_BUG_ON_INODE(!IS_ANON_FILE(inode)): inode:... fs:hfs mode:0
> opflags:0xc flags:0x0 state:0x0 count:2
>   ------------[ 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
> 
> Fix it in two steps:
> 
>  - give the B-tree inodes an S_IFREG type in hfs_btree_open(), so the
>    "every cached inode has a valid S_IFMT type" VFS invariant holds
>    regardless of what is on disk;
> 
>  - refuse, in hfs_iget(), catalog records that address the reserved
>    B-tree CNIDs so a malformed image cannot alias internal objects
>    into dentries at all.  Returning NULL follows the existing
>    convention for unaddressable records: hfs_fill_super() turns it
>    into a mount failure and hfs_lookup() into -EACCES.
> 
> Without CONFIG_DEBUG_VFS the assertion compiles away and the same
> crafted image silently makes a typeless inode openable, so this is
> not
> merely a debug-config nuisance.
> 
> Found by syzkaller-style fuzzing of v7.2-rc7; still reproducible on
> current mainline.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: 1sh1ro <yangguikun19@qq.com>
> ---
>  fs/hfs/btree.c | 8 ++++++++
>  fs/hfs/inode.c | 11 +++++++++++
>  2 files changed, 19 insertions(+)
> 
> diff --git a/fs/hfs/btree.c b/fs/hfs/btree.c
> index 2eb37a2f64e8..bd69ace8f309 100644
> --- a/fs/hfs/btree.c
> +++ b/fs/hfs/btree.c
> @@ -43,6 +43,14 @@ struct hfs_btree *hfs_btree_open(struct
> super_block *sb, u32 id, btree_keycmp ke
>     if (!tree->inode)
>        goto free_tree;
>     BUG_ON(!(inode_state_read_once(tree->inode) & I_NEW));
> +
> +   /* B-tree inodes are internal objects that must never be exposed
> +    * through directory entries.  Give them a REG type so the VFS
> +    * invariant "every cached inode has a valid S_IFMT type" holds
> even
> +    * when a corrupted image contains catalog records aliasing these
> +    * reserved CNIDs.
> +    */
> +   tree->inode->i_mode = S_IFREG;

In HFS+ we call hfsplus_iget() in hfs_btree_open() [1] and it assign
the S_IFREG in hfsplus_system_read_inode() [2]. But in HFS we call
iget_locked() instead. I think we need to call hfs_iget() there and to
have something like hfs_system_read_inode() that will assign S_IFREG to
the i_mode. But current fix doesn't look like right solution now. Could
you please explore this direction for the fix?

Thanks,
Slava.

>     {
>     struct hfs_mdb *mdb = HFS_SB(sb)->mdb;
>     HFS_I(tree->inode)->flags = 0;
> diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c
> index ac4a9055c5c0..bb759345f1fc 100644
> --- a/fs/hfs/inode.c
> +++ b/fs/hfs/inode.c
> @@ -432,6 +432,17 @@ struct inode *hfs_iget(struct super_block *sb,
> struct hfs_cat_key *key, hfs_cat_
>     default:
>        return NULL;
>     }
> +
> +   /* Refuse catalog records that address the internal B-tree
> objects:
> +    * a crafted image can otherwise make hfs_lookup() return the
> cached
> +    * typeless B-tree inode, which then reaches may_open() with
> +    * i_mode == 0 and trips VFS_BUG_ON_INODE() there.  NULL follows
> the
> +    * existing convention for unaddressable records (mount failure /
> +    * -EACCES).
> +    */
> +   if (cnid == HFS_EXT_CNID || cnid == HFS_CAT_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);
> --
> 2.39.5

[1]
https://elixir.bootlin.com/linux/v7.3-rc3/source/fs/hfsplus/btree.c#L286
[2]
https://elixir.bootlin.com/linux/v7.3-rc3/source/fs/hfsplus/super.c#L60

[PATCH v2 0/2] hfs: keep the internal B-tree inodes out of the VFS open path

[1sh1ro]

Hi Viacheslav,

v2, following your suggestion.  hfs_btree_open() no longer patches i_mode
up by hand: there is now an hfs_system_iget()/hfs_system_read_inode()
pair, the HFS counterpart of hfsplus_iget()/hfsplus_system_read_inode().
The B-tree inodes are created through the generic inode-cache path and
get their S_IFREG there, and the extent-record validation still runs
before hfs_btree_open() succeeds.

hfs_iget() itself is keyed on a catalog record (key + rec) and the
B-tree inodes have no catalog record, so hfs_system_iget() is a sibling
entry point sharing the same inode-cache path.  Folding it into
hfs_iget() is a small change if you prefer that shape.

Patch 2 keeps the alias guard from v1 - a catalog record must not address
CNID 3/4 - as hardening on top of patch 1.  Drop it if you consider
hfs_is_valid_cnid()'s acceptance of HFS_CDR_FIL records for those CNIDs
intentional.

The series applies to v7.3-rc5 and fs/hfs builds clean with clang.

Thanks,
1sh1ro

1sh1ro (2):
  hfs: create the internal B-tree inodes through the generic iget path
  hfs: refuse catalog records that address the reserved B-tree CNIDs

 fs/hfs/btree.c  |  25 +++++--------------------
 fs/hfs/hfs_fs.h |   1 +
 fs/hfs/inode.c  |  85 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 91 insertions(+), 20 deletions(-)

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

[1sh1ro]

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.

Signed-off-by: 1sh1ro <yangguikun19@qq.com>
---
  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().
+	 */
+	if (cnid == HFS_EXT_CNID || cnid == HFS_CAT_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);
-- 
2.39.5

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@qq.com>

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

[PATCH v3] hfs: give the internal B-tree inodes a type and keep catalog records off them

[yangguikun]

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.  Two problems follow from that, and both are
reachable from a crafted image.

First, a catalog record that names CNID 3 or 4 makes hfs_lookup() -> hfs_iget()
return the cached B-tree 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.

Second, once the inode has a type, such a record still hands the internal
B-tree file out as a regular dentry: it can be opened and read, which exposes
the B-tree, and from then on the VFS treats an inode that has no catalog
record behind it as an ordinary file.  hfs_is_valid_cnid() does not close this
path: it runs from hfs_read_inode(), and a lookup that hits the already cached
B-tree inode never reaches that function.

Fix both.  Add hfs_system_read_inode(), which reads the fork from the MDB and
assigns S_IFREG - 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(), and the extent-record validation moves into the new read
helper.  hfs_iget() refuses catalog records that name a reserved CNID below
HFS_FIRSTUSER_CNID; the root directory, the one reserved CNID that legitimately
comes through there, is exempt.

Found by syzkaller-style fuzzing of v7.2-rc7; still reproducible on current
mainline.

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>
---
 fs/hfs/btree.c  | 39 +++------------------------
 fs/hfs/hfs_fs.h |  1 +
 fs/hfs/inode.c  | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 86 insertions(+), 36 deletions(-)

diff --git a/fs/hfs/btree.c b/fs/hfs/btree.c
index 41b4e8f..b5892de 100644
--- a/fs/hfs/btree.c
+++ b/fs/hfs/btree.c
@@ -155,44 +155,11 @@ 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);
+	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..5c00383 100644
--- a/fs/hfs/inode.c
+++ b/fs/hfs/inode.c
@@ -421,6 +421,87 @@ 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.
+ */
+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));
+		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.
+	 */
+	inode->i_mode = S_IFREG;
+
+	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.
+ */
+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()
  *
@@ -446,6 +527,15 @@ 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 only, so a catalog record naming
+	 * a reserved CNID would hand out a B-tree inode as a regular dentry.
+	 * The root directory is the one reserved CNID that comes through here.
+	 */
+	if (cnid != HFS_ROOT_CNID && cnid < HFS_FIRSTUSER_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);
-- 
2.39.5

Re: [PATCH v3] hfs: give the internal B-tree inodes a type and keep catalog records off them

[Viacheslav Dubeyko]

On Tue, 2026-10-13 at 14:44 +0800, yangguikun 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.  Two problems follow from that, and
> both are
> reachable from a crafted image.
> 
> First, a catalog record that names CNID 3 or 4 makes hfs_lookup() ->
> hfs_iget()
> return the cached B-tree 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.
> 
> Second, once the inode has a type, such a record still hands the
> internal
> B-tree file out as a regular dentry: it can be opened and read, which
> exposes
> the B-tree, and from then on the VFS treats an inode that has no
> catalog
> record behind it as an ordinary file.  hfs_is_valid_cnid() does not
> close this
> path: it runs from hfs_read_inode(), and a lookup that hits the
> already cached
> B-tree inode never reaches that function.
> 
> Fix both.  Add hfs_system_read_inode(), which reads the fork from the
> MDB and
> assigns S_IFREG - 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(), and the extent-record validation moves into the
> new read
> helper.  hfs_iget() refuses catalog records that name a reserved CNID
> below
> HFS_FIRSTUSER_CNID; the root directory, the one reserved CNID that
> legitimately
> comes through there, is exempt.
> 
> Found by syzkaller-style fuzzing of v7.2-rc7; still reproducible on
> current
> mainline.
> 
> 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>
> ---
>  fs/hfs/btree.c  | 39 +++------------------------
>  fs/hfs/hfs_fs.h |  1 +
>  fs/hfs/inode.c  | 82
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 86 insertions(+), 36 deletions(-)
> 
> diff --git a/fs/hfs/btree.c b/fs/hfs/btree.c
> index 41b4e8f..b5892de 100644
> --- a/fs/hfs/btree.c
> +++ b/fs/hfs/btree.c
> @@ -155,44 +155,11 @@ 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;

I worried that we jump to free_tree now. Before, we cannot get inode
but now method can or cannot get inode. We need somehow distinguish
these cases now and to jump to free_tree or to free_inode. Or we need
to manage it in more smart way.

> -	BUG_ON(!(inode_state_read_once(tree->inode) & I_NEW));

I am not sure that it's good idea to drop this. If hfs_system_iget()
quietly returns an existing inode. In hfs_btree_open() that would mean
the same tree was opened twice.

> -	{
> -	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);
> +	tree->inode->i_mapping->a_ops = &hfs_btree_aops;

 Set a_ops inside hfs_system_read_inode(). Right now tree->inode-
>i_mapping->a_ops = &hfs_btree_aops; runs in hfs_btree_open() after
unlock_new_inode(), so the inode is visible in the hash briefly without
its 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..5c00383 100644
> --- a/fs/hfs/inode.c
> +++ b/fs/hfs/inode.c
> @@ -421,6 +421,87 @@ 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.
> + */
> +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));
> +		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.
> +	 */
> +	inode->i_mode = S_IFREG;
> +
> +	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.
> + */
> +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()
>   *
> @@ -446,6 +527,15 @@ 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 only, so a catalog
> record naming
> +	 * a reserved CNID would hand out a B-tree inode as a
> regular dentry.
> +	 * The root directory is the one reserved CNID that comes
> through here.
> +	 */
> +	if (cnid != HFS_ROOT_CNID && cnid < HFS_FIRSTUSER_CNID)

I am guessing maybe we need to introduce a special method:

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;
}

What do you think?

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

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

[yangguikun]

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.
+	 */
+	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.
+ */
+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;
+}
+
 /*
  * 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.
+ */
+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));
+		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.
+	 */
+	inode->i_mode = S_IFREG;
+
+	/* Set before hfs_system_iget() publishes the inode. */
+	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.
+ */
+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.
+	 */
+	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.
+	 */
+	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);
-- 
2.39.5

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

[Viacheslav Dubeyko]

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

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

[yangguikun]

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 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);
 	}
 	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;
+
+	inode->i_uid = hsb->s_uid;
+	inode->i_gid = hsb->s_gid;
+	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);
+	}
+
+	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);

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

[Viacheslav Dubeyko]

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

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

[yangguikun]

Hi Slava,

Thanks for the detailed review.

On set_nlink(): I removed the explicit call, but please note that it
was a no-op: inode_init_always_gfp() sets __i_nlink = 1 (fs/inode.c:238)
for every new inode, and the original iget_locked() path in
hfs_btree_open() never cleared it. So the B-tree inodes have had
i_nlink == 1 since before this series, inode_generic_drop() returns
false for them, and the final iput() in hfs_btree_close() already took
the !drop branch with writeback. I could not construct the
still-dirty-at-put_super scenario outside of an error in the umount
sync; if you have a concrete case in mind, I am happy to test it. The
redundant set_nlink() is still worth removing for clarity.

tz_secondswest is only consumed by hfs_revalidate_dentry(), which runs
from dentries; the B-tree inodes never get one, so I removed that
initialization as well.

I will use the WARN_ON_ONCE() form for the already-cached inode case.

On hfs_iget(): agreed on both counts. Returning ERR_PTR() lets the
corruption cases (-EIO) and allocation failure (-ENOMEM) stay
distinguishable, and both callers currently guess: hfs_lookup()
translates NULL to -EACCES and hfs_fill_super() reports -EINVAL.
minix_iget() already works this way. In v5 I only did what the v4
review suggested (-EIO), but the complete fix is the ERR_PTR()
conversion, so I reverted the dir.c change and will send it as a
separate patch with the caller updates.

Patch v6 follows.

Thanks,
yangguikun

From 014ad11799ffc5fb857c23337e26121ee571f76a Mon Sep 17 00:00:00 2001
From: yangguikun <yangguikun19@qq.com>
Date: Sat, 17 Oct 2026 01:49:17 +0800
Subject: [PATCH v6] hfs: initialize B-tree inodes and validate catalog CNIDs

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 v6:
 - Drop redundant set_nlink(inode, 1); the link count remains 1.
 - Drop tz_secondswest initialization for B-tree inodes.
 - Use WARN_ON_ONCE() for an already-cached B-tree inode.
 - Defer hfs_iget() error propagation to a separate patch.

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/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..a66de7b 100644
--- a/fs/hfs/inode.c
+++ b/fs/hfs/inode.c
@@ -421,6 +421,69 @@ 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);
+
+	inode->i_uid = hsb->s_uid;
+	inode->i_gid = hsb->s_gid;
+
+	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 (WARN_ON_ONCE(!(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 +509,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);
-- 
2.53.0.windows.1

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

[Viacheslav Dubeyko]

Hi Yangguikun,

Could you please resend the v6 patch with proper subject? Otherwise, it
looks like a mess. I see it as Re: [PATCH v5] hfs: initialize B-tree
inodes and validate catalog CNIDs (but not as v6 patch).

The patch looks good, by the way. I think I would like to run the
xfstests for the v6 patch after receiving it as proper patch. ;) And I
will need to have the second patch too for the check.

Thanks,
Slava.

On Sat, 2026-10-17 at 13:14 +0800, yangguikun wrote:
> Hi Slava,
> 
> Thanks for the detailed review.
> 
> On set_nlink(): I removed the explicit call, but please note that it
> was a no-op: inode_init_always_gfp() sets __i_nlink = 1
> (fs/inode.c:238)
> for every new inode, and the original iget_locked() path in
> hfs_btree_open() never cleared it. So the B-tree inodes have had
> i_nlink == 1 since before this series, inode_generic_drop() returns
> false for them, and the final iput() in hfs_btree_close() already
> took
> the !drop branch with writeback. I could not construct the
> still-dirty-at-put_super scenario outside of an error in the umount
> sync; if you have a concrete case in mind, I am happy to test it. The
> redundant set_nlink() is still worth removing for clarity.
> 
> tz_secondswest is only consumed by hfs_revalidate_dentry(), which
> runs
> from dentries; the B-tree inodes never get one, so I removed that
> initialization as well.
> 
> I will use the WARN_ON_ONCE() form for the already-cached inode case.
> 
> On hfs_iget(): agreed on both counts. Returning ERR_PTR() lets the
> corruption cases (-EIO) and allocation failure (-ENOMEM) stay
> distinguishable, and both callers currently guess: hfs_lookup()
> translates NULL to -EACCES and hfs_fill_super() reports -EINVAL.
> minix_iget() already works this way. In v5 I only did what the v4
> review suggested (-EIO), but the complete fix is the ERR_PTR()
> conversion, so I reverted the dir.c change and will send it as a
> separate patch with the caller updates.
> 
> Patch v6 follows.
> 
> Thanks,
> yangguikun
> 
> From 014ad11799ffc5fb857c23337e26121ee571f76a Mon Sep 17 00:00:00
> 2001
> From: yangguikun <yangguikun19@qq.com>
> Date: Sat, 17 Oct 2026 01:49:17 +0800
> Subject: [PATCH v6] hfs: initialize B-tree inodes and validate
> catalog CNIDs
> 
> 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 v6:
>  - Drop redundant set_nlink(inode, 1); the link count remains 1.
>  - Drop tz_secondswest initialization for B-tree inodes.
>  - Use WARN_ON_ONCE() for an already-cached B-tree inode.
>  - Defer hfs_iget() error propagation to a separate patch.
> 
> 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/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..a66de7b 100644
> --- a/fs/hfs/inode.c
> +++ b/fs/hfs/inode.c
> @@ -421,6 +421,69 @@ 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);
> +
> +	inode->i_uid = hsb->s_uid;
> +	inode->i_gid = hsb->s_gid;
> +
> +	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 (WARN_ON_ONCE(!(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 +509,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);

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

[1sh1ro]

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@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: 1sh1ro <yangguikun19@qq.com>
---
  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");
-			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);
+
+	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
+ *
+ * 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().
+ */
+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.
+	 */
+	inode->i_mode = S_IFREG;
+
+	return 0;
+}
+
+/*
+ * hfs_system_iget
+ *
+ * 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+.
+ */
+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()
  *
-- 
2.39.5

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@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: 1sh1ro <yangguikun19@qq.com>

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()
>   *