[PATCH v5 0/2] Amiga RDB partition support fixes

[PATCH v5 0/2] Amiga RDB partition support fixes

[Michael Schmitz]

yet another new version of the Amiga RDB partition table patch. 
I've split off the part fixing the incorrect use of signed int    
for partition start address and size as separate patch. This change
should be incontroversial (I hope). It does fix the bug that Martin   
Steigerwald reported six years ago for a 2 TB disk. I have tested this
patch with the identical RDB partition data that Martin saw the bug with.
I have incorporated Andreas' feedback on proper use of casts to avoid
integer overflow in this version.

The simple fix still leaves ample room for overflows in calculating start
address and size of a RDB partition, though such overflows should only be
seen in rather unusual cases. To address these potential overflows, checks
are added in the second patch of this series.

The use of u64 instead of sector_t in the second patch is required to catch
overflows even in cases where sector_t is 32 bit wide (i.e. no LBD support).

Cheers,

	Michael

[PATCH v5 1/2] block: fix signed int overflow in Amiga partition support

[Michael Schmitz]

The Amiga partition parser module uses signed int for partition sector
address and count, which will overflow for disks larger than 1 TB.

Use sector_t 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.

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. This patch differs from Joanne's patch only in its use of
sector_t instead of unsigned int. No checking for overflows is done
(see patch 2 of this series for that).

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>
Tested-by: Martin Steigerwald <Martin@lichtvoll.de>

---

Changes from v3:

- split off change of sector address type as quick fix.
- cast to sector_t in sector address calculations.
- move overflow checking to separate patch for more thorough review.

Changes from v4:

Andreas Schwab:
- correct cast to sector_t in sector address calculations
---
 block/partitions/amiga.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/block/partitions/amiga.c b/block/partitions/amiga.c
index 5609366..7ea9540 100644
--- a/block/partitions/amiga.c
+++ b/block/partitions/amiga.c
@@ -32,7 +32,8 @@ 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;
+	sector_t start_sect, nr_sects;
+	int blk, part, res = 0;
 	int blksize = 1;	/* Multiplier for disk block size */
 	int slot = 1;
 	char b[BDEVNAME_SIZE];
@@ -100,14 +101,14 @@ int amiga_partition(struct parsed_partitions *state)
 
 		/* Tell Kernel about it */
 
-		nr_sects = (be32_to_cpu(pb->pb_Environment[10]) + 1 -
-			    be32_to_cpu(pb->pb_Environment[9])) *
+		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 = be32_to_cpu(pb->pb_Environment[9]) *
+		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;
-- 
1.9.1

Re: [PATCH v5 1/2] block: fix signed int overflow in Amiga partition support

[Geert Uytterhoeven]

Hi Michael,

On Fri, Oct 12, 2018 at 2:27 AM Michael Schmitz <schmitzmic@gmail.com> 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 sector_t 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.
>
> 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. This patch differs from Joanne's patch only in its use of
> sector_t instead of unsigned int. No checking for overflows is done
> (see patch 2 of this series for that).
>
> 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>
> Tested-by: Martin Steigerwald <Martin@lichtvoll.de>

Thanks for your patch!

> Changes from v4:
>
> Andreas Schwab:
> - correct cast to sector_t in sector address calculations

Which you only did for the first case...

> --- a/block/partitions/amiga.c
> +++ b/block/partitions/amiga.c

> @@ -100,14 +101,14 @@ int amiga_partition(struct parsed_partitions *state)
>
>                 /* Tell Kernel about it */
>
> -               nr_sects = (be32_to_cpu(pb->pb_Environment[10]) + 1 -
> -                           be32_to_cpu(pb->pb_Environment[9])) *
> +               nr_sects = ((sector_t) be32_to_cpu(pb->pb_Environment[10])

...here ...

> +                          + 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 = be32_to_cpu(pb->pb_Environment[9]) *
> +               start_sect = (sector_t) be32_to_cpu(pb->pb_Environment[9]) *

... but not here?

>                              be32_to_cpu(pb->pb_Environment[3]) *
>                              be32_to_cpu(pb->pb_Environment[5]) *
>                              blksize;

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: [PATCH v5 1/2] block: fix signed int overflow in Amiga partition support

[Michael Schmitz]

Hi Geert,

Am 12.10.2018 um 21:59 schrieb Geert Uytterhoeven:

>> Changes from v4:
>>
>> Andreas Schwab:
>> - correct cast to sector_t in sector address calculations
>
> Which you only did for the first case...
>
>> --- a/block/partitions/amiga.c
>> +++ b/block/partitions/amiga.c
>
>> @@ -100,14 +101,14 @@ int amiga_partition(struct parsed_partitions *state)
>>
>>                 /* Tell Kernel about it */
>>
>> -               nr_sects = (be32_to_cpu(pb->pb_Environment[10]) + 1 -
>> -                           be32_to_cpu(pb->pb_Environment[9])) *
>> +               nr_sects = ((sector_t) be32_to_cpu(pb->pb_Environment[10])
>
> ...here ...
>
>> +                          + 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 = be32_to_cpu(pb->pb_Environment[9]) *
>> +               start_sect = (sector_t) be32_to_cpu(pb->pb_Environment[9]) *
>
> ... but not here?

I meant to cast the first operand to sector_t here?

Using

int main(int argc, char *argv) {
   unsigned int a, b;
   unsigned long long c, d;

   a = 0xffffffff;
   b = 0xffffffff;

   c = a * b;
   d = (unsigned long long) a * b;

   fprintf(stdout, "%llx %llx\n", c, d);
}

I get this:

1 fffffffe00000001

But I can add brackets if you think the compiler might get this wrong.

Cheers,

	Michael

>
>>                              be32_to_cpu(pb->pb_Environment[3]) *
>>                              be32_to_cpu(pb->pb_Environment[5]) *
>>                              blksize;
>
> Gr{oetje,eeting}s,
>
>                         Geert
>

Re: [PATCH v5 1/2] block: fix signed int overflow in Amiga partition support

[Geert Uytterhoeven]

Hi Michael,

On Sat, Oct 13, 2018 at 4:46 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
> Am 12.10.2018 um 21:59 schrieb Geert Uytterhoeven:
> >> Changes from v4:
> >>
> >> Andreas Schwab:
> >> - correct cast to sector_t in sector address calculations
> >
> > Which you only did for the first case...
> >
> >> --- a/block/partitions/amiga.c
> >> +++ b/block/partitions/amiga.c
> >
> >> @@ -100,14 +101,14 @@ int amiga_partition(struct parsed_partitions *state)
> >>
> >>                 /* Tell Kernel about it */
> >>
> >> -               nr_sects = (be32_to_cpu(pb->pb_Environment[10]) + 1 -
> >> -                           be32_to_cpu(pb->pb_Environment[9])) *
> >> +               nr_sects = ((sector_t) be32_to_cpu(pb->pb_Environment[10])
> >
> > ...here ...
> >
> >> +                          + 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 = be32_to_cpu(pb->pb_Environment[9]) *
> >> +               start_sect = (sector_t) be32_to_cpu(pb->pb_Environment[9]) *
> >
> > ... but not here?
>
> I meant to cast the first operand to sector_t here?

Silly me.

> But I can add brackets if you think the compiler might get this wrong.

No it's fine as-is.

Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>


Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

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

[Michael Schmitz]

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@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>

---

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()
---
 block/partitions/amiga.c | 86 +++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 74 insertions(+), 12 deletions(-)

diff --git a/block/partitions/amiga.c b/block/partitions/amiga.c
index 7ea9540..193e6c3 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;
+	u64 start_sect, nr_sects;
+	sector_t end_sect;
+	u32 cylblk;		/* rdb_CylBlocks = nr_heads*sect_per_track */
+	u32 nr_hd, nr_sect, lo_cyl, hi_cyl;
 	int blk, part, res = 0;
-	int blksize = 1;	/* Multiplier for disk block size */
+	unsigned int blksize = 1;	/* Multiplier for disk block size */
 	int slot = 1;
 	char b[BDEVNAME_SIZE];
 
@@ -99,19 +110,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 */
-- 
1.9.1

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

[Geert Uytterhoeven]

Hi Michael,

On Fri, Oct 12, 2018 at 2:27 AM Michael Schmitz <schmitzmic@gmail.com> 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@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>

 Thanks for being persistent!

BTW, there's another possible overflow in "blk *= blksize", but that one
is very unlikely to happen, as most (all?) partitioners store partition
blocks close to the beginning of the disk.

> --- a/block/partitions/amiga.c
> +++ b/block/partitions/amiga.c

> @@ -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;
> +       u64 start_sect, nr_sects;
> +       sector_t end_sect;
> +       u32 cylblk;             /* rdb_CylBlocks = nr_heads*sect_per_track */
> +       u32 nr_hd, nr_sect, lo_cyl, hi_cyl;
>         int blk, part, res = 0;
> -       int blksize = 1;        /* Multiplier for disk block size */
> +       unsigned int blksize = 1;       /* Multiplier for disk block size */
>         int slot = 1;
>         char b[BDEVNAME_SIZE];
>
> @@ -99,19 +110,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);

Unlike the comparison with 32-bit rdb_CylBlocks above, this is an
artificial limitation, right?
You can relax it by using 64-bit arithmetic, but that would complicate the
calculation of start_sect and nr_sects below, as they may overflow 64-bit.

> +                       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);

As lo_cyl and cylblk are both u32, the multiplication will be done using
32-bit arithmetic, and may overflow.  Hence you should cast one of the
multiplicands, instead of the result:

    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);

Likewise.

>
> -               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);

The cast to u64 is not needed.

> +                       continue;
> +               }
> +
> +               /* Tell Kernel about it */
> +
>                 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@linux-m68k.org

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

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

[Michael Schmitz]

Hi Geert,

Am 12.10.2018 um 21:54 schrieb Geert Uytterhoeven:
>  Thanks for being persistent!
>
> BTW, there's another possible overflow in "blk *= blksize", but that one
> is very unlikely to happen, as most (all?) partitioners store partition
> blocks close to the beginning of the disk.

Thanks - we can at least change the type of blk to sector_t to limit the 
potential for multiplication overflow, but with RDB_ALLOCATION_LIMIT at 
16, we would miss RDB partition blocks not at the very beginning of a 
disk anyway.

>
>> --- a/block/partitions/amiga.c
>> +++ b/block/partitions/amiga.c
>
>> @@ -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;
>> +       u64 start_sect, nr_sects;
>> +       sector_t end_sect;
>> +       u32 cylblk;             /* rdb_CylBlocks = nr_heads*sect_per_track */
>> +       u32 nr_hd, nr_sect, lo_cyl, hi_cyl;
>>         int blk, part, res = 0;
>> -       int blksize = 1;        /* Multiplier for disk block size */
>> +       unsigned int blksize = 1;       /* Multiplier for disk block size */
>>         int slot = 1;
>>         char b[BDEVNAME_SIZE];
>>
>> @@ -99,19 +110,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);
>
> Unlike the comparison with 32-bit rdb_CylBlocks above, this is an
> artificial limitation, right?
> You can relax it by using 64-bit arithmetic, but that would complicate the
> calculation of start_sect and nr_sects below, as they may overflow 64-bit.

Correct - this allows me to skip the overflow check on the final result 
(see comment below). But making cylblk a 32 bit type for the purpose of 
this overflow check has tripped me up below.

I'd still like to retain this check - it is highly unlikely to ever 
trigger with RDB blocks in current use, and should disks ever get so 
large as to require the total number of 512 byte blocks per cylinder to 
exceed the 32 bit limit, I'd certainly hope other partition table 
options get chosen.

>
>> +                       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);
>
> As lo_cyl and cylblk are both u32, the multiplication will be done using
> 32-bit arithmetic, and may overflow.  Hence you should cast one of the
> multiplicands, instead of the result:
>
>     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);
>
> Likewise.

Thanks, I'll fix those.

>
>>
>> -               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);
>
> The cast to u64 is not needed.

It is, in case the kernel is compiled without LBD support (making 
end_sect a 32 bit sector_t). I haven't found a better way to check 
whether the partition exceeds what's possible to represent without LBD 
support.
OTOH, no other partition parser does bother to check for that, so maybe 
it's overkill.

Thanks again,

	Michael

>
>> +                       continue;
>> +               }
>> +
>> +               /* Tell Kernel about it */
>> +
>>                 put_partition(state,slot++,start_sect,nr_sects);
>>                 {
>>                         /* Be even more informative to aid mounting */
>
> Gr{oetje,eeting}s,
>
>                         Geert
>

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

[Geert Uytterhoeven]

Hi Michael,

On Sat, Oct 13, 2018 at 4:23 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
> Am 12.10.2018 um 21:54 schrieb Geert Uytterhoeven:
> >  Thanks for being persistent!
> >
> > BTW, there's another possible overflow in "blk *= blksize", but that one
> > is very unlikely to happen, as most (all?) partitioners store partition
> > blocks close to the beginning of the disk.
>
> Thanks - we can at least change the type of blk to sector_t to limit the
> potential for multiplication overflow, but with RDB_ALLOCATION_LIMIT at
> 16, we would miss RDB partition blocks not at the very beginning of a
> disk anyway.

I thought so, too, but then realized that RDB_ALLOCATION_LIMIT applies to
the RDB block itself, not to the partition blocks.

> >> --- a/block/partitions/amiga.c
> >> +++ b/block/partitions/amiga.c
> >
> >> @@ -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;
> >> +       u64 start_sect, nr_sects;
> >> +       sector_t end_sect;
> >> +       u32 cylblk;             /* rdb_CylBlocks = nr_heads*sect_per_track */
> >> +       u32 nr_hd, nr_sect, lo_cyl, hi_cyl;
> >>         int blk, part, res = 0;
> >> -       int blksize = 1;        /* Multiplier for disk block size */
> >> +       unsigned int blksize = 1;       /* Multiplier for disk block size */
> >>         int slot = 1;
> >>         char b[BDEVNAME_SIZE];
> >>
> >> @@ -99,19 +110,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);
> >
> > Unlike the comparison with 32-bit rdb_CylBlocks above, this is an
> > artificial limitation, right?
> > You can relax it by using 64-bit arithmetic, but that would complicate the
> > calculation of start_sect and nr_sects below, as they may overflow 64-bit.
>
> Correct - this allows me to skip the overflow check on the final result
> (see comment below). But making cylblk a 32 bit type for the purpose of
> this overflow check has tripped me up below.
>
> I'd still like to retain this check - it is highly unlikely to ever
> trigger with RDB blocks in current use, and should disks ever get so
> large as to require the total number of 512 byte blocks per cylinder to
> exceed the 32 bit limit, I'd certainly hope other partition table
> options get chosen.

Fair enough.

> >> +               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);
> >
> > The cast to u64 is not needed.
>
> It is, in case the kernel is compiled without LBD support (making
> end_sect a 32 bit sector_t). I haven't found a better way to check
> whether the partition exceeds what's possible to represent without LBD
> support.

You're right. I missed that end_sect was not converted to u64.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

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

[Michael Schmitz]

Hi Geert,

Am 13.10.2018 um 20:02 schrieb Geert Uytterhoeven:
> Hi Michael,
>
> On Sat, Oct 13, 2018 at 4:23 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
>> Am 12.10.2018 um 21:54 schrieb Geert Uytterhoeven:
>>>  Thanks for being persistent!
>>>
>>> BTW, there's another possible overflow in "blk *= blksize", but that one
>>> is very unlikely to happen, as most (all?) partitioners store partition
>>> blocks close to the beginning of the disk.
>>
>> Thanks - we can at least change the type of blk to sector_t to limit the
>> potential for multiplication overflow, but with RDB_ALLOCATION_LIMIT at
>> 16, we would miss RDB partition blocks not at the very beginning of a
>> disk anyway.
>
> I thought so, too, but then realized that RDB_ALLOCATION_LIMIT applies to
> the RDB block itself, not to the partition blocks.

Right, I now see. blk should be sector_t anyway (which is no help 
without LBD support, hm). Better fix that.

>>>> --- a/block/partitions/amiga.c
>>>> +++ b/block/partitions/amiga.c
>>>
>>>> @@ -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;
>>>> +       u64 start_sect, nr_sects;
>>>> +       sector_t end_sect;
>>>> +       u32 cylblk;             /* rdb_CylBlocks = nr_heads*sect_per_track */
>>>> +       u32 nr_hd, nr_sect, lo_cyl, hi_cyl;
>>>>         int blk, part, res = 0;
>>>> -       int blksize = 1;        /* Multiplier for disk block size */
>>>> +       unsigned int blksize = 1;       /* Multiplier for disk block size */
>>>>         int slot = 1;
>>>>         char b[BDEVNAME_SIZE];
>>>>
>>>> @@ -99,19 +110,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);
>>>
>>> Unlike the comparison with 32-bit rdb_CylBlocks above, this is an
>>> artificial limitation, right?
>>> You can relax it by using 64-bit arithmetic, but that would complicate the
>>> calculation of start_sect and nr_sects below, as they may overflow 64-bit.
>>
>> Correct - this allows me to skip the overflow check on the final result
>> (see comment below). But making cylblk a 32 bit type for the purpose of
>> this overflow check has tripped me up below.
>>
>> I'd still like to retain this check - it is highly unlikely to ever
>> trigger with RDB blocks in current use, and should disks ever get so
>> large as to require the total number of 512 byte blocks per cylinder to
>> exceed the 32 bit limit, I'd certainly hope other partition table
>> options get chosen.
>
> Fair enough.

Thanks.

Cheers,

	Michael


>>>> +               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);
>>>
>>> The cast to u64 is not needed.
>>
>> It is, in case the kernel is compiled without LBD support (making
>> end_sect a 32 bit sector_t). I haven't found a better way to check
>> whether the partition exceeds what's possible to represent without LBD
>> support.
>
> You're right. I missed that end_sect was not converted to u64.
>
> Gr{oetje,eeting}s,
>
>                         Geert
>

Re: [PATCH v5 0/2] Amiga RDB partition support fixes

[Martin Steigerwald]

Michael Schmitz - 12.10.18, 02:26:
> yet another new version of the Amiga RDB partition table patch.
> I've split off the part fixing the incorrect use of signed int
> for partition start address and size as separate patch. This change
> should be incontroversial (I hope). It does fix the bug that Martin
> Steigerwald reported six years ago for a 2 TB disk. I have tested this
> patch with the identical RDB partition data that Martin saw the bug
> with. I have incorporated Andreas' feedback on proper use of casts to
> avoid integer overflow in this version.

Just as a note for all the AmigaOS users out there: Hyperion 
Entertainment released AmigaOS 3.1.4 for various Amiga models. This 
should fix all (known) issues with large disks for good directly in the 
Kickstart ROM instead of via SetPatch updating the ROM by replacing 
modules or by using NSDPatch approach. Olaf Barthel was one of the 
developers involved with the update. Digital downloads only so far, 
physical releases with floppy disks and ROM chips are announced.

This would be another reason to support large disks in Linux as well.

Thank you, Michael.

(Omitting link as to not too directly advertise proprietary products 
here, it is easy enough to find.)

Thanks,
-- 
Martin

Re: [PATCH v5 0/2] Amiga RDB partition support fixes

[John Paul Adrian Glaubitz]

On 10/12/18 10:17 AM, Martin Steigerwald wrote:
> Just as a note for all the AmigaOS users out there: Hyperion
> Entertainment released AmigaOS 3.1.4 for various Amiga models. This
> should fix all (known) issues with large disks for good directly in the
> Kickstart ROM instead of via SetPatch updating the ROM by replacing
> modules or by using NSDPatch approach. Olaf Barthel was one of the
> developers involved with the update. Digital downloads only so far,
> physical releases with floppy disks and ROM chips are announced.


Thanks for the heads-up. It's 29,95 Euro per copy. I would actually buy
this, but since I would rather want to update all of my Amigas, this
would be pretty expensive.

I'm going to contact them and ask whether they have a discount.

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaubitz@debian.org
`. `'   Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913