Re: [PATCH v2 2/2] hfsplus: validate the wrapper and partition map before following them

[Viacheslav Dubeyko <slava@dubeyko.com>]

On Fri, 2026-10-02 at 00:21 +0800, Matthias Goergens wrote:
> hfsplus_read_wrapper() rereads the volume header through a bare "goto
> reread", following an HFS wrapper's embedded-volume descriptor or, if
> the header matches neither signature, the partition map found by
> hfs_part_find().  hfsplus_read_mdb() does not check the offset it
> returns, and hfs_part_find() does not check the start of a new-style
> map
> entry, so a descriptor or entry with a zero offset leaves part_start
> where it was and the mount loops forever.
> 
> Check both where they are parsed.  TN1150 places the embedded volume
> in
> the wrapper's allocation blocks, and the boot blocks, MDB and volume
> bitmap of an HFS volume are not part of any allocation block.  So
> hfsplus_read_mdb() now rejects a wrapper whose allocation blocks
> start
> at or before its MDB (drAlBlSt <= 2), whose embedded volume is empty,
> or
> whose embedded volume ends past the wrapper's last allocation block
> (drNmAlBlks).  hfs_part_find() gets the same check as in hfs: a
> partition must be non-empty, start inside the device and, for a
> new-style map, start after the map, which begins at block 1 and whose
> size the first entry's pmMapBlkCnt gives (TN1189).
> 
> A wrapper hop now moves part_start forward by at least three sectors,
> and a partition-table hop moves it past the map entries it has read. 
> So
> the loop ends, at the latest when a read goes past the end of the
> device, and the maps read on the way do not overlap.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@vger.kernel.org
> Signed-off-by: Matthias Goergens <matthias.goergens@gmail.com>
> ---
> v2: check the wrapper in hfsplus_read_mdb() and the entries in
> hfs_part_find() instead of limiting the hops in
> hfsplus_read_wrapper().
> 
> A wrapper whose embedded-volume descriptor points back at itself
> hangs
> the mount without this patch and fails at once with it:
> 
>   img=hfsplus-wrapper-loop.img
>   put() { printf "$2" | dd of=$img bs=1 seek=$1 conv=notrunc
> status=none; }
>   truncate --size=64K $img
>   put $((1024 + 0x00)) '\x42\x44'          # drSigWord 'BD'
>   put $((1024 + 0x0a)) '\x82\x00'          # drAtrb: SLOCK | SPARED
>   put $((1024 + 0x14)) '\x00\x00\x02\x00'  # drAlBlkSiz 512
>   put $((1024 + 0x7c)) '\x48\x2b'          # drEmbedSigWord 'H+'
>   put $((1024 + 0x7e)) '\x00\x00\x00\x64'  # drEmbedExtent: start 0,
> count 100
>   mount -o ro,loop -t hfsplus $img /mnt
> 
> The partition map from the hfs patch hangs an hfsplus mount the same
> way, with "-t hfsplus".
> 
>  fs/hfsplus/part_tbl.c      | 24 +++++++++++++++++++++++-
>  fs/hfsplus/wrapper.c       | 16 +++++++++++++++-
>  include/linux/hfs_common.h |  1 +
>  3 files changed, 39 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/hfsplus/part_tbl.c b/fs/hfsplus/part_tbl.c
> index 9ec21664eda6b..ceba3139f1f56 100644
> --- a/fs/hfsplus/part_tbl.c
> +++ b/fs/hfsplus/part_tbl.c
> @@ -67,6 +67,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 hfsplus_read_wrapper() cannot loop and
> + * reads each block of a map at most once.
> + */
> +static bool hfs_part_valid(struct super_block *sb, sector_t base,
> +			   u64 first, u32 start, u32 size)
> +{
> +	return start >= first && size &&
> +	       base + start < bdev_nr_sectors(sb->s_bdev);
> +}

Please, see my comments for HFS patch. I have pretty the same comments
here.

> +
>  static int hfs_parse_old_pmap(struct super_block *sb, struct
> old_pmap *pm,
>  		sector_t *part_start, sector_t *part_size)
>  {
> @@ -76,7 +92,9 @@ static int hfs_parse_old_pmap(struct super_block
> *sb, struct old_pmap *pm,
>  	for (i = 0; i < 42; i++) {
>  		struct old_pmap_entry *p = &pm->pdEntry[i];
>  
> -		if (p->pdStart && p->pdSize &&
> +		if (hfs_part_valid(sb, *part_start, HFS_DD_BLK + 1,
> +				   be32_to_cpu(p->pdStart),
> +				   be32_to_cpu(p->pdSize)) &&
>  		    p->pdFSID == cpu_to_be32(0x54465331)/*"TFS1"*/
> &&
>  		    (sbi->part < 0 || sbi->part == i)) {
>  			*part_start += be32_to_cpu(p->pdStart);
> @@ -99,6 +117,10 @@ static int hfs_parse_new_pmap(struct super_block
> *sb, void *buf,
>  
>  	do {
>  		if (!memcmp(pm->pmPartType, "Apple_HFS", 9) &&
> +		    hfs_part_valid(sb, *part_start,
> +				   HFS_PMAP_BLK + (u64)size,
> +				   be32_to_cpu(pm->pmPyPartStart),
> +				   be32_to_cpu(pm->pmPartBlkCnt)) &&
>  		    (sbi->part < 0 || sbi->part == i)) {
>  			*part_start += be32_to_cpu(pm-
> >pmPyPartStart);
>  			*part_size = be32_to_cpu(pm->pmPartBlkCnt);
> diff --git a/fs/hfsplus/wrapper.c b/fs/hfsplus/wrapper.c
> index 30cf4fe78b3d2..35cd2563dfdfe 100644
> --- a/fs/hfsplus/wrapper.c
> +++ b/fs/hfsplus/wrapper.c
> @@ -66,7 +66,7 @@ int hfsplus_submit_bio(struct super_block *sb,
> sector_t sector,
>  static int hfsplus_read_mdb(void *bufptr, struct hfsplus_wd *wd)
>  {
>  	u32 extent;
> -	u16 attrib;
> +	u16 attrib, nmalblks;
>  	__be16 sig;
>  
>  	sig = *(__be16 *)(bufptr + HFSP_WRAPOFF_EMBEDSIG);
> @@ -87,11 +87,25 @@ static int hfsplus_read_mdb(void *bufptr, struct
> hfsplus_wd *wd)
>  		return 0;
>  	wd->ablk_start =
>  		be16_to_cpu(*(__be16 *)(bufptr +
> HFSP_WRAPOFF_ABLKSTART));
> +	nmalblks = be16_to_cpu(*(__be16 *)(bufptr +
> HFSP_WRAPOFF_NMALBLKS));
>  
>  	extent = get_unaligned_be32(bufptr + HFSP_WRAPOFF_EMBEDEXT);
>  	wd->embed_start = (extent >> 16) & 0xFFFF;
>  	wd->embed_count = extent & 0xFFFF;
>  
> +	/*
> +	 * The boot blocks, MDB and volume bitmap of an HFS volume
> are not
> +	 * part of any allocation block, and the embedded volume
> occupies
> +	 * allocation blocks of the wrapper (TN1150).  So the
> embedded
> +	 * volume starts after the wrapper's MDB and ends inside the
> +	 * wrapper.
> +	 */
> +	if (wd->ablk_start <= HFS_MDB_BLK)
> +		return 0;
> +	if (!wd->embed_count ||
> +	    wd->embed_start + wd->embed_count > nmalblks)
> +		return 0;
> +
>  	return 1;
>  }
>  
> diff --git a/include/linux/hfs_common.h b/include/linux/hfs_common.h
> index d6a615e74b26e..123475efe5b37 100644
> --- a/include/linux/hfs_common.h
> +++ b/include/linux/hfs_common.h
> @@ -44,6 +44,7 @@
>  
>  #define HFSP_WRAPOFF_SIG		0x00
>  #define HFSP_WRAPOFF_ATTRIB		0x0A
> +#define HFSP_WRAPOFF_NMALBLKS		0x12

Does this value is based on specification? Where does it come from?

Thanks,
Slava.

>  #define HFSP_WRAPOFF_ABLKSIZE		0x14
>  #define HFSP_WRAPOFF_ABLKSTART		0x1C
>  #define HFSP_WRAPOFF_EMBEDSIG		0x7C