[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@xxxxxxxxxx>
---
 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