[PATCH v2 0/2] hfsplus: avoid re-allocating header node 0 and stop on already-hashed nodes

[PATCH v2 0/2] hfsplus: avoid re-allocating header node 0 and stop on already-hashed nodes

[Shardul Bankar]

Hi,

syzbot reported a warning and crash when mounting a corrupted HFS+ image where
the on-disk B-tree bitmap has node 0 (header node) marked free. In that case
hfs_bmap_alloc() can try to allocate node 0 and reach hfs_bnode_create() with
an already-hashed node number.

Patch 1 prevents allocating the reserved header node (node 0) even if the bitmap
is corrupted.

Patch 2 follows Slava's review suggestion and changes the "already hashed" path
in hfs_bnode_create() to return ERR_PTR(-EEXIST) instead of returning the existing
node pointer, so we don't continue in a non-"business as usual" situation.

v2 changes:
  - Implement Slava's suggestion: return ERR_PTR(-EEXIST) for already-hashed nodes.
  - Keep the node-0 allocation guard as a minimal, targeted hardening measure.

Reported-by: syzbot+1c8ff72d0cd8a50dfeaa@syzkaller.appspotmail.com
Link: https://syzkaller.appspot.com/bug?extid=1c8ff72d0cd8a50dfeaa
Link: https://lore.kernel.org/all/20251213233215.368558-1-shardul.b@mpiricsoftware.com/

Shardul Bankar (2):
  hfsplus: skip node 0 in hfs_bmap_alloc
  hfsplus: return error when node already exists in hfs_bnode_create

 fs/hfsplus/bnode.c | 2 +-
 fs/hfsplus/btree.c | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

-- 
2.34.1

[PATCH v2 1/2] hfsplus: skip node 0 in hfs_bmap_alloc

[Shardul Bankar]

Node 0 is the header node in HFS+ B-trees and should always be allocated.
However, if a filesystem image has node 0's bitmap bit unset (e.g., due to
corruption or a buggy image generator), hfs_bmap_alloc() will find node 0
as free and attempt to allocate it. This causes a conflict because node 0
already exists as the header node, leading to a WARN_ON(1) in
hfs_bnode_create() when the node is found already hashed.

This issue can occur with syzkaller-generated HFS+ images or corrupted
real-world filesystems. Add a guard in hfs_bmap_alloc() to skip node 0
during allocation, providing defense-in-depth against such corruption.

Reported-by: syzbot+1c8ff72d0cd8a50dfeaa@syzkaller.appspotmail.com
Link: https://syzkaller.appspot.com/bug?extid=1c8ff72d0cd8a50dfeaa
Signed-off-by: Shardul Bankar <shardul.b@mpiricsoftware.com>
---
 v2:
 - Keep the node-0 allocation guard as targeted hardening for corrupted images.
 fs/hfsplus/btree.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/hfsplus/btree.c b/fs/hfsplus/btree.c
index 229f25dc7c49..60985f449450 100644
--- a/fs/hfsplus/btree.c
+++ b/fs/hfsplus/btree.c
@@ -411,6 +411,9 @@ struct hfs_bnode *hfs_bmap_alloc(struct hfs_btree *tree)
 			if (byte != 0xff) {
 				for (m = 0x80, i = 0; i < 8; m >>= 1, i++) {
 					if (!(byte & m)) {
+						/* Skip node 0 (header node, always allocated) */
+						if (idx == 0 && i == 0)
+							continue;
 						idx += i;
 						data[off] |= m;
 						set_page_dirty(*pagep);
-- 
2.34.1

Re: [PATCH v2 1/2] hfsplus: skip node 0 in hfs_bmap_alloc

[Viacheslav Dubeyko]

On Wed, 2025-12-24 at 20:43 +0530, Shardul Bankar wrote:
> Node 0 is the header node in HFS+ B-trees and should always be
> allocated.
> However, if a filesystem image has node 0's bitmap bit unset (e.g.,
> due to
> corruption or a buggy image generator), hfs_bmap_alloc() will find
> node 0
> as free and attempt to allocate it. This causes a conflict because
> node 0
> already exists as the header node, leading to a WARN_ON(1) in
> hfs_bnode_create() when the node is found already hashed.
> 
> This issue can occur with syzkaller-generated HFS+ images or
> corrupted
> real-world filesystems. Add a guard in hfs_bmap_alloc() to skip node
> 0
> during allocation, providing defense-in-depth against such
> corruption.
> 
> Reported-by: syzbot+1c8ff72d0cd8a50dfeaa@syzkaller.appspotmail.com
> Link: https://syzkaller.appspot.com/bug?extid=1c8ff72d0cd8a50dfeaa
> Signed-off-by: Shardul Bankar <shardul.b@mpiricsoftware.com>
> ---
>  v2:
>  - Keep the node-0 allocation guard as targeted hardening for
> corrupted images.
>  fs/hfsplus/btree.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/hfsplus/btree.c b/fs/hfsplus/btree.c
> index 229f25dc7c49..60985f449450 100644
> --- a/fs/hfsplus/btree.c
> +++ b/fs/hfsplus/btree.c
> @@ -411,6 +411,9 @@ struct hfs_bnode *hfs_bmap_alloc(struct hfs_btree
> *tree)
>  			if (byte != 0xff) {
>  				for (m = 0x80, i = 0; i < 8; m >>=
> 1, i++) {
>  					if (!(byte & m)) {
> +						/* Skip node 0
> (header node, always allocated) */
> +						if (idx == 0 && i ==
> 0)
> +							continue;

I think that it's not completely correct fix. First of all, we have
bitmap corruption. It means that we need to complain about it and
return error code. Logic cannot continue to work normally because we
cannot rely on bitmap anymore. It could contain multiple corrupted
bits.

Technically speaking, we need to check that bitmap is corrupted when we
create b-trees during mount operation (we can define it for node 0 but
it could be tricky for other nodes). If we have detected the
corruption, then we can recommend to run FSCK tool and we can mount in
READ-ONLY mode.

I think we can check the bitmap when we are trying to open/create not a
new node but already existing in the tree. I mean if we mounted the
volume this b-tree containing several nodes on the volume, we can check
that bitmap contains the set bit for these nodes. And if the bit is not
there, then it's clear sign of bitmap corruption. Currently, I haven't
idea how to check corrupted bits that showing presence of not existing
nodes in the b-tree. But I suppose that we can do some check in
driver's logic. Finally, if we detected corruption, then we should
complain about the corruption. Ideally, it will be good to remount in
READ-ONLY mode.

Does it make sense to you?

Thanks,
Slava. 

>  						idx += i;
>  						data[off] |= m;
>  						set_page_dirty(*page
> p);

Re: [PATCH v2 1/2] hfsplus: skip node 0 in hfs_bmap_alloc

[Shardul Bankar]

On Wed, 2025-12-24 at 20:02 -0800, Viacheslav Dubeyko wrote:
> 
> I think that it's not completely correct fix. First of all, we have
> bitmap corruption. It means that we need to complain about it and
> return error code. Logic cannot continue to work normally because we
> cannot rely on bitmap anymore. It could contain multiple corrupted
> bits.
> 
> Technically speaking, we need to check that bitmap is corrupted when
> we
> create b-trees during mount operation (we can define it for node 0
> but
> it could be tricky for other nodes). If we have detected the
> corruption, then we can recommend to run FSCK tool and we can mount
> in
> READ-ONLY mode.
> 
> I think we can check the bitmap when we are trying to open/create not
> a
> new node but already existing in the tree. I mean if we mounted the
> volume this b-tree containing several nodes on the volume, we can
> check
> that bitmap contains the set bit for these nodes. And if the bit is
> not
> there, then it's clear sign of bitmap corruption. Currently, I
> haven't
> idea how to check corrupted bits that showing presence of not
> existing
> nodes in the b-tree. But I suppose that we can do some check in
> driver's logic. Finally, if we detected corruption, then we should
> complain about the corruption. Ideally, it will be good to remount in
> READ-ONLY mode.
> 
> Does it make sense to you?
> 
Hi Slava,

Yes, that makes sense.

Skipping node 0 indeed looks like only a local workaround: if the
bitmap is already inconsistent, we shouldn’t proceed as if it is
trustworthy for further allocations, because other bits could be wrong
as well.

For the next revision I plan to replace the “skip node 0” guard with a
bitmap sanity check during btree open/mount. At minimum, I will verify
that the header node (node 0) is marked allocated, and I will also
investigate whether other existing nodes can be validated as well. If
corruption is detected, the driver will report it and force a read-only
mount, along with a recommendation to run fsck.hfsplus. This avoids
continuing RW operation with a known-bad allocator state.

In parallel, I plan to keep the -EEXIST change in hfs_bnode_create() as
a robustness fix for any remaining or future inconsistency paths.

I’ll post a respin shortly.

If you’re OK with it, I can also post the hfs_bnode_create() -EEXIST
change as a standalone fix, since it independently prevents a refcount
underflow and panic even outside the bitmap-corruption scenario. I’ll
continue working on the bitmap validation in parallel.

Thanks,
Shardul

RE: [PATCH v2 1/2] hfsplus: skip node 0 in hfs_bmap_alloc

[Viacheslav Dubeyko]

On Thu, 2025-12-25 at 12:02 +0530, Shardul Bankar wrote:
> On Wed, 2025-12-24 at 20:02 -0800, Viacheslav Dubeyko wrote:
> > 
> > I think that it's not completely correct fix. First of all, we have
> > bitmap corruption. It means that we need to complain about it and
> > return error code. Logic cannot continue to work normally because we
> > cannot rely on bitmap anymore. It could contain multiple corrupted
> > bits.
> > 
> > Technically speaking, we need to check that bitmap is corrupted when
> > we
> > create b-trees during mount operation (we can define it for node 0
> > but
> > it could be tricky for other nodes). If we have detected the
> > corruption, then we can recommend to run FSCK tool and we can mount
> > in
> > READ-ONLY mode.
> > 
> > I think we can check the bitmap when we are trying to open/create not
> > a
> > new node but already existing in the tree. I mean if we mounted the
> > volume this b-tree containing several nodes on the volume, we can
> > check
> > that bitmap contains the set bit for these nodes. And if the bit is
> > not
> > there, then it's clear sign of bitmap corruption. Currently, I
> > haven't
> > idea how to check corrupted bits that showing presence of not
> > existing
> > nodes in the b-tree. But I suppose that we can do some check in
> > driver's logic. Finally, if we detected corruption, then we should
> > complain about the corruption. Ideally, it will be good to remount in
> > READ-ONLY mode.
> > 
> > Does it make sense to you?
> > 
> Hi Slava,
> 
> Yes, that makes sense.
> 
> Skipping node 0 indeed looks like only a local workaround: if the
> bitmap is already inconsistent, we shouldn’t proceed as if it is
> trustworthy for further allocations, because other bits could be wrong
> as well.
> 
> For the next revision I plan to replace the “skip node 0” guard with a
> bitmap sanity check during btree open/mount. At minimum, I will verify
> that the header node (node 0) is marked allocated, and I will also
> investigate whether other existing nodes can be validated as well. If
> corruption is detected, the driver will report it and force a read-only
> mount, along with a recommendation to run fsck.hfsplus. This avoids
> continuing RW operation with a known-bad allocator state.
> 
> In parallel, I plan to keep the -EEXIST change in hfs_bnode_create() as
> a robustness fix for any remaining or future inconsistency paths.
> 
> I’ll post a respin shortly.

Sounds good.

> 
> If you’re OK with it, I can also post the hfs_bnode_create() -EEXIST
> change as a standalone fix, since it independently prevents a refcount
> underflow and panic even outside the bitmap-corruption scenario. I’ll
> continue working on the bitmap validation in parallel.
> 

Yes, we can do it in this way. Makes sense to me.

Thanks,
Slava.

[PATCH v2 2/2] hfsplus: return error when node already exists in hfs_bnode_create

[Shardul Bankar]

When hfs_bnode_create() finds that a node is already hashed (which should
not happen in normal operation), it currently returns the existing node
without incrementing its reference count. This causes a reference count
inconsistency that leads to a kernel panic when the node is later freed
in hfs_bnode_put():

    kernel BUG at fs/hfsplus/bnode.c:676!
    BUG_ON(!atomic_read(&node->refcnt))

This scenario can occur when hfs_bmap_alloc() attempts to allocate a node
that is already in use (e.g., when node 0's bitmap bit is incorrectly
unset), or due to filesystem corruption.

Returning an existing node from a create path is not normal operation.

Fix this by returning ERR_PTR(-EEXIST) instead of the node when it's
already hashed. This properly signals the error condition to callers,
which already check for IS_ERR() return values.

Reported-by: syzbot+1c8ff72d0cd8a50dfeaa@syzkaller.appspotmail.com
Link: https://syzkaller.appspot.com/bug?extid=1c8ff72d0cd8a50dfeaa
Fixes: 634725a92938 ("[PATCH] hfs: cleanup HFS+ prints")
Signed-off-by: Shardul Bankar <shardul.b@mpiricsoftware.com>
---
 v2:
 - Return ERR_PTR(-EEXIST) for already-hashed nodes, per Slava's suggestion.
 fs/hfsplus/bnode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/hfsplus/bnode.c b/fs/hfsplus/bnode.c
index 191661af9677..250a226336ea 100644
--- a/fs/hfsplus/bnode.c
+++ b/fs/hfsplus/bnode.c
@@ -629,7 +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);
-		return node;
+		return ERR_PTR(-EEXIST);
 	}
 	node = __hfs_bnode_create(tree, num);
 	if (!node)
-- 
2.34.1