* [PATCH v8 0/2] Amiga RDB partition support fixes
@ 2022-07-26 4:57 Michael Schmitz
2022-07-26 4:57 ` [PATCH v8 1/2] block: fix signed int overflow in Amiga partition support Michael Schmitz
2022-07-26 4:57 ` [PATCH v8 2/2] block: add overflow checks for " Michael Schmitz
0 siblings, 2 replies; 11+ messages in thread
From: Michael Schmitz @ 2022-07-26 4:57 UTC (permalink / raw)
To: linux-block, axboe; +Cc: linux-m68k, geert
Resend of the final version of the Amiga RDB partition table patch,
updated to current linux-block.
I've split off the part fixing the incorrect use of signed int
for partition start address and size as separate patch. 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. Comments by Geert have been
addressed in full.
Both patches now reviewed by Geert - Reviewed-by tags added.
Cheers,
Michael
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH v8 1/2] block: fix signed int overflow in Amiga partition support 2022-07-26 4:57 [PATCH v8 0/2] Amiga RDB partition support fixes Michael Schmitz @ 2022-07-26 4:57 ` Michael Schmitz 2022-07-26 11:41 ` Christoph Hellwig 2022-07-26 4:57 ` [PATCH v8 2/2] block: add overflow checks for " Michael Schmitz 1 sibling, 1 reply; 11+ messages in thread From: Michael Schmitz @ 2022-07-26 4:57 UTC (permalink / raw) To: linux-block, axboe; +Cc: linux-m68k, geert, 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> Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org> --- 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 5c8624e26a54..f98191545d9a 100644 --- a/block/partitions/amiga.c +++ b/block/partitions/amiga.c @@ -31,7 +31,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; @@ -96,14 +97,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; -- 2.17.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v8 1/2] block: fix signed int overflow in Amiga partition support 2022-07-26 4:57 ` [PATCH v8 1/2] block: fix signed int overflow in Amiga partition support Michael Schmitz @ 2022-07-26 11:41 ` Christoph Hellwig 0 siblings, 0 replies; 11+ messages in thread From: Christoph Hellwig @ 2022-07-26 11:41 UTC (permalink / raw) To: Michael Schmitz; +Cc: linux-block, axboe, linux-m68k, geert On Tue, Jul 26, 2022 at 04:57:46PM +1200, Michael Schmitz 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> > Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org> > --- > 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 5c8624e26a54..f98191545d9a 100644 > --- a/block/partitions/amiga.c > +++ b/block/partitions/amiga.c > @@ -31,7 +31,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; > > @@ -96,14 +97,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])) * Nit: we usually don't leave a whitespace afte a cast and the operators go onto the previous line, i.e. this should be: + nr_sects = ((sector_t)be32_to_cpu(pb->pb_Environment[10]) + 1 - + be32_to_cpu(pb->pb_Environment[9])) * Otherwise looks good: Reviewed-by: Christoph Hellwig <hch@lst.de> ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v8 2/2] block: add overflow checks for Amiga partition support 2022-07-26 4:57 [PATCH v8 0/2] Amiga RDB partition support fixes Michael Schmitz 2022-07-26 4:57 ` [PATCH v8 1/2] block: fix signed int overflow in Amiga partition support Michael Schmitz @ 2022-07-26 4:57 ` Michael Schmitz 2022-07-26 11:42 ` Christoph Hellwig 2022-07-26 12:53 ` kernel test robot 1 sibling, 2 replies; 11+ messages in thread From: Michael Schmitz @ 2022-07-26 4:57 UTC (permalink / raw) To: linux-block, axboe; +Cc: linux-m68k, geert, 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> Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org> --- block/partitions/amiga.c | 111 +++++++++++++++++++++++++++++++-------- 1 file changed, 89 insertions(+), 22 deletions(-) diff --git a/block/partitions/amiga.c b/block/partitions/amiga.c index f98191545d9a..7356b39cbe10 100644 --- a/block/partitions/amiga.c +++ b/block/partitions/amiga.c @@ -11,10 +11,18 @@ #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" +/* 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) { @@ -31,9 +39,12 @@ int amiga_partition(struct parsed_partitions *state) unsigned char *data; struct RigidDiskBlock *rdb; struct PartitionBlock *pb; - sector_t start_sect, nr_sects; - int blk, part, res = 0; - int blksize = 1; /* Multiplier for disk block size */ + u64 start_sect, nr_sects; + sector_t blk, end_sect; + u32 cylblk; /* rdb_CylBlocks = nr_heads*sect_per_track */ + u32 nr_hd, nr_sect, lo_cyl, hi_cyl; + int part, res = 0; + unsigned int blksize = 1; /* Multiplier for disk block size */ int slot = 1; for (blk = 0; ; blk++, put_dev_sector(sect)) { @@ -41,8 +52,8 @@ int amiga_partition(struct parsed_partitions *state) goto rdb_done; data = read_part_sector(state, blk, §); if (!data) { - pr_err("Dev %s: unable to read RDB block %d\n", - state->disk->disk_name, blk); + pr_err("Dev %s: unable to read RDB block %llu\n", + state->disk->disk_name, (u64) blk); res = -1; goto rdb_done; } @@ -58,13 +69,13 @@ int amiga_partition(struct parsed_partitions *state) *(__be32 *)(data+0xdc) = 0; if (checksum_block((__be32 *)data, be32_to_cpu(rdb->rdb_SummedLongs) & 0x7F)==0) { - pr_err("Trashed word at 0xd0 in block %d ignored in checksum calculation\n", - blk); + pr_err("Trashed word at 0xd0 in block %llu ignored in checksum calculation\n", + (u64) blk); break; } - pr_err("Dev %s: RDB in block %d has bad checksum\n", - state->disk->disk_name, blk); + pr_err("Dev %s: RDB in block %llu has bad checksum\n", + state->disk->disk_name, (u64) blk); } /* blksize is blocks per 512 byte standard block */ @@ -80,11 +91,16 @@ int amiga_partition(struct parsed_partitions *state) blk = be32_to_cpu(rdb->rdb_PartitionList); put_dev_sector(sect); for (part = 1; blk>0 && part<=16; part++, put_dev_sector(sect)) { - blk *= blksize; /* Read in terms partition table understands */ + /* Read in terms partition table understands */ + if (check_mul_overflow(blk, (sector_t) blksize, &blk)) { + pr_err("Dev %s: overflow calculating partition block %llu! Skipping partitions %u and beyond\n", + state->disk->disk_name, (u64) blk, part); + break; + } data = read_part_sector(state, blk, §); if (!data) { - pr_err("Dev %s: unable to read partition block %d\n", - state->disk->disk_name, blk); + pr_err("Dev %s: unable to read partition block %llu\n", + state->disk->disk_name, (u64) blk); res = -1; goto rdb_done; } @@ -95,19 +111,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", + state->disk->disk_name, 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", + state->disk->disk_name, 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", + state->disk->disk_name, 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", + state->disk->disk_name, 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", + state->disk->disk_name, 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 */ -- 2.17.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v8 2/2] block: add overflow checks for Amiga partition support 2022-07-26 4:57 ` [PATCH v8 2/2] block: add overflow checks for " Michael Schmitz @ 2022-07-26 11:42 ` Christoph Hellwig 2022-07-26 22:28 ` Michael Schmitz 2022-08-11 11:40 ` Geert Uytterhoeven 2022-07-26 12:53 ` kernel test robot 1 sibling, 2 replies; 11+ messages in thread From: Christoph Hellwig @ 2022-07-26 11:42 UTC (permalink / raw) To: Michael Schmitz; +Cc: linux-block, axboe, linux-m68k, geert On Tue, Jul 26, 2022 at 04:57:47PM +1200, Michael Schmitz 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> > Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org> > --- > block/partitions/amiga.c | 111 +++++++++++++++++++++++++++++++-------- > 1 file changed, 89 insertions(+), 22 deletions(-) > > diff --git a/block/partitions/amiga.c b/block/partitions/amiga.c > index f98191545d9a..7356b39cbe10 100644 > --- a/block/partitions/amiga.c > +++ b/block/partitions/amiga.c > @@ -11,10 +11,18 @@ > #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" > > +/* magic offsets in partition DosEnvVec */ > +#define NR_HD 3 > +#define NR_SECT 5 > +#define LO_CYL 9 > +#define HI_CYL 10 The last line has a tab after the #define while the previous three don't. Pick one style and stick to it for the others. > if (!data) { > - pr_err("Dev %s: unable to read RDB block %d\n", > - state->disk->disk_name, blk); > + pr_err("Dev %s: unable to read RDB block %llu\n", > + state->disk->disk_name, (u64) blk); No need for the various printk casts, a sector_t is always an unsigned long long. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v8 2/2] block: add overflow checks for Amiga partition support 2022-07-26 11:42 ` Christoph Hellwig @ 2022-07-26 22:28 ` Michael Schmitz 2022-08-11 11:40 ` Geert Uytterhoeven 1 sibling, 0 replies; 11+ messages in thread From: Michael Schmitz @ 2022-07-26 22:28 UTC (permalink / raw) To: Christoph Hellwig; +Cc: linux-block, axboe, linux-m68k, geert Hi Christoph, On 26/07/22 23:42, Christoph Hellwig wrote: > On Tue, Jul 26, 2022 at 04:57:47PM +1200, Michael Schmitz 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> >> Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org> >> --- >> block/partitions/amiga.c | 111 +++++++++++++++++++++++++++++++-------- >> 1 file changed, 89 insertions(+), 22 deletions(-) >> >> diff --git a/block/partitions/amiga.c b/block/partitions/amiga.c >> index f98191545d9a..7356b39cbe10 100644 >> --- a/block/partitions/amiga.c >> +++ b/block/partitions/amiga.c >> @@ -11,10 +11,18 @@ >> #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" >> >> +/* magic offsets in partition DosEnvVec */ >> +#define NR_HD 3 >> +#define NR_SECT 5 >> +#define LO_CYL 9 >> +#define HI_CYL 10 > The last line has a tab after the #define while the previous three > don't. Pick one style and stick to it for the others. > >> if (!data) { >> - pr_err("Dev %s: unable to read RDB block %d\n", >> - state->disk->disk_name, blk); >> + pr_err("Dev %s: unable to read RDB block %llu\n", >> + state->disk->disk_name, (u64) blk); > No need for the various printk casts, a sector_t is always an > unsigned long long. Thanks, wil fix (and address the other issues raised). Cheers, Michael ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v8 2/2] block: add overflow checks for Amiga partition support 2022-07-26 11:42 ` Christoph Hellwig 2022-07-26 22:28 ` Michael Schmitz @ 2022-08-11 11:40 ` Geert Uytterhoeven 2022-08-22 20:38 ` Michael Schmitz 1 sibling, 1 reply; 11+ messages in thread From: Geert Uytterhoeven @ 2022-08-11 11:40 UTC (permalink / raw) To: Christoph Hellwig Cc: Michael Schmitz, linux-block, Jens Axboe, Linux/m68k, Greg KH Hi Christoph, On Tue, Jul 26, 2022 at 1:43 PM Christoph Hellwig <hch@infradead.org> wrote: > On Tue, Jul 26, 2022 at 04:57:47PM +1200, Michael Schmitz 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> > > Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org> > > --- a/block/partitions/amiga.c > > +++ b/block/partitions/amiga.c > > if (!data) { > > - pr_err("Dev %s: unable to read RDB block %d\n", > > - state->disk->disk_name, blk); > > + pr_err("Dev %s: unable to read RDB block %llu\n", > > + state->disk->disk_name, (u64) blk); > > No need for the various printk casts, a sector_t is always an > unsigned long long. That is true, as of commit 72deb455b5ec619f ("block: remove CONFIG_LBDAF") in v5.2. Since 4.9, 4.14, and 4.19 are still receiving stable updates, the cast should be re-added when this is backported. 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v8 2/2] block: add overflow checks for Amiga partition support 2022-08-11 11:40 ` Geert Uytterhoeven @ 2022-08-22 20:38 ` Michael Schmitz 2022-08-22 21:03 ` Geert Uytterhoeven 0 siblings, 1 reply; 11+ messages in thread From: Michael Schmitz @ 2022-08-22 20:38 UTC (permalink / raw) To: Geert Uytterhoeven, Christoph Hellwig Cc: linux-block, Jens Axboe, Linux/m68k, Greg KH Hi Geert, On 11/08/22 23:40, Geert Uytterhoeven wrote: > Hi Christoph, > > On Tue, Jul 26, 2022 at 1:43 PM Christoph Hellwig <hch@infradead.org> wrote: >> On Tue, Jul 26, 2022 at 04:57:47PM +1200, Michael Schmitz 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> >>> Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org> >>> --- a/block/partitions/amiga.c >>> +++ b/block/partitions/amiga.c >>> if (!data) { >>> - pr_err("Dev %s: unable to read RDB block %d\n", >>> - state->disk->disk_name, blk); >>> + pr_err("Dev %s: unable to read RDB block %llu\n", >>> + state->disk->disk_name, (u64) blk); >> No need for the various printk casts, a sector_t is always an >> unsigned long long. > That is true, as of commit 72deb455b5ec619f > ("block: remove CONFIG_LBDAF") in v5.2. > Since 4.9, 4.14, and 4.19 are still receiving stable updates, the > cast should be re-added when this is backported. Does this require a note in the commit message, or explicit CC to Greg? Cheers, Michael > 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v8 2/2] block: add overflow checks for Amiga partition support 2022-08-22 20:38 ` Michael Schmitz @ 2022-08-22 21:03 ` Geert Uytterhoeven 2022-08-22 21:09 ` Michael Schmitz 0 siblings, 1 reply; 11+ messages in thread From: Geert Uytterhoeven @ 2022-08-22 21:03 UTC (permalink / raw) To: Michael Schmitz Cc: Christoph Hellwig, linux-block, Jens Axboe, Linux/m68k, Greg KH Hi Michael, On Mon, Aug 22, 2022 at 10:38 PM Michael Schmitz <schmitzmic@gmail.com> wrote: > On 11/08/22 23:40, Geert Uytterhoeven wrote: > > On Tue, Jul 26, 2022 at 1:43 PM Christoph Hellwig <hch@infradead.org> wrote: > >> On Tue, Jul 26, 2022 at 04:57:47PM +1200, Michael Schmitz 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> > >>> Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org> > >>> --- a/block/partitions/amiga.c > >>> +++ b/block/partitions/amiga.c > >>> if (!data) { > >>> - pr_err("Dev %s: unable to read RDB block %d\n", > >>> - state->disk->disk_name, blk); > >>> + pr_err("Dev %s: unable to read RDB block %llu\n", > >>> + state->disk->disk_name, (u64) blk); > >> No need for the various printk casts, a sector_t is always an > >> unsigned long long. > > That is true, as of commit 72deb455b5ec619f > > ("block: remove CONFIG_LBDAF") in v5.2. > > Since 4.9, 4.14, and 4.19 are still receiving stable updates, the > > cast should be re-added when this is backported. > > Does this require a note in the commit message, or explicit CC to Greg? According to [1], you should add Cc: <stable@vger.kernel.org> # 5.2 [1] https://docs.kernel.org/process/stable-kernel-rules.html?highlight=prerequisites 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v8 2/2] block: add overflow checks for Amiga partition support 2022-08-22 21:03 ` Geert Uytterhoeven @ 2022-08-22 21:09 ` Michael Schmitz 0 siblings, 0 replies; 11+ messages in thread From: Michael Schmitz @ 2022-08-22 21:09 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Christoph Hellwig, linux-block, Jens Axboe, Linux/m68k, Greg KH Hi Geert, saw this a minute too late - will resend again. Cheers, Michael On 23/08/22 09:03, Geert Uytterhoeven wrote: > Hi Michael, > > On Mon, Aug 22, 2022 at 10:38 PM Michael Schmitz <schmitzmic@gmail.com> wrote: >> On 11/08/22 23:40, Geert Uytterhoeven wrote: >>> On Tue, Jul 26, 2022 at 1:43 PM Christoph Hellwig <hch@infradead.org> wrote: >>>> On Tue, Jul 26, 2022 at 04:57:47PM +1200, Michael Schmitz 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> >>>>> Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org> >>>>> --- a/block/partitions/amiga.c >>>>> +++ b/block/partitions/amiga.c >>>>> if (!data) { >>>>> - pr_err("Dev %s: unable to read RDB block %d\n", >>>>> - state->disk->disk_name, blk); >>>>> + pr_err("Dev %s: unable to read RDB block %llu\n", >>>>> + state->disk->disk_name, (u64) blk); >>>> No need for the various printk casts, a sector_t is always an >>>> unsigned long long. >>> That is true, as of commit 72deb455b5ec619f >>> ("block: remove CONFIG_LBDAF") in v5.2. >>> Since 4.9, 4.14, and 4.19 are still receiving stable updates, the >>> cast should be re-added when this is backported. >> Does this require a note in the commit message, or explicit CC to Greg? > According to [1], you should add > > Cc: <stable@vger.kernel.org> # 5.2 > > [1] https://docs.kernel.org/process/stable-kernel-rules.html?highlight=prerequisites > > 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v8 2/2] block: add overflow checks for Amiga partition support 2022-07-26 4:57 ` [PATCH v8 2/2] block: add overflow checks for " Michael Schmitz 2022-07-26 11:42 ` Christoph Hellwig @ 2022-07-26 12:53 ` kernel test robot 1 sibling, 0 replies; 11+ messages in thread From: kernel test robot @ 2022-07-26 12:53 UTC (permalink / raw) To: Michael Schmitz, linux-block, axboe Cc: kbuild-all, linux-m68k, geert, Michael Schmitz Hi Michael, I love your patch! Perhaps something to improve: [auto build test WARNING on axboe-block/for-next] [also build test WARNING on linus/master v5.19-rc8 next-20220725] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Michael-Schmitz/Amiga-RDB-partition-support-fixes/20220726-125830 base: https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next config: sparc-randconfig-s041-20220724 (https://download.01.org/0day-ci/archive/20220726/202207262047.yRImNV4v-lkp@intel.com/config) compiler: sparc-linux-gcc (GCC) 12.1.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # apt-get install sparse # sparse version: v0.6.4-39-gce1a6720-dirty # https://github.com/intel-lab-lkp/linux/commit/ccbc09d2b458d51ac395c4f8ea7cf703f6e83fdf git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review Michael-Schmitz/Amiga-RDB-partition-support-fixes/20220726-125830 git checkout ccbc09d2b458d51ac395c4f8ea7cf703f6e83fdf # save the config file mkdir build_dir && cp config build_dir/.config COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=sparc SHELL=/bin/bash block/partitions/ If you fix the issue, kindly add following tag where applicable Reported-by: kernel test robot <lkp@intel.com> sparse warnings: (new ones prefixed by >>) >> block/partitions/amiga.c:132:30: sparse: sparse: cast to restricted __be32 block/partitions/amiga.c:133:25: sparse: sparse: cast to restricted __be32 vim +132 block/partitions/amiga.c 35 36 int amiga_partition(struct parsed_partitions *state) 37 { 38 Sector sect; 39 unsigned char *data; 40 struct RigidDiskBlock *rdb; 41 struct PartitionBlock *pb; 42 u64 start_sect, nr_sects; 43 sector_t blk, end_sect; 44 u32 cylblk; /* rdb_CylBlocks = nr_heads*sect_per_track */ 45 u32 nr_hd, nr_sect, lo_cyl, hi_cyl; 46 int part, res = 0; 47 unsigned int blksize = 1; /* Multiplier for disk block size */ 48 int slot = 1; 49 50 for (blk = 0; ; blk++, put_dev_sector(sect)) { 51 if (blk == RDB_ALLOCATION_LIMIT) 52 goto rdb_done; 53 data = read_part_sector(state, blk, §); 54 if (!data) { 55 pr_err("Dev %s: unable to read RDB block %llu\n", 56 state->disk->disk_name, (u64) blk); 57 res = -1; 58 goto rdb_done; 59 } 60 if (*(__be32 *)data != cpu_to_be32(IDNAME_RIGIDDISK)) 61 continue; 62 63 rdb = (struct RigidDiskBlock *)data; 64 if (checksum_block((__be32 *)data, be32_to_cpu(rdb->rdb_SummedLongs) & 0x7F) == 0) 65 break; 66 /* Try again with 0xdc..0xdf zeroed, Windows might have 67 * trashed it. 68 */ 69 *(__be32 *)(data+0xdc) = 0; 70 if (checksum_block((__be32 *)data, 71 be32_to_cpu(rdb->rdb_SummedLongs) & 0x7F)==0) { 72 pr_err("Trashed word at 0xd0 in block %llu ignored in checksum calculation\n", 73 (u64) blk); 74 break; 75 } 76 77 pr_err("Dev %s: RDB in block %llu has bad checksum\n", 78 state->disk->disk_name, (u64) blk); 79 } 80 81 /* blksize is blocks per 512 byte standard block */ 82 blksize = be32_to_cpu( rdb->rdb_BlockBytes ) / 512; 83 84 { 85 char tmp[7 + 10 + 1 + 1]; 86 87 /* Be more informative */ 88 snprintf(tmp, sizeof(tmp), " RDSK (%d)", blksize * 512); 89 strlcat(state->pp_buf, tmp, PAGE_SIZE); 90 } 91 blk = be32_to_cpu(rdb->rdb_PartitionList); 92 put_dev_sector(sect); 93 for (part = 1; blk>0 && part<=16; part++, put_dev_sector(sect)) { 94 /* Read in terms partition table understands */ 95 if (check_mul_overflow(blk, (sector_t) blksize, &blk)) { 96 pr_err("Dev %s: overflow calculating partition block %llu! Skipping partitions %u and beyond\n", 97 state->disk->disk_name, (u64) blk, part); 98 break; 99 } 100 data = read_part_sector(state, blk, §); 101 if (!data) { 102 pr_err("Dev %s: unable to read partition block %llu\n", 103 state->disk->disk_name, (u64) blk); 104 res = -1; 105 goto rdb_done; 106 } 107 pb = (struct PartitionBlock *)data; 108 blk = be32_to_cpu(pb->pb_Next); 109 if (pb->pb_ID != cpu_to_be32(IDNAME_PARTITION)) 110 continue; 111 if (checksum_block((__be32 *)pb, be32_to_cpu(pb->pb_SummedLongs) & 0x7F) != 0 ) 112 continue; 113 114 /* RDB gives us more than enough rope to hang ourselves with, 115 * many times over (2^128 bytes if all fields max out). 116 * Some careful checks are in order, so check for potential 117 * overflows. 118 * We are multiplying four 32 bit numbers to one sector_t! 119 */ 120 121 nr_hd = be32_to_cpu(pb->pb_Environment[NR_HD]); 122 nr_sect = be32_to_cpu(pb->pb_Environment[NR_SECT]); 123 124 /* CylBlocks is total number of blocks per cylinder */ 125 if (check_mul_overflow(nr_hd, nr_sect, &cylblk)) { 126 pr_err("Dev %s: heads*sects %u overflows u32, skipping partition!\n", 127 state->disk->disk_name, cylblk); 128 continue; 129 } 130 131 /* check for consistency with RDB defined CylBlocks */ > 132 if (cylblk > be32_to_cpu(rdb->rdb_CylBlocks)) { -- 0-DAY CI Kernel Test Service https://01.org/lkp ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2022-08-22 21:09 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-07-26 4:57 [PATCH v8 0/2] Amiga RDB partition support fixes Michael Schmitz 2022-07-26 4:57 ` [PATCH v8 1/2] block: fix signed int overflow in Amiga partition support Michael Schmitz 2022-07-26 11:41 ` Christoph Hellwig 2022-07-26 4:57 ` [PATCH v8 2/2] block: add overflow checks for " Michael Schmitz 2022-07-26 11:42 ` Christoph Hellwig 2022-07-26 22:28 ` Michael Schmitz 2022-08-11 11:40 ` Geert Uytterhoeven 2022-08-22 20:38 ` Michael Schmitz 2022-08-22 21:03 ` Geert Uytterhoeven 2022-08-22 21:09 ` Michael Schmitz 2022-07-26 12:53 ` kernel test robot
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.