* [PATCH v10 1/3] block: fix signed int overflow in Amiga partition support [not found] <20230615030837.8518-1-schmitzmic@gmail.com> @ 2023-06-15 3:08 ` Michael Schmitz 2023-06-15 3:08 ` [PATCH v10 2/3] block: change annotation of rdb_CylBlocks in affs_hardblocks.h Michael Schmitz 2023-06-15 3:08 ` [PATCH v10 3/3] block: add overflow checks for Amiga partition support Michael Schmitz 2 siblings, 0 replies; 15+ messages in thread From: Michael Schmitz @ 2023-06-15 3:08 UTC (permalink / raw) To: linux-block, axboe Cc: linux-m68k, geert, hch, martin, fthain, Michael Schmitz, stable 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 3 of this series for that). Reported-by: Martin Steigerwald <Martin@lichtvoll.de> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=43511 Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") Message-ID: <201206192146.09327.Martin@lichtvoll.de> Cc: <stable@vger.kernel.org> # 5.2 Signed-off-by: Michael Schmitz <schmitzmic@gmail.com> Tested-by: Martin Steigerwald <Martin@lichtvoll.de> Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org> Reviewed-by: Christoph Hellwig <hch@lst.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 Changes from v7: Christoph Hellwig - correct style issues Changes from v9: - add Fixes: tags and stable backport prereq --- 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..85c5c79aae48 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] 15+ messages in thread
* [PATCH v10 2/3] block: change annotation of rdb_CylBlocks in affs_hardblocks.h [not found] <20230615030837.8518-1-schmitzmic@gmail.com> 2023-06-15 3:08 ` [PATCH v10 1/3] block: fix signed int overflow in Amiga partition support Michael Schmitz @ 2023-06-15 3:08 ` Michael Schmitz 2023-06-15 4:17 ` Christoph Hellwig 2023-06-15 3:08 ` [PATCH v10 3/3] block: add overflow checks for Amiga partition support Michael Schmitz 2 siblings, 1 reply; 15+ messages in thread From: Michael Schmitz @ 2023-06-15 3:08 UTC (permalink / raw) To: linux-block, axboe Cc: linux-m68k, geert, hch, martin, fthain, Michael Schmitz, stable 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. 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 patch 1 of this series). Patch 3 (this series) adds additional error checking and warning messages. One of the error checks now makes use of the previously unused rdb_CylBlocks field, which causes a 'sparse' warning (cast to restricted __be32). Annotate rdb_CylBlocks field as big endian to shut up the warning. Add comment to document that the same annotation is going to be needed for any other fields that may be used by the kernel in future. Reported-by: Martin Steigerwald <Martin@lichtvoll.de> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=43511 Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") Message-ID: <201206192146.09327.Martin@lichtvoll.de> Cc: <stable@vger.kernel.org> # 5.2 Signed-off-by: Michael Schmitz <schmitzmic@gmail.com> --- include/uapi/linux/affs_hardblocks.h | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/include/uapi/linux/affs_hardblocks.h b/include/uapi/linux/affs_hardblocks.h index 5e2fb8481252..9da5bc607939 100644 --- a/include/uapi/linux/affs_hardblocks.h +++ b/include/uapi/linux/affs_hardblocks.h @@ -6,6 +6,11 @@ /* Just the needed definitions for the RDB of an Amiga HD. */ +/* MSch 20230615: any field used by the Linux kernel must be + * annotated __be32! If any fields require increase to 64 + * bit size, rdb_ID _must_ be changed! + */ + struct RigidDiskBlock { __u32 rdb_ID; __be32 rdb_SummedLongs; @@ -32,7 +37,7 @@ struct RigidDiskBlock { __u32 rdb_RDBBlocksHi; __u32 rdb_LoCylinder; __u32 rdb_HiCylinder; - __u32 rdb_CylBlocks; + __be32 rdb_CylBlocks; __u32 rdb_AutoParkSeconds; __u32 rdb_HighRDSKBlock; __u32 rdb_Reserved4; -- 2.17.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v10 2/3] block: change annotation of rdb_CylBlocks in affs_hardblocks.h 2023-06-15 3:08 ` [PATCH v10 2/3] block: change annotation of rdb_CylBlocks in affs_hardblocks.h Michael Schmitz @ 2023-06-15 4:17 ` Christoph Hellwig 2023-06-15 4:50 ` Michael Schmitz 0 siblings, 1 reply; 15+ messages in thread From: Christoph Hellwig @ 2023-06-15 4:17 UTC (permalink / raw) To: Michael Schmitz Cc: linux-block, axboe, linux-m68k, geert, hch, martin, fthain, stable On Thu, Jun 15, 2023 at 03:08:36PM +1200, Michael Schmitz wrote: > +/* MSch 20230615: any field used by the Linux kernel must be > + * annotated __be32! If any fields require increase to 64 > + * bit size, rdb_ID _must_ be changed! > + */ This is a really weird comment. If you change on-disk format it is a different format and needs to be marked as such, sure. And as far as I can tell everything that is a __u32 here should be an __be32 because it is a big endian on-disk format. Why would you change only a single field? ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v10 2/3] block: change annotation of rdb_CylBlocks in affs_hardblocks.h 2023-06-15 4:17 ` Christoph Hellwig @ 2023-06-15 4:50 ` Michael Schmitz 2023-06-15 5:53 ` Christoph Hellwig 0 siblings, 1 reply; 15+ messages in thread From: Michael Schmitz @ 2023-06-15 4:50 UTC (permalink / raw) To: Christoph Hellwig Cc: linux-block, axboe, linux-m68k, geert, martin, fthain, stable Hi Christoph, thanks for your comments! Am 15.06.2023 um 16:17 schrieb Christoph Hellwig: > On Thu, Jun 15, 2023 at 03:08:36PM +1200, Michael Schmitz wrote: >> +/* MSch 20230615: any field used by the Linux kernel must be >> + * annotated __be32! If any fields require increase to 64 >> + * bit size, rdb_ID _must_ be changed! >> + */ > > This is a really weird comment. If you change on-disk format it > is a different format and needs to be marked as such, sure. I concede this isn't relevant to the matter at hand, and it's something that _should_ be obvious. This came up in my off-list discussion with Joanne Dow as a caveat for future huge disk sizes, so I thought I'd add it to the comment. > And as far as I can tell everything that is a __u32 here should > be an __be32 because it is a big endian on-disk format. Why > would you change only a single field? Because that's all I needed, and wanted to avoid excess patch churn. Plus (appeal to authority here :-)) it's in keeping with what Al Viro did when the __be32 annotations were first added. I can change all __u32 to __be32 and drop the comment if that's preferred. Cheers, Michael ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v10 2/3] block: change annotation of rdb_CylBlocks in affs_hardblocks.h 2023-06-15 4:50 ` Michael Schmitz @ 2023-06-15 5:53 ` Christoph Hellwig 2023-06-15 7:09 ` Michael Schmitz 2023-06-15 7:21 ` Geert Uytterhoeven 0 siblings, 2 replies; 15+ messages in thread From: Christoph Hellwig @ 2023-06-15 5:53 UTC (permalink / raw) To: Michael Schmitz Cc: Christoph Hellwig, linux-block, axboe, linux-m68k, geert, martin, fthain, stable On Thu, Jun 15, 2023 at 04:50:45PM +1200, Michael Schmitz wrote: >> And as far as I can tell everything that is a __u32 here should >> be an __be32 because it is a big endian on-disk format. Why >> would you change only a single field? > > Because that's all I needed, and wanted to avoid excess patch churn. Plus > (appeal to authority here :-)) it's in keeping with what Al Viro did when > the __be32 annotations were first added. > > I can change all __u32 to __be32 and drop the comment if that's preferred. That would be great! ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v10 2/3] block: change annotation of rdb_CylBlocks in affs_hardblocks.h 2023-06-15 5:53 ` Christoph Hellwig @ 2023-06-15 7:09 ` Michael Schmitz 2023-06-15 7:21 ` Geert Uytterhoeven 1 sibling, 0 replies; 15+ messages in thread From: Michael Schmitz @ 2023-06-15 7:09 UTC (permalink / raw) To: Christoph Hellwig Cc: linux-block, axboe, linux-m68k, geert, martin, fthain, stable Hi Christoph, Am 15.06.2023 um 17:53 schrieb Christoph Hellwig: > On Thu, Jun 15, 2023 at 04:50:45PM +1200, Michael Schmitz wrote: >>> And as far as I can tell everything that is a __u32 here should >>> be an __be32 because it is a big endian on-disk format. Why >>> would you change only a single field? >> >> Because that's all I needed, and wanted to avoid excess patch churn. Plus >> (appeal to authority here :-)) it's in keeping with what Al Viro did when >> the __be32 annotations were first added. >> >> I can change all __u32 to __be32 and drop the comment if that's preferred. > > That would be great! OK, will respin tomorrow. Cheers, Michael ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v10 2/3] block: change annotation of rdb_CylBlocks in affs_hardblocks.h 2023-06-15 5:53 ` Christoph Hellwig 2023-06-15 7:09 ` Michael Schmitz @ 2023-06-15 7:21 ` Geert Uytterhoeven 2023-06-15 19:53 ` Michael Schmitz 1 sibling, 1 reply; 15+ messages in thread From: Geert Uytterhoeven @ 2023-06-15 7:21 UTC (permalink / raw) To: Michael Schmitz Cc: Christoph Hellwig, linux-block, axboe, linux-m68k, martin, fthain, stable Hi Michael, Thanks for your patch! On Thu, Jun 15, 2023 at 7:53 AM Christoph Hellwig <hch@lst.de> wrote: > On Thu, Jun 15, 2023 at 04:50:45PM +1200, Michael Schmitz wrote: > >> And as far as I can tell everything that is a __u32 here should > >> be an __be32 because it is a big endian on-disk format. Why > >> would you change only a single field? > > > > Because that's all I needed, and wanted to avoid excess patch churn. Plus > > (appeal to authority here :-)) it's in keeping with what Al Viro did when > > the __be32 annotations were first added. > > > > I can change all __u32 to __be32 and drop the comment if that's preferred. > > That would be great! I totally agree with Christoph. 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] 15+ messages in thread
* Re: [PATCH v10 2/3] block: change annotation of rdb_CylBlocks in affs_hardblocks.h 2023-06-15 7:21 ` Geert Uytterhoeven @ 2023-06-15 19:53 ` Michael Schmitz 2023-06-16 5:48 ` Christoph Hellwig 0 siblings, 1 reply; 15+ messages in thread From: Michael Schmitz @ 2023-06-15 19:53 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Christoph Hellwig, linux-block, axboe, linux-m68k, martin, fthain, stable Hi Geert, On 15/06/23 19:21, Geert Uytterhoeven wrote: > Hi Michael, > > Thanks for your patch! > > On Thu, Jun 15, 2023 at 7:53 AM Christoph Hellwig <hch@lst.de> wrote: >> On Thu, Jun 15, 2023 at 04:50:45PM +1200, Michael Schmitz wrote: >>>> And as far as I can tell everything that is a __u32 here should >>>> be an __be32 because it is a big endian on-disk format. Why >>>> would you change only a single field? >>> Because that's all I needed, and wanted to avoid excess patch churn. Plus >>> (appeal to authority here :-)) it's in keeping with what Al Viro did when >>> the __be32 annotations were first added. >>> >>> I can change all __u32 to __be32 and drop the comment if that's preferred. >> That would be great! > I totally agree with Christoph. Thanks - now there's two __s32 fields in that header - one checksum each for RDB and PB. No one has so far seen the need for a 'signed big endian 32 bit' type, and I'd rather avoid adding one to types.h. I'll leave those as they are (with the tacit understanding that they are equally meant to be big endian). Cheers, Michael > Gr{oetje,eeting}s, > > Geert > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v10 2/3] block: change annotation of rdb_CylBlocks in affs_hardblocks.h 2023-06-15 19:53 ` Michael Schmitz @ 2023-06-16 5:48 ` Christoph Hellwig 2023-06-16 7:20 ` Michael Schmitz 0 siblings, 1 reply; 15+ messages in thread From: Christoph Hellwig @ 2023-06-16 5:48 UTC (permalink / raw) To: Michael Schmitz Cc: Geert Uytterhoeven, Christoph Hellwig, linux-block, axboe, linux-m68k, martin, fthain, stable On Fri, Jun 16, 2023 at 07:53:11AM +1200, Michael Schmitz wrote: > Thanks - now there's two __s32 fields in that header - one checksum each > for RDB and PB. No one has so far seen the need for a 'signed big endian 32 > bit' type, and I'd rather avoid adding one to types.h. I'll leave those as > they are (with the tacit understanding that they are equally meant to be > big endian). We have those in a few other pleases and store them as __be32 as well. The (implicit) cast to s32 will make them signed again. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v10 2/3] block: change annotation of rdb_CylBlocks in affs_hardblocks.h 2023-06-16 5:48 ` Christoph Hellwig @ 2023-06-16 7:20 ` Michael Schmitz 2023-06-16 7:25 ` Christoph Hellwig 2023-06-16 7:28 ` Geert Uytterhoeven 0 siblings, 2 replies; 15+ messages in thread From: Michael Schmitz @ 2023-06-16 7:20 UTC (permalink / raw) To: Christoph Hellwig Cc: Geert Uytterhoeven, linux-block, axboe, linux-m68k, martin, fthain, stable Hi Christoph, Am 16.06.2023 um 17:48 schrieb Christoph Hellwig: > On Fri, Jun 16, 2023 at 07:53:11AM +1200, Michael Schmitz wrote: >> Thanks - now there's two __s32 fields in that header - one checksum each >> for RDB and PB. No one has so far seen the need for a 'signed big endian 32 >> bit' type, and I'd rather avoid adding one to types.h. I'll leave those as >> they are (with the tacit understanding that they are equally meant to be >> big endian). > > We have those in a few other pleases and store them as __be32 as well. The > (implicit) cast to s32 will make them signed again. Where's that cast to s32 hidden? I've only seen #define __be32_to_cpu(x) ((__force __u32)(__be32)(x)) which would make the checksums unsigned if __be32 was used. Whether the checksum code uses signed or unsigned math would require inspection of the Amiga partitioning tool source which I don't have, so I've kept __s32 to be safe. Thanks for reviewing v11! Cheers, Michael ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v10 2/3] block: change annotation of rdb_CylBlocks in affs_hardblocks.h 2023-06-16 7:20 ` Michael Schmitz @ 2023-06-16 7:25 ` Christoph Hellwig 2023-06-16 7:51 ` Michael Schmitz 2023-06-16 7:28 ` Geert Uytterhoeven 1 sibling, 1 reply; 15+ messages in thread From: Christoph Hellwig @ 2023-06-16 7:25 UTC (permalink / raw) To: Michael Schmitz Cc: Christoph Hellwig, Geert Uytterhoeven, linux-block, axboe, linux-m68k, martin, fthain, stable On Fri, Jun 16, 2023 at 07:20:52PM +1200, Michael Schmitz wrote: > Am 16.06.2023 um 17:48 schrieb Christoph Hellwig: >> On Fri, Jun 16, 2023 at 07:53:11AM +1200, Michael Schmitz wrote: >>> Thanks - now there's two __s32 fields in that header - one checksum each >>> for RDB and PB. No one has so far seen the need for a 'signed big endian 32 >>> bit' type, and I'd rather avoid adding one to types.h. I'll leave those as >>> they are (with the tacit understanding that they are equally meant to be >>> big endian). >> >> We have those in a few other pleases and store them as __be32 as well. The >> (implicit) cast to s32 will make them signed again. > > Where's that cast to s32 hidden? I've only seen > > #define __be32_to_cpu(x) ((__force __u32)(__be32)(x)) > > which would make the checksums unsigned if __be32 was used. > > Whether the checksum code uses signed or unsigned math would require > inspection of the Amiga partitioning tool source which I don't have, so > I've kept __s32 to be safe. Well, the return value of be32_to_cpu is going to be a assigned to a, presumably signed, variable. With that you get an implicit cast. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v10 2/3] block: change annotation of rdb_CylBlocks in affs_hardblocks.h 2023-06-16 7:25 ` Christoph Hellwig @ 2023-06-16 7:51 ` Michael Schmitz 0 siblings, 0 replies; 15+ messages in thread From: Michael Schmitz @ 2023-06-16 7:51 UTC (permalink / raw) To: Christoph Hellwig Cc: Geert Uytterhoeven, linux-block, axboe, linux-m68k, martin, fthain, stable Hi Christoph, Am 16.06.2023 um 19:25 schrieb Christoph Hellwig: >>> We have those in a few other pleases and store them as __be32 as well. The >>> (implicit) cast to s32 will make them signed again. >> >> Where's that cast to s32 hidden? I've only seen >> >> #define __be32_to_cpu(x) ((__force __u32)(__be32)(x)) >> >> which would make the checksums unsigned if __be32 was used. >> >> Whether the checksum code uses signed or unsigned math would require >> inspection of the Amiga partitioning tool source which I don't have, so >> I've kept __s32 to be safe. > > Well, the return value of be32_to_cpu is going to be a assigned to a, > presumably signed, variable. With that you get an implicit cast. I see - as Geert explained, signed or unsigned doesn't matter for the type of checksum algorithm used in this case, so there is no potential for confusion once you think about it (which I didn't, obviously). Cheers, Michael ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v10 2/3] block: change annotation of rdb_CylBlocks in affs_hardblocks.h 2023-06-16 7:20 ` Michael Schmitz 2023-06-16 7:25 ` Christoph Hellwig @ 2023-06-16 7:28 ` Geert Uytterhoeven 2023-06-16 7:45 ` Michael Schmitz 1 sibling, 1 reply; 15+ messages in thread From: Geert Uytterhoeven @ 2023-06-16 7:28 UTC (permalink / raw) To: Michael Schmitz Cc: Christoph Hellwig, linux-block, axboe, linux-m68k, martin, fthain, stable Hi Michael, On Fri, Jun 16, 2023 at 9:21 AM Michael Schmitz <schmitzmic@gmail.com> wrote: > Am 16.06.2023 um 17:48 schrieb Christoph Hellwig: > > On Fri, Jun 16, 2023 at 07:53:11AM +1200, Michael Schmitz wrote: > >> Thanks - now there's two __s32 fields in that header - one checksum each > >> for RDB and PB. No one has so far seen the need for a 'signed big endian 32 > >> bit' type, and I'd rather avoid adding one to types.h. I'll leave those as > >> they are (with the tacit understanding that they are equally meant to be > >> big endian). > > > > We have those in a few other pleases and store them as __be32 as well. The > > (implicit) cast to s32 will make them signed again. > > Where's that cast to s32 hidden? I've only seen > > #define __be32_to_cpu(x) ((__force __u32)(__be32)(x)) > > which would make the checksums unsigned if __be32 was used. > > Whether the checksum code uses signed or unsigned math would require > inspection of the Amiga partitioning tool source which I don't have, so > I've kept __s32 to be safe. Unsurprisingly, block/partitions/amiga.c:checksum_block() calculates a checksum over __be32 words. The actual signedness of the checksum field doesn't matter much[*], as using two-complement numbers, you can just assign a signed value to an unsigned field. It should definitely be __be32. [*] I guess it was made signed because the procedure to update the check goes like this: 1. set checksum field to zero, 2. calculate sum, 3. store negated sum in checksum field. 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] 15+ messages in thread
* Re: [PATCH v10 2/3] block: change annotation of rdb_CylBlocks in affs_hardblocks.h 2023-06-16 7:28 ` Geert Uytterhoeven @ 2023-06-16 7:45 ` Michael Schmitz 0 siblings, 0 replies; 15+ messages in thread From: Michael Schmitz @ 2023-06-16 7:45 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Christoph Hellwig, linux-block, axboe, linux-m68k, martin, fthain, stable Hi Geert, Am 16.06.2023 um 19:28 schrieb Geert Uytterhoeven: > Hi Michael, > > On Fri, Jun 16, 2023 at 9:21 AM Michael Schmitz <schmitzmic@gmail.com> wrote: >> Am 16.06.2023 um 17:48 schrieb Christoph Hellwig: >>> On Fri, Jun 16, 2023 at 07:53:11AM +1200, Michael Schmitz wrote: >>>> Thanks - now there's two __s32 fields in that header - one checksum each >>>> for RDB and PB. No one has so far seen the need for a 'signed big endian 32 >>>> bit' type, and I'd rather avoid adding one to types.h. I'll leave those as >>>> they are (with the tacit understanding that they are equally meant to be >>>> big endian). >>> >>> We have those in a few other pleases and store them as __be32 as well. The >>> (implicit) cast to s32 will make them signed again. >> >> Where's that cast to s32 hidden? I've only seen >> >> #define __be32_to_cpu(x) ((__force __u32)(__be32)(x)) >> >> which would make the checksums unsigned if __be32 was used. >> >> Whether the checksum code uses signed or unsigned math would require >> inspection of the Amiga partitioning tool source which I don't have, so >> I've kept __s32 to be safe. > > Unsurprisingly, block/partitions/amiga.c:checksum_block() calculates > a checksum over __be32 words. The actual signedness of the checksum > field doesn't matter much[*], as using two-complement numbers, you can > just assign a signed value to an unsigned field. > It should definitely be __be32. > > [*] I guess it was made signed because the procedure to update the > check goes like this: > 1. set checksum field to zero, > 2. calculate sum, > 3. store negated sum in checksum field. Thanks, that explains why the result of checksum_block() is tested against zero. Makes sense now. Will fix in v12 ... Cheers, Michael > > Gr{oetje,eeting}s, > > Geert > ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v10 3/3] block: add overflow checks for Amiga partition support [not found] <20230615030837.8518-1-schmitzmic@gmail.com> 2023-06-15 3:08 ` [PATCH v10 1/3] block: fix signed int overflow in Amiga partition support Michael Schmitz 2023-06-15 3:08 ` [PATCH v10 2/3] block: change annotation of rdb_CylBlocks in affs_hardblocks.h Michael Schmitz @ 2023-06-15 3:08 ` Michael Schmitz 2 siblings, 0 replies; 15+ messages in thread From: Michael Schmitz @ 2023-06-15 3:08 UTC (permalink / raw) To: linux-block, axboe Cc: linux-m68k, geert, hch, martin, fthain, Michael Schmitz, stable 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 patch 1 in this series). This patch adds additional error checking and warning messages. Reported-by: Martin Steigerwald <Martin@lichtvoll.de> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=43511 Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") Message-ID: <201206192146.09327.Martin@lichtvoll.de> Cc: <stable@vger.kernel.org> # 5.2 Signed-off-by: Michael Schmitz <schmitzmic@gmail.com> Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org> Reviewed-by: Christoph Hellwig <hch@infradead.org> --- 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() Changes from v5: Geert Uytterhoeven: - correct ineffective u64 cast to avoid 32 bit mult. overflow - fix mult. overflow in partition block address calculation Changes from v6: Geert Uytterhoeven: - don't fail hard on partition block address overflow Changes from v7: - replace bdevname(state->bdev, b) by state->disk->disk_name - drop warn_no_part conditionals - remove remaining warn_no_part Changes from v8: Christoph Hellwig: - whitespace fix, drop unnecessary u64 casts kbuild warning: - sparse warning fix Changes from v9: - revert ineffective sparse warning fix, and rely on change to annotation of rdb_CylBlocks (patch 2 this series) instead. - add Fixes: tags and stable backport prereq --- block/partitions/amiga.c | 103 ++++++++++++++++++++++++++++++++------- 1 file changed, 85 insertions(+), 18 deletions(-) diff --git a/block/partitions/amiga.c b/block/partitions/amiga.c index 85c5c79aae48..ed222b9c901b 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,7 +52,7 @@ 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", + pr_err("Dev %s: unable to read RDB block %llu\n", state->disk->disk_name, blk); res = -1; goto rdb_done; @@ -58,12 +69,12 @@ 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", + pr_err("Trashed word at 0xd0 in block %llu ignored in checksum calculation\n", blk); break; } - pr_err("Dev %s: RDB in block %d has bad checksum\n", + pr_err("Dev %s: RDB in block %llu has bad checksum\n", state->disk->disk_name, blk); } @@ -80,10 +91,15 @@ 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, blk, part); + break; + } data = read_part_sector(state, blk, §); if (!data) { - pr_err("Dev %s: unable to read partition block %d\n", + pr_err("Dev %s: unable to read partition block %llu\n", state->disk->disk_name, 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, 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] 15+ messages in thread
end of thread, other threads:[~2023-06-16 7:51 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20230615030837.8518-1-schmitzmic@gmail.com>
2023-06-15 3:08 ` [PATCH v10 1/3] block: fix signed int overflow in Amiga partition support Michael Schmitz
2023-06-15 3:08 ` [PATCH v10 2/3] block: change annotation of rdb_CylBlocks in affs_hardblocks.h Michael Schmitz
2023-06-15 4:17 ` Christoph Hellwig
2023-06-15 4:50 ` Michael Schmitz
2023-06-15 5:53 ` Christoph Hellwig
2023-06-15 7:09 ` Michael Schmitz
2023-06-15 7:21 ` Geert Uytterhoeven
2023-06-15 19:53 ` Michael Schmitz
2023-06-16 5:48 ` Christoph Hellwig
2023-06-16 7:20 ` Michael Schmitz
2023-06-16 7:25 ` Christoph Hellwig
2023-06-16 7:51 ` Michael Schmitz
2023-06-16 7:28 ` Geert Uytterhoeven
2023-06-16 7:45 ` Michael Schmitz
2023-06-15 3:08 ` [PATCH v10 3/3] block: add overflow checks for Amiga partition support Michael Schmitz
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox