[v7,2/2] block: add overflow checks for Amiga partition support

Commit Message

Michael Schmitz Oct. 15, 2018, 2:32 a.m. UTC

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 <[e-mail]>. 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 <[e-mail]>
Message-ID: <[e-mail]>
Signed-off-by: Michael Schmitz <[e-mail]>
Reviewed-by: Geert Uytterhoeven <[e-mail]>

---

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
---
 block/partitions/amiga.c | 111 +++++++++++++++++++++++++++++++++++++----------
 1 file changed, 89 insertions(+), 22 deletions(-)

Comments

Martin Steigerwald July 25, 2022, 12:36 p.m. UTC | #1

Michael Schmitz - 15.10.18, 04:32:27 CEST:
> The Amiga partition parser module uses signed int for partition sector
> address and count, which will overflow for disks larger than 1 TB.

Did this go in? I did not find anything in git log. I wonder whether I 
can close the bug report mentioned below.

Best,
Martin

> 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 <[e-mail]>. 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 <[e-mail]>
> Message-ID: <[e-mail]>
> Signed-off-by: Michael Schmitz <[e-mail]>
> Reviewed-by: Geert Uytterhoeven <[e-mail]>
> 
> ---
> 
> 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
> ---
>  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 7ea9540..d1b1535 100644
> --- a/block/partitions/amiga.c
> +++ b/block/partitions/amiga.c
> @@ -11,11 +11,19 @@
>  #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"
>  #include "amiga.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)
>  {
> @@ -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;
> -	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;
>  	char b[BDEVNAME_SIZE];
> 
> @@ -44,8 +55,8 @@ int amiga_partition(struct parsed_partitions *state)
> data = read_part_sector(state, blk, &sect);
>  		if (!data) {
>  			if (warn_no_part)
> -				pr_err("Dev %s: unable to read RDB block 
%d\n",
> -				       bdevname(state->bdev, b), blk);
> +				pr_err("Dev %s: unable to read RDB block 
%llu\n",
> +				       bdevname(state->bdev, b), (u64) 
blk);
>  			res = -1;
>  			goto rdb_done;
>  		}
> @@ -61,13 +72,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",
> -		       bdevname(state->bdev, b), blk);
> +		pr_err("Dev %s: RDB in block %llu has bad checksum\n",
> +		       bdevname(state->bdev, b), (u64) blk);
>  	}
> 
>  	/* blksize is blocks per 512 byte standard block */
> @@ -83,12 +94,17 @@ 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", +				
bdevname(state->bdev, b),
> (u64) blk, part);
> +			break;
> +		}
>  		data = read_part_sector(state, blk, &sect);
>  		if (!data) {
>  			if (warn_no_part)
> -				pr_err("Dev %s: unable to read partition 
block %d\n",
> -				       bdevname(state->bdev, b), blk);
> +				pr_err("Dev %s: unable to read partition 
block %llu\n",
> +				       bdevname(state->bdev, b), (u64) 
blk);
>  			res = -1;
>  			goto rdb_done;
>  		}
> @@ -99,19 +115,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);
> +			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", +				bdevname(state->bdev, b), 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", +				bdevname(state->bdev, 
b), 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 */

Jens Axboe July 25, 2022, 11:03 p.m. UTC | #2

On 7/25/22 6:36 AM, Martin Steigerwald wrote:
> Michael Schmitz - 15.10.18, 04:32:27 CEST:
>> The Amiga partition parser module uses signed int for partition sector
>> address and count, which will overflow for disks larger than 1 TB.
> 
> Did this go in? I did not find anything in git log. I wonder whether I 
> can close the bug report mentioned below.

Looking at the history (2018!), doesn't like they ever got reviewed or
went upstream. Looks like they are still needed, however they no longer
apply as-is.

Michael Schmitz July 26, 2022, 1:53 a.m. UTC | #3

Hi Jens,

there's been quite a bit of review on this patch series back in the day 
(most of that would have been on linux-m68k IIRC; see Geert's 
Reviewed-By tag), and I addressed the issues raised but as you say, it 
did never get merged.

I've found a copy of the linux-block repo that has these patches, will 
see if I can get them updated to apply to current linux-block.

Cheers,

     Michael



On 26/07/22 11:03, Jens Axboe wrote:
> On 7/25/22 6:36 AM, Martin Steigerwald wrote:
>> Michael Schmitz - 15.10.18, 04:32:27 CEST:
>>> The Amiga partition parser module uses signed int for partition sector
>>> address and count, which will overflow for disks larger than 1 TB.
>> Did this go in? I did not find anything in git log. I wonder whether I
>> can close the bug report mentioned below.
> Looking at the history (2018!), doesn't like they ever got reviewed or
> went upstream. Looks like they are still needed, however they no longer
> apply as-is.
>

Jens Axboe July 26, 2022, 3:40 a.m. UTC | #4

On 7/25/22 7:53 PM, Michael Schmitz wrote:
> Hi Jens,
> 
> there's been quite a bit of review on this patch series back in the
> day (most of that would have been on linux-m68k IIRC; see Geert's
> Reviewed-By tag), and I addressed the issues raised but as you say, it
> did never get merged.
> 
> I've found a copy of the linux-block repo that has these patches, will
> see if I can get them updated to apply to current linux-block.

Thanks, please do resend them and we can get them applied.

Michael Schmitz July 26, 2022, 3:58 a.m. UTC | #5

Hi Jens,

Am 26.07.2022 um 15:40 schrieb Jens Axboe:
> On 7/25/22 7:53 PM, Michael Schmitz wrote:
>> Hi Jens,
>>
>> there's been quite a bit of review on this patch series back in the
>> day (most of that would have been on linux-m68k IIRC; see Geert's
>> Reviewed-By tag), and I addressed the issues raised but as you say, it
>> did never get merged.
>>
>> I've found a copy of the linux-block repo that has these patches, will
>> see if I can get them updated to apply to current linux-block.
>
> Thanks, please do resend them and we can get them applied.

Will do - running final compile tests.

Cheers,

	Michael

Martin Steigerwald Aug. 21, 2022, 8:59 p.m. UTC | #6

Michael Schmitz - 26.07.22, 05:58:40 CEST:
> Am 26.07.2022 um 15:40 schrieb Jens Axboe:
> > On 7/25/22 7:53 PM, Michael Schmitz wrote:
> >> Hi Jens,
> >> 
> >> there's been quite a bit of review on this patch series back in the
> >> day (most of that would have been on linux-m68k IIRC; see Geert's
> >> Reviewed-By tag), and I addressed the issues raised but as you say,
> >> it did never get merged.
> >> 
> >> I've found a copy of the linux-block repo that has these patches,
> >> will see if I can get them updated to apply to current
> >> linux-block.> 
> > Thanks, please do resend them and we can get them applied.
> 
> Will do - running final compile tests.

Just reminding. Did this go in meanwhile?

Thanks,

Michael Schmitz Aug. 22, 2022, 5:46 a.m. UTC | #7

Martin,

Am 22.08.2022 um 08:59 schrieb Martin Steigerwald:
> Michael Schmitz - 26.07.22, 05:58:40 CEST:
>> Am 26.07.2022 um 15:40 schrieb Jens Axboe:
>>> On 7/25/22 7:53 PM, Michael Schmitz wrote:
>>>> Hi Jens,
>>>>
>>>> there's been quite a bit of review on this patch series back in the
>>>> day (most of that would have been on linux-m68k IIRC; see Geert's
>>>> Reviewed-By tag), and I addressed the issues raised but as you say,
>>>> it did never get merged.
>>>>
>>>> I've found a copy of the linux-block repo that has these patches,
>>>> will see if I can get them updated to apply to current
>>>> linux-block.>
>>> Thanks, please do resend them and we can get them applied.
>>
>> Will do - running final compile tests.
>
> Just reminding. Did this go in meanwhile?

Not yet, at last count.

Christoph had reviewed v8 and I've addressed his comments in v9. Should 
have added his Reviewed-by tag for both patches perhaps, not just patch 1.

Shall I resend v9 with that omission rectified, Jens?

Cheers,

	Michael

>
> Thanks,
>

Jens Axboe Aug. 22, 2022, 1:57 p.m. UTC | #8

On 8/21/22 5:46 AM, Michael Schmitz wrote:
> Martin,
> 
> Am 22.08.2022 um 08:59 schrieb Martin Steigerwald:
>> Michael Schmitz - 26.07.22, 05:58:40 CEST:
>>> Am 26.07.2022 um 15:40 schrieb Jens Axboe:
>>>> On 7/25/22 7:53 PM, Michael Schmitz wrote:
>>>>> Hi Jens,
>>>>>
>>>>> there's been quite a bit of review on this patch series back in the
>>>>> day (most of that would have been on linux-m68k IIRC; see Geert's
>>>>> Reviewed-By tag), and I addressed the issues raised but as you say,
>>>>> it did never get merged.
>>>>>
>>>>> I've found a copy of the linux-block repo that has these patches,
>>>>> will see if I can get them updated to apply to current
>>>>> linux-block.>
>>>> Thanks, please do resend them and we can get them applied.
>>>
>>> Will do - running final compile tests.
>>
>> Just reminding. Did this go in meanwhile?
> 
> Not yet, at last count.
> 
> Christoph had reviewed v8 and I've addressed his comments in v9.
> Should have added his Reviewed-by tag for both patches perhaps, not
> just patch 1.
> 
> Shall I resend v9 with that omission rectified, Jens?

Yes please, thanks.

Michael Schmitz Aug. 22, 2022, 8:39 p.m. UTC | #9

Hi Jens,

will do - just waiting to hear back what needs to be done regarding 
backporting issues raised by Geert.

Cheers,

     Michael

On 23/08/22 01:57, Jens Axboe wrote:
> On 8/21/22 5:46 AM, Michael Schmitz wrote:
>> Martin,
>>
>> Am 22.08.2022 um 08:59 schrieb Martin Steigerwald:
>>> Michael Schmitz - 26.07.22, 05:58:40 CEST:
>>>> Am 26.07.2022 um 15:40 schrieb Jens Axboe:
>>>>> On 7/25/22 7:53 PM, Michael Schmitz wrote:
>>>>>> Hi Jens,
>>>>>>
>>>>>> there's been quite a bit of review on this patch series back in the
>>>>>> day (most of that would have been on linux-m68k IIRC; see Geert's
>>>>>> Reviewed-By tag), and I addressed the issues raised but as you say,
>>>>>> it did never get merged.
>>>>>>
>>>>>> I've found a copy of the linux-block repo that has these patches,
>>>>>> will see if I can get them updated to apply to current
>>>>>> linux-block.>
>>>>> Thanks, please do resend them and we can get them applied.
>>>> Will do - running final compile tests.
>>> Just reminding. Did this go in meanwhile?
>> Not yet, at last count.
>>
>> Christoph had reviewed v8 and I've addressed his comments in v9.
>> Should have added his Reviewed-by tag for both patches perhaps, not
>> just patch 1.
>>
>> Shall I resend v9 with that omission rectified, Jens?
> Yes please, thanks.
>

Jens Axboe Aug. 22, 2022, 8:41 p.m. UTC | #10

On 8/22/22 2:39 PM, Michael Schmitz wrote:
> Hi Jens,
> 
> will do - just waiting to hear back what needs to be done regarding
> backporting issues raised by Geert.

It needs to go upstream first before it can go to stable. Just mark it
with the right Fixes tags and that will happen automatically.

Michael Schmitz Aug. 22, 2022, 8:56 p.m. UTC | #11

Hi Jens,

thanks - the Fixes tag in my patches refers to Martin's bug report and 
won't be useful to decide how far back this must be applied.

Now the bug pre-dates git, making the commit to 'fix' 
1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 ... That one's a bit special, 
please yell if you want me to lie about this and use a later commit 
specific to the partition parser code.

Cheers,

     Michael


On 23/08/22 08:41, Jens Axboe wrote:
> On 8/22/22 2:39 PM, Michael Schmitz wrote:
>> Hi Jens,
>>
>> will do - just waiting to hear back what needs to be done regarding
>> backporting issues raised by Geert.
> It needs to go upstream first before it can go to stable. Just mark it
> with the right Fixes tags and that will happen automatically.
>

Martin Steigerwald June 13, 2023, 7:25 a.m. UTC | #12

Hi Michael, hi Jens, Hi Geert.

Michael Schmitz - 22.08.22, 22:56:10 CEST:
> On 23/08/22 08:41, Jens Axboe wrote:
> > On 8/22/22 2:39 PM, Michael Schmitz wrote:
> >> Hi Jens,
> >> 
> >> will do - just waiting to hear back what needs to be done regarding
> >> backporting issues raised by Geert.
> > 
> > It needs to go upstream first before it can go to stable. Just mark
> > it with the right Fixes tags and that will happen automatically.
[…]
> thanks - the Fixes tag in my patches refers to Martin's bug report and
> won't be useful to decide how far back this must be applied.
> 
> Now the bug pre-dates git, making the commit to 'fix'
> 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 ... That one's a bit special,
> please yell if you want me to lie about this and use a later commit
> specific to the partition parser code.

After this discussion happened I thought the patch went in. However… as 
John Paul Adrian asked in "Status of affs support in the kernel and 
affstools" thread on linux-m68k and debian-68k mailing list, I searched 
for the patch in git history but did not find it.

So did it go in meanwhile?

Ciao,

Michael Schmitz June 13, 2023, 8:18 a.m. UTC | #13

Hi Martin,

Am 13.06.2023 um 19:25 schrieb Martin Steigerwald:
> Hi Michael, hi Jens, Hi Geert.
>
> Michael Schmitz - 22.08.22, 22:56:10 CEST:
>> On 23/08/22 08:41, Jens Axboe wrote:
>>> On 8/22/22 2:39 PM, Michael Schmitz wrote:
>>>> Hi Jens,
>>>>
>>>> will do - just waiting to hear back what needs to be done regarding
>>>> backporting issues raised by Geert.
>>>
>>> It needs to go upstream first before it can go to stable. Just mark
>>> it with the right Fixes tags and that will happen automatically.
> […]
>> thanks - the Fixes tag in my patches refers to Martin's bug report and
>> won't be useful to decide how far back this must be applied.
>>
>> Now the bug pre-dates git, making the commit to 'fix'
>> 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 ... That one's a bit special,
>> please yell if you want me to lie about this and use a later commit
>> specific to the partition parser code.
>
> After this discussion happened I thought the patch went in. However… as
> John Paul Adrian asked in "Status of affs support in the kernel and
> affstools" thread on linux-m68k and debian-68k mailing list, I searched
> for the patch in git history but did not find it.

I may have messed that one up, as it turns out. Last version was v9 
which I had to resend twice, and depending on what Jens uses to keep 
track of patches, the resends may not have shown up in his tool. I 
should have bumped the version number instead.

I'll see if my latest version still applies cleanly ...

Cheers,

	Michael


> So did it go in meanwhile?
>
> Ciao,
>

Martin Steigerwald June 13, 2023, 10:57 a.m. UTC | #14

Michael Schmitz - 13.06.23, 10:18:24 CEST:
> Am 13.06.2023 um 19:25 schrieb Martin Steigerwald:
> > Hi Michael, hi Jens, Hi Geert.
> > 
> > Michael Schmitz - 22.08.22, 22:56:10 CEST:
> >> On 23/08/22 08:41, Jens Axboe wrote:
> >>> On 8/22/22 2:39 PM, Michael Schmitz wrote:
> >>>> Hi Jens,
> >>>> 
> >>>> will do - just waiting to hear back what needs to be done
> >>>> regarding
> >>>> backporting issues raised by Geert.
> >>> 
> >>> It needs to go upstream first before it can go to stable. Just
> >>> mark
> >>> it with the right Fixes tags and that will happen automatically.
> > 
> > […]
> > 
> >> thanks - the Fixes tag in my patches refers to Martin's bug report
> >> and won't be useful to decide how far back this must be applied.
> >> 
> >> Now the bug pre-dates git, making the commit to 'fix'
> >> 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 ... That one's a bit
> >> special, please yell if you want me to lie about this and use a
> >> later commit specific to the partition parser code.
> > 
> > After this discussion happened I thought the patch went in. However…
> > as John Paul Adrian asked in "Status of affs support in the kernel
> > and affstools" thread on linux-m68k and debian-68k mailing list, I
> > searched for the patch in git history but did not find it.
> 
> I may have messed that one up, as it turns out. Last version was v9
> which I had to resend twice, and depending on what Jens uses to keep
> track of patches, the resends may not have shown up in his tool. I
> should have bumped the version number instead.
> 
> I'll see if my latest version still applies cleanly ...

Many thanks!

Would be nice to see it finally go in.

Best,

Michael Schmitz June 13, 2023, 10:11 p.m. UTC | #15

Hi Martin,

On 13/06/23 22:57, Martin Steigerwald wrote:
> Michael Schmitz - 13.06.23, 10:18:24 CEST:
>> Am 13.06.2023 um 19:25 schrieb Martin Steigerwald:
>>> Hi Michael, hi Jens, Hi Geert.
>>>
>>> Michael Schmitz - 22.08.22, 22:56:10 CEST:
>>>> On 23/08/22 08:41, Jens Axboe wrote:
>>>>> On 8/22/22 2:39 PM, Michael Schmitz wrote:
>>>>>> Hi Jens,
>>>>>>
>>>>>> will do - just waiting to hear back what needs to be done
>>>>>> regarding
>>>>>> backporting issues raised by Geert.
>>>>> It needs to go upstream first before it can go to stable. Just
>>>>> mark
>>>>> it with the right Fixes tags and that will happen automatically.
>>> […]
>>>
>>>> thanks - the Fixes tag in my patches refers to Martin's bug report
>>>> and won't be useful to decide how far back this must be applied.
>>>>
>>>> Now the bug pre-dates git, making the commit to 'fix'
>>>> 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 ... That one's a bit
>>>> special, please yell if you want me to lie about this and use a
>>>> later commit specific to the partition parser code.
>>> After this discussion happened I thought the patch went in. However…
>>> as John Paul Adrian asked in "Status of affs support in the kernel
>>> and affstools" thread on linux-m68k and debian-68k mailing list, I
>>> searched for the patch in git history but did not find it.
>> I may have messed that one up, as it turns out. Last version was v9
>> which I had to resend twice, and depending on what Jens uses to keep
>> track of patches, the resends may not have shown up in his tool. I
>> should have bumped the version number instead.
>>
>> I'll see if my latest version still applies cleanly ...
> Many thanks!
>
> Would be nice to see it finally go in.
My last version (v9) still applies, but that one still threw a sparse 
warning for patch 2:

Link:https://lore.kernel.org/all/[e-mail]

Not sure how to treat that one - rdb_CylBlocks is not declared as big 
endian so the warning is correct, but as far as I can see, for all 
practical purposes rdb_CylBlocks would be expected to be in big endian 
order (partition usually prepared on a big endian system)?

I can drop the be32_to_cpu conversion (and would expect to see a warning 
printed on little endian systems), or force the cast to __be32. Or 
rather drop that consistency check outright...

Cheers,

     Michael

>
> Best,

Finn Thain June 14, 2023, 12:07 a.m. UTC | #16

On Wed, 14 Jun 2023, Michael Schmitz wrote:

> My last version (v9) still applies, but that one still threw a sparse 
> warning for patch 2:
> 
> Link:https://lore.kernel.org/all/[e-mail]
> 
> Not sure how to treat that one - rdb_CylBlocks is not declared as big 
> endian so the warning is correct, but as far as I can see, for all 
> practical purposes rdb_CylBlocks would be expected to be in big endian 
> order (partition usually prepared on a big endian system)?
> 
> I can drop the be32_to_cpu conversion (and would expect to see a warning 
> printed on little endian systems), or force the cast to __be32. Or 
> rather drop that consistency check outright...
> 

The new warning comes from this new code:

		if (cylblk > be32_to_cpu((__be32)rdb->rdb_CylBlocks)) {
			pr_warn("Dev %s: cylblk %u > rdb_CylBlocks %u!\n",
				state->disk->disk_name, cylblk,
				be32_to_cpu(rdb->rdb_CylBlocks));
		}

The __be32 cast appears in the first line but not the fourth, which is an 
inconsistency you might want to tidy up. However, both lines produce the 
same sparse warning here.

The inconsistent use of big-endian and native-endian members in struct 
RigidDiskBlock looks like the root cause to me but I know nothing about 
Amiga partition maps so I'm not going to guess.

Michael Schmitz June 14, 2023, 1:20 a.m. UTC | #17

Hi Finn,

On 14/06/23 12:07, Finn Thain wrote:
> On Wed, 14 Jun 2023, Michael Schmitz wrote:
>
>> My last version (v9) still applies, but that one still threw a sparse
>> warning for patch 2:
>>
>> Link:https://lore.kernel.org/all/[e-mail]
>>
>> Not sure how to treat that one - rdb_CylBlocks is not declared as big
>> endian so the warning is correct, but as far as I can see, for all
>> practical purposes rdb_CylBlocks would be expected to be in big endian
>> order (partition usually prepared on a big endian system)?
>>
>> I can drop the be32_to_cpu conversion (and would expect to see a warning
>> printed on little endian systems), or force the cast to __be32. Or
>> rather drop that consistency check outright...
>>
> The new warning comes from this new code:
>
> 		if (cylblk > be32_to_cpu((__be32)rdb->rdb_CylBlocks)) {
> 			pr_warn("Dev %s: cylblk %u > rdb_CylBlocks %u!\n",
> 				state->disk->disk_name, cylblk,
> 				be32_to_cpu(rdb->rdb_CylBlocks));
> 		}
>
> The __be32 cast appears in the first line but not the fourth, which is an
> inconsistency you might want to tidy up. However, both lines produce the
> same sparse warning here.

Thanks for checking that - the cast is redundant as-is (be32_to_cpu() 
contains the same cast already). Does use of (__force __be32) instead 
make the warning go away? (I haven't managed to get sparse working for 
me, so I have no way of checking.)

> The inconsistent use of big-endian and native-endian members in struct
> RigidDiskBlock looks like the root cause to me but I know nothing about
> Amiga partition maps so I'm not going to guess.

The check appeared in the first version of the patch, after discussion 
around the RFC version at length. Going over that thread again, I 
haven't found why that check was added. It's probably been out of an 
overabundance of caution (as I know little about RDB, too) and can 
probably be removed.

Cheers,

     Michael

Martin Steigerwald June 14, 2023, 7:19 a.m. UTC | #18

Hi Michael, hi Joanne.

@Joanne: I am cc'ing you in this patch as I bet you might be able to 
confirm whether the rdb_CylBlocks value in Amiga RDB is big endian. I 
hope you do not mind. I would assume that everything in Amiga RDB is big 
endian, but I bet you know for certain.

Michael Schmitz - 14.06.23, 00:11:45 CEST:
> On 13/06/23 22:57, Martin Steigerwald wrote:
> > Michael Schmitz - 13.06.23, 10:18:24 CEST:
> >> Am 13.06.2023 um 19:25 schrieb Martin Steigerwald:
> >>> Hi Michael, hi Jens, Hi Geert.
> >>> 
> >>> Michael Schmitz - 22.08.22, 22:56:10 CEST:
> >>>> On 23/08/22 08:41, Jens Axboe wrote:
> >>>>> On 8/22/22 2:39 PM, Michael Schmitz wrote:
> >>>>>> Hi Jens,
> >>>>>> 
> >>>>>> will do - just waiting to hear back what needs to be done
> >>>>>> regarding
> >>>>>> backporting issues raised by Geert.
> >>>>> 
> >>>>> It needs to go upstream first before it can go to stable. Just
> >>>>> mark
> >>>>> it with the right Fixes tags and that will happen automatically.
> >>> 
> >>> […]
> >>> 
> >>>> thanks - the Fixes tag in my patches refers to Martin's bug
> >>>> report
> >>>> and won't be useful to decide how far back this must be applied.
> >>>> 
> >>>> Now the bug pre-dates git, making the commit to 'fix'
> >>>> 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 ... That one's a bit
> >>>> special, please yell if you want me to lie about this and use a
> >>>> later commit specific to the partition parser code.
> >>> 
> >>> After this discussion happened I thought the patch went in.
> >>> However…
> >>> as John Paul Adrian asked in "Status of affs support in the kernel
> >>> and affstools" thread on linux-m68k and debian-68k mailing list, I
> >>> searched for the patch in git history but did not find it.
> >> 
> >> I may have messed that one up, as it turns out. Last version was v9
> >> which I had to resend twice, and depending on what Jens uses to
> >> keep
> >> track of patches, the resends may not have shown up in his tool. I
> >> should have bumped the version number instead.
> >> 
> >> I'll see if my latest version still applies cleanly ...
> > 
> > Many thanks!
> > 
> > Would be nice to see it finally go in.
> 
> My last version (v9) still applies, but that one still threw a sparse
> warning for patch 2:
> 
> Link:https://lore.kernel.org/all/[e-mail]
> 
> Not sure how to treat that one - rdb_CylBlocks is not declared as big
> endian so the warning is correct, but as far as I can see, for all
> practical purposes rdb_CylBlocks would be expected to be in big endian
> order (partition usually prepared on a big endian system)?

While I did not specifically check myself I would be highly surprised in 
case anything in RDB data structure would be little endian. Amiga is a 
big endian platform. I'd expect little endian only to be used where 
there was need to interface with little endian platforms – like in PC 
emulators.

It may not be easy to find any confirmation in developer documentation, 
I'd assume that wherever it would not have been stated differently, big 
endian is assumed for AmigaOS.

> I can drop the be32_to_cpu conversion (and would expect to see a
> warning printed on little endian systems), or force the cast to
> __be32. Or rather drop that consistency check outright...

So "be32_to_cpu" converts big to little endian in case the CPU 
architecture Linux runs on is little endian?

Well again… I'd say it is safe to assume that anything in Amiga RDB is 
big endian.

Best,

Michael Schmitz June 14, 2023, 8:43 a.m. UTC | #19

Hi Martin,

Am 14.06.2023 um 19:19 schrieb Martin Steigerwald:
> Hi Michael, hi Joanne.
>
> @Joanne: I am cc'ing you in this patch as I bet you might be able to
> confirm whether the rdb_CylBlocks value in Amiga RDB is big endian. I
> hope you do not mind. I would assume that everything in Amiga RDB is big
> endian, but I bet you know for certain.
>
> Michael Schmitz - 14.06.23, 00:11:45 CEST:
>> On 13/06/23 22:57, Martin Steigerwald wrote:
>>> Michael Schmitz - 13.06.23, 10:18:24 CEST:
>>>> Am 13.06.2023 um 19:25 schrieb Martin Steigerwald:
>>>>> Hi Michael, hi Jens, Hi Geert.
>>>>>
>>>>> Michael Schmitz - 22.08.22, 22:56:10 CEST:
>>>>>> On 23/08/22 08:41, Jens Axboe wrote:
>>>>>>> On 8/22/22 2:39 PM, Michael Schmitz wrote:
>>>>>>>> Hi Jens,
>>>>>>>>
>>>>>>>> will do - just waiting to hear back what needs to be done
>>>>>>>> regarding
>>>>>>>> backporting issues raised by Geert.
>>>>>>>
>>>>>>> It needs to go upstream first before it can go to stable. Just
>>>>>>> mark
>>>>>>> it with the right Fixes tags and that will happen automatically.
>>>>>
>>>>> […]
>>>>>
>>>>>> thanks - the Fixes tag in my patches refers to Martin's bug
>>>>>> report
>>>>>> and won't be useful to decide how far back this must be applied.
>>>>>>
>>>>>> Now the bug pre-dates git, making the commit to 'fix'
>>>>>> 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 ... That one's a bit
>>>>>> special, please yell if you want me to lie about this and use a
>>>>>> later commit specific to the partition parser code.
>>>>>
>>>>> After this discussion happened I thought the patch went in.
>>>>> However…
>>>>> as John Paul Adrian asked in "Status of affs support in the kernel
>>>>> and affstools" thread on linux-m68k and debian-68k mailing list, I
>>>>> searched for the patch in git history but did not find it.
>>>>
>>>> I may have messed that one up, as it turns out. Last version was v9
>>>> which I had to resend twice, and depending on what Jens uses to
>>>> keep
>>>> track of patches, the resends may not have shown up in his tool. I
>>>> should have bumped the version number instead.
>>>>
>>>> I'll see if my latest version still applies cleanly ...
>>>
>>> Many thanks!
>>>
>>> Would be nice to see it finally go in.
>>
>> My last version (v9) still applies, but that one still threw a sparse
>> warning for patch 2:
>>
>> Link:https://lore.kernel.org/all/[e-mail]
>>
>> Not sure how to treat that one - rdb_CylBlocks is not declared as big
>> endian so the warning is correct, but as far as I can see, for all
>> practical purposes rdb_CylBlocks would be expected to be in big endian
>> order (partition usually prepared on a big endian system)?
>
> While I did not specifically check myself I would be highly surprised in
> case anything in RDB data structure would be little endian. Amiga is a
> big endian platform. I'd expect little endian only to be used where
> there was need to interface with little endian platforms – like in PC
> emulators.

That's what I thought - mind you, rdb_CylBlocks is not declared little 
endian, just __u32 which can be either.

The reason why only rdb_SummedLongs, rdb_BlockBytes and 
rdb_PartitionList are explicitly declared big endian is probably quite 
simple - nothing else was used by the Linux RDB parser. My patch adds 
rdb_CylBlocks to that list, so that ought to be changed to big endian, too.

affs_hardblocks.h is a UAPI header - what are the rules and 
ramifications around changes to those? Might not be worth the hassle in 
the end.

> It may not be easy to find any confirmation in developer documentation,
> I'd assume that wherever it would not have been stated differently, big
> endian is assumed for AmigaOS.
>
>> I can drop the be32_to_cpu conversion (and would expect to see a
>> warning printed on little endian systems), or force the cast to
>> __be32. Or rather drop that consistency check outright...
>
> So "be32_to_cpu" converts big to little endian in case the CPU
> architecture Linux runs on is little endian?

Correct - in order to use RDB partitions on little endian systems, all 
of the data used by the Linux kernel must be converted to host byte 
order before using them in calculations.

>
> Well again… I'd say it is safe to assume that anything in Amiga RDB is
> big endian.

Let's do that then. Unless someone feels strongly otherwise, I'll drop 
the rdb_CylBlocks check.

Cheers,

	Michael

>
> Best,
>

Michael Schmitz June 14, 2023, 7:46 p.m. UTC | #20

Hi Joanne,

this is about the 2 TB disk issue - 32 bit integer overflow on 
start_sect and nr_sects. The fix for that never went in.

Thanks for confirming everything in the RDB is meant to be big endian.

On 14/06/23 20:54, jdow wrote:
>
> The patch I did a LONG time ago read the RDBs correctly. I presume it 
> was removed from the kernel build and lost? (I fixed what had been 
> there because it would not mount some of my disks. RDBs permit some 
> very um "unsavory" things such as nested partitions.)
>
The other 'unsavoury' thing that worries us here is the fact that the 
number of heads and sectors (per cylinder) is defined in the partition 
block as well as in the RDB. We use the data from the partition block to 
calculate start sector and number of sectors for a partition, and my 
overflow checking path added a test that heads*sectors ==  
rdb_CylBlocks. I wonder how important that test is - do you know of any 
case where the head and sector numbers in RDB and partition blocks differ?

Thanks again,

     Michael

>  {^_^}
>
> On 20230614 01:43:32, Michael Schmitz wrote:
>> Hi Martin,
>>
>> Am 14.06.2023 um 19:19 schrieb Martin Steigerwald:
>>> Hi Michael, hi Joanne.
>>>
>>> @Joanne: I am cc'ing you in this patch as I bet you might be able to
>>> confirm whether the rdb_CylBlocks value in Amiga RDB is big endian. I
>>> hope you do not mind. I would assume that everything in Amiga RDB is 
>>> big
>>> endian, but I bet you know for certain.
>>>
>>> Michael Schmitz - 14.06.23, 00:11:45 CEST:
>>>> On 13/06/23 22:57, Martin Steigerwald wrote:
>>>>> Michael Schmitz - 13.06.23, 10:18:24 CEST:
>>>>>> Am 13.06.2023 um 19:25 schrieb Martin Steigerwald:
>>>>>>> Hi Michael, hi Jens, Hi Geert.
>>>>>>>
>>>>>>> Michael Schmitz - 22.08.22, 22:56:10 CEST:
>>>>>>>> On 23/08/22 08:41, Jens Axboe wrote:
>>>>>>>>> On 8/22/22 2:39 PM, Michael Schmitz wrote:
>>>>>>>>>> Hi Jens,
>>>>>>>>>>
>>>>>>>>>> will do - just waiting to hear back what needs to be done
>>>>>>>>>> regarding
>>>>>>>>>> backporting issues raised by Geert.
>>>>>>>>>
>>>>>>>>> It needs to go upstream first before it can go to stable. Just
>>>>>>>>> mark
>>>>>>>>> it with the right Fixes tags and that will happen automatically.
>>>>>>>
>>>>>>> […]
>>>>>>>
>>>>>>>> thanks - the Fixes tag in my patches refers to Martin's bug
>>>>>>>> report
>>>>>>>> and won't be useful to decide how far back this must be applied.
>>>>>>>>
>>>>>>>> Now the bug pre-dates git, making the commit to 'fix'
>>>>>>>> 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 ... That one's a bit
>>>>>>>> special, please yell if you want me to lie about this and use a
>>>>>>>> later commit specific to the partition parser code.
>>>>>>>
>>>>>>> After this discussion happened I thought the patch went in.
>>>>>>> However…
>>>>>>> as John Paul Adrian asked in "Status of affs support in the kernel
>>>>>>> and affstools" thread on linux-m68k and debian-68k mailing list, I
>>>>>>> searched for the patch in git history but did not find it.
>>>>>>
>>>>>> I may have messed that one up, as it turns out. Last version was v9
>>>>>> which I had to resend twice, and depending on what Jens uses to
>>>>>> keep
>>>>>> track of patches, the resends may not have shown up in his tool. I
>>>>>> should have bumped the version number instead.
>>>>>>
>>>>>> I'll see if my latest version still applies cleanly ...
>>>>>
>>>>> Many thanks!
>>>>>
>>>>> Would be nice to see it finally go in.
>>>>
>>>> My last version (v9) still applies, but that one still threw a sparse
>>>> warning for patch 2:
>>>>
>>>> Link:https://lore.kernel.org/all/[e-mail]
>>>>
>>>> Not sure how to treat that one - rdb_CylBlocks is not declared as big
>>>> endian so the warning is correct, but as far as I can see, for all
>>>> practical purposes rdb_CylBlocks would be expected to be in big endian
>>>> order (partition usually prepared on a big endian system)?
>>>
>>> While I did not specifically check myself I would be highly 
>>> surprised in
>>> case anything in RDB data structure would be little endian. Amiga is a
>>> big endian platform. I'd expect little endian only to be used where
>>> there was need to interface with little endian platforms – like in PC
>>> emulators.
>>
>> That's what I thought - mind you, rdb_CylBlocks is not declared 
>> little endian, just __u32 which can be either.
>>
>> The reason why only rdb_SummedLongs, rdb_BlockBytes and 
>> rdb_PartitionList are explicitly declared big endian is probably 
>> quite simple - nothing else was used by the Linux RDB parser. My 
>> patch adds rdb_CylBlocks to that list, so that ought to be changed to 
>> big endian, too.
>>
>> affs_hardblocks.h is a UAPI header - what are the rules and 
>> ramifications around changes to those? Might not be worth the hassle 
>> in the end.
>>
>>> It may not be easy to find any confirmation in developer documentation,
>>> I'd assume that wherever it would not have been stated differently, big
>>> endian is assumed for AmigaOS.
>>>
>>>> I can drop the be32_to_cpu conversion (and would expect to see a
>>>> warning printed on little endian systems), or force the cast to
>>>> __be32. Or rather drop that consistency check outright...
>>>
>>> So "be32_to_cpu" converts big to little endian in case the CPU
>>> architecture Linux runs on is little endian?
>>
>> Correct - in order to use RDB partitions on little endian systems, 
>> all of the data used by the Linux kernel must be converted to host 
>> byte order before using them in calculations.
>>
>>>
>>> Well again… I'd say it is safe to assume that anything in Amiga RDB is
>>> big endian.
>>
>> Let's do that then. Unless someone feels strongly otherwise, I'll 
>> drop the rdb_CylBlocks check.
>>
>> Cheers,
>>
>>     Michael
>>
>>>
>>> Best,
>>>

Finn Thain June 15, 2023, 12:13 a.m. UTC | #21

On Wed, 14 Jun 2023, Michael Schmitz wrote:

> 
> The reason why only rdb_SummedLongs, rdb_BlockBytes and 
> rdb_PartitionList are explicitly declared big endian is probably quite 
> simple - nothing else was used by the Linux RDB parser. My patch adds 
> rdb_CylBlocks to that list, so that ought to be changed to big endian, 
> too.
> 
> affs_hardblocks.h is a UAPI header - what are the rules and 
> ramifications around changes to those? Might not be worth the hassle in 
> the end.
> 

I think it's safe to fix the UAPI header if we are talking about 
endianness annotations that affect static checking and not code 
generation. The existing annotations in that struct would appear to 
support that notion, if indeed they were put there for the benefit of the 
kernel.

Michael Schmitz June 15, 2023, 1:06 a.m. UTC | #22

Hi Finn,

[resent after it bounced from vger ...]

On 15/06/23 12:13, Finn Thain wrote:
> On Wed, 14 Jun 2023, Michael Schmitz wrote:
>
>> The reason why only rdb_SummedLongs, rdb_BlockBytes and
>> rdb_PartitionList are explicitly declared big endian is probably quite
>> simple - nothing else was used by the Linux RDB parser. My patch adds
>> rdb_CylBlocks to that list, so that ought to be changed to big endian,
>> too.
>>
>> affs_hardblocks.h is a UAPI header - what are the rules and
>> ramifications around changes to those? Might not be worth the hassle in
>> the end.
>>
> I think it's safe to fix the UAPI header if we are talking about
> endianness annotations that affect static checking and not code
> generation. The existing annotations in that struct would appear to
> support that notion, if indeed they were put there for the benefit of the
> kernel.

The annotations pre-date mainstream git history - looking at Thomas 
Gleixner's full history git 
(https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/), the 
annotations were added by Al Viro in 2004 (commit 
380768f5a50524ff7eab00d03d82a56cf2fdfe7b 
<https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/commit/include/linux/affs_hardblocks.h?id=380768f5a50524ff7eab00d03d82a56cf2fdfe7b> 
there).

Al said at the time that he only annotated only those fields that are 
used by the kernel, and left everything else alone even though it all 
ought to be big endian.

Reason enough to now change the rdb_CylBlocks annotation.

Thanks,

     Michael

Christoph Hellwig June 15, 2023, 4:28 a.m. UTC | #23

On Thu, Jun 15, 2023 at 10:13:14AM +1000, Finn Thain wrote:
> > affs_hardblocks.h is a UAPI header - what are the rules and 
> > ramifications around changes to those? Might not be worth the hassle in 
> > the end.
> > 
> 
> I think it's safe to fix the UAPI header if we are talking about 
> endianness annotations that affect static checking and not code 
> generation. The existing annotations in that struct would appear to 
> support that notion, if indeed they were put there for the benefit of the 
> kernel.

More importantly there has never been any API gurantee in the UAPI
headers.  Compiling user space code absolutely may break due to changes
to them (although we avoid that if we can), we just must not change the
kernel ABI.  But as as said the __be annotations are no-ops for
compilers, and do the right thing even in userspace for projects using
sparse.

Patch

diff --git a/block/partitions/amiga.c b/block/partitions/amiga.c
index 7ea9540..d1b1535 100644
--- a/block/partitions/amiga.c
+++ b/block/partitions/amiga.c
@@ -11,11 +11,19 @@ 
 #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"
 #include "amiga.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)
 {
@@ -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;
-	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;
 	char b[BDEVNAME_SIZE];
 
@@ -44,8 +55,8 @@  int amiga_partition(struct parsed_partitions *state)
 		data = read_part_sector(state, blk, &sect);
 		if (!data) {
 			if (warn_no_part)
-				pr_err("Dev %s: unable to read RDB block %d\n",
-				       bdevname(state->bdev, b), blk);
+				pr_err("Dev %s: unable to read RDB block %llu\n",
+				       bdevname(state->bdev, b), (u64) blk);
 			res = -1;
 			goto rdb_done;
 		}
@@ -61,13 +72,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",
-		       bdevname(state->bdev, b), blk);
+		pr_err("Dev %s: RDB in block %llu has bad checksum\n",
+		       bdevname(state->bdev, b), (u64) blk);
 	}
 
 	/* blksize is blocks per 512 byte standard block */
@@ -83,12 +94,17 @@  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",
+				bdevname(state->bdev, b), (u64) blk, part);
+			break;
+		}
 		data = read_part_sector(state, blk, &sect);
 		if (!data) {
 			if (warn_no_part)
-				pr_err("Dev %s: unable to read partition block %d\n",
-				       bdevname(state->bdev, b), blk);
+				pr_err("Dev %s: unable to read partition block %llu\n",
+				       bdevname(state->bdev, b), (u64) blk);
 			res = -1;
 			goto rdb_done;
 		}
@@ -99,19 +115,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);
+			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",
+				bdevname(state->bdev, b), 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",
+				bdevname(state->bdev, b), 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 */