[PATCH v2 0/2] hfsplus: avoid re-allocating header node 0 and stop on already-hashed nodes
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...
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(-)
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);
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)
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 ==
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);
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
linux-stable-mirror@lists.linaro.org
-
Shardul Bankar -
Viacheslav Dubeyko