Hi Michael, On Mon, Jul 2, 2018 at 7:30 AM Michael Schmitz <schmitzmic@xxxxxxxxx> wrote: > The Amiga partition parser module uses signed int for partition sector > address and count, which will overflow for disks larger than 1 TB. > > 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@xxxxxxxxxxxxx>. A patch had been > discussed and reviewed on linux-m68k at that time but never officially > submitted. This patch adds additional error checking and warning messages. > > Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=43511 > Reported-by: Martin Steigerwald <Martin@xxxxxxxxxxxx> > Message-ID: <201206192146.09327.Martin@xxxxxxxxxxxx> > Signed-off-by: Michael Schmitz <schmitzmic@xxxxxxxxx> > Tested-by: Martin Steigerwald <Martin@xxxxxxxxxxxx> > > 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). Thanks for your patch! > --- a/block/partitions/amiga.c > +++ b/block/partitions/amiga.c > @@ -11,6 +11,7 @@ > #define pr_fmt(fmt) fmt > > #include <linux/types.h> > +#include <linux/log2.h> > #include <linux/affs_hardblocks.h> > > #include "check.h" > @@ -32,7 +33,9 @@ int amiga_partition(struct parsed_partitions *state) > unsigned char *data; > struct RigidDiskBlock *rdb; > struct PartitionBlock *pb; > - int start_sect, nr_sects, blk, part, res = 0; > + u64 start_sect, nr_sects; > + u64 cylblk, cylblk_res; /* rdb_CylBlocks = nr_heads*sect_per_track */ > + int blk, part, res = 0, blk_shift = 0, did_warn = 0; > int blksize = 1; /* Multiplier for disk block size */ > int slot = 1; > char b[BDEVNAME_SIZE]; > @@ -98,6 +101,79 @@ int amiga_partition(struct parsed_partitions *state) > if (checksum_block((__be32 *)pb, be32_to_cpu(pb->pb_SummedLongs) & 0x7F) != 0 ) > continue; > > + /* 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. > + */ > + > + /* CylBlocks is total number of blocks per cylinder */ > + cylblk = be32_to_cpu(pb->pb_Environment[3]) * > + be32_to_cpu(pb->pb_Environment[5]); Does the above really do a 32 * 32 = 64 bit multiplication? be32 is unsigned int, and multiplying it will be done in 32-bit arithmetic: unsigned int a = 100000; unsigned int b = 100000; unsigned long long c = a * b; unsigned long long d = (unsigned long long)a * b; printf("c = %llu\n", c); printf("d = %llu\n", d); prints: c = 1410065408 d = 10000000000 If it does work for you, what am I missing? > + > + /* check for consistency with RDB defined CylBlocks */ > + if (cylblk > be32_to_cpu(rdb->rdb_CylBlocks)) { > + pr_err("Dev %s: cylblk 0x%lx > rdb_CylBlocks 0x%x!\n", > + bdevname(state->bdev, b), > + (unsigned long) cylblk, Why the cast? This will truncate the value on 32-bit platforms. Just use %llu (IMHO decimal is better suited here). > + be32_to_cpu(rdb->rdb_CylBlocks)); > + } > + > + /* check for potential overflows - we are going to multiply > + * three 32 bit numbers to one 64 bit result later! > + * Condition 1: nr_heads * sects_per_track must fit u32! > + * NB: This is a HARD limit for AmigaDOS. We don't care much. So, is condition 1 really needed? > + */ > + > + if (cylblk > UINT_MAX) { > + pr_err("Dev %s: hds*sects 0x%lx > UINT_MAX!\n", > + bdevname(state->bdev, b), > + (unsigned long) cylblk); Again, why the cast/truncation? > + > + /* lop off low 32 bits */ > + cylblk_res = cylblk >> 32; > + > + /* check for further overflow in end result */ > + if (be32_to_cpu(pb->pb_Environment[9]) * > + cylblk_res * blksize > UINT_MAX) { > + pr_err("Dev %s: start_sect overflows u64!\n", > + bdevname(state->bdev, b)); > + res = -1; > + goto rdb_done; > + } > + > + if (be32_to_cpu(pb->pb_Environment[10]) * > + cylblk_res * blksize > UINT_MAX) { > + pr_err("Dev %s: end_sect overflows u64!\n", > + bdevname(state->bdev, b)); > + res = -1; > + goto rdb_done; > + } No need to reinvent the wheel, #include <linux/overflow.h>, and use check_mul_overflow(), like array3_size() does. > + } > + > + /* Condition 2: even if CylBlocks did not overflow, the end > + * result must still fit u64! > + */ > + > + /* how many bits above 32 in cylblk * blksize ? */ > + if (cylblk*blksize > (u64) UINT_MAX) > + blk_shift = ilog2(cylblk*blksize) - 32; > + > + if (be32_to_cpu(pb->pb_Environment[9]) > + > (u64) UINT_MAX>>blk_shift) { > + pr_err("Dev %s: start_sect overflows u64!\n", > + bdevname(state->bdev, b)); > + res = -1; > + goto rdb_done; > + } > + > + if (be32_to_cpu(pb->pb_Environment[10]) > + > (u64) UINT_MAX>>blk_shift) { > + pr_err("Dev %s: end_sect overflows u64!\n", > + bdevname(state->bdev, b)); > + res = -1; > + goto rdb_done; check_mul_overflow() > + } > + > /* Tell Kernel about it */ > > nr_sects = (be32_to_cpu(pb->pb_Environment[10]) + 1 - > @@ -111,6 +187,27 @@ int amiga_partition(struct parsed_partitions *state) > be32_to_cpu(pb->pb_Environment[3]) * > be32_to_cpu(pb->pb_Environment[5]) * > blksize; I'm still not convinced the above is done in 64-bit arithmetic... > + > + /* Warn user if start_sect or nr_sects overflow u32 */ > + if ((nr_sects > UINT_MAX || start_sect > UINT_MAX || > + (start_sect + nr_sects) > UINT_MAX) && !did_warn) { I guess "start_sect + nr_sects > UINT_MAX" is sufficient? I would remove the did_warn check, as multiple partitions may be affected. Also, RDB doesn't enforce partition ordering, IIRC, so e.g. partitions 1 and 3 could be outside the 2 TiB area, while 2 could be within. > + pr_err("Dev %s: partition 32 bit overflow! ", pr_warn() > + bdevname(state->bdev, b)); > + pr_cont("start_sect 0x%llX, nr_sects 0x%llx\n", > + start_sect, nr_sects); No need for pr_cont(), just merge the two statements. > + pr_err("Dev %s: partition may need 64 bit ", pr_warn() Drop the "may"? > + bdevname(state->bdev, b)); I would print the partition index, too. > + pr_cont("device support on native AmigaOS!\n"); Just merge the two statements. I think it can also be just one line printed instead of 2? pr_warn("Dev %s: partition %u (%llu-%llu) needs 64-bit device support\n", ... > + /* Without LBD support, better bail out */ > + if (!IS_ENABLED(CONFIG_LBDAF)) { > + pr_err("Dev %s: no LBD support, aborting!", pr_err("Dev %s: no LBD support, skipping partition %u\n", ...)? > + bdevname(state->bdev, b)); > + res = -1; > + goto rdb_done; Aborting here renders any later partitions within the 2 TiB area unaccessible. So please continue. > + } > + did_warn++; > + } > + > put_partition(state,slot++,start_sect,nr_sects); > { > /* Be even more informative to aid mounting */ Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
