Re: [PATCH v2] hfs: validate allocation block parameters in hfs_mdb_get()

[Viacheslav Dubeyko <slava@dubeyko.com>]

On Thu, 2026-09-24 at 07:04 +0000, Hui Peng wrote:
> In hfs_mdb_get(), drNmAlBlks (fs_ablocks), drAlBlkSiz (alloc_blksz),
> and
> drFreeBks (free_ablocks) are read from the on-disk Master Directory
> Block
> without checking their mutual consistency against the partition size,
> and
> failure to allocate HFS_SB(sb)->bitmap returns -ENOMEM without
> releasing
> HFS_SB(sb)->mdb_bh, HFS_SB(sb)->mdb, HFS_SB(sb)->alt_mdb_bh, or
> HFS_SB(sb)->alt_mdb.
> 
> Validate that fs_ablocks is non-zero, free_ablocks does not exceed
> fs_ablocks, and drAlBlSt + fs_ablocks * (alloc_blksz >>
> HFS_SECTOR_SIZE_BITS) fits within part_size, and clean up via
> hfs_mdb_put(sb) on error.
> 
> Tested in QEMU against Linux 7.3.0-rc3 by mounting a crafted HFS
> image
> with drNmAlBlks = 10 and drFreeBks = 50: on the unfixed kernel
> hfs_mdb_get() accepts the inconsistent MDB parameters, causing
> filesystem
> corruption ("hfs: (loop2): extents (cnid 0x3) bitmap corrupted");
> whereas
> with the fix applied mount fails immediately with "hfs: inconsistent
> allocation block parameters in MDB" (-EINVAL).
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@vger.kernel.org
> Assisted-by: LLM
> Signed-off-by: Hui Peng <benquike@gmail.com>
> ---
> Changes in v2:
> - Validate consistency between drNmAlBlks, drAlBlkSiz, drFreeBks, and
>   part_size in hfs_mdb_get(), as requested by Viacheslav Dubeyko.
> 
>  fs/hfs/mdb.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/fs/hfs/mdb.c b/fs/hfs/mdb.c
> index 277de712f9d4..665753a2cba9 100644
> --- a/fs/hfs/mdb.c
> +++ b/fs/hfs/mdb.c
> @@ -214,6 +214,14 @@ int hfs_mdb_get(struct super_block *sb)
>  
>  	/* These parameters are read from and written to the MDB */
>  	HFS_SB(sb)->free_ablocks = be16_to_cpu(mdb->drFreeBks);
> +	if (!HFS_SB(sb)->fs_ablocks ||
> +	    HFS_SB(sb)->free_ablocks > HFS_SB(sb)->fs_ablocks ||
> +	    (sector_t)be16_to_cpu(mdb->drAlBlSt) +
> +	    (sector_t)HFS_SB(sb)->fs_ablocks *
> +	    (HFS_SB(sb)->alloc_blksz >> HFS_SECTOR_SIZE_BITS) >
> part_size)

I would like to see the dedicated function for this check. Currently,
it looks like a mess.

> {
> +		pr_err("inconsistent allocation block parameters in
> MDB\n");

pr_warn("filesystem possibly corrupted, running fsck.hfs is
recommended.\n");

You can add more details about corruption in the message.

> +		goto out_err;

I don't see the point to introduce this way of managing the error case.
Let's return  the error right here.

> +	}
>  	atomic64_set(&HFS_SB(sb)->next_id, be32_to_cpu(mdb-
> >drNxtCNID));
>  	HFS_SB(sb)->root_files = be16_to_cpu(mdb->drNmFls);
>  	HFS_SB(sb)->root_dirs = be16_to_cpu(mdb->drNmRtDirs);
> @@ -305,6 +313,10 @@ int hfs_mdb_get(struct super_block *sb)
>  	}
>  
>  	return 0;
> +
> +out_err:
> +	hfs_mdb_put(sb);

You don't need to do it here. The hfs_fill_super() will do it in the
case of error.

> +	return -EINVAL;

It is not the case of -EINVAL. We have corruption here. Another code
error should be used. -EIO sounds better, for example.

But we have invalid free blocks value. It sounds to me that we still
can mount the file system in READ-ONLY mode. Am I right?

Thanks,
Slava.

>  }
>  
>  /*