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

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

On Sat, 2026-02-28 at 17:53 +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(). Using the newly introduced hfs_bmap_test_bit()
> helper, 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=3Xh1Zs8_REuLRLZWUeZtdPNWJAn9_uLWnGXCc-c5fi_fDbKHAGZHLiy9hnVwiCdw&s=28-20JIeoIS56JYKcsVH4GIMrpUgMAnM8UAVmznGshc&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=3Xh1Zs8_REuLRLZWUeZtdPNWJAn9_uLWnGXCc-c5fi_fDbKHAGZHLiy9hnVwiCdw&s=njxWkO06rTfLLR1NjYq1vfuJLtGXfPVmWMjIuvQhpWY&e= 
> Signed-off-by: Shardul Bankar <shardul.b@mpiricsoftware.com>
> ---
>  fs/hfsplus/btree.c | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
> 
> diff --git a/fs/hfsplus/btree.c b/fs/hfsplus/btree.c
> index 87650e23cd65..ee1edb03a38e 100644
> --- a/fs/hfsplus/btree.c
> +++ b/fs/hfsplus/btree.c
> @@ -239,15 +239,31 @@ static int hfs_bmap_clear_bit(struct hfs_bnode *node, u32 bit_idx)
>  	return 0;
>  }
>  
> +static const char *hfs_btree_name(u32 cnid)
> +{
> +	static const char * const tree_names[] = {
> +		[HFSPLUS_EXT_CNID] = "Extents",
> +		[HFSPLUS_CAT_CNID] = "Catalog",
> +		[HFSPLUS_ATTR_CNID] = "Attributes",
> +	};
> +
> +	if (cnid < ARRAY_SIZE(tree_names) && tree_names[cnid])
> +		return tree_names[cnid];
> +

#define HFS_POR_CNID		1	/* Parent Of the Root */
#define HFSPLUS_POR_CNID	HFS_POR_CNID
#define HFS_ROOT_CNID		2	/* ROOT directory */
#define HFSPLUS_ROOT_CNID	HFS_ROOT_CNID
#define HFS_EXT_CNID		3	/* EXTents B-tree */
#define HFSPLUS_EXT_CNID	HFS_EXT_CNID
#define HFS_CAT_CNID		4	/* CATalog B-tree */
#define HFSPLUS_CAT_CNID	HFS_CAT_CNID
#define HFS_BAD_CNID		5	/* BAD blocks file */
#define HFSPLUS_BAD_CNID	HFS_BAD_CNID
#define HFS_ALLOC_CNID		6	/* ALLOCation file (HFS+) */
#define HFSPLUS_ALLOC_CNID	HFS_ALLOC_CNID
#define HFS_START_CNID		7	/* STARTup file (HFS+) */
#define HFSPLUS_START_CNID	HFS_START_CNID
#define HFS_ATTR_CNID		8	/* ATTRibutes file (HFS+) */
#define HFSPLUS_ATTR_CNID	HFS_ATTR_CNID
#define HFS_EXCH_CNID		15	/* ExchangeFiles temp id */
#define HFSPLUS_EXCH_CNID	HFS_EXCH_CNID
#define HFS_FIRSTUSER_CNID	16	/* first available user id */
#define HFSPLUS_FIRSTUSER_CNID	HFS_FIRSTUSER_CNID

What if cnid will be 1, 2, 5? How correctly will logic works? For may taste, the
declaration looks slightly dangerous.

It will much easier simply introduce the string constants:

#define HFS_EXTENT_TREE_NAME  "Extents"
...
#define HFS_UNKNOWN_BTREE_NAME  "Unknown"

Probably, simple switch will be simpler implementation here:

switch (cnid) {
case HFSPLUS_EXT_CNID:
    return HFS_EXTENT_TREE_NAME;
...
default:
    return HFS_UNKNOWN_BTREE_NAME;
}

Or it needs to introduce array that will initialize all items from 0 - 15.

Maybe, I am too picky here. This logic should work. But I prefer to have string
declarations outside of function.

> +	return "Unknown";
> +}
> +
>  /* 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)
> @@ -352,6 +368,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);
> +	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:

This logic looks mostly good. My main remarks are in the first patch.

Thanks,
Slava.