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