Re: [PATCH v4 2/2] hfsplus: validate b-tree node 0 bitmap at mount time

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

On Thu, 2026-02-26 at 14:42 +0530, Shardul Bankar wrote:
> Syzkaller reported an issue with corrupted HFS+ images where the b-tree
> allocation bitmap indicates that the header node (Node 0) is free. Node 0
> must always be allocated as it contains the b-tree header record and the
> allocation bitmap itself. Violating this invariant leads to allocator
> corruption, which can cascade into kernel panics or undefined behavior
> when the filesystem attempts to allocate blocks.
> 
> Prevent trusting a corrupted allocator state by adding a validation check
> during hfs_btree_open(). Using the newly introduced map-access helper,
> verify that the MSB of the first bitmap byte (representing Node 0) is
> marked as allocated. Additionally, catch any errors if the map record
> itself is structurally invalid.
> 
> If corruption is detected, print a warning identifying the specific
> corrupted tree (Extents, Catalog, or Attributes) and force the
> filesystem to mount read-only (SB_RDONLY). This prevents kernel panics
> from corrupted images while enabling data recovery by allowing the mount
> to proceed in a safe, read-only mode rather than failing completely.
> 
> Reported-by: syzbot+1c8ff72d0cd8a50dfeaa@syzkaller.appspotmail.com
> Link: https://urldefense.proofpoint.com/v2/url?u=https-3A__syzkaller.appspot.com_bug-3Fextid-3D1c8ff72d0cd8a50dfeaa&d=DwIDAg&c=BSDicqBQBDjDI9RkVyTcHQ&r=q5bIm4AXMzc8NJu1_RGmnQ2fMWKq4Y4RAkElvUgSs00&m=V6ouPNIE325L1Uhl0gL5R48hhzA9Rw4PSf7m7HPyLgNqtuBbiZmizg1krgnu0p47&s=83lgWM491j3f76U6MdBkt3ON-Qi37Pu4KURHxIkDlD0&e= 
> Link: https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_all_54dc9336b514fb10547e27c7d6e1b8b967ee2eda.camel-40ibm.com_&d=DwIDAg&c=BSDicqBQBDjDI9RkVyTcHQ&r=q5bIm4AXMzc8NJu1_RGmnQ2fMWKq4Y4RAkElvUgSs00&m=V6ouPNIE325L1Uhl0gL5R48hhzA9Rw4PSf7m7HPyLgNqtuBbiZmizg1krgnu0p47&s=CEFAhZ2iv4JVGNj2KbN19b0wzKEUx9kZsZIpMdGom8A&e= 
> Signed-off-by: Shardul Bankar <shardul.b@mpiricsoftware.com>
> ---
>  fs/hfsplus/btree.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
> 
> diff --git a/fs/hfsplus/btree.c b/fs/hfsplus/btree.c
> index 22efd6517ef4..e34716cd661b 100644
> --- a/fs/hfsplus/btree.c
> +++ b/fs/hfsplus/btree.c
> @@ -176,9 +176,14 @@ struct hfs_btree *hfs_btree_open(struct super_block *sb, u32 id)
>  	struct hfs_btree *tree;
>  	struct hfs_btree_header_rec *head;
>  	struct address_space *mapping;
> +	struct hfs_bnode *node;
> +	const char *tree_name;
> +	unsigned int page_idx;
>  	struct inode *inode;
>  	struct page *page;
>  	unsigned int size;
> +	u16 bitmap_off, len;
> +	u8 *map_page;
>  
>  	tree = kzalloc_obj(*tree);
>  	if (!tree)
> @@ -283,6 +288,46 @@ struct hfs_btree *hfs_btree_open(struct super_block *sb, u32 id)
>  
>  	kunmap_local(head);
>  	put_page(page);
> +
> +	node = hfs_bnode_find(tree, HFSPLUS_TREE_HEAD);
> +	if (IS_ERR(node))
> +		goto free_inode;
> +
> +	switch (id) {
> +	case HFSPLUS_EXT_CNID:
> +		tree_name = "Extents";
> +		break;
> +	case HFSPLUS_CAT_CNID:
> +		tree_name = "Catalog";
> +		break;
> +	case HFSPLUS_ATTR_CNID:
> +		tree_name = "Attributes";
> +		break;
> +	default:
> +		tree_name = "Unknown";
> +		break;
> +	}

Frankly speaking, it could be enough to share only cnid. But if you would like
to be really nice and to share the tree's name, then I prefer to see an array of
constant strings where you can use cnid as an index. And macro or static inline
method that can check cnid as a input argument. At minimum, simply move this
code into the static inline method. But, array of constant strings could be much
compact and elegant solution for my taste. Because, art of programming is to
represent everything as arrays of something and to apply the generalized loops.
:)

> +
> +	map_page = hfs_bmap_get_map_page(node, &bitmap_off, &len, &page_idx);

I will talk about bitmap_off, len, page_idx at the place of function definition.

You are safe in hfs_btree_open() method because nobody yet will try to access
the bitmap. But you need to use lock_page()/unlock_page() for other logic.

I prefer not to have the obligation of using this asynchronous paradigm of
kmap_local()/kunmap_local(). It will be great to keep this inside of
hfs_bmap_get_map_<something>() method.

I prefer not to keep the whole page/folio for complete operation locked. And,
frankly speaking, you don't need in the whole page because you need a byte or
unsigned long portion of bitmap. So, we can consider likewise interface:

u8 hfs_bmap_get_map_byte(struct hfs_bnode *node, u32 bit_index);

Here, you simply need to check the state of bit in byte (READ-ONLY operation).
So, you can atomically copy the state of the byte in local variable and to check
the bit state in local variable.

If you need to allocate and go change the state of the bit, then you need to use
lock for the whole operation:

<lock_bitmap>
hfs_bmap_get_map_byte();
<check and change byte state in local variable>
hfs_bmap_set_map_byte(); <-- set byte and make memory page dirty
<unlock_bitmap>

Thanks,
Slava.

> +
> +	if (IS_ERR(map_page)) {
> +		pr_warn("(%s): %s Btree (cnid 0x%x) map record invalid/corrupted, forcing read-only.\n",
> +				sb->s_id, tree_name, id);
> +		pr_warn("Run fsck.hfsplus to repair.\n");
> +		sb->s_flags |= SB_RDONLY;
> +		hfs_bnode_put(node);
> +		return tree;
> +	}
> +
> +	if (!(map_page[bitmap_off] & HFSPLUS_BTREE_NODE0_BIT)) {
> +		pr_warn("(%s): %s Btree (cnid 0x%x) bitmap corruption detected, forcing read-only.\n",
> +				sb->s_id, tree_name, id);
> +		pr_warn("Run fsck.hfsplus to repair.\n");
> +		sb->s_flags |= SB_RDONLY;
> +	}
> +	kunmap_local(map_page);
> +	hfs_bnode_put(node);
> +
>  	return tree;
>  
>   fail_page: