Re: [PATCH] hfs: reject volume bitmaps overlapping the MDB

[Viacheslav Dubeyko]

On Wed, 2026-05-27 at 23:32 +0800, Yue Sun wrote:
> hfs_mdb_commit() writes the volume bitmap while HFS_SB(sb)->mdb_bh is
> locked. A crafted image can set drVBMSt so that the bitmap byte range
> resolves to the same buffer_head as the MDB. When writeback later
> calls
> lock_buffer() for that bitmap block, the task tries to lock mdb_bh
> again
> and self-deadlocks in __lock_buffer().
> 
> Reject the image during mount if the bitmap byte range maps to
> mdb_bh.
> This checks the aliasing condition that can deadlock writeback
> without
> trying to validate unrelated HFS layout fields.

Even if we reject to mount the corrupted, then it doesn't resolve the
deadlock itself. Because, location of volume bitmap can be potentially
incorrectly modified by HFS driver internal logic. So, anyway, we need
to have the check and the fix during MDB commit, anyway. 

> 
> Reported-by: Yue Sun <samsun1006219@xxxxxxxxx>
> Closes:
> https://lore.kernel.org/all/CAEkJfYMB47v1yOWHB8q2dc8kf=uj-rLO=+yMyudwPguJ8Kd3jA@xxxxxxxxxxxxxx/
> Signed-off-by: Yue Sun <samsun1006219@xxxxxxxxx>
> ---
>  fs/hfs/mdb.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/fs/hfs/mdb.c b/fs/hfs/mdb.c
> index a97cea35ca2e..abaa636db938 100644
> --- a/fs/hfs/mdb.c
> +++ b/fs/hfs/mdb.c
> @@ -99,8 +99,10 @@ int hfs_mdb_get(struct super_block *sb)
>  	char *ptr;
>  	int off2, len, size, sect;
>  	sector_t part_start, part_size;
> -	loff_t off;
> +	sector_t mdb_block, vbm_first, vbm_last;
> +	loff_t off, vbm_off;
>  	__be16 attrib;
> +	u32 bitmap_size;
>  
>  	/* set the device driver to 512-byte blocks */
>  	size = sb_min_blocksize(sb, HFS_SECTOR_SIZE);
> @@ -185,6 +187,25 @@ int hfs_mdb_get(struct super_block *sb)
>  		sb->s_flags |= SB_RDONLY;
>  	}
>  
> +	/*
> +	 * hfs_mdb_commit() writes the bitmap while holding mdb_bh.
> Reject
> +	 * images whose bitmap range can resolve to the MDB
> buffer_head.
> +	 */
> +	bitmap_size = DIV_ROUND_UP(HFS_SB(sb)->fs_ablocks,
> BITS_PER_BYTE);

If you are trying to operate with MDB field (mdb->drVBMSt), then you
need to use drNmAlBlks (number of allocation blocks). I assume we
already have likewise calculation already. Could we reuse this logic?
Or HFS hasn't this calculation logic already?

> +	if (!bitmap_size) {
> +		pr_err("bad volume bitmap size\n");
> +		return -EIO;
> +	}
> +	vbm_off = (loff_t)(part_start + be16_to_cpu(mdb->drVBMSt))
> <<
> +		  HFS_SECTOR_SIZE_BITS;
> +	vbm_first = vbm_off >> sb->s_blocksize_bits;
> +	vbm_last = (vbm_off + bitmap_size - 1) >> sb-
> >s_blocksize_bits;
> +	mdb_block = HFS_SB(sb)->mdb_bh->b_blocknr;
> +	if (vbm_first <= mdb_block && mdb_block <= vbm_last) {
> +		pr_err("volume bitmap overlaps MDB\n");
> +		return -EIO;
> +	}

The volume bitmap should be located on the volume after the MDB record
(superblock). So, it's completely enough to use the mdb->drVBMSt for
checking the overlapping with main MDB record. Potentially, bitmap_size
can be use to check the overlapping with with alternative MDB record at
the end of the volume. However, volume bitmap cannot be so big.

Thanks,
Slava.

> +
>  	/* TRY to get the alternate (backup) MDB. */
>  	sect = part_start + part_size - 2;
>  	bh = sb_bread512(sb, sect, mdb2);