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

[Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>]

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