In hfs_brec_insert() and hfs_brec_update_parent(), hfs_bnode_find()
may return ERR_PTR() on failure, but the result was used without
checking, risking NULL pointer dereference or invalid pointer usage.

Add IS_ERR() checks after these calls and return PTR_ERR()
on error.

Signed-off-by: Haotian Zhang <vulab@iscas.ac.cn>
---
 fs/hfs/brec.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/hfs/brec.c b/fs/hfs/brec.c
index e49a141c87e5..afa1840a4847 100644
--- a/fs/hfs/brec.c
+++ b/fs/hfs/brec.c
@@ -149,6 +149,8 @@ int hfs_brec_insert(struct hfs_find_data *fd, void *entry, int entry_len)
 			new_node->parent = tree->root;
 		}
 		fd->bnode = hfs_bnode_find(tree, new_node->parent);
+		if (IS_ERR(fd->bnode))
+			return PTR_ERR(fd->bnode);
 
 		/* create index data entry */
 		cnid = cpu_to_be32(new_node->this);
@@ -449,6 +451,8 @@ static int hfs_brec_update_parent(struct hfs_find_data *fd)
 			new_node->parent = tree->root;
 		}
 		fd->bnode = hfs_bnode_find(tree, new_node->parent);
+		if (IS_ERR(fd->bnode))
+			return PTR_ERR(fd->bnode);
 		/* create index key and entry */
 		hfs_bnode_read_key(new_node, fd->search_key, 14);
 		cnid = cpu_to_be32(new_node->this);
-- 
2.25.1

On Tue, 2025-12-09 at 10:14 +0800, Haotian Zhang wrote:
> In hfs_brec_insert() and hfs_brec_update_parent(), hfs_bnode_find()
> may return ERR_PTR() on failure, but the result was used without
> checking, risking NULL pointer dereference or invalid pointer usage.
> 
> Add IS_ERR() checks after these calls and return PTR_ERR()
> on error.
> 
> Signed-off-by: Haotian Zhang <vulab@iscas.ac.cn>
> ---
>  fs/hfs/brec.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/fs/hfs/brec.c b/fs/hfs/brec.c
> index e49a141c87e5..afa1840a4847 100644
> --- a/fs/hfs/brec.c
> +++ b/fs/hfs/brec.c
> @@ -149,6 +149,8 @@ int hfs_brec_insert(struct hfs_find_data *fd,
> void *entry, int entry_len)
>  			new_node->parent = tree->root;
>  		}
>  		fd->bnode = hfs_bnode_find(tree, new_node->parent);
> +		if (IS_ERR(fd->bnode))
> +			return PTR_ERR(fd->bnode);
>  
>  		/* create index data entry */
>  		cnid = cpu_to_be32(new_node->this);
> @@ -449,6 +451,8 @@ static int hfs_brec_update_parent(struct
> hfs_find_data *fd)
>  			new_node->parent = tree->root;
>  		}
>  		fd->bnode = hfs_bnode_find(tree, new_node->parent);
> +		if (IS_ERR(fd->bnode))
> +			return PTR_ERR(fd->bnode);
>  		/* create index key and entry */
>  		hfs_bnode_read_key(new_node, fd->search_key, 14);
>  		cnid = cpu_to_be32(new_node->this);

Frankly speaking, I am not sure that we need to add this check.
Because, we are trying to find the parent node that already has been
found in above logic of the method. So, we should have the parent node
available. Potentially, logic could work in wrong way, but we should
already have a reported bug already.

Even if this check makes sense, then we cannot simply return the error
code here. If you check the following logic, then you can see that we
call hfs_bnode_put() for the new node. So, if this check doesn't do
this in the case of error, then we create the leak here.

Have you ever reproduced the issue that you are trying to fix?

Thanks,
Slava.

> In hfs_brec_insert() and hfs_brec_update_parent(), hfs_bnode_find()
> may return ERR_PTR() on failure, but the result was used without
> checking, risking NULL pointer dereference or invalid pointer usage.
> 
> Add IS_ERR() checks after these calls and return PTR_ERR()
> on error.

* Can an other word wrapping look eventually a bit nicer here?
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.18#n658

* How do you think about to add any tags (like “Fixes” and “Cc”) accordingly?
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.18#n145


Regards,
Markus