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

Viacheslav Dubeyko
2026-03-16T22:35:21Z

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;
>