| Message ID | 1530509350-25410-1-git-send-email-schmitzmic@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Op ma 2 jul. 2018 om 07:29 schreef Michael Schmitz <schmitzmic@gmail.com>: > @@ -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]); > + Could you please create #defines for all these magic offsets in pb_Environment? Below are a few more. This makes the code much more readable. Thanks! > + /* 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, > + 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. > + */ > + > + if (cylblk > UINT_MAX) { > + pr_err("Dev %s: hds*sects 0x%lx > UINT_MAX!\n", > + bdevname(state->bdev, b), > + (unsigned long) cylblk); > + > + /* 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; > + } > + } > + > + /* 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; > + } > + > /* Tell Kernel about it */ > > nr_sects = (be32_to_cpu(pb->pb_Environment[10]) + 1 -
Hi Michael, On Mon, Jul 2, 2018 at 7:30 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. 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> > Tested-by: Martin Steigerwald <Martin@lichtvoll.de> > > 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
Hi Michael. I felt free to Cc my mail address. Michael Schmitz - 02.07.18, 07:29: > 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. First off, I have an reply by AmigaOS developer Olaf Barthel regarding his recommendations for Linux that I may post here. Quoted part is my question, unquoted part is his reply. --------------------------------------------------------------- > Would you, at the current time, given the circumstances you described, > recommend any additional limitations to the Linux RDB parser > implementation? If so, which ones, and why? Note this is just about the > RDB parser for now, not AFFS in Linux and not amiga-fdisk, parted or > what else. I am aware that AFFS in Linux would also need to be checked > for limit handling :). That's where the restrictions governing RDB and file system diverge. You can have a perfectly consistent RDB, but the file system may be unable to make use of it. This is the case, for example, with the default ROM file system in Kickstart 2.x/3.x which will accept what scsi.device found in the RDB, but might not work out well at all later because the ROM file system does not know its own limitations. Generally, keeping the RDB consistent should be the primary objective. The question is how you are going to deal with the consequences. If the AFFS file system can be relied upon to know its limitations then you might end up with a good pairing with the RDB parser for Linux only. The problems begin with how an AmigaOS-native file system is expected to deal with the RDB. If you want to limit the risk of data loss, then the cautious approach would be to put limitations into the partitioning software (fdisk), like we did for HDToolBox/ProdPrep, namely: 1. Do not create new partitions which reach beyond the 4 Gigabyte boundary of the storage medium. 2. Do not create new partitions which exceed 2 Gigabytes in size. 3. Do not allow existing partitions to be moved or resized so that they reside entirely beyond the 4 Gigabyte boundary, or reach beyond that boundary. These limitations would make it less likely that the AmigaOS 2.x/3.x default ROM file system would get itself into trouble, and the same goes for the various mass storage driver RDB parser implementations. Note that these limitations are just precautions baked into policy and the user should be able to override and disable them. I have no idea how this would work within the partitioning framework used by Linux, though, which I expect is geared towards giving the user maximum control rather than "training wheels". Maximum control implies awareness of the consequences of choosing a partition layout which requires specific file system and mass storage driver capabilities, so while a warning message "device needs 64 bit disk device support in native OS" is helpful, it may be better to say that the layout is not compatible with what the Amiga ROM operating system version 1.x/2.x/3.x can safely support. --------------------------------------------------------------- Amiga ROM operating system means the Kickstart ROM. AmigaOS 3.5 and 3.9 patch the Kickstart ROM and replace the default device driver "scsi.device" while doing so. Aside from that I´d say that this pretty much resembles the result of our discussion: Be cautious in the partitioning tools, but permissive in the RDB parser. I think I ask him about limitations in Amiga Fast Filesystem as well. Maybe he can give me something to quote about that as well. > 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 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> > Tested-by: Martin Steigerwald <Martin@lichtvoll.de> > > 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). > --- > block/partitions/amiga.c | 99 +++++++++++++++++++++++++++++++++++++++++++++- > 1 files changed, 98 insertions(+), 1 deletions(-) > > diff --git a/block/partitions/amiga.c b/block/partitions/amiga.c > index 5609366..6184d83 100644 > --- 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]); I agree Geert about using constants there. > + > + /* 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, > + 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. > + */ > + > + if (cylblk > UINT_MAX) { > + pr_err("Dev %s: hds*sects 0x%lx > UINT_MAX!\n", > + bdevname(state->bdev, b), > + (unsigned long) cylblk); > + > + /* 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; > + } > + } > + > + /* 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; > + } > + > /* 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; > + > + /* 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) { > + pr_err("Dev %s: partition 32 bit overflow! ", > + bdevname(state->bdev, b)); > + pr_cont("start_sect 0x%llX, nr_sects 0x%llx\n", > + start_sect, nr_sects); > + pr_err("Dev %s: partition may need 64 bit ", two spaces between "may" and "need" > + bdevname(state->bdev, b)); > + pr_cont("device support on native AmigaOS!\n"); Well an option would be to use Olaf´s wording, like so: partition not safely supported on Amiga ROM operating system version 1.x/2.x/3.x I wonder about how to shorten this, well maybe: partition not safely supported on Amiga ROM OS 1.x/2.x/3.x Or: partition not safely support on Amiga Kickstart ROM 1.x/2.x/3.x Albeit you already decided to go with a more generic warning, which in my point of view is also fine. Cause it is a bit challenging to word it in a correct way when mentioning version numbers. Well this is what Olaf suggested. > + /* Without LBD support, better bail out */ > + if (!IS_ENABLED(CONFIG_LBDAF)) { > + pr_err("Dev %s: no LBD support, aborting!", > + bdevname(state->bdev, b)); > + res = -1; > + goto rdb_done; > + } > + did_warn++; > + } > + > put_partition(state,slot++,start_sect,nr_sects); > { > /* Be even more informative to aid mounting */ > Thanks,
Martin Steigerwald - 02.07.18, 21:36: > Hi Michael. > > I felt free to Cc my mail address. It was Cc´d. KMail truncated the Cc list with an option to expand it.
Hi Kars, thanks for your comments - will do. Cheers, Michael On Mon, Jul 2, 2018 at 6:38 PM, Kars de Jong <jongk@linux-m68k.org> wrote: > Op ma 2 jul. 2018 om 07:29 schreef Michael Schmitz <schmitzmic@gmail.com>: >> @@ -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]); >> + > > Could you please create #defines for all these magic offsets in pb_Environment? > Below are a few more. > This makes the code much more readable. > Thanks! > >> + /* 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, >> + 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. >> + */ >> + >> + if (cylblk > UINT_MAX) { >> + pr_err("Dev %s: hds*sects 0x%lx > UINT_MAX!\n", >> + bdevname(state->bdev, b), >> + (unsigned long) cylblk); >> + >> + /* 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; >> + } >> + } >> + >> + /* 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; >> + } >> + >> /* Tell Kernel about it */ >> >> nr_sects = (be32_to_cpu(pb->pb_Environment[10]) + 1 -
Hi Geert, thanks for your comments! On Mon, Jul 2, 2018 at 8:29 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > >> + /* 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? Not sure - I had begun to tease apart the assembly to answer that, but got sidetracked. You may well be right that I'm still doing 32 bit muls. The old parser used signed int which is why it overflowed above 1 TB. Haven't had the time to gin up an RDB exceeding 2 TB yet. > >> + >> + /* 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). Will do that. >> + 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? Just an optimization for my overflow calculations ... >> + /* 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. Thanks, I knew I was missing something there. >> + } >> + >> /* 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... I hear you ... >> + >> + /* 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? No, we need to catch any partition address overflowing. nr_sects > UINT_MAX may be redundant though. > I would remove the did_warn check, as multiple partitions may be affected. Any partition overflowing means danger lurks (in AmigaDOS of sufficient vintage, that is) > 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. The first partition (partly) outside 2 TB will warn. But the point about partition ordering later is well taken. > >> + pr_err("Dev %s: partition 32 bit overflow! ", > > pr_warn() OK. > >> + 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. Checkpatch catch-22 (thou shalt not exceed 80 cols, thou shalt not split string consts over multiple lines, and thou shalt not use pr_cont() without good cause). I'll ignore the 80 cols error then. > >> + pr_err("Dev %s: partition may need 64 bit ", > > pr_warn() > > Drop the "may"? s/may/will/ ... > >> + 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? Can do, yes. > > 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. Right. I wasn't aware of the funky ordering feature (only seen it on MacOS before, and I have always held that's as weird as it gets). I'll try to get a new version out before tomorrow. Cheers, > >> + } >> + 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@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
Hi Michael, On Tue, Jul 3, 2018 at 1:58 AM Michael Schmitz <schmitzmic@gmail.com> wrote: > On Mon, Jul 2, 2018 at 8:29 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > >> + > >> + /* 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? > > No, we need to catch any partition address overflowing. nr_sects > > UINT_MAX may be redundant though. The three tests may have been needed when both variables were 32-bit, but when using unsigned 64-bit arithmetic, it shouldn't matter: if any of the two values doesn't fit in 32-bit, the sum also doesn't. Or is this also used to catch 64-bit add overflow? In that case, please use check_add_overflow(). > > I would remove the did_warn check, as multiple partitions may be affected. > > Any partition overflowing means danger lurks (in AmigaDOS of > sufficient vintage, that is) Yes, that's what I meant: dropping the did_warn check means always printing the warning, not just for the first "unusable" partition. > > 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. > > The first partition (partly) outside 2 TB will warn. But the point > about partition ordering later is well taken. > > > >> + pr_err("Dev %s: partition 32 bit overflow! ", > > > > pr_warn() > > OK. > > > > >> + 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. > > Checkpatch catch-22 (thou shalt not exceed 80 cols, thou shalt not > split string consts over multiple lines, and thou shalt not use > pr_cont() without good cause). I'll ignore the 80 cols error then. That's the sane thing to do: single pr_warn() statement, and ignoring the 80 columns error if it would mean splitting the string, so people can easily grep for it when they see the message on their consoles. Gr{oetje,eeting}s, Geert
Hi Geert, Am 03.07.2018 um 19:22 schrieb Geert Uytterhoeven: > Hi Michael, > > On Tue, Jul 3, 2018 at 1:58 AM Michael Schmitz <schmitzmic@gmail.com> wrote: >> On Mon, Jul 2, 2018 at 8:29 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: >>>> + >>>> + /* 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? >> >> No, we need to catch any partition address overflowing. nr_sects > >> UINT_MAX may be redundant though. > > The three tests may have been needed when both variables were 32-bit, > but when using unsigned 64-bit arithmetic, it shouldn't matter: if any of the > two values doesn't fit in 32-bit, the sum also doesn't. Right, got it now. Too late for v2 of the patch ... I just see I've messed up some other things (duplicate types.h inclusion) - I'll have to respin anyway. I'll hold off on that waiting for futher feedback for now. > Or is this also used to catch 64-bit add overflow? In that case, please use > check_add_overflow(). No, the start sector is directly calculated from what's stored in the DosEnvVec struct (or whatever the precise name was), as product of four 32 bit numbers. nr_sects is calculated from the 32 bit cylinder address difference and the other three 32 bit numbers, no addition overflow there. >>> I would remove the did_warn check, as multiple partitions may be affected. >> >> Any partition overflowing means danger lurks (in AmigaDOS of >> sufficient vintage, that is) > > Yes, that's what I meant: dropping the did_warn check means always printing > the warning, not just for the first "unusable" partition. Yes, I've done that now. >>> No need for pr_cont(), just merge the two statements. >> >> Checkpatch catch-22 (thou shalt not exceed 80 cols, thou shalt not >> split string consts over multiple lines, and thou shalt not use >> pr_cont() without good cause). I'll ignore the 80 cols error then. > > That's the sane thing to do: single pr_warn() statement, and ignoring the 80 > columns error if it would mean splitting the string, so people can easily > grep for it when they see the message on their consoles. True - and there isn't a checkpatch error for overlong format strings these days (there might have been at some time, which I remembered). Thanks again! Cheers, Michael > > Gr{oetje,eeting}s, > > Geert >
On 20180703 00:22, Geert Uytterhoeven wrote: > Hi Michael, > > On Tue, Jul 3, 2018 at 1:58 AM Michael Schmitz <schmitzmic@gmail.com> wrote: >> On Mon, Jul 2, 2018 at 8:29 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: >>>> + >>>> + /* 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? >> >> No, we need to catch any partition address overflowing. nr_sects > >> UINT_MAX may be redundant though. > > The three tests may have been needed when both variables were 32-bit, > but when using unsigned 64-bit arithmetic, it shouldn't matter: if any of the > two values doesn't fit in 32-bit, the sum also doesn't. > Or is this also used to catch 64-bit add overflow? In that case, please use > check_add_overflow(). Um, both values can be high 32 bits with the sum being greater than 32 bits. Seems to me, though, that the only test needed is whether the sum is greater than 32 bits. If either number is greater the sum certainly is. (Of course, either a comment to that effect or code that pounds the concept into the code's future reader should appear.) The 2TB issue is one that may surprise old hands given the appearance of the RDBs being able to support absurdly large, NSA pleasing, disks. It's hard to decide where warnings should appear. (And it's difficult to justify them in the RDB parser unless it overflows the interface to the Linux OS. The mkfs-(RDB reading Amiga filesystem) took should be where the warnings are posted. >>> I would remove the did_warn check, as multiple partitions may be affected. >> >> Any partition overflowing means danger lurks (in AmigaDOS of >> sufficient vintage, that is) > > Yes, that's what I meant: dropping the did_warn check means always printing > the warning, not just for the first "unusable" partition. Actually anything greater than 2 gigabytes may be a problem on some versions of AmigaDOS that spoke to hard disks of one kind or another. A newcomer to the surprisingly active Amiga scene could get distressed by that error. Old timers know of the fixes that circulated. Those fixes appear to work with greater than 2TB disks to some limited degree. There are probably some official "Commodore" OS utilities which will blow their little minds at 2TB if not sooner. That's one that can be protected for in the partitioning tool. The RDB parser should warn if it gets confused or overflows its interface into the OS. Ideally it should back out of mounting partitions it does not understand. Note that the original RDB parser patch I made was part of patch to allow 2k physical block size to match some 640 megabyte Fujitsu magneto-optical drives. I hope the OS can still cope with that situation. (I don't know if I can recover the actual data written to the disk. I can't remember if I limited the LSEG data, which Linux should ignore anyway, to 512 bytes or not when I built the low level partitioning tools. So LONG ago and a mind with a 7 in front of its age gets tad flaky remembering such ancient history.) >>> 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. >> >> The first partition (partly) outside 2 TB will warn. But the point >> about partition ordering later is well taken. >>> >>>> + pr_err("Dev %s: partition 32 bit overflow! ", >>> >>> pr_warn() >> >> OK. >> >>> >>>> + 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. >> >> Checkpatch catch-22 (thou shalt not exceed 80 cols, thou shalt not >> split string consts over multiple lines, and thou shalt not use >> pr_cont() without good cause). I'll ignore the 80 cols error then. > > That's the sane thing to do: single pr_warn() statement, and ignoring the 80 > columns error if it would mean splitting the string, so people can easily > grep for it when they see the message on their consoles. > > Gr{oetje,eeting}s, > > Geert And thank you for your work with the Linux community, Geert. I've noticed and admired it. {^_^} Joanne Dow (Her just cruseing face.)
diff --git a/block/partitions/amiga.c b/block/partitions/amiga.c index 5609366..6184d83 100644 --- 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]); + + /* 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, + 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. + */ + + if (cylblk > UINT_MAX) { + pr_err("Dev %s: hds*sects 0x%lx > UINT_MAX!\n", + bdevname(state->bdev, b), + (unsigned long) cylblk); + + /* 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; + } + } + + /* 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; + } + /* 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; + + /* 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) { + pr_err("Dev %s: partition 32 bit overflow! ", + bdevname(state->bdev, b)); + pr_cont("start_sect 0x%llX, nr_sects 0x%llx\n", + start_sect, nr_sects); + pr_err("Dev %s: partition may need 64 bit ", + bdevname(state->bdev, b)); + pr_cont("device support on native AmigaOS!\n"); + /* Without LBD support, better bail out */ + if (!IS_ENABLED(CONFIG_LBDAF)) { + pr_err("Dev %s: no LBD support, aborting!", + bdevname(state->bdev, b)); + res = -1; + goto rdb_done; + } + did_warn++; + } + put_partition(state,slot++,start_sect,nr_sects); { /* Be even more informative to aid mounting */