Re: [PATCH v2 1/2] hfs: validate partition map entries in hfs_part_find()

[Viacheslav Dubeyko]

On Fri, 2026-10-02 at 00:21 +0800, Matthias Goergens wrote:
> hfs_mdb_get() loops around hfs_part_find(), rereading the MDB
> wherever
> the partition map points, and hfs_part_find() takes the start of a
> matching entry as it is.  A new-style entry with pmPyPartStart 0
> leaves
> part_start where it was, so the loop reads the same blocks forever
> and
> the mount hangs.
> 
> Check each entry before following it.  The partition must be non-
> empty
> and start inside the device.  For a new-style map it must also start
> after the map, which begins at block 1 and whose size the first
> entry's
> pmMapBlkCnt gives (TN1189); the old-style parser keeps its rule of a
> non-zero start.  An entry that fails these checks is skipped, as the
> old-style parser already skipped entries with a zero start or size. 
> The
> old-style parser now also stops at the first match, as the new-style
> one
> and hfsplus's copy do: it used to add up the starts of all matching
> entries, so the start it returned was not one that had been checked.
> 
> Every partition-table hop now moves part_start past the map entries
> it
> has read, so the loop in hfs_mdb_get() ends and reads each block of a
> map at most once.  Rejecting only a zero start would end the loop
> too,
> but a crafted new-style map with an entry in every block could then
> make
> each of many small hops rescan most of the device.
> 
> The end of the partition is not checked against the device.  hfs
> already
> mounts such a volume without its alternate MDB, and the block layer
> likewise keeps a partition that runs past the end of the disk,
> trimmed
> to fit.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Matthias Goergens <matthias.goergens@xxxxxxxxx>
> ---
> v2: check the entries in hfs_part_find() instead of limiting the hops
> in hfs_mdb_get(); stop at the first matching old-style entry.
> 
> A partition entry pointing at its own map hangs the mount without
> this
> patch and fails at once with it:
> 
>   img=hfs-partmap-loop.img
>   put() { printf "$2" | dd of=$img bs=1 seek=$1 conv=notrunc
> status=none; }
>   truncate --size=64K $img
>   put $((512+0x00)) '\x50\x4d'          # pmSig 'PM'
>   put $((512+0x04)) '\x00\x00\x00\x01'  # pmMapBlkCnt 1
>   put $((512+0x08)) '\x00\x00\x00\x00'  # pmPyPartStart 0 (self)
>   put $((512+0x0c)) '\x00\x00\x00\x64'  # pmPartBlkCnt 100
>   put $((512+0x30)) 'Apple_HFS'         # pmPartType
>   mount -o ro,loop -t hfs $img /mnt
> 
>  fs/hfs/part_tbl.c | 27 ++++++++++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/hfs/part_tbl.c b/fs/hfs/part_tbl.c
> index 36add537d153e..2c15afe098127 100644
> --- a/fs/hfs/part_tbl.c
> +++ b/fs/hfs/part_tbl.c
> @@ -9,6 +9,8 @@
>   * a patch contributed by Holger Schemel (aeglos@xxxxxxxxxxxxxx).
>   */
>  
> +#include <linux/blkdev.h>
> +
>  #include "hfs_fs.h"
>  
>  /*
> @@ -49,6 +51,22 @@ struct old_pmap {
>  	}	pdEntry[42];
>  } __packed;
>  
> +/*
> + * Check a partition map entry before following it.  The partition
> must
> + * be non-empty, start inside the device and start at or after
> @first:
> + * after the driver descriptor map in block 0 for an old-style map,
> and
> + * after the whole of a new-style map, whose size the first entry's
> + * pmMapBlkCnt gives (TN1189).  Every hop then moves forward, past
> the
> + * map entries just read, so hfs_mdb_get() cannot loop and reads
> each
> + * block of a map at most once.
> + */

Frankly speaking, comment is long and it only complicates everything. I
don't follow what hop means. Could we make the comment short, clear,
and more informative?

> +static bool hfs_part_valid(struct super_block *sb, sector_t base,
> +			   u64 first, u32 start, u32 size)

The set of argument is very confusing. As a result, it's really hard to
follow what we are checking and it is correct check or not. I think it
will be more clear to provide struct old_pmap pointer as argument. Why
not use part_start instead of base? What the first argument means?

> +{
> +	return start >= first && size &&
> +	       base + start < bdev_nr_sectors(sb->s_bdev);

We have part_size. Is it not the same as bdev_nr_sectors(sb->s_bdev)?

> +}
> +
>  /*
>   * hfs_part_find()
>   *
> @@ -77,12 +95,15 @@ int hfs_part_find(struct super_block *sb,
>  		p = pm->pdEntry;
>  		size = 42;
>  		for (i = 0; i < size; p++, i++) {
> -			if (p->pdStart && p->pdSize &&
> +			if (hfs_part_valid(sb, *part_start,
> HFS_DD_BLK + 1,

Do you mean HFS_PMAP_BLK here by HFS_DD_BLK + 1?

> +					   be32_to_cpu(p->pdStart),
> +					   be32_to_cpu(p->pdSize))
> &&
>  			    p->pdFSID ==
> cpu_to_be32(0x54465331)/*"TFS1"*/ &&
>  			    (HFS_SB(sb)->part < 0 || HFS_SB(sb)-
> >part == i)) {

This check becomes too long and complicated now. I think we need to
introduce some good checking function.

>  				*part_start += be32_to_cpu(p-
> >pdStart);
>  				*part_size = be32_to_cpu(p->pdSize);
>  				res = 0;
> +				break;
>  			}
>  		}
>  		break;
> @@ -95,6 +116,10 @@ int hfs_part_find(struct super_block *sb,
>  		size = be32_to_cpu(pm->pmMapBlkCnt);
>  		for (i = 0; i < size;) {
>  			if (!memcmp(pm->pmPartType,"Apple_HFS", 9)
> &&
> +			    hfs_part_valid(sb, *part_start,
> +					   HFS_PMAP_BLK + (u64)size,

Why exactly HFS_PMAP_BLK + (u64)size?

> +					   be32_to_cpu(pm-
> >pmPyPartStart),
> +					   be32_to_cpu(pm-
> >pmPartBlkCnt)) &&
>  			    (HFS_SB(sb)->part < 0 || HFS_SB(sb)-
> >part == i)) {

Ditto.

Thanks,
Slava.

>  				*part_start += be32_to_cpu(pm-
> >pmPyPartStart);
>  				*part_size = be32_to_cpu(pm-
> >pmPartBlkCnt);