[PATCH] hfsplus: fix missing hfs_bnode_get() in hfs_bnode_create()

[PATCH] hfsplus: fix missing hfs_bnode_get() in hfs_bnode_create()

[Shardul Bankar]

When hfs_bnode_create() finds an existing node in the hash table, it
returns the node without incrementing its reference count. This causes
the reference count to become inconsistent, leading to a kernel panic
when hfs_bnode_put() is later called with refcnt=0:

    BUG_ON(!atomic_read(&node->refcnt))

This occurs because hfs_bmap_alloc() calls hfs_bnode_create() expecting
to receive a node with a proper reference count, but if the node is
already in the hash table, it is returned without the required refcnt
increment.

Fix this by calling hfs_bnode_get() when returning an existing node,
ensuring the reference count is properly incremented. This follows the
same pattern as the fix in __hfs_bnode_create() (commit 152af1142878
 ("hfsplus: fix missing hfs_bnode_get() in __hfs_bnode_create")).

Note: While finding an existing node in hfs_bnode_create() is unexpected
(indicated by the pr_crit warning), we still need proper refcnt management
to prevent crashes. The warning will still fire to alert about the
underlying issue (e.g., bitmap corruption or logic error causing an
existing node to be requested for allocation).

Link: https://syzkaller.appspot.com/bug?extid=1c8ff72d0cd8a50dfeaa
Fixes: 634725a92938 ("[PATCH] hfs: cleanup HFS+ prints")
Signed-off-by: Shardul Bankar <shardul.b@mpiricsoftware.com>
---
 fs/hfsplus/bnode.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/hfsplus/bnode.c b/fs/hfsplus/bnode.c
index 191661af9677..e098e05add43 100644
--- a/fs/hfsplus/bnode.c
+++ b/fs/hfsplus/bnode.c
@@ -629,6 +629,7 @@ struct hfs_bnode *hfs_bnode_create(struct hfs_btree *tree, u32 num)
 	if (node) {
 		pr_crit("new node %u already hashed?\n", num);
 		WARN_ON(1);
+		hfs_bnode_get(node);
 		return node;
 	}
 	node = __hfs_bnode_create(tree, num);
-- 
2.34.1

Re: [PATCH] hfsplus: fix missing hfs_bnode_get() in hfs_bnode_create()

[Viacheslav Dubeyko]

On Sun, 2025-12-14 at 05:02 +0530, Shardul Bankar wrote:
> When hfs_bnode_create() finds an existing node in the hash table, it
> returns the node without incrementing its reference count. This causes
> the reference count to become inconsistent, leading to a kernel panic
> when hfs_bnode_put() is later called with refcnt=0:
> 
>     BUG_ON(!atomic_read(&node->refcnt))
> 
> This occurs because hfs_bmap_alloc() calls hfs_bnode_create() expecting
> to receive a node with a proper reference count, but if the node is
> already in the hash table, it is returned without the required refcnt
> increment.
> 
> Fix this by calling hfs_bnode_get() when returning an existing node,
> ensuring the reference count is properly incremented. This follows the
> same pattern as the fix in __hfs_bnode_create() (commit 152af1142878
>  ("hfsplus: fix missing hfs_bnode_get() in __hfs_bnode_create")).
> 
> Note: While finding an existing node in hfs_bnode_create() is unexpected
> (indicated by the pr_crit warning), we still need proper refcnt management
> to prevent crashes. The warning will still fire to alert about the
> underlying issue (e.g., bitmap corruption or logic error causing an
> existing node to be requested for allocation).
> 
> Link: https://syzkaller.appspot.com/bug?extid=1c8ff72d0cd8a50dfeaa  
> Fixes: 634725a92938 ("[PATCH] hfs: cleanup HFS+ prints")
> Signed-off-by: Shardul Bankar <shardul.b@mpiricsoftware.com>
> ---
>  fs/hfsplus/bnode.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/hfsplus/bnode.c b/fs/hfsplus/bnode.c
> index 191661af9677..e098e05add43 100644
> --- a/fs/hfsplus/bnode.c
> +++ b/fs/hfsplus/bnode.c
> @@ -629,6 +629,7 @@ struct hfs_bnode *hfs_bnode_create(struct hfs_btree *tree, u32 num)
>  	if (node) {
>  		pr_crit("new node %u already hashed?\n", num);
>  		WARN_ON(1);
> +		hfs_bnode_get(node);

Frankly speaking, I don't see the fix here. You are trying to hide the issue but
not fix it. This is situation of the wrong call because we sharing error message
and call WARN_ON() here. And the critical question here: why do we call
hfs_bnode_create() for already created node? Is it issue of tree->node_hash? Or
something wrong with logic that calls the hfs_bnode_create()? You don't suggest
answer to this question(s). I've tried to debug likewise issue two months ago
and I don't know the answer yet. So, you need to dive deeper in the issue or,
please, convince that I am wrong here. Currently, your commit message doesn't
convince me at all.

Thanks,
Slava.

>  		return node;
>  	}
>  	node = __hfs_bnode_create(tree, num);

Re: [PATCH] hfsplus: fix missing hfs_bnode_get() in hfs_bnode_create()

[Shardul Bankar]

On Mon, 2025-12-15 at 19:29 +0000, Viacheslav Dubeyko wrote:
> Frankly speaking, I don't see the fix here. You are trying to hide
> the issue but
> not fix it. This is situation of the wrong call because we sharing
> error message
> and call WARN_ON() here. And the critical question here: why do we
> call
> hfs_bnode_create() for already created node? Is it issue of tree-
> >node_hash? Or
> something wrong with logic that calls the hfs_bnode_create()? You
> don't suggest
> answer to this question(s). I've tried to debug likewise issue two
> months ago
> and I don't know the answer yet. So, you need to dive deeper in the
> issue or,
> please, convince that I am wrong here. Currently, your commit message
> doesn't
> convince me at all.
> 

Hi Slava,

Thank you for the review. You are absolutely correct- the panic is a
symptom of a deeper logic error where the allocator attempts to re-
allocate an existing node.

I have investigated the root cause using the Syzkaller reproducer and
analyzed the crash logs. I found two distinct issues that need to be
addressed, which I plan to submit as a v2 patch series.

1. The Root Cause: Corrupted Allocation Bitmap (Allocator Logic Error):
The Syzkaller-generated image has a corrupted allocation bitmap where
Node 0 (the Header Node) is marked as "Free" (0).

    Mechanism: hfs_bmap_alloc trusts the on-disk bitmap, sees bit 0 is
clear, and attempts to allocate Node 0.

    Conflict: It calls hfs_bnode_create(tree, 0). Since Node 0 is the
header, it is already in the hash table. hfs_bnode_create correctly
detects this and warns:
[41767.838946] hfsplus: new node 0 already hashed?
[41767.839097] WARNING: fs/hfsplus/bnode.c:631 at
hfsplus_bnode_create.cold+0x41/0x49

Proposed Fix (Patch 1): Modify hfs_bmap_alloc to explicitly guard
against allocating Node 0. Node 0 is the B-Tree header and is
structurally reserved; it should never be allocated as a record node,
regardless of what the bitmap claims.

2. The Crash: Unsafe Error Handling (Refcount Violation) Even though
the allocator shouldn't request Node 0, hfs_bnode_create currently
handles the "node exists" case unsafely.

    Mechanism: When it finds the existing node (the header), it prints
the warning but returns the pointer without incrementing the reference
count.

    Result: The caller receives a node pointer, uses it, and eventually
calls hfs_bnode_put. Since the refcount wasn't incremented, this leads
to a refcount underflow/panic.

    Evidence: The panic occurs later in the execution flow (e.g.,
inside hfs_bnode_split or hfsplus_create_cat), proving the system is
running with a "ticking time bomb" node pointer.
[41767.840709] kernel BUG at fs/hfsplus/bnode.c:676!
[41767.840751] RIP: 0010:hfsplus_bnode_put+0x4a0/0x5c0
[41767.840826] Call Trace:
[41767.840833]  hfs_btree_inc_height.isra.0+0x64e/0x8b0
[41767.840878]  hfsplus_brec_insert+0x97b/0xcf0

Proposed Fix (Patch 2): I still believe we should add
hfs_bnode_get(node) in hfs_bnode_create (the original patch). Even if
the allocator is fixed, hfs_bnode_create should be robust. If it
returns a valid pointer, it must guarantee that pointer has a valid
reference reference to prevent UAF/panics, consistent with the fix in
__hfs_bnode_create (commit 152af1142878).

Plan for v2:

    Patch 1: hfsplus: prevent allocation of header node (node 0) (Fixes
the logic error)

    Patch 2: hfsplus: fix missing hfs_bnode_get() in hfs_bnode_create()
(Fixes the crash safety)

Does this approach and analysis address your concerns?

Thanks, Shardul

RE: [PATCH] hfsplus: fix missing hfs_bnode_get() in hfs_bnode_create()

[Viacheslav Dubeyko]

Hi Shardul,

On Tue, 2025-12-16 at 11:31 +0530, Shardul Bankar wrote:
> On Mon, 2025-12-15 at 19:29 +0000, Viacheslav Dubeyko wrote:
> > Frankly speaking, I don't see the fix here. You are trying to hide
> > the issue but
> > not fix it. This is situation of the wrong call because we sharing
> > error message
> > and call WARN_ON() here. And the critical question here: why do we
> > call
> > hfs_bnode_create() for already created node? Is it issue of tree-
> > > node_hash? Or
> > something wrong with logic that calls the hfs_bnode_create()? You
> > don't suggest
> > answer to this question(s). I've tried to debug likewise issue two
> > months ago
> > and I don't know the answer yet. So, you need to dive deeper in the
> > issue or,
> > please, convince that I am wrong here. Currently, your commit message
> > doesn't
> > convince me at all.
> > 
> 
> Hi Slava,
> 
> Thank you for the review. You are absolutely correct- the panic is a
> symptom of a deeper logic error where the allocator attempts to re-
> allocate an existing node.
> 
> I have investigated the root cause using the Syzkaller reproducer and
> analyzed the crash logs. I found two distinct issues that need to be
> addressed, which I plan to submit as a v2 patch series.
> 
> 1. The Root Cause: Corrupted Allocation Bitmap (Allocator Logic Error):
> The Syzkaller-generated image has a corrupted allocation bitmap where
> Node 0 (the Header Node) is marked as "Free" (0).
> 
>     Mechanism: hfs_bmap_alloc trusts the on-disk bitmap, sees bit 0 is
> clear, and attempts to allocate Node 0.
> 
>     Conflict: It calls hfs_bnode_create(tree, 0). Since Node 0 is the
> header, it is already in the hash table. hfs_bnode_create correctly
> detects this and warns:
> [41767.838946] hfsplus: new node 0 already hashed?
> [41767.839097] WARNING: fs/hfsplus/bnode.c:631 at
> hfsplus_bnode_create.cold+0x41/0x49
> 
> Proposed Fix (Patch 1): Modify hfs_bmap_alloc to explicitly guard
> against allocating Node 0. Node 0 is the B-Tree header and is
> structurally reserved; it should never be allocated as a record node,
> regardless of what the bitmap claims.
> 
> 2. The Crash: Unsafe Error Handling (Refcount Violation) Even though
> the allocator shouldn't request Node 0, hfs_bnode_create currently
> handles the "node exists" case unsafely.
> 
>     Mechanism: When it finds the existing node (the header), it prints
> the warning but returns the pointer without incrementing the reference
> count.
> 
>     Result: The caller receives a node pointer, uses it, and eventually
> calls hfs_bnode_put. Since the refcount wasn't incremented, this leads
> to a refcount underflow/panic.
> 
>     Evidence: The panic occurs later in the execution flow (e.g.,
> inside hfs_bnode_split or hfsplus_create_cat), proving the system is
> running with a "ticking time bomb" node pointer.
> [41767.840709] kernel BUG at fs/hfsplus/bnode.c:676!
> [41767.840751] RIP: 0010:hfsplus_bnode_put+0x4a0/0x5c0
> [41767.840826] Call Trace:
> [41767.840833]  hfs_btree_inc_height.isra.0+0x64e/0x8b0
> [41767.840878]  hfsplus_brec_insert+0x97b/0xcf0
> 
> Proposed Fix (Patch 2): I still believe we should add
> hfs_bnode_get(node) in hfs_bnode_create (the original patch). Even if
> the allocator is fixed, hfs_bnode_create should be robust. If it
> returns a valid pointer, it must guarantee that pointer has a valid
> reference reference to prevent UAF/panics, consistent with the fix in
> __hfs_bnode_create (commit 152af1142878).
> 
> Plan for v2:
> 
>     Patch 1: hfsplus: prevent allocation of header node (node 0) (Fixes
> the logic error)
> 
>     Patch 2: hfsplus: fix missing hfs_bnode_get() in hfs_bnode_create()
> (Fixes the crash safety)
> 
> Does this approach and analysis address your concerns?
> 
> 

The fix in hfs_bmap_alloc() sounds reasonable to me. But I don't see the point
of adding hfs_bnode_get() in hfs_bnode_create() for the case of erroneous
situation [1]:

	if (node) {
		pr_crit("new node %u already hashed?\n", num);
		WARN_ON(1);
		return node;
	}

It will be much better to return ERR_PTR(-EEXIST) here. Because, it is not
situation of "doing business as usual". We should not continue to believe that
"sun is shining for us", but we should stop the logic somehow.

Thanks,
Slava.

[1] https://elixir.bootlin.com/linux/v6.18/source/fs/hfs/bnode.c#L518

Re: [PATCH] hfsplus: fix missing hfs_bnode_get() in hfs_bnode_create()

[Shardul Bankar]

On Tue, 2025-12-16 at 20:28 +0000, Viacheslav Dubeyko wrote:
> 
> The fix in hfs_bmap_alloc() sounds reasonable to me. But I don't see
> the point
> of adding hfs_bnode_get() in hfs_bnode_create() for the case of
> erroneous
> situation [1]:
> 
>         if (node) {
>                 pr_crit("new node %u already hashed?\n", num);
>                 WARN_ON(1);
>                 return node;
>         }
> 
> It will be much better to return ERR_PTR(-EEXIST) here. Because, it
> is not
> situation of "doing business as usual". We should not continue to
> believe that
> "sun is shining for us", but we should stop the logic somehow.
> 
> Thanks,
> Slava.
> 
> [1] https://elixir.bootlin.com/linux/v6.18/source/fs/hfs/bnode.c#L518

Hi Slava,

Thanks, agreed.

I’ll keep the hfs_bmap_alloc() change to ensure node 0 is never
allocated.

And I agree that the “already hashed?” case in hfs_bnode_create()
should not try to continue by returning a pointer (even with an extra
hfs_bnode_get()). Callers like hfs_btree_inc_height() and
hfs_bnode_split() treat the returned node as a freshly allocated node
and immediately rewrite its header/records. If hfs_bnode_create()
returns an existing hashed node, that effectively overwrites live node
contents and amplifies corruption, which can then cascade into later
failures.

So I’ll rework v2 as a small series:
1/2: guard against allocating node 0 in hfs_bmap_alloc()
2/2: make the “already hashed?” path return ERR_PTR(-EEXIST) and
propagate the error

I’ll send the updated series shortly.

Thanks,
Shardul

Re: [PATCH] hfsplus: fix missing hfs_bnode_get() in hfs_bnode_create()

[Shardul Bankar]

On Wed, 2025-12-24 at 17:30 +0530, Shardul Bankar wrote:
> 
> I’ll send the updated series shortly.
> 

I’ve posted this as a v2 series in a new thread.
Link:
https://lore.kernel.org/all/20251224151347.1861896-1-shardul.b@mpiricsoftware.com/

Thanks,
Shardul