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
--
To unsubscribe from this list: send the line "unsubscribe linux-m68k" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
