Hi MIchael,
Thanks for your patch!
On Tue, Jul 4, 2023 at 7:50 AM Michael Schmitz <schmitzmic@xxxxxxxxx> 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.
That's the explanation for what this patch does.
The below is not directly related to that, so IMHO it does not
belong in the description of this patch.
We do not really have a way to record comments in git history
after the fact. The best you can do is to reply to the email thread
where the patch was submitted. When people follow the Link:
tag to the lore archive in the original commit, they can read any follow-ups.
> Testing by Christian also exposed another aspect of the old
> bug fixed in commits fc3d092c6b ("block: fix signed int
> overflow in Amiga partition support") and b6f3f28f60
> ("block: add overflow checks for Amiga partition support"):
>
> Partitions that did overflow the disk size (due to 32 bit int
> overflow) were not skipped but truncated to the end of the
> disk. Users who missed the warning message during boot would
I am confused. So before, the partition size as seen by Linux after
the truncation, was correct?
> go on to create a filesystem with a size exceeding the
> actual partition size. Now that the 32 bit overflow has been
But if Linux did see the correct size, mkfs would have used the correct
size, too, and the size in the recorded file system should be correct?
> corrected, such filesystems may refuse to mount with a
> 'filesystem exceeds partition size' error. Users should
> either correct the partition size, or resize the filesystem
> before attempting to boot a kernel with the RDB fixes in
> place.
Hence there is no need to resize the file system, just to fix the
partition size in the RDB?
> Reported-by: Christian Zigotzky <chzigotzky@xxxxxxxxxxx>
> Fixes: b6f3f28f60 ("block: add overflow checks for Amiga partition support")
> Message-ID: 024ce4fa-cc6d-50a2-9aae-3701d0ebf668@xxxxxxxxxxx
> Cc: <stable@xxxxxxxxxxxxxxx> # 6.4
> Link: https://lore.kernel.org/r/024ce4fa-cc6d-50a2-9aae-3701d0ebf668@xxxxxxxxxxx
> Signed-off-by: Michael Schmitz <schmitzmic@xxxxxxxxx>
> Tested-by: Christian Zigotzky <chzigotzky@xxxxxxxxxxx>
>
> --
>
> Changes since v2:
>
> Adrian Glaubitz:
> - fix typo in commit message
>
> Changes since v1:
>
> - corrected Fixes: tag
> - added Tested-by:
> - reworded commit message to describe filesystem partition
> size mismatch problem
> --- 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)) {
And this block number is supposed to be in the first 2 cylinders of
the disk, so it can never be equal or larger than 1 << 31, right?
We only really expect to see -1 here, not just any negative number.
So I think it would be safer to check against -1.
Or against U32_MAX, to avoid the cast.
> /* 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",
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
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
