Re: [PATCH v4 2/2] hfsplus: validate b-tree fork extents at mount time

[Viacheslav Dubeyko]

On Sat, 2026-09-12 at 20:24 +0700, Nguyen Ngoc Thang wrote:
> A hfsplus_check_fork() pass over a special file's eight fork extents,
> called from hfs_btree_open() for the extents, catalog and attributes
> trees:
> 
>  - block_count == 0 but start_block != 0: garbage left in a slot that
>    should be blank (this is what the syzbot-reported image has in the
>    extents overflow file's fork, slots 3 and 6);
>  - start_block + block_count > sbi->total_blocks: an extent pointing
>    past the end of the volume;
>  - a non-zero extent following a zero one: a hole in the used range.
> 
> If the first extent itself fails these checks, the b-tree's location
> on disk is unknown and there is nothing to recover, so
> hfs_btree_open()
> fails as it already does for the other structural checks in that
> function, and the mount fails.
> 
> If only a later extent is affected, the tree can still be opened (its
> first extent, and hence its root node, is fine); mark it corrupt and
> let the caller decide. hfsplus_fill_super() forces the volume
> read-only in that case, and hfsplus_reconfigure() checks the same
> per-tree flag on remount instead of re-deriving it, refusing to go
> back to read-write. attr_tree may be NULL (volumes without an
> attributes fork), so both checks guard for that.
> 
> This also gives the previous patch's hfsplus_file_extend() fix a
> mount-time backstop: a fuzzed or damaged extents overflow fork like
> the one in the syzbot report is caught here before any write ever
> reaches it.
> 
> Reported-by: syzbot+f8ce6c197125ab9d72ce@xxxxxxxxxxxxxxxxxxxxxxxxx
> Signed-off-by: Nguyen Ngoc Thang <ngocthang2710.1999@xxxxxxxxx>
> Co-Authored-By: Claude Sonnet 5 <noreply@xxxxxxxxxxxxx>
> ---
>  fs/hfsplus/btree.c      | 12 ++++++++++++
>  fs/hfsplus/extents.c    | 37 +++++++++++++++++++++++++++++++++++++
>  fs/hfsplus/hfsplus_fs.h |  4 ++++
>  fs/hfsplus/super.c      |  9 +++++++++
>  4 files changed, 62 insertions(+)
> 
> diff --git a/fs/hfsplus/btree.c b/fs/hfsplus/btree.c
> index 2ea8cd5658e1..0a05ade53070 100644
> --- a/fs/hfsplus/btree.c
> +++ b/fs/hfsplus/btree.c
> @@ -293,6 +293,18 @@ struct hfs_btree *hfs_btree_open(struct
> super_block *sb, u32 id)
>  		goto free_inode;
>  	}
>  
> +	switch (hfsplus_check_fork(sb, HFSPLUS_I(tree->inode)-
> >first_extents)) {

If we return error code for corrupted fork (that makes more sense),
then we don't need in switch here.

> +	case -EIO:
> +		pr_err("%s (cnid 0x%x) fork's first extent is
> corrupt\n",
> +			hfs_btree_name(id), id);
> +		goto free_inode;
> +	case 1:

I don't see the point returning 1 from the function. It should be error
code.

> +		pr_warn("%s (cnid 0x%x) fork has corrupt extents,
> forcing read-only.\n",
> +			hfs_btree_name(id), id);
> +		tree->corrupt = true;
> +		break;
> +	}
> +
>  	mapping = tree->inode->i_mapping;
>  	page = read_mapping_page(mapping, 0, NULL);
>  	if (IS_ERR(page))
> diff --git a/fs/hfsplus/extents.c b/fs/hfsplus/extents.c
> index 236f2d9a7a2d..a9303ce5bf8f 100644
> --- a/fs/hfsplus/extents.c
> +++ b/fs/hfsplus/extents.c
> @@ -95,6 +95,43 @@ static bool hfsplus_ext_fork_full(struct
> hfsplus_extent *ext)
>  	return true;
>  }
>  
> +/*
> + * Check a fork's eight extents for the corruption a fuzzed or
> damaged
> + * volume header can contain: garbage in a slot that should be
> unused,
> + * an extent that runs past the end of the volume, or a used extent
> + * following an unused one.
> + *
> + * Returns 0 if the fork is fully consistent, 1 if only extents
> after
> + * the first are affected (the b-tree can still be located, so it's
> + * safe to mount read-only), or -EIO if the first extent itself is
> + * unusable.
> + */
> +int hfsplus_check_fork(struct super_block *sb, struct hfsplus_extent
> *ext)

Why not struct hfsplus_fork_raw here for check?

> +{
> +	struct hfsplus_sb_info *sbi = HFSPLUS_SB(sb);
> +	bool seen_hole = false;
> +	int i;
> +
> +	for (i = 0; i < 8; i++, ext++) {

Ditto. Related to hardcoded value.

> +		u32 start = be32_to_cpu(ext->start_block);
> +		u32 count = be32_to_cpu(ext->block_count);
> +		bool bad;
> +
> +		if (!count) {
> +			bad = start != 0;
> +			seen_hole = true;
> +		} else {
> +			bad = seen_hole || start + count < start ||
> +			      start + count > sbi->total_blocks;
> +		}

I think that current logic of check looks complicated. And I think not
all possible cases are checked. For example, fork cannot be completely
empty. Could we rework the logic to be more clear? Maybe, we need to
introduce the function for extent check, function for checking the
extents are logically contiguous?

Also, the fork contains more details to check:

struct hfsplus_fork_raw {
	__be64 total_size;
	__be32 clump_size;
	__be32 total_blocks;
	hfsplus_extent_rec extents;
} __packed;

Why are we not check the fork itself?

> +
> +		if (bad)
> +			return i ? 1 : -EIO;

Ditto. Related to 1. I prefer to have error code instead.

> +	}
> +
> +	return 0;
> +}
> +
>  static int __hfsplus_ext_write_extent(struct inode *inode,
>  		struct hfs_find_data *fd)
>  {
> diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h
> index 1e5b58e6a13f..8d47219e67d3 100644
> --- a/fs/hfsplus/hfsplus_fs.h
> +++ b/fs/hfsplus/hfsplus_fs.h
> @@ -56,6 +56,9 @@ struct hfs_btree {
>  	unsigned int max_key_len;
>  	unsigned int depth;
>  
> +	/* fork extents past the first were found corrupt at open
> time */
> +	bool corrupt;
> +

I don't want to say that this direction is wrong. However, we have
flags:

#define HFSPLUS_I_CAT_DIRTY	1	/* has changes in the catalog
tree */
#define HFSPLUS_I_EXT_DIRTY	2	/* has changes in the extent
tree */
#define HFSPLUS_I_ALLOC_DIRTY	3	/* has changes in the
allocation file */
#define HFSPLUS_I_ATTR_DIRTY	4	/* has changes in the
attributes tree */

And we are using inode's flag to track the dirty state of the tree.
Potentially, we can introduce the HFSPLUS_I_CORRUPT_TREE. And I think
one flags for all b-tree will be enough because inode is dedicated for
a particular tree. What do you think?

>  	struct mutex tree_lock;
>  
>  	unsigned int pages_per_bnode;
> @@ -440,6 +443,7 @@ int hfsplus_free_fork(struct super_block *sb, u32
> cnid,
>  		      struct hfsplus_fork_raw *fork, int type);
>  int hfsplus_file_extend(struct inode *inode, bool zeroout);
>  void hfsplus_file_truncate(struct inode *inode);
> +int hfsplus_check_fork(struct super_block *sb, struct hfsplus_extent
> *ext);
>  
>  /* inode.c */
>  extern const struct address_space_operations hfsplus_aops;
> diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
> index ff7d6b3336a6..b65edb8ee589 100644
> --- a/fs/hfsplus/super.c
> +++ b/fs/hfsplus/super.c
> @@ -400,6 +400,11 @@ static int hfsplus_reconfigure(struct fs_context
> *fc)
>  			pr_warn("filesystem is marked journaled,
> leaving read-only.\n");
>  			sb->s_flags |= SB_RDONLY;
>  			fc->sb_flags |= SB_RDONLY;
> +		} else if (sbi->ext_tree->corrupt || sbi->cat_tree-
> >corrupt ||
> +				(sbi->attr_tree && sbi->attr_tree-
> >corrupt)) {

Currently, only hfsplus_fill_super() can detect the b-tree corruption.
Why do we have the check here? Do you mean that xattr b-tree can be
created and to be corrupted?

> +			pr_warn("a b-tree fork was corrupt at mount
> time, leaving read-only.\n");
> +			sb->s_flags |= SB_RDONLY;
> +			fc->sb_flags |= SB_RDONLY;
>  		}
>  	}
>  	return 0;
> @@ -564,6 +569,10 @@ static int hfsplus_fill_super(struct super_block
> *sb, struct fs_context *fc)
>  	}
>  	sb->s_xattr = hfsplus_xattr_handlers;
>  
> +	if (sbi->ext_tree->corrupt || sbi->cat_tree->corrupt ||
> +	    (sbi->attr_tree && sbi->attr_tree->corrupt))
> +		sb->s_flags |= SB_RDONLY;

If we fail to check any b-tree, then logic should stop. Why haven't we
checked the error code of hfs_btree_open()?

Thanks,
Slava.

> +
>  	inode = hfsplus_iget(sb, HFSPLUS_ALLOC_CNID);
>  	if (IS_ERR(inode)) {
>  		pr_err("failed to load allocation file\n");