[PATCH v13 1/3] block: fix signed int overflow in Amiga partition support

[PATCH v13 1/3] block: fix signed int overflow in Amiga partition support

[Michael Schmitz]

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

Use sector_t as type for sector address and size to allow using disks
up to 2 TB without LBD support, and disks larger than 2 TB with LBD.

This bug was reported originally in 2012, and the fix was created by
the RDB author, Joanne Dow <jdow@earthlink.net>. A patch had been
discussed and reviewed on linux-m68k at that time but never officially
submitted. This patch differs from Joanne's patch only in its use of
sector_t instead of unsigned int. No checking for overflows is done
(see patch 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

[PATCH v13 2/3] block: change all __u32 annotations to __be32 in affs_hardblocks.h

[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.

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 all 32 bit fields in affs_hardblocks.h as __be32, as the
on-disk format of RDB and partition blocks is always big endian.

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: Christoph Hellwig <hch@lst.de>
Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>

---

Changes from v10:

Christoph Hellwig:
- change annotation of all __u32 fields to __be32

Changes from v11:

Geert Uytterhoeven:
- also change annotation of the two __s32 checksum fields

Changes from v12:

- change patch subject to reflect changes in v11 and v12
---
 include/uapi/linux/affs_hardblocks.h | 68 ++++++++++++++--------------
 1 file changed, 34 insertions(+), 34 deletions(-)

diff --git a/include/uapi/linux/affs_hardblocks.h b/include/uapi/linux/affs_hardblocks.h
index 5e2fb8481252..a5aff2eb5f70 100644
--- a/include/uapi/linux/affs_hardblocks.h
+++ b/include/uapi/linux/affs_hardblocks.h
@@ -7,42 +7,42 @@
 /* Just the needed definitions for the RDB of an Amiga HD. */
 
 struct RigidDiskBlock {
-	__u32	rdb_ID;
+	__be32	rdb_ID;
 	__be32	rdb_SummedLongs;
-	__s32	rdb_ChkSum;
-	__u32	rdb_HostID;
+	__be32	rdb_ChkSum;
+	__be32	rdb_HostID;
 	__be32	rdb_BlockBytes;
-	__u32	rdb_Flags;
-	__u32	rdb_BadBlockList;
+	__be32	rdb_Flags;
+	__be32	rdb_BadBlockList;
 	__be32	rdb_PartitionList;
-	__u32	rdb_FileSysHeaderList;
-	__u32	rdb_DriveInit;
-	__u32	rdb_Reserved1[6];
-	__u32	rdb_Cylinders;
-	__u32	rdb_Sectors;
-	__u32	rdb_Heads;
-	__u32	rdb_Interleave;
-	__u32	rdb_Park;
-	__u32	rdb_Reserved2[3];
-	__u32	rdb_WritePreComp;
-	__u32	rdb_ReducedWrite;
-	__u32	rdb_StepRate;
-	__u32	rdb_Reserved3[5];
-	__u32	rdb_RDBBlocksLo;
-	__u32	rdb_RDBBlocksHi;
-	__u32	rdb_LoCylinder;
-	__u32	rdb_HiCylinder;
-	__u32	rdb_CylBlocks;
-	__u32	rdb_AutoParkSeconds;
-	__u32	rdb_HighRDSKBlock;
-	__u32	rdb_Reserved4;
+	__be32	rdb_FileSysHeaderList;
+	__be32	rdb_DriveInit;
+	__be32	rdb_Reserved1[6];
+	__be32	rdb_Cylinders;
+	__be32	rdb_Sectors;
+	__be32	rdb_Heads;
+	__be32	rdb_Interleave;
+	__be32	rdb_Park;
+	__be32	rdb_Reserved2[3];
+	__be32	rdb_WritePreComp;
+	__be32	rdb_ReducedWrite;
+	__be32	rdb_StepRate;
+	__be32	rdb_Reserved3[5];
+	__be32	rdb_RDBBlocksLo;
+	__be32	rdb_RDBBlocksHi;
+	__be32	rdb_LoCylinder;
+	__be32	rdb_HiCylinder;
+	__be32	rdb_CylBlocks;
+	__be32	rdb_AutoParkSeconds;
+	__be32	rdb_HighRDSKBlock;
+	__be32	rdb_Reserved4;
 	char	rdb_DiskVendor[8];
 	char	rdb_DiskProduct[16];
 	char	rdb_DiskRevision[4];
 	char	rdb_ControllerVendor[8];
 	char	rdb_ControllerProduct[16];
 	char	rdb_ControllerRevision[4];
-	__u32	rdb_Reserved5[10];
+	__be32	rdb_Reserved5[10];
 };
 
 #define	IDNAME_RIGIDDISK	0x5244534B	/* "RDSK" */
@@ -50,16 +50,16 @@ struct RigidDiskBlock {
 struct PartitionBlock {
 	__be32	pb_ID;
 	__be32	pb_SummedLongs;
-	__s32	pb_ChkSum;
-	__u32	pb_HostID;
+	__be32	pb_ChkSum;
+	__be32	pb_HostID;
 	__be32	pb_Next;
-	__u32	pb_Flags;
-	__u32	pb_Reserved1[2];
-	__u32	pb_DevFlags;
+	__be32	pb_Flags;
+	__be32	pb_Reserved1[2];
+	__be32	pb_DevFlags;
 	__u8	pb_DriveName[32];
-	__u32	pb_Reserved2[15];
+	__be32	pb_Reserved2[15];
 	__be32	pb_Environment[17];
-	__u32	pb_EReserved[15];
+	__be32	pb_EReserved[15];
 };
 
 #define	IDNAME_PARTITION	0x50415254	/* "PART" */
-- 
2.17.1

[PATCH v13 3/3] block: add overflow checks for Amiga partition support

[Michael Schmitz]

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

Use u64 as type for sector address and size to allow using disks up to
2 TB without LBD support, and disks larger than 2 TB with LBD. The RBD
format allows to specify disk sizes up to 2^128 bytes (though native
OS limitations reduce this somewhat, to max 2^68 bytes), so check for
u64 overflow carefully to protect against overflowing sector_t.

Bail out if sector addresses overflow 32 bits on kernels without LBD
support.

This bug was reported originally in 2012, and the fix was created by
the RDB author, Joanne Dow <jdow@earthlink.net>. A patch had been
discussed and reviewed on linux-m68k at that time but never officially
submitted (now resubmitted as 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, &sect);
 		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, &sect);
 		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

[PATCH v4 1/1] block: bugfix for Amiga partition overflow check patch

[Michael Schmitz]

Making 'blk' sector_t (i.e. 64 bit if LBD support is active)
fails the 'blk>0' test in the partition block loop if a
value of (signed int) -1 is used to mark the end of the
partition block list.

This bug was introduced in patch 3 of my prior Amiga partition
support fixes series, and spotted by Christian Zigotzky when
testing the latest block updates.

Explicitly cast 'blk' to signed int to allow use of -1 to
terminate the partition block linked list.

Reported-by: Christian Zigotzky <chzigotzky@xenosoft.de>
Fixes: b6f3f28f60 ("block: add overflow checks for Amiga partition support")
Message-ID: 024ce4fa-cc6d-50a2-9aae-3701d0ebf668@xenosoft.de
Cc: <stable@vger.kernel.org> # 5.2
Link: https://lore.kernel.org/r/024ce4fa-cc6d-50a2-9aae-3701d0ebf668@xenosoft.de
Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>
Reviewed-by: Martin Steigerwald <martin@lichtvoll.de>
Tested-by: Christian Zigotzky <chzigotzky@xenosoft.de>

--

Changes since v1:

- corrected Fixes: tag
- added Tested-by:
- reworded commit message to describe filesystem partition
  size mismatch problem

Changes since v2:

Adrian Glaubitz:
- fix typo in commit message

Changes since v3:

Greg KH:
- fix stable tag

Geert Uytterhoeven:
- revert changes to commit message since v1
---
 block/partitions/amiga.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/partitions/amiga.c b/block/partitions/amiga.c
index ed222b9c901b..506921095412 100644
--- a/block/partitions/amiga.c
+++ b/block/partitions/amiga.c
@@ -90,7 +90,7 @@ 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)) {
+	for (part = 1; (s32) blk>0 && part<=16; part++, put_dev_sector(sect)) {
 		/* 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",
-- 
2.17.1

Re: [PATCH v4 1/1] block: bugfix for Amiga partition overflow check patch

[Geert Uytterhoeven]

On Wed, Jul 5, 2023 at 1:38 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
> Making 'blk' sector_t (i.e. 64 bit if LBD support is active)
> fails the 'blk>0' test in the partition block loop if a
> value of (signed int) -1 is used to mark the end of the
> partition block list.
>
> This bug was introduced in patch 3 of my prior Amiga partition
> support fixes series, and spotted by Christian Zigotzky when
> testing the latest block updates.
>
> Explicitly cast 'blk' to signed int to allow use of -1 to
> terminate the partition block linked list.
>
> Reported-by: Christian Zigotzky <chzigotzky@xenosoft.de>
> Fixes: b6f3f28f60 ("block: add overflow checks for Amiga partition support")
> Message-ID: 024ce4fa-cc6d-50a2-9aae-3701d0ebf668@xenosoft.de

Please drop this line.

> Cc: <stable@vger.kernel.org> # 5.2
> Link: https://lore.kernel.org/r/024ce4fa-cc6d-50a2-9aae-3701d0ebf668@xenosoft.de
> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>
> Reviewed-by: Martin Steigerwald <martin@lichtvoll.de>
> Tested-by: Christian Zigotzky <chzigotzky@xenosoft.de>

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

Gr{oetje,eeting}s,

                        Geert

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

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v4 1/1] block: bugfix for Amiga partition overflow check patch

[Michael Schmitz]

Hi Geert,

thanks for the review!

Am 05.07.2023 um 19:28 schrieb Geert Uytterhoeven:
> On Wed, Jul 5, 2023 at 1:38 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
>> Making 'blk' sector_t (i.e. 64 bit if LBD support is active)
>> fails the 'blk>0' test in the partition block loop if a
>> value of (signed int) -1 is used to mark the end of the
>> partition block list.
>>
>> This bug was introduced in patch 3 of my prior Amiga partition
>> support fixes series, and spotted by Christian Zigotzky when
>> testing the latest block updates.
>>
>> Explicitly cast 'blk' to signed int to allow use of -1 to
>> terminate the partition block linked list.
>>
>> Reported-by: Christian Zigotzky <chzigotzky@xenosoft.de>
>> Fixes: b6f3f28f60 ("block: add overflow checks for Amiga partition support")
>> Message-ID: 024ce4fa-cc6d-50a2-9aae-3701d0ebf668@xenosoft.de
>
> Please drop this line.

Because it's redundant, as I've also used Link:?

Cheers,

	Michael


>
>> Cc: <stable@vger.kernel.org> # 5.2
>> Link: https://lore.kernel.org/r/024ce4fa-cc6d-50a2-9aae-3701d0ebf668@xenosoft.de
>> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>
>> Reviewed-by: Martin Steigerwald <martin@lichtvoll.de>
>> Tested-by: Christian Zigotzky <chzigotzky@xenosoft.de>
>
> Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>
>
> Gr{oetje,eeting}s,
>
>                         Geert
>

Re: [PATCH v4 1/1] block: bugfix for Amiga partition overflow check patch

[Geert Uytterhoeven]

Hi Michael,

On Wed, Jul 5, 2023 at 10:53 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
> Am 05.07.2023 um 19:28 schrieb Geert Uytterhoeven:
> > On Wed, Jul 5, 2023 at 1:38 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
> >> Making 'blk' sector_t (i.e. 64 bit if LBD support is active)
> >> fails the 'blk>0' test in the partition block loop if a
> >> value of (signed int) -1 is used to mark the end of the
> >> partition block list.
> >>
> >> This bug was introduced in patch 3 of my prior Amiga partition
> >> support fixes series, and spotted by Christian Zigotzky when
> >> testing the latest block updates.
> >>
> >> Explicitly cast 'blk' to signed int to allow use of -1 to
> >> terminate the partition block linked list.
> >>
> >> Reported-by: Christian Zigotzky <chzigotzky@xenosoft.de>
> >> Fixes: b6f3f28f60 ("block: add overflow checks for Amiga partition support")
> >> Message-ID: 024ce4fa-cc6d-50a2-9aae-3701d0ebf668@xenosoft.de
> >
> > Please drop this line.
>
> Because it's redundant, as I've also used Link:?

(That, too ;-)

Because the use of the Message-ID: tag in patches is not documented.
IIRC, it might also cause issues when applying, as the downloaded patch
will appear to have two Message-IDs.
I'm not sure the sample git hook in Documentation/maintainer/configure-git.rst
(and all variants the various maintainers are using) handles this correctly.

Gr{oetje,eeting}s,

                        Geert

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

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v4 1/1] block: bugfix for Amiga partition overflow check patch

[Michael Schmitz]

Hi Geert,

On 5/07/23 21:08, Geert Uytterhoeven wrote:
>
>>>> Fixes: b6f3f28f60 ("block: add overflow checks for Amiga partition support")
>>>> Message-ID: 024ce4fa-cc6d-50a2-9aae-3701d0ebf668@xenosoft.de
>>> Please drop this line.
>> Because it's redundant, as I've also used Link:?
> (That, too ;-)
>
> Because the use of the Message-ID: tag in patches is not documented.

Now I wonder where I picked up that habit ...

> IIRC, it might also cause issues when applying, as the downloaded patch
> will appear to have two Message-IDs.
That's correct (if you refer to a patch in mbox format), but from the 
context of the two Message-ID lines, it ought to be clear which one 
matters.
> I'm not sure the sample git hook in Documentation/maintainer/configure-git.rst
> (and all variants the various maintainers are using) handles this correctly.

You're right, it won't check for context there.

In this particular instance, it won't use my Message-ID tag anyway 
though (forgot the angle brackets).

I'll fix that in v5 later.

Cheers,

     Michael


>
> Gr{oetje,eeting}s,
>
>                          Geert
>

Re: [PATCH v4 1/1] block: bugfix for Amiga partition overflow check patch

[Jens Axboe]

On 7/4/23 5:38?PM, Michael Schmitz wrote:
> Reported-by: Christian Zigotzky <chzigotzky@xenosoft.de>
> Fixes: b6f3f28f60 ("block: add overflow checks for Amiga partition support")
> Cc: <stable@vger.kernel.org> # 5.2

This is confusing - it's being marked for stable, but also labeled as
fixing a commit that isn't even a release yet?

-- 
Jens Axboe

Re: [PATCH v4 1/1] block: bugfix for Amiga partition overflow check patch

[Michael Schmitz]

Hi Jens,

On 6/07/23 08:42, Jens Axboe wrote:
> On 7/4/23 5:38?PM, Michael Schmitz wrote:
>> Reported-by: Christian Zigotzky <chzigotzky@xenosoft.de>
>> Fixes: b6f3f28f60 ("block: add overflow checks for Amiga partition support")
>> Cc: <stable@vger.kernel.org> # 5.2
> This is confusing - it's being marked for stable, but also labeled as
> fixing a commit that isn't even a release yet?

True - but as you had pointed out, the commit this fixes is set in 
stone. How do we ensure this bugfix is picked up as well when the other 
patches are backported? Does that  happen automatically, or do I need to 
add a Link: tag to the patch being fixed?

(Greg didn't seem to object to the Fixes: as such, just to the incorrect 
version prereq)

Cheers,

     Michael

Re: [PATCH v4 1/1] block: bugfix for Amiga partition overflow check patch

[Jens Axboe]

On 7/5/23 3:41?PM, Michael Schmitz wrote:
> Hi Jens,
> 
> On 6/07/23 08:42, Jens Axboe wrote:
>> On 7/4/23 5:38?PM, Michael Schmitz wrote:
>>> Reported-by: Christian Zigotzky <chzigotzky@xenosoft.de>
>>> Fixes: b6f3f28f60 ("block: add overflow checks for Amiga partition support")
>>> Cc: <stable@vger.kernel.org> # 5.2
>> This is confusing - it's being marked for stable, but also labeled as
>> fixing a commit that isn't even a release yet?
> 
> True - but as you had pointed out, the commit this fixes is set in
> stone. How do we ensure this bugfix is picked up as well when the
> other patches are backported? Does that  happen automatically, or do I
> need to add a Link: tag to the patch being fixed?

This:

Cc: <stable@vger.kernel.org> # 5.2

should be enough for it to go into stable from 5.2 and onwards.

> (Greg didn't seem to object to the Fixes: as such, just to the
> incorrect version prereq)

I think it's really confusing... A patch should only have a Fixes tag if
it's fixing a specific bug in that patch. Either it is, in which case
you would not need Cc stable at all since it's only in 6.5-rc, or it
isn't and you should just have the stable tag with 5.2+ as you already
have.

I'll apply this and remove the Fixes line, and the message id thing
too.

-- 
Jens Axboe

Re: [PATCH v4 1/1] block: bugfix for Amiga partition overflow check patch

[Michael Schmitz]

Hi Jens,

On 6/07/23 09:44, Jens Axboe wrote:
> On 7/5/23 3:41?PM, Michael Schmitz wrote:
>> Hi Jens,
>>
>> On 6/07/23 08:42, Jens Axboe wrote:
>>> On 7/4/23 5:38?PM, Michael Schmitz wrote:
>>>> Reported-by: Christian Zigotzky <chzigotzky@xenosoft.de>
>>>> Fixes: b6f3f28f60 ("block: add overflow checks for Amiga partition support")
>>>> Cc: <stable@vger.kernel.org> # 5.2
>>> This is confusing - it's being marked for stable, but also labeled as
>>> fixing a commit that isn't even a release yet?
>> True - but as you had pointed out, the commit this fixes is set in
>> stone. How do we ensure this bugfix is picked up as well when the
>> other patches are backported? Does that  happen automatically, or do I
>> need to add a Link: tag to the patch being fixed?
> This:
>
> Cc: <stable@vger.kernel.org> # 5.2
>
> should be enough for it to go into stable from 5.2 and onwards.
OK - I wasn't certain whether you wanted the Fixes or stable tag dropped.
>> (Greg didn't seem to object to the Fixes: as such, just to the
>> incorrect version prereq)
> I think it's really confusing... A patch should only have a Fixes tag if
> it's fixing a specific bug in that patch. Either it is, in which case
> you would not need Cc stable at all since it's only in 6.5-rc, or it
It is fixing a bug in b6f3f28f60. I should have checked whether the 
patch series had already gone to release, not just -rc, instead of just 
adding the stable tag out of caution.
> isn't and you should just have the stable tag with 5.2+ as you already
> have.
>
> I'll apply this and remove the Fixes line, and the message id thing
> too.

Thanks - whatever is least confusing is fine, as long as it's backported 
to stable in the end.

Won't be sending v5 then...

Cheers,

     Michael

Re: [PATCH v4 1/1] block: bugfix for Amiga partition overflow check patch

[Jens Axboe]

>> should be enough for it to go into stable from 5.2 and onwards.
> OK - I wasn't certain whether you wanted the Fixes or stable tag dropped.
>>> (Greg didn't seem to object to the Fixes: as such, just to the
>>> incorrect version prereq)
>> I think it's really confusing... A patch should only have a Fixes tag if
>> it's fixing a specific bug in that patch. Either it is, in which case
>> you would not need Cc stable at all since it's only in 6.5-rc, or it
>
> It is fixing a bug in b6f3f28f60. I should have checked whether the
> patch series had already gone to release, not just -rc, instead of
> just adding the stable tag out of caution.
But this is the confusion - if it's fixing a bug in b6f3f28f60, then why
is it marked as needing to get backported much further back, predating
that commit?

-- 
Jens Axboe

Re: [PATCH v4 1/1] block: bugfix for Amiga partition overflow check patch

[Michael Schmitz]

Hi Jens,

On 6/07/23 10:13, Jens Axboe wrote:
>>> should be enough for it to go into stable from 5.2 and onwards.
>> OK - I wasn't certain whether you wanted the Fixes or stable tag dropped.
>>>> (Greg didn't seem to object to the Fixes: as such, just to the
>>>> incorrect version prereq)
>>> I think it's really confusing... A patch should only have a Fixes tag if
>>> it's fixing a specific bug in that patch. Either it is, in which case
>>> you would not need Cc stable at all since it's only in 6.5-rc, or it
>> It is fixing a bug in b6f3f28f60. I should have checked whether the
>> patch series had already gone to release, not just -rc, instead of
>> just adding the stable tag out of caution.
> But this is the confusion - if it's fixing a bug in b6f3f28f60, then why
> is it marked as needing to get backported much further back, predating
> that commit?

I see - it doesn't need to be backported that far back _alone_. It only 
needs to be applied after  b6f3f28f60 once that one has been backported.

Cheers,

     Michael

Re: [PATCH v4 1/1] block: bugfix for Amiga partition overflow check patch

[Jens Axboe]

On 7/5/23 4:25?PM, Michael Schmitz wrote:
> Hi Jens,
> 
> On 6/07/23 10:13, Jens Axboe wrote:
>>>> should be enough for it to go into stable from 5.2 and onwards.
>>> OK - I wasn't certain whether you wanted the Fixes or stable tag dropped.
>>>>> (Greg didn't seem to object to the Fixes: as such, just to the
>>>>> incorrect version prereq)
>>>> I think it's really confusing... A patch should only have a Fixes tag if
>>>> it's fixing a specific bug in that patch. Either it is, in which case
>>>> you would not need Cc stable at all since it's only in 6.5-rc, or it
>>> It is fixing a bug in b6f3f28f60. I should have checked whether the
>>> patch series had already gone to release, not just -rc, instead of
>>> just adding the stable tag out of caution.
>> But this is the confusion - if it's fixing a bug in b6f3f28f60, then why
>> is it marked as needing to get backported much further back, predating
>> that commit?
> 
> I see - it doesn't need to be backported that far back _alone_. It
> only needs to be applied after  b6f3f28f60 once that one has been
> backported.

OK I see - I think there's some serious misunderstandings here then :-)

It sounds like it fixes a bug in b6f3f28f60 alone, and it has no
business going into stable. The commit should _just_ be marked with it
fixing that. If someone were to backport that previous series, then
their tooling or diligence should notice this dependency and this
current commit should be picked as well.

There should be no Cc: stable on this patch at all, I'll fix it up.

-- 
Jens Axboe

Re: [PATCH v4 1/1] block: bugfix for Amiga partition overflow check patch

[Jens Axboe]

On 7/5/23 4:34?PM, Jens Axboe wrote:
> On 7/5/23 4:25?PM, Michael Schmitz wrote:
>> Hi Jens,
>>
>> On 6/07/23 10:13, Jens Axboe wrote:
>>>>> should be enough for it to go into stable from 5.2 and onwards.
>>>> OK - I wasn't certain whether you wanted the Fixes or stable tag dropped.
>>>>>> (Greg didn't seem to object to the Fixes: as such, just to the
>>>>>> incorrect version prereq)
>>>>> I think it's really confusing... A patch should only have a Fixes tag if
>>>>> it's fixing a specific bug in that patch. Either it is, in which case
>>>>> you would not need Cc stable at all since it's only in 6.5-rc, or it
>>>> It is fixing a bug in b6f3f28f60. I should have checked whether the
>>>> patch series had already gone to release, not just -rc, instead of
>>>> just adding the stable tag out of caution.
>>> But this is the confusion - if it's fixing a bug in b6f3f28f60, then why
>>> is it marked as needing to get backported much further back, predating
>>> that commit?
>>
>> I see - it doesn't need to be backported that far back _alone_. It
>> only needs to be applied after  b6f3f28f60 once that one has been
>> backported.
> 
> OK I see - I think there's some serious misunderstandings here then :-)
> 
> It sounds like it fixes a bug in b6f3f28f60 alone, and it has no
> business going into stable. The commit should _just_ be marked with it
> fixing that. If someone were to backport that previous series, then
> their tooling or diligence should notice this dependency and this
> current commit should be picked as well.
> 
> There should be no Cc: stable on this patch at all, I'll fix it up.

Here's what I have:

https://git.kernel.dk/cgit/linux/commit/?h=block-6.5&id=7eb1e47696aa231b1a567846bbe3a1e1befe1854

which has the following manual edits:

1) Change the title/subject line of the patch. "bugfix for Amiga
partition overflow check patch" means very little. The fact that this
patch is a bug fix for a previous commit is explicit with the Fixes
line.

2) Break lines at 72-74 chars, yours were very short.

3) Drop message-id

4) Drop cc stable tag

5) Drop the revision history. This should be behind three '---' lines
and then it's dropped automatically.

Let's hope this is it for Amiga partition handling!

-- 
Jens Axboe

Re: [PATCH v4 1/1] block: bugfix for Amiga partition overflow check patch

[Michael Schmitz]

Hi Jens,

thanks for patching this all up!

On 6/07/23 10:38, Jens Axboe wrote:
> On 7/5/23 4:34?PM, Jens Axboe wrote:
>> On 7/5/23 4:25?PM, Michael Schmitz wrote:
>>> Hi Jens,
>>>
>>> On 6/07/23 10:13, Jens Axboe wrote:
>>>>>> should be enough for it to go into stable from 5.2 and onwards.
>>>>> OK - I wasn't certain whether you wanted the Fixes or stable tag dropped.
>>>>>>> (Greg didn't seem to object to the Fixes: as such, just to the
>>>>>>> incorrect version prereq)
>>>>>> I think it's really confusing... A patch should only have a Fixes tag if
>>>>>> it's fixing a specific bug in that patch. Either it is, in which case
>>>>>> you would not need Cc stable at all since it's only in 6.5-rc, or it
>>>>> It is fixing a bug in b6f3f28f60. I should have checked whether the
>>>>> patch series had already gone to release, not just -rc, instead of
>>>>> just adding the stable tag out of caution.
>>>> But this is the confusion - if it's fixing a bug in b6f3f28f60, then why
>>>> is it marked as needing to get backported much further back, predating
>>>> that commit?
>>> I see - it doesn't need to be backported that far back _alone_. It
>>> only needs to be applied after  b6f3f28f60 once that one has been
>>> backported.
>> OK I see - I think there's some serious misunderstandings here then :-)
That's probably understating it. I need to reread the patch submission 
notes.
>> It sounds like it fixes a bug in b6f3f28f60 alone, and it has no
>> business going into stable. The commit should _just_ be marked with it
>> fixing that. If someone were to backport that previous series, then
>> their tooling or diligence should notice this dependency and this
>> current commit should be picked as well.
>>
>> There should be no Cc: stable on this patch at all, I'll fix it up.
> Here's what I have:
>
> https://git.kernel.dk/cgit/linux/commit/?h=block-6.5&id=7eb1e47696aa231b1a567846bbe3a1e1befe1854
>
> which has the following manual edits:
>
> 1) Change the title/subject line of the patch. "bugfix for Amiga
> partition overflow check patch" means very little. The fact that this
> patch is a bug fix for a previous commit is explicit with the Fixes
> line.
>
> 2) Break lines at 72-74 chars, yours were very short.
Somehow a limit of 60 had stuck in my mind.
>
> 3) Drop message-id
>
> 4) Drop cc stable tag
>
> 5) Drop the revision history. This should be behind three '---' lines
> and then it's dropped automatically.

Argh - looks like I've made that mistake more often than not in the past 
few years. I'll remember that now.

>
> Let's hope this is it for Amiga partition handling!

I should certainly hope so!

Cheers,

     Michael

Re: [PATCH v4 1/1] block: bugfix for Amiga partition overflow check patch

[Martin Steigerwald]

Michael Schmitz - 06.07.23, 01:54:49 CEST:
> Let's hope this is it for Amiga partition handling!
> 
> I should certainly hope so!

Yeah, really. For good.

Thank you, Michael and Jens!

-- 
Martin