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