[PATCH] hfsplus: free cached B-tree nodes on hfs_btree_open() error path

[PATCH] hfsplus: free cached B-tree nodes on hfs_btree_open() error path

[Mahmut Emin Kurhan]

hfs_btree_open() can fail after hfs_bnode_find(tree, HFSPLUS_TREE_HEAD)
has already inserted the head node into tree->node_hash.

__hfs_bnode_create() inserts the new bnode into tree->node_hash before
it reads the node's pages; if a page read fails it sets HFS_BNODE_ERROR
and returns the node still hashed. hfs_bnode_find() then takes its
node_error path, which calls hfs_bnode_put(). hfs_bnode_put() only frees
a node once its refcount reaches zero *and* HFS_BNODE_DELETED is set; for
the errored head node that flag is not set, so the node stays in
tree->node_hash with a zero refcount.

hfs_btree_open() then sees IS_ERR(node) and jumps to free_tree:, which
does a bare kfree(tree). Only hfs_btree_close() walks tree->node_hash[]
and frees the cached nodes, so the head node is leaked. Mounting a
crafted HFS+ image whose head B-tree node fails to read therefore leaks
kernel memory on every attempt.

Reported by kmemleak while fuzzing HFS+ image mounts:

  BUG: memory leak
  unreferenced object (size 192):
    __hfs_bnode_create+0x105/0x8d0 fs/hfsplus/bnode.c
    hfsplus_bnode_find             fs/hfsplus/bnode.c
    hfsplus_btree_open             fs/hfsplus/btree.c
    hfsplus_fill_super             fs/hfsplus/super.c

Free any nodes still present in tree->node_hash on the error path before
freeing the tree. The paths that reach free_tree before hfs_bnode_find()
have an empty hash, so the loop is a no-op there.

Found via coverage-guided fuzzing (syzkaller + kmemleak) by Noroxi.

Signed-off-by: Mahmut Emin Kurhan <guvenlik@noroxi.com>
---
 fs/hfsplus/btree.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/fs/hfsplus/btree.c b/fs/hfsplus/btree.c
index 2ea8cd565..3de32f221 100644
--- a/fs/hfsplus/btree.c
+++ b/fs/hfsplus/btree.c
@@ -403,6 +403,24 @@ struct hfs_btree *hfs_btree_open(struct super_block *sb, u32 id)
 	tree->inode->i_mapping->a_ops = &hfsplus_aops;
 	iput(tree->inode);
  free_tree:
+	/*
+	 * A B*tree node may already have been inserted into tree->node_hash
+	 * (e.g. an errored head node from hfs_bnode_find()).  Only
+	 * hfs_btree_close() frees hashed nodes, so a bare kfree(tree) here
+	 * leaks them.  Release them before freeing the tree.
+	 */
+	{
+		int i;
+		struct hfs_bnode *node;
+
+		for (i = 0; i < NODE_HASH_SIZE; i++) {
+			while ((node = tree->node_hash[i])) {
+				tree->node_hash[i] = node->next_hash;
+				hfs_bnode_free(node);
+				tree->node_hash_cnt--;
+			}
+		}
+	}
 	kfree(tree);
 	return NULL;
 }
--
2.43.0

Re: [PATCH] hfsplus: free cached B-tree nodes on hfs_btree_open() error path

[Viacheslav Dubeyko]

On Wed, 2026-09-30 at 20:50 +0200, Mahmut Emin Kurhan wrote:
> hfs_btree_open() can fail after hfs_bnode_find(tree,
> HFSPLUS_TREE_HEAD)
> has already inserted the head node into tree->node_hash.
> 
> __hfs_bnode_create() inserts the new bnode into tree->node_hash
> before
> it reads the node's pages; if a page read fails it sets
> HFS_BNODE_ERROR
> and returns the node still hashed. hfs_bnode_find() then takes its
> node_error path, which calls hfs_bnode_put(). hfs_bnode_put() only
> frees
> a node once its refcount reaches zero *and* HFS_BNODE_DELETED is set;
> for
> the errored head node that flag is not set, so the node stays in
> tree->node_hash with a zero refcount.
> 
> hfs_btree_open() then sees IS_ERR(node) and jumps to free_tree:,
> which
> does a bare kfree(tree). Only hfs_btree_close() walks tree-
> >node_hash[]
> and frees the cached nodes, so the head node is leaked. Mounting a
> crafted HFS+ image whose head B-tree node fails to read therefore
> leaks
> kernel memory on every attempt.

I assume that HFS code requires the same fix. Am I right?

> 
> Reported by kmemleak while fuzzing HFS+ image mounts:
> 
>   BUG: memory leak
>   unreferenced object (size 192):
>     __hfs_bnode_create+0x105/0x8d0 fs/hfsplus/bnode.c
>     hfsplus_bnode_find             fs/hfsplus/bnode.c
>     hfsplus_btree_open             fs/hfsplus/btree.c
>     hfsplus_fill_super             fs/hfsplus/super.c
> 
> Free any nodes still present in tree->node_hash on the error path
> before
> freeing the tree. The paths that reach free_tree before
> hfs_bnode_find()
> have an empty hash, so the loop is a no-op there.
> 
> Found via coverage-guided fuzzing (syzkaller + kmemleak) by Noroxi.
> 
> Signed-off-by: Mahmut Emin Kurhan <guvenlik@noroxi.com>
> ---
>  fs/hfsplus/btree.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/fs/hfsplus/btree.c b/fs/hfsplus/btree.c
> index 2ea8cd565..3de32f221 100644
> --- a/fs/hfsplus/btree.c
> +++ b/fs/hfsplus/btree.c
> @@ -403,6 +403,24 @@ struct hfs_btree *hfs_btree_open(struct
> super_block *sb, u32 id)
>  	tree->inode->i_mapping->a_ops = &hfsplus_aops;
>  	iput(tree->inode);
>   free_tree:
> +	/*
> +	 * A B*tree node may already have been inserted into tree-
> >node_hash
> +	 * (e.g. an errored head node from hfs_bnode_find()).  Only
> +	 * hfs_btree_close() frees hashed nodes, so a bare
> kfree(tree) here
> +	 * leaks them.  Release them before freeing the tree.
> +	 */
> +	{

I don't like of introducing the brackets here. Let's declare the
variables at the beginning of the method.

> +		int i;
> +		struct hfs_bnode *node;

We already has this declaration [1].

> +
> +		for (i = 0; i < NODE_HASH_SIZE; i++) {
> +			while ((node = tree->node_hash[i])) {
> +				tree->node_hash[i] = node-
> >next_hash;
> +				hfs_bnode_free(node);
> +				tree->node_hash_cnt--;
> +			}
> +		}

This logic looks pretty similar to the hfs_btree_close(). Should we
introduce a small method that can be reused in both cases?

I assume that you are not using the spin_lock(&tree->hash_lock) because
the tree creation is not finished and nobody can try to use the tree.
Am I right?

Thanks,
Slava.

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

> +	}
>  	kfree(tree);
>  	return NULL;
>  }
> --
> 2.43.0

[PATCH v2 0/2] hfsplus, hfs: fix B-tree node leak on hfs_btree_open() error path

[Mahmut Emin Kurhan]

This is v2 of the hfsplus B-tree node leak fix.

Slava, thanks for the review. Addressing your points:

- HFS: you are right. The classic HFS driver has the identical leak:
  __hfs_bnode_create() hashes the node before reading its pages,
  hfs_bnode_put() only frees a node when HFS_BNODE_DELETED is set, and
  hfs_btree_open() does a bare kfree(tree) on its error path. Fixed in
  patch 2/2.

- Dropped the inline block and the duplicate variable declarations. The
  freeing loop is now a small helper, hfs_bnode_hash_free(), reused by
  both hfs_btree_close() and the hfs_btree_open() error path, so nothing
  extra is declared at the call site.

- hash_lock: correct -- it is not taken because hfs_btree_open() has not
  published the tree yet (it is only returned on success), so no other
  thread can reach node_hash. hfs_btree_close() omits it for the same
  reason.

v1: https://lore.kernel.org/linux-fsdevel/20260930185033.1335238-1-guvenlik@noroxi.com

Changes since v1:
 - factor the node-hash freeing into hfs_bnode_hash_free() (Slava Dubeyko)
 - no inline braces / no duplicate declarations (Slava Dubeyko)
 - add the equivalent fix for the classic HFS driver (Slava Dubeyko)

Mahmut Emin Kurhan (2):
  hfsplus: free cached B-tree nodes on hfs_btree_open() error path
  hfs: free cached B-tree nodes on hfs_btree_open() error path

 fs/hfs/btree.c     | 34 ++++++++++++++++++++--------------
 fs/hfsplus/btree.c | 35 ++++++++++++++++++++---------------
 2 files changed, 40 insertions(+), 29 deletions(-)

-- 
2.43.0

[PATCH v2 1/2] hfsplus: free cached B-tree nodes on hfs_btree_open() error path

[Mahmut Emin Kurhan]

hfs_btree_open() can fail after hfs_bnode_find(tree, HFSPLUS_TREE_HEAD) has
already inserted the head node into tree->node_hash.

__hfs_bnode_create() inserts the new bnode into tree->node_hash before it
reads the node pages; if a page read fails it sets HFS_BNODE_ERROR and
returns the node still hashed. hfs_bnode_find() then takes its node_error
path and calls hfs_bnode_put(), which only frees a node once its refcount
reaches zero *and* HFS_BNODE_DELETED is set. For the errored head node that
flag is not set, so it stays in tree->node_hash with a zero refcount.

hfs_btree_open() then sees IS_ERR(node) and jumps to free_tree:, doing a
bare kfree(tree). Only hfs_btree_close() walks tree->node_hash[] and frees
the cached nodes, so the head node is leaked. Mounting a crafted HFS+ image
whose head B-tree node fails to read leaks kernel memory on every attempt.

Factor the node-hash freeing out of hfs_btree_close() into a small helper
hfs_bnode_hash_free() and call it from both hfs_btree_close() and the
hfs_btree_open() error path. No hash_lock is taken because the tree is not
published yet, so no other thread can reach it.

Found via coverage-guided fuzzing (syzkaller + kmemleak) by Noroxi.

Signed-off-by: Mahmut Emin Kurhan <guvenlik@noroxi.com>
---
 fs/hfsplus/btree.c | 35 ++++++++++++++++++++---------------
 1 file changed, 20 insertions(+), 15 deletions(-)

diff --git a/fs/hfsplus/btree.c b/fs/hfsplus/btree.c
index 2ea8cd565..41e01380d 100644
--- a/fs/hfsplus/btree.c
+++ b/fs/hfsplus/btree.c
@@ -265,6 +265,24 @@ static const char *hfs_btree_name(u32 cnid)
 }
 
 /* Get a reference to a B*Tree and do some initial checks */
+static void hfs_bnode_hash_free(struct hfs_btree *tree)
+{
+	struct hfs_bnode *node;
+	int i;
+
+	for (i = 0; i < NODE_HASH_SIZE; i++) {
+		while ((node = tree->node_hash[i])) {
+			tree->node_hash[i] = node->next_hash;
+			if (atomic_read(&node->refcnt))
+				pr_crit("node %d:%d still has %d user(s)!\n",
+					node->tree->cnid, node->this,
+					atomic_read(&node->refcnt));
+			hfs_bnode_free(node);
+			tree->node_hash_cnt--;
+		}
+	}
+}
+
 struct hfs_btree *hfs_btree_open(struct super_block *sb, u32 id)
 {
 	struct hfs_btree *tree;
@@ -403,6 +421,7 @@ struct hfs_btree *hfs_btree_open(struct super_block *sb, u32 id)
 	tree->inode->i_mapping->a_ops = &hfsplus_aops;
 	iput(tree->inode);
  free_tree:
+	hfs_bnode_hash_free(tree);
 	kfree(tree);
 	return NULL;
 }
@@ -410,24 +429,10 @@ struct hfs_btree *hfs_btree_open(struct super_block *sb, u32 id)
 /* Release resources used by a btree */
 void hfs_btree_close(struct hfs_btree *tree)
 {
-	struct hfs_bnode *node;
-	int i;
-
 	if (!tree)
 		return;
 
-	for (i = 0; i < NODE_HASH_SIZE; i++) {
-		while ((node = tree->node_hash[i])) {
-			tree->node_hash[i] = node->next_hash;
-			if (atomic_read(&node->refcnt))
-				pr_crit("node %d:%d "
-						"still has %d user(s)!\n",
-					node->tree->cnid, node->this,
-					atomic_read(&node->refcnt));
-			hfs_bnode_free(node);
-			tree->node_hash_cnt--;
-		}
-	}
+	hfs_bnode_hash_free(tree);
 	iput(tree->inode);
 	kfree(tree);
 }
-- 
2.43.0

[PATCH v2 2/2] hfs: free cached B-tree nodes on hfs_btree_open() error path

[Mahmut Emin Kurhan]

The classic HFS driver has the same B-tree node leak as hfsplus: on the
hfs_btree_open() error path after hfs_bnode_find(tree, HFS_TREE_HEAD), an
errored head node left in tree->node_hash is not freed because free_tree:
does a bare kfree(tree) instead of walking the hash.

Apply the same fix: factor the node-hash freeing into hfs_bnode_hash_free()
and call it from hfs_btree_close() and the hfs_btree_open() error path.

Found via coverage-guided fuzzing (syzkaller + kmemleak) by Noroxi.

Signed-off-by: Mahmut Emin Kurhan <guvenlik@noroxi.com>
---
 fs/hfs/btree.c | 34 ++++++++++++++++++++--------------
 1 file changed, 20 insertions(+), 14 deletions(-)

diff --git a/fs/hfs/btree.c b/fs/hfs/btree.c
index 41b4e8fc9..51d1e19a4 100644
--- a/fs/hfs/btree.c
+++ b/fs/hfs/btree.c
@@ -131,6 +131,24 @@ static int hfs_bmap_clear_bit(struct hfs_bnode *node, u32 node_bit_idx)
 }
 
 /* Get a reference to a B*Tree and do some initial checks */
+static void hfs_bnode_hash_free(struct hfs_btree *tree)
+{
+	struct hfs_bnode *node;
+	int i;
+
+	for (i = 0; i < NODE_HASH_SIZE; i++) {
+		while ((node = tree->node_hash[i])) {
+			tree->node_hash[i] = node->next_hash;
+			if (atomic_read(&node->refcnt))
+				pr_err("node %d:%d still has %d user(s)!\n",
+				       node->tree->cnid, node->this,
+				       atomic_read(&node->refcnt));
+			hfs_bnode_free(node);
+			tree->node_hash_cnt--;
+		}
+	}
+}
+
 struct hfs_btree *hfs_btree_open(struct super_block *sb, u32 id, btree_keycmp keycmp)
 {
 	struct hfs_btree *tree;
@@ -296,6 +314,7 @@ struct hfs_btree *hfs_btree_open(struct super_block *sb, u32 id, btree_keycmp ke
 	tree->inode->i_mapping->a_ops = &hfs_aops;
 	iput(tree->inode);
 free_tree:
+	hfs_bnode_hash_free(tree);
 	kfree(tree);
 	return NULL;
 }
@@ -303,23 +322,10 @@ struct hfs_btree *hfs_btree_open(struct super_block *sb, u32 id, btree_keycmp ke
 /* Release resources used by a btree */
 void hfs_btree_close(struct hfs_btree *tree)
 {
-	struct hfs_bnode *node;
-	int i;
-
 	if (!tree)
 		return;
 
-	for (i = 0; i < NODE_HASH_SIZE; i++) {
-		while ((node = tree->node_hash[i])) {
-			tree->node_hash[i] = node->next_hash;
-			if (atomic_read(&node->refcnt))
-				pr_err("node %d:%d still has %d user(s)!\n",
-				       node->tree->cnid, node->this,
-				       atomic_read(&node->refcnt));
-			hfs_bnode_free(node);
-			tree->node_hash_cnt--;
-		}
-	}
+	hfs_bnode_hash_free(tree);
 	iput(tree->inode);
 	kfree(tree);
 }
-- 
2.43.0

Re: [PATCH v2 1/2] hfsplus: free cached B-tree nodes on hfs_btree_open() error path

[Viacheslav Dubeyko]

On Thu, 2026-10-01 at 01:23 +0200, Mahmut Emin Kurhan wrote:
> hfs_btree_open() can fail after hfs_bnode_find(tree,
> HFSPLUS_TREE_HEAD) has
> already inserted the head node into tree->node_hash.
> 
> __hfs_bnode_create() inserts the new bnode into tree->node_hash
> before it
> reads the node pages; if a page read fails it sets HFS_BNODE_ERROR
> and
> returns the node still hashed. hfs_bnode_find() then takes its
> node_error
> path and calls hfs_bnode_put(), which only frees a node once its
> refcount
> reaches zero *and* HFS_BNODE_DELETED is set. For the errored head
> node that
> flag is not set, so it stays in tree->node_hash with a zero refcount.
> 
> hfs_btree_open() then sees IS_ERR(node) and jumps to free_tree:,
> doing a
> bare kfree(tree). Only hfs_btree_close() walks tree->node_hash[] and
> frees
> the cached nodes, so the head node is leaked. Mounting a crafted HFS+
> image
> whose head B-tree node fails to read leaks kernel memory on every
> attempt.
> 
> Factor the node-hash freeing out of hfs_btree_close() into a small
> helper
> hfs_bnode_hash_free() and call it from both hfs_btree_close() and the
> hfs_btree_open() error path. No hash_lock is taken because the tree
> is not
> published yet, so no other thread can reach it.
> 
> Found via coverage-guided fuzzing (syzkaller + kmemleak) by Noroxi.
> 
> Signed-off-by: Mahmut Emin Kurhan <guvenlik@noroxi.com>
> ---
>  fs/hfsplus/btree.c | 35 ++++++++++++++++++++---------------
>  1 file changed, 20 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/hfsplus/btree.c b/fs/hfsplus/btree.c
> index 2ea8cd565..41e01380d 100644
> --- a/fs/hfsplus/btree.c
> +++ b/fs/hfsplus/btree.c
> @@ -265,6 +265,24 @@ static const char *hfs_btree_name(u32 cnid)
>  }
>  
>  /* Get a reference to a B*Tree and do some initial checks */
> +static void hfs_bnode_hash_free(struct hfs_btree *tree)
> +{
> +	struct hfs_bnode *node;
> +	int i;
> +
> +	for (i = 0; i < NODE_HASH_SIZE; i++) {
> +		while ((node = tree->node_hash[i])) {
> +			tree->node_hash[i] = node->next_hash;
> +			if (atomic_read(&node->refcnt))
> +				pr_crit("node %d:%d still has %d
> user(s)!\n",
> +					node->tree->cnid, node-
> >this,
> +					atomic_read(&node->refcnt));
> +			hfs_bnode_free(node);
> +			tree->node_hash_cnt--;
> +		}
> +	}
> +}
> +
>  struct hfs_btree *hfs_btree_open(struct super_block *sb, u32 id)
>  {
>  	struct hfs_btree *tree;
> @@ -403,6 +421,7 @@ struct hfs_btree *hfs_btree_open(struct
> super_block *sb, u32 id)
>  	tree->inode->i_mapping->a_ops = &hfsplus_aops;
>  	iput(tree->inode);
>   free_tree:
> +	hfs_bnode_hash_free(tree);
>  	kfree(tree);
>  	return NULL;
>  }
> @@ -410,24 +429,10 @@ struct hfs_btree *hfs_btree_open(struct
> super_block *sb, u32 id)
>  /* Release resources used by a btree */
>  void hfs_btree_close(struct hfs_btree *tree)
>  {
> -	struct hfs_bnode *node;
> -	int i;
> -
>  	if (!tree)
>  		return;
>  
> -	for (i = 0; i < NODE_HASH_SIZE; i++) {
> -		while ((node = tree->node_hash[i])) {
> -			tree->node_hash[i] = node->next_hash;
> -			if (atomic_read(&node->refcnt))
> -				pr_crit("node %d:%d "
> -						"still has %d
> user(s)!\n",
> -					node->tree->cnid, node-
> >this,
> -					atomic_read(&node->refcnt));
> -			hfs_bnode_free(node);
> -			tree->node_hash_cnt--;
> -		}
> -	}
> +	hfs_bnode_hash_free(tree);
>  	iput(tree->inode);
>  	kfree(tree);
>  }

Now your patch conflicts with [1]. Please, base your patch on for-next
branch of HFS/HFS+ git tree [2].

Thanks,
Slava.

[1] https://lore.kernel.org/r/20260921153729.600313-1-bruno.produit@trailofbits.com
[2]
https://git.kernel.org/pub/scm/linux/kernel/git/vdubeyko/hfs.git/log/?h=for-next

Re: [PATCH v2 2/2] hfs: free cached B-tree nodes on hfs_btree_open() error path

[Viacheslav Dubeyko]

On Thu, 2026-10-01 at 01:23 +0200, Mahmut Emin Kurhan wrote:
> The classic HFS driver has the same B-tree node leak as hfsplus: on
> the
> hfs_btree_open() error path after hfs_bnode_find(tree,
> HFS_TREE_HEAD), an
> errored head node left in tree->node_hash is not freed because
> free_tree:
> does a bare kfree(tree) instead of walking the hash.
> 
> Apply the same fix: factor the node-hash freeing into
> hfs_bnode_hash_free()
> and call it from hfs_btree_close() and the hfs_btree_open() error
> path.
> 
> Found via coverage-guided fuzzing (syzkaller + kmemleak) by Noroxi.
> 
> Signed-off-by: Mahmut Emin Kurhan <guvenlik@noroxi.com>
> ---
>  fs/hfs/btree.c | 34 ++++++++++++++++++++--------------
>  1 file changed, 20 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/hfs/btree.c b/fs/hfs/btree.c
> index 41b4e8fc9..51d1e19a4 100644
> --- a/fs/hfs/btree.c
> +++ b/fs/hfs/btree.c
> @@ -131,6 +131,24 @@ static int hfs_bmap_clear_bit(struct hfs_bnode
> *node, u32 node_bit_idx)
>  }
>  
>  /* Get a reference to a B*Tree and do some initial checks */
> +static void hfs_bnode_hash_free(struct hfs_btree *tree)
> +{
> +	struct hfs_bnode *node;
> +	int i;
> +
> +	for (i = 0; i < NODE_HASH_SIZE; i++) {
> +		while ((node = tree->node_hash[i])) {
> +			tree->node_hash[i] = node->next_hash;
> +			if (atomic_read(&node->refcnt))
> +				pr_err("node %d:%d still has %d
> user(s)!\n",
> +				       node->tree->cnid, node->this,
> +				       atomic_read(&node->refcnt));
> +			hfs_bnode_free(node);
> +			tree->node_hash_cnt--;
> +		}
> +	}
> +}
> +
>  struct hfs_btree *hfs_btree_open(struct super_block *sb, u32 id,
> btree_keycmp keycmp)
>  {
>  	struct hfs_btree *tree;
> @@ -296,6 +314,7 @@ struct hfs_btree *hfs_btree_open(struct
> super_block *sb, u32 id, btree_keycmp ke
>  	tree->inode->i_mapping->a_ops = &hfs_aops;
>  	iput(tree->inode);
>  free_tree:
> +	hfs_bnode_hash_free(tree);
>  	kfree(tree);
>  	return NULL;
>  }
> @@ -303,23 +322,10 @@ struct hfs_btree *hfs_btree_open(struct
> super_block *sb, u32 id, btree_keycmp ke
>  /* Release resources used by a btree */
>  void hfs_btree_close(struct hfs_btree *tree)
>  {
> -	struct hfs_bnode *node;
> -	int i;
> -
>  	if (!tree)
>  		return;
>  
> -	for (i = 0; i < NODE_HASH_SIZE; i++) {
> -		while ((node = tree->node_hash[i])) {
> -			tree->node_hash[i] = node->next_hash;
> -			if (atomic_read(&node->refcnt))
> -				pr_err("node %d:%d still has %d
> user(s)!\n",
> -				       node->tree->cnid, node->this,
> -				       atomic_read(&node->refcnt));
> -			hfs_bnode_free(node);
> -			tree->node_hash_cnt--;
> -		}
> -	}
> +	hfs_bnode_hash_free(tree);
>  	iput(tree->inode);
>  	kfree(tree);
>  }

Ditto. Now your patch conflicts with [1]. Please, base your patch on
for-next branch of HFS/HFS+ git tree [2].

Thanks,
Slava.

[1]
https://lore.kernel.org/r/20260921153729.600313-1-bruno.produit@trailofbits.com
[2]
https://git.kernel.org/pub/scm/linux/kernel/git/vdubeyko/hfs.git/log/?h=for-next

[PATCH v3 0/2] hfsplus, hfs: fix B-tree node leak on hfs_btree_open() error path

[Mahmut Emin Kurhan]

This is v3 of the hfsplus/hfs B-tree node leak fix, rebased onto the
for-next branch of the HFS/HFS+ tree as requested.

The leak is unchanged: hfs_btree_open() can fail after the head node has
been inserted into tree->node_hash, and the error path does a bare
kfree(tree) without freeing the hashed nodes. The fix factors the
node-hash freeing out of hfs_btree_close() into hfs_bnode_hash_free() and
calls it from both hfs_btree_close() and the hfs_btree_open() error path.

Changes since v2:
 - rebased on for-next; the extracted helper now wraps the hash_lock
   serialized close loop introduced by commit dce0e0248205 ("hfs/hfsplus:
   serialize B-tree close against folio release") (Slava Dubeyko)
 - on the open error path the tree is not published yet, so the lock is
   uncontended but kept for consistency
 - no functional change to the leak fix itself

v2: https://lore.kernel.org/linux-fsdevel/20260930232312.1405042-1-guvenlik@noroxi.com

Mahmut Emin Kurhan (2):
  hfsplus: free cached B-tree nodes on hfs_btree_open() error path
  hfs: free cached B-tree nodes on hfs_btree_open() error path

 fs/hfs/btree.c     | 40 +++++++++++++++++++++++-----------------
 fs/hfsplus/btree.c | 41 +++++++++++++++++++++++------------------
 2 files changed, 46 insertions(+), 35 deletions(-)

-- 
2.43.0

[PATCH v3 1/2] hfsplus: free cached B-tree nodes on hfs_btree_open() error path

[Mahmut Emin Kurhan]

hfs_btree_open() can fail after hfs_bnode_find(tree, HFSPLUS_TREE_HEAD) has
already inserted the head node into tree->node_hash.

__hfs_bnode_create() inserts the new bnode into tree->node_hash before it
reads the node pages; if a page read fails it sets HFS_BNODE_ERROR and
returns the node still hashed. hfs_bnode_find() then takes its node_error
path and calls hfs_bnode_put(), which only frees a node once its refcount
reaches zero *and* HFS_BNODE_DELETED is set. For the errored head node that
flag is not set, so it stays in tree->node_hash with a zero refcount.

hfs_btree_open() then sees IS_ERR(node) and jumps to free_tree:, doing a
bare kfree(tree). Only hfs_btree_close() walks tree->node_hash[] and frees
the cached nodes, so the head node is leaked. Mounting a crafted HFS+ image
whose head B-tree node fails to read leaks kernel memory on every attempt.

Factor the node-hash freeing out of hfs_btree_close() into a small helper
hfs_bnode_hash_free() and call it from both hfs_btree_close() and the
hfs_btree_open() error path. The helper keeps the hash_lock serialization
added in commit dce0e0248205 ("hfs/hfsplus: serialize B-tree close against
folio release"); on the open error path the tree has not been published yet,
so the lock is uncontended but harmless.

Found via coverage-guided fuzzing (syzkaller + kmemleak) by Noroxi.

Signed-off-by: Mahmut Emin Kurhan <guvenlik@noroxi.com>
---
 fs/hfsplus/btree.c | 41 +++++++++++++++++++++++------------------
 1 file changed, 23 insertions(+), 18 deletions(-)

diff --git a/fs/hfsplus/btree.c b/fs/hfsplus/btree.c
index bd4dbdbc8f..2183828d5d 100644
--- a/fs/hfsplus/btree.c
+++ b/fs/hfsplus/btree.c
@@ -265,6 +265,27 @@ static const char *hfs_btree_name(u32 cnid)
 }
 
 /* Get a reference to a B*Tree and do some initial checks */
+static void hfs_bnode_hash_free(struct hfs_btree *tree)
+{
+	struct hfs_bnode *node;
+	int i;
+
+	for (i = 0; i < NODE_HASH_SIZE; i++) {
+		spin_lock(&tree->hash_lock);
+		while ((node = tree->node_hash[i])) {
+			hfs_bnode_unhash(node);
+			spin_unlock(&tree->hash_lock);
+			if (atomic_read(&node->refcnt))
+				pr_crit("node %d:%d still has %d user(s)!\n",
+					node->tree->cnid, node->this,
+					atomic_read(&node->refcnt));
+			hfs_bnode_free(node);
+			spin_lock(&tree->hash_lock);
+		}
+		spin_unlock(&tree->hash_lock);
+	}
+}
+
 struct hfs_btree *hfs_btree_open(struct super_block *sb, u32 id)
 {
 	struct hfs_btree *tree;
@@ -403,6 +424,7 @@ struct hfs_btree *hfs_btree_open(struct super_block *sb, u32 id)
 	tree->inode->i_mapping->a_ops = &hfsplus_aops;
 	iput(tree->inode);
  free_tree:
+	hfs_bnode_hash_free(tree);
 	kfree(tree);
 	return NULL;
 }
@@ -410,27 +432,10 @@ struct hfs_btree *hfs_btree_open(struct super_block *sb, u32 id)
 /* Release resources used by a btree */
 void hfs_btree_close(struct hfs_btree *tree)
 {
-	struct hfs_bnode *node;
-	int i;
-
 	if (!tree)
 		return;
 
-	for (i = 0; i < NODE_HASH_SIZE; i++) {
-		spin_lock(&tree->hash_lock);
-		while ((node = tree->node_hash[i])) {
-			hfs_bnode_unhash(node);
-			spin_unlock(&tree->hash_lock);
-			if (atomic_read(&node->refcnt))
-				pr_crit("node %d:%d "
-						"still has %d user(s)!\n",
-					node->tree->cnid, node->this,
-					atomic_read(&node->refcnt));
-			hfs_bnode_free(node);
-			spin_lock(&tree->hash_lock);
-		}
-		spin_unlock(&tree->hash_lock);
-	}
+	hfs_bnode_hash_free(tree);
 	iput(tree->inode);
 	kfree(tree);
 }
-- 
2.43.0

[PATCH v3 2/2] hfs: free cached B-tree nodes on hfs_btree_open() error path

[Mahmut Emin Kurhan]

The classic HFS driver has the same B-tree node leak as hfsplus: on the
hfs_btree_open() error path after hfs_bnode_find(tree, HFS_TREE_HEAD), an
errored head node left in tree->node_hash is not freed because free_tree:
does a bare kfree(tree) instead of walking the hash.

Apply the same fix: factor the node-hash freeing into hfs_bnode_hash_free()
and call it from hfs_btree_close() and the hfs_btree_open() error path. The
helper keeps the hash_lock serialization from commit dce0e0248205 ("hfs/hfsplus:
serialize B-tree close against folio release").

Found via coverage-guided fuzzing (syzkaller + kmemleak) by Noroxi.

Signed-off-by: Mahmut Emin Kurhan <guvenlik@noroxi.com>
---
 fs/hfs/btree.c | 40 +++++++++++++++++++++++-----------------
 1 file changed, 23 insertions(+), 17 deletions(-)

diff --git a/fs/hfs/btree.c b/fs/hfs/btree.c
index 4f0ddc76e8..140cdb5ec6 100644
--- a/fs/hfs/btree.c
+++ b/fs/hfs/btree.c
@@ -131,6 +131,27 @@ static int hfs_bmap_clear_bit(struct hfs_bnode *node, u32 node_bit_idx)
 }
 
 /* Get a reference to a B*Tree and do some initial checks */
+static void hfs_bnode_hash_free(struct hfs_btree *tree)
+{
+	struct hfs_bnode *node;
+	int i;
+
+	for (i = 0; i < NODE_HASH_SIZE; i++) {
+		spin_lock(&tree->hash_lock);
+		while ((node = tree->node_hash[i])) {
+			hfs_bnode_unhash(node);
+			spin_unlock(&tree->hash_lock);
+			if (atomic_read(&node->refcnt))
+				pr_err("node %d:%d still has %d user(s)!\n",
+				       node->tree->cnid, node->this,
+				       atomic_read(&node->refcnt));
+			hfs_bnode_free(node);
+			spin_lock(&tree->hash_lock);
+		}
+		spin_unlock(&tree->hash_lock);
+	}
+}
+
 struct hfs_btree *hfs_btree_open(struct super_block *sb, u32 id, btree_keycmp keycmp)
 {
 	struct hfs_btree *tree;
@@ -296,6 +317,7 @@ struct hfs_btree *hfs_btree_open(struct super_block *sb, u32 id, btree_keycmp ke
 	tree->inode->i_mapping->a_ops = &hfs_aops;
 	iput(tree->inode);
 free_tree:
+	hfs_bnode_hash_free(tree);
 	kfree(tree);
 	return NULL;
 }
@@ -303,26 +325,10 @@ struct hfs_btree *hfs_btree_open(struct super_block *sb, u32 id, btree_keycmp ke
 /* Release resources used by a btree */
 void hfs_btree_close(struct hfs_btree *tree)
 {
-	struct hfs_bnode *node;
-	int i;
-
 	if (!tree)
 		return;
 
-	for (i = 0; i < NODE_HASH_SIZE; i++) {
-		spin_lock(&tree->hash_lock);
-		while ((node = tree->node_hash[i])) {
-			hfs_bnode_unhash(node);
-			spin_unlock(&tree->hash_lock);
-			if (atomic_read(&node->refcnt))
-				pr_err("node %d:%d still has %d user(s)!\n",
-				       node->tree->cnid, node->this,
-				       atomic_read(&node->refcnt));
-			hfs_bnode_free(node);
-			spin_lock(&tree->hash_lock);
-		}
-		spin_unlock(&tree->hash_lock);
-	}
+	hfs_bnode_hash_free(tree);
 	iput(tree->inode);
 	kfree(tree);
 }
-- 
2.43.0