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

[Shardul Bankar <shardul.b@mpiricsoftware.com>]

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