Re: [PATCH v7 2/2] block: add overflow checks for Amiga partition support

[Martin Steigerwald <martin@lichtvoll.de>]

Michael Schmitz - 15.10.18, 04:32:27 CEST:
> The Amiga partition parser module uses signed int for partition sector
> address and count, which will overflow for disks larger than 1 TB.

Did this go in? I did not find anything in git log. I wonder whether I 
can close the bug report mentioned below.

Best,
Martin

> Use u64 as type for sector address and size to allow using disks up to
> 2 TB without LBD support, and disks larger than 2 TB with LBD. The
> RBD format allows to specify disk sizes up to 2^128 bytes (though
> native OS limitations reduce this somewhat, to max 2^68 bytes), so
> check for u64 overflow carefully to protect against overflowing
> sector_t.
> 
> Bail out if sector addresses overflow 32 bits on kernels without LBD
> support.
> 
> This bug was reported originally in 2012, and the fix was created by
> the RDB author, Joanne Dow <jdow@earthlink.net>. A patch had been
> discussed and reviewed on linux-m68k at that time but never officially
> submitted (now resubmitted as separate patch).
> This patch adds additional error checking and warning messages.
> 
> Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=43511
> Reported-by: Martin Steigerwald <Martin@lichtvoll.de>
> Message-ID: <201206192146.09327.Martin@lichtvoll.de>
> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>
> Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>
> 
> ---
> 
> Changes from RFC:
> 
> - use u64 instead of sector_t, since that may be u32 without LBD
> support - check multiplication overflows each step - 3 u32 values may
> exceed u64! - warn against use on AmigaDOS if partition data overflow
> u32 sector count. - warn if partition CylBlocks larger than what's
> stored in the RDSK header. - bail out if we were to overflow sector_t
> (32 or 64 bit).
> 
> Changes from v1:
> 
> Kars de Jong:
> - use defines for magic offsets in DosEnvec struct
> 
> Geert Uytterhoeven:
> - use u64 cast for multiplications of u32 numbers
> - use array3_size for overflow checks
> - change pr_err to pr_warn
> - discontinue use of pr_cont
> - reword log messages
> - drop redundant nr_sects overflow test
> - warn against 32 bit overflow for each affected partition
> - skip partitions that overflow sector_t size instead of aborting scan
> 
> Changes from v2:
> 
> - further trim 32 bit overflow test
> - correct duplicate types.h inclusion introduced in v2
> 
> Changes from v3:
> 
> - split off sector address type fix for independent review
> - change blksize to unsigned
> - use check_mul_overflow() instead of array3_size()
> - rewrite checks to avoid 64 bit divisions in check_mul_overflow()
> 
> Changes from v5:
> 
> Geert Uytterhoeven:
> - correct ineffective u64 cast to avoid 32 bit mult. overflow
> - fix mult. overflow in partition block address calculation
> 
> Changes from v6:
> 
> Geert Uytterhoeven:
> - don't fail hard on partition block address overflow
> ---
>  block/partitions/amiga.c | 111
> +++++++++++++++++++++++++++++++++++++---------- 1 file changed, 89
> insertions(+), 22 deletions(-)
> 
> diff --git a/block/partitions/amiga.c b/block/partitions/amiga.c
> index 7ea9540..d1b1535 100644
> --- a/block/partitions/amiga.c
> +++ b/block/partitions/amiga.c
> @@ -11,11 +11,19 @@
>  #define pr_fmt(fmt) fmt
> 
>  #include <linux/types.h>
> +#include <linux/mm_types.h>
> +#include <linux/overflow.h>
>  #include <linux/affs_hardblocks.h>
> 
>  #include "check.h"
>  #include "amiga.h"
> 
> +/* magic offsets in partition DosEnvVec */
> +#define NR_HD	3
> +#define NR_SECT	5
> +#define LO_CYL	9
> +#define	HI_CYL	10
> +
>  static __inline__ u32
>  checksum_block(__be32 *m, int size)
>  {
> @@ -32,9 +40,12 @@ int amiga_partition(struct parsed_partitions
> *state) unsigned char *data;
>  	struct RigidDiskBlock *rdb;
>  	struct PartitionBlock *pb;
> -	sector_t start_sect, nr_sects;
> -	int blk, part, res = 0;
> -	int blksize = 1;	/* Multiplier for disk block size */
> +	u64 start_sect, nr_sects;
> +	sector_t blk, end_sect;
> +	u32 cylblk;		/* rdb_CylBlocks = 
nr_heads*sect_per_track */
> +	u32 nr_hd, nr_sect, lo_cyl, hi_cyl;
> +	int part, res = 0;
> +	unsigned int blksize = 1;	/* Multiplier for disk block size */
>  	int slot = 1;
>  	char b[BDEVNAME_SIZE];
> 
> @@ -44,8 +55,8 @@ int amiga_partition(struct parsed_partitions *state)
> data = read_part_sector(state, blk, &sect);
>  		if (!data) {
>  			if (warn_no_part)
> -				pr_err("Dev %s: unable to read RDB block 
%d\n",
> -				       bdevname(state->bdev, b), blk);
> +				pr_err("Dev %s: unable to read RDB block 
%llu\n",
> +				       bdevname(state->bdev, b), (u64) 
blk);
>  			res = -1;
>  			goto rdb_done;
>  		}
> @@ -61,13 +72,13 @@ int amiga_partition(struct parsed_partitions
> *state) *(__be32 *)(data+0xdc) = 0;
>  		if (checksum_block((__be32 *)data,
>  				be32_to_cpu(rdb->rdb_SummedLongs) & 
0x7F)==0) {
> -			pr_err("Trashed word at 0xd0 in block %d ignored 
in checksum
> calculation\n", -			       blk);
> +			pr_err("Trashed word at 0xd0 in block %llu ignored 
in checksum
> calculation\n", +			       (u64) blk);
>  			break;
>  		}
> 
> -		pr_err("Dev %s: RDB in block %d has bad checksum\n",
> -		       bdevname(state->bdev, b), blk);
> +		pr_err("Dev %s: RDB in block %llu has bad checksum\n",
> +		       bdevname(state->bdev, b), (u64) blk);
>  	}
> 
>  	/* blksize is blocks per 512 byte standard block */
> @@ -83,12 +94,17 @@ int amiga_partition(struct parsed_partitions
> *state) blk = be32_to_cpu(rdb->rdb_PartitionList);
>  	put_dev_sector(sect);
>  	for (part = 1; blk>0 && part<=16; part++, put_dev_sector(sect)) 
{
> -		blk *= blksize;	/* Read in terms partition table 
understands */
> +		/* Read in terms partition table understands */
> +		if (check_mul_overflow(blk, (sector_t) blksize, &blk)) {
> +			pr_err("Dev %s: overflow calculating partition 
block %llu!
> Skipping partitions %u and beyond\n", +				
bdevname(state->bdev, b),
> (u64) blk, part);
> +			break;
> +		}
>  		data = read_part_sector(state, blk, &sect);
>  		if (!data) {
>  			if (warn_no_part)
> -				pr_err("Dev %s: unable to read partition 
block %d\n",
> -				       bdevname(state->bdev, b), blk);
> +				pr_err("Dev %s: unable to read partition 
block %llu\n",
> +				       bdevname(state->bdev, b), (u64) 
blk);
>  			res = -1;
>  			goto rdb_done;
>  		}
> @@ -99,19 +115,70 @@ int amiga_partition(struct parsed_partitions
> *state) if (checksum_block((__be32 *)pb,
> be32_to_cpu(pb->pb_SummedLongs) & 0x7F) != 0 ) continue;
> 
> -		/* Tell Kernel about it */
> +		/* RDB gives us more than enough rope to hang ourselves 
with,
> +		 * many times over (2^128 bytes if all fields max out).
> +		 * Some careful checks are in order, so check for 
potential
> +		 * overflows.
> +		 * We are multiplying four 32 bit numbers to one 
sector_t!
> +		 */
> +
> +		nr_hd   = be32_to_cpu(pb->pb_Environment[NR_HD]);
> +		nr_sect = be32_to_cpu(pb->pb_Environment[NR_SECT]);
> +
> +		/* CylBlocks is total number of blocks per cylinder */
> +		if (check_mul_overflow(nr_hd, nr_sect, &cylblk)) {
> +			pr_err("Dev %s: heads*sects %u overflows u32, 
skipping
> partition!\n", +				bdevname(state->bdev, b), 
cylblk);
> +			continue;
> +		}
> +
> +		/* check for consistency with RDB defined CylBlocks */
> +		if (cylblk > be32_to_cpu(rdb->rdb_CylBlocks)) {
> +			pr_warn("Dev %s: cylblk %u > rdb_CylBlocks %u!\n",
> +				bdevname(state->bdev, b), cylblk,
> +				be32_to_cpu(rdb->rdb_CylBlocks));
> +		}
> +
> +		/* RDB allows for variable logical block size -
> +		 * normalize to 512 byte blocks and check result.
> +		 */
> +
> +		if (check_mul_overflow(cylblk, blksize, &cylblk)) {
> +			pr_err("Dev %s: partition %u bytes per cyl. 
overflows u32,
> skipping partition!\n", +				bdevname(state->bdev, 
b), part);
> +			continue;
> +		}
> +
> +		/* Calculate partition start and end. Limit of 32 bit on 
cylblk
> +		 * guarantees no overflow occurs if LBD support is 
enabled.
> +		 */
> +
> +		lo_cyl = be32_to_cpu(pb->pb_Environment[LO_CYL]);
> +		start_sect = ((u64) lo_cyl * cylblk);
> +
> +		hi_cyl = be32_to_cpu(pb->pb_Environment[HI_CYL]);
> +		nr_sects = (((u64) hi_cyl - lo_cyl + 1) * cylblk);
> 
> -		nr_sects = ((sector_t) be32_to_cpu(pb-
>pb_Environment[10])
> -			   + 1 - be32_to_cpu(pb->pb_Environment[9])) *
> -			   be32_to_cpu(pb->pb_Environment[3]) *
> -			   be32_to_cpu(pb->pb_Environment[5]) *
> -			   blksize;
>  		if (!nr_sects)
>  			continue;
> -		start_sect = (sector_t) be32_to_cpu(pb-
>pb_Environment[9]) *
> -			     be32_to_cpu(pb->pb_Environment[3]) *
> -			     be32_to_cpu(pb->pb_Environment[5]) *
> -			     blksize;
> +
> +		/* Warn user if partition end overflows u32 (AmigaDOS 
limit) */
> +
> +		if ((start_sect + nr_sects) > UINT_MAX) {
> +			pr_warn("Dev %s: partition %u (%llu-%llu) needs 64 
bit device
> support!\n", +				bdevname(state->bdev, b), part,
> +				start_sect, start_sect + nr_sects);
> +		}
> +
> +		if (check_add_overflow(start_sect, nr_sects, &end_sect)) {
> +			pr_err("Dev %s: partition %u (%llu-%llu) needs LBD 
device support,
> skipping partition!\n", +				bdevname(state->bdev, 
b), part,
> +				start_sect, (u64) end_sect);
> +			continue;
> +		}
> +
> +		/* Tell Kernel about it */
> +
>  		put_partition(state,slot++,start_sect,nr_sects);
>  		{
>  			/* Be even more informative to aid mounting */


-- 
Martin