Re: [PATCH v2] hfs: prevent MDB and bitmap buffer_head aliasing

[Viacheslav Dubeyko]

On Fri, 2026-05-29 at 18:34 +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 block 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 images whose volume bitmap starts at or before the MDB during
> mount. Also guard the bitmap writeback path itself: if the bitmap block
> would resolve to mdb_bh, force the filesystem read-only and stop bitmap
> writeback before taking the buffer lock. This keeps the deadlock fix in
> the MDB commit path and reuses the existing bitmap size/writeback logic.
> 
> Reported-by: Yue Sun <samsun1006219@xxxxxxxxx>
> Closes: https://lore.kernel.org/all/CAEkJfYMB47v1yOWHB8q2dc8kf=uj-rLO=+yMyudwPguJ8Kd3jA@xxxxxxxxxxxxxx/
> Signed-off-by: Yue Sun <samsun1006219@xxxxxxxxx>
> ---
> Changes in v2:
> - Add a commit-time guard before locking bitmap buffer_heads.
> - Replace the mount-time byte-range check with a simple drVBMSt check.
> - Reuse the existing bitmap writeback size calculation.
> 
>  fs/hfs/mdb.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/fs/hfs/mdb.c b/fs/hfs/mdb.c
> index a97cea35ca2e..53cd137892b5 100644
> --- a/fs/hfs/mdb.c
> +++ b/fs/hfs/mdb.c
> @@ -185,6 +185,11 @@ int hfs_mdb_get(struct super_block *sb)
>  		sb->s_flags |= SB_RDONLY;
>  	}
>  
> +	if (be16_to_cpu(mdb->drVBMSt) <= HFS_MDB_BLK) {

Technically speaking, if we are trying to check the overlapping of volume bitmap
with the main MDB record, then we need to check the overlapping with alternative
MDB record, and with Catalog File and Extents Overflow File. However, it sounds
like we are trying to add some FSCK logic here. :)

> +		pr_err("volume bitmap overlaps MDB\n");

This situation means volume corruption. It makes sense to recommend to run FSCK.

> +		return -EIO;

This code error is wrong because the read operation was OK. But we have
corrupted volume. Even if we have overlapping of volume bitmap with MDB record,
then we cannot reject mount operation. We must mount in READ-ONLY mode because,
potentially, the rest of metadata could be completely OK. We simply cannot mount
in RW mode.

> +	}
> +
>  	/* TRY to get the alternate (backup) MDB. */
>  	sect = part_start + part_size - 2;
>  	bh = sb_bread512(sb, sect, mdb2);
> @@ -341,6 +346,11 @@ void hfs_mdb_commit(struct super_block *sb)
>  		size = (HFS_SB(sb)->fs_ablocks + 7) / 8;
>  		ptr = (u8 *)HFS_SB(sb)->bitmap;
>  		while (size) {
> +			if (unlikely(block == HFS_SB(sb)->mdb_bh->b_blocknr)) {
> +				pr_err("volume bitmap overlaps MDB, forcing read-only\n");
> +				sb->s_flags |= SB_RDONLY;
> +				break;
> +			}

At this point, we already wrote main MDB and alternative MDB to the volume.
Theoretically, it is possible to imagine that if size of volume bitmap is big
enough, then we could partially process the bitmap too. Probably, we need to
check the overlapping at the beginning of the method and reject the whole
superblock commit.

Initial issue was the deadlock. Could we implement some check that buffer_heads
don't overlap before trying to lock? Does it make sense to you?

Thanks,
Slava.

>  			bh = sb_bread(sb, block);
>  			if (!bh) {
>  				pr_err("unable to read volume bitmap\n");