Hi Geert,
Am 13.10.2018 um 20:02 schrieb Geert Uytterhoeven:
Hi Michael,
On Sat, Oct 13, 2018 at 4:23 AM Michael Schmitz <schmitzmic@xxxxxxxxx> wrote:
Am 12.10.2018 um 21:54 schrieb Geert Uytterhoeven:
Thanks for being persistent!
BTW, there's another possible overflow in "blk *= blksize", but that one
is very unlikely to happen, as most (all?) partitioners store partition
blocks close to the beginning of the disk.
Thanks - we can at least change the type of blk to sector_t to limit the
potential for multiplication overflow, but with RDB_ALLOCATION_LIMIT at
16, we would miss RDB partition blocks not at the very beginning of a
disk anyway.
I thought so, too, but then realized that RDB_ALLOCATION_LIMIT applies to
the RDB block itself, not to the partition blocks.
Right, I now see. blk should be sector_t anyway (which is no help
without LBD support, hm). Better fix that.
--- a/block/partitions/amiga.c
+++ b/block/partitions/amiga.c
@@ -32,9 +40,12 @@ int amiga_partition(struct parsed_partitions *state)
unsigned char *data;
struct RigidDiskBlock *rdb;
struct PartitionBlock *pb;
- sector_t start_sect, nr_sects;
+ u64 start_sect, nr_sects;
+ sector_t end_sect;
+ u32 cylblk; /* rdb_CylBlocks = nr_heads*sect_per_track */
+ u32 nr_hd, nr_sect, lo_cyl, hi_cyl;
int blk, part, res = 0;
- int blksize = 1; /* Multiplier for disk block size */
+ unsigned int blksize = 1; /* Multiplier for disk block size */
int slot = 1;
char b[BDEVNAME_SIZE];
@@ -99,19 +110,70 @@ int amiga_partition(struct parsed_partitions *state)
if (checksum_block((__be32 *)pb, be32_to_cpu(pb->pb_SummedLongs) & 0x7F) != 0 )
continue;
- /* Tell Kernel about it */
+ /* RDB gives us more than enough rope to hang ourselves with,
+ * many times over (2^128 bytes if all fields max out).
+ * Some careful checks are in order, so check for potential
+ * overflows.
+ * We are multiplying four 32 bit numbers to one sector_t!
+ */
+
+ nr_hd = be32_to_cpu(pb->pb_Environment[NR_HD]);
+ nr_sect = be32_to_cpu(pb->pb_Environment[NR_SECT]);
+
+ /* CylBlocks is total number of blocks per cylinder */
+ if (check_mul_overflow(nr_hd, nr_sect, &cylblk)) {
+ pr_err("Dev %s: heads*sects %u overflows u32, skipping partition!\n",
+ bdevname(state->bdev, b), cylblk);
+ continue;
+ }
+
+ /* check for consistency with RDB defined CylBlocks */
+ if (cylblk > be32_to_cpu(rdb->rdb_CylBlocks)) {
+ pr_warn("Dev %s: cylblk %u > rdb_CylBlocks %u!\n",
+ bdevname(state->bdev, b), cylblk,
+ be32_to_cpu(rdb->rdb_CylBlocks));
+ }
+
+ /* RDB allows for variable logical block size -
+ * normalize to 512 byte blocks and check result.
+ */
+
+ if (check_mul_overflow(cylblk, blksize, &cylblk)) {
+ pr_err("Dev %s: partition %u bytes per cyl. overflows u32, skipping partition!\n",
+ bdevname(state->bdev, b), part);
Unlike the comparison with 32-bit rdb_CylBlocks above, this is an
artificial limitation, right?
You can relax it by using 64-bit arithmetic, but that would complicate the
calculation of start_sect and nr_sects below, as they may overflow 64-bit.
Correct - this allows me to skip the overflow check on the final result
(see comment below). But making cylblk a 32 bit type for the purpose of
this overflow check has tripped me up below.
I'd still like to retain this check - it is highly unlikely to ever
trigger with RDB blocks in current use, and should disks ever get so
large as to require the total number of 512 byte blocks per cylinder to
exceed the 32 bit limit, I'd certainly hope other partition table
options get chosen.
Fair enough.
Thanks.
Cheers,
Michael
+ if (check_add_overflow(start_sect, nr_sects, &end_sect)) {
+ pr_err("Dev %s: partition %u (%llu-%llu) needs LBD device support, skipping partition!\n",
+ bdevname(state->bdev, b), part,
+ start_sect, (u64) end_sect);
The cast to u64 is not needed.
It is, in case the kernel is compiled without LBD support (making
end_sect a 32 bit sector_t). I haven't found a better way to check
whether the partition exceeds what's possible to represent without LBD
support.
You're right. I missed that end_sect was not converted to u64.
Gr{oetje,eeting}s,
Geert
