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

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

On Sun, 2026-03-15 at 22:50 +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 cascades 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(). Introduce the hfs_bmap_test_bit() helper
> (utilizing the newly added map-access API) to safely verify that the MSB
> of the first bitmap byte (representing Node 0) is marked as allocated.
> 
> If corruption is detected (either structurally invalid map records or an
> illegally cleared bit), print a warning identifying the specific
> corrupted tree and force the filesystem to mount read-only (SB_RDONLY).
> This prevents kernel panics from corrupted images while enabling data
> recovery.
> 
> 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=7dlIxjaWFhoMAoS7ynWJQTY1_vifSFvOUWF3arXEWP24YytxvUupuv7_gWKWUUu1&s=ySMGzbg10Br2OVgCYK-CRCdGleeuQlfw4PenzGgbfsY&e= 
> Link: https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_all_20260228122305.1406308-2D1-2Dshardul.b-40mpiricsoftware.com_&d=DwIDAg&c=BSDicqBQBDjDI9RkVyTcHQ&r=q5bIm4AXMzc8NJu1_RGmnQ2fMWKq4Y4RAkElvUgSs00&m=7dlIxjaWFhoMAoS7ynWJQTY1_vifSFvOUWF3arXEWP24YytxvUupuv7_gWKWUUu1&s=Jz2TFxYLe-GFT6dlKhzUs44DoackTNwb0yGFtrUEU0Q&e= 
> Signed-off-by: Shardul Bankar <shardul.b@mpiricsoftware.com>
> ---
>  fs/hfsplus/btree.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 67 insertions(+)
> 
> diff --git a/fs/hfsplus/btree.c b/fs/hfsplus/btree.c
> index 1c6a27e397fb..7c98b5858f99 100644
> --- a/fs/hfsplus/btree.c
> +++ b/fs/hfsplus/btree.c
> @@ -185,6 +185,32 @@ static struct page *hfs_bmap_get_map_page(struct hfs_bnode *node, struct hfs_bma
>  	return node->page[ctx->page_idx];
>  }
>  
> +/**
> + * hfs_bmap_test_bit - test a bit in the b-tree map
> + * @node: the b-tree node containing the map record
> + * @node_bit_idx: the relative bit index within the node's map record
> + *
> + * Returns 1 if set, 0 if clear, or a negative error code on failure.
> + */
> +static int hfs_bmap_test_bit(struct hfs_bnode *node, u32 node_bit_idx)

Why not return bool data type?

> +{
> +	struct hfs_bmap_ctx ctx;
> +	struct page *page;
> +	u8 *bmap, byte, mask;
> +
> +	page = hfs_bmap_get_map_page(node, &ctx, node_bit_idx / BITS_PER_BYTE);
> +	if (IS_ERR(page))
> +		return PTR_ERR(page);

We can return false for the case of error.

> +
> +	bmap = kmap_local_page(page);
> +	byte = bmap[ctx.off];
> +	kunmap_local(bmap);
> +
> +	mask = 1 << (7 - (node_bit_idx % BITS_PER_BYTE));
> +	return (byte & mask) ? 1 : 0;

This is why I would like to see bool data type. :)

> +}
> +
> +
>  /**
>   * hfs_bmap_clear_bit - clear a bit in the b-tree map
>   * @node: the b-tree node containing the map record
> @@ -218,15 +244,36 @@ static int hfs_bmap_clear_bit(struct hfs_bnode *node, u32 node_bit_idx)
>  	return 0;
>  }
>  
> +#define HFS_EXTENT_TREE_NAME  "Extents"

Maybe we need to have Extents Overflow File (or B-tree), Catalog file,
Attributes file?

> +#define HFS_CATALOG_TREE_NAME "Catalog"
> +#define HFS_ATTR_TREE_NAME    "Attributes"
> +#define HFS_UNKNOWN_TREE_NAME "Unknown"
> +
> +static const char *hfs_btree_name(u32 cnid)
> +{
> +	switch (cnid) {
> +	case HFSPLUS_EXT_CNID:
> +		return HFS_EXTENT_TREE_NAME;
> +	case HFSPLUS_CAT_CNID:
> +		return HFS_CATALOG_TREE_NAME;
> +	case HFSPLUS_ATTR_CNID:
> +		return HFS_ATTR_TREE_NAME;
> +	default:
> +		return HFS_UNKNOWN_TREE_NAME;
> +	}
> +}
> +
>  /* Get a reference to a B*Tree and do some initial checks */
>  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;
>  	struct inode *inode;
>  	struct page *page;
>  	unsigned int size;
> +	int res;
>  
>  	tree = kzalloc_obj(*tree);
>  	if (!tree)
> @@ -331,6 +378,26 @@ 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;
> +
> +	res = hfs_bmap_test_bit(node, 0);

We definitely can return false for both cases.

Thanks,
Slava.

> +	if (res < 0) {
> +		pr_warn("(%s): %s Btree (cnid 0x%x) map record invalid/corrupted, forcing read-only.\n",
> +				sb->s_id, hfs_btree_name(id), id);
> +		pr_warn("Run fsck.hfsplus to repair.\n");
> +		sb->s_flags |= SB_RDONLY;
> +	} else if (res == 0) {
> +		pr_warn("(%s): %s Btree (cnid 0x%x) bitmap corruption detected, forcing read-only.\n",
> +				sb->s_id, hfs_btree_name(id), id);
> +		pr_warn("Run fsck.hfsplus to repair.\n");
> +		sb->s_flags |= SB_RDONLY;
> +	}
> +
> +	hfs_bnode_put(node);
> +
>  	return tree;
>  
>   fail_page: