Hi Michael,
On Mon, Jul 2, 2018 at 7:30 AM Michael Schmitz <[email protected]> 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 <[email protected]>. 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 <[email protected]>
> Message-ID: <[email protected]>
> Signed-off-by: Michael Schmitz <[email protected]>
> Tested-by: Martin Steigerwald <[email protected]>
>
> 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 -- [email protected]
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
--
To unsubscribe from this list: send the line "unsubscribe linux-m68k" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
