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

[Viacheslav Dubeyko <slava@dubeyko.com>]

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