| Message ID | 20230704054955.16906-1-schmitzmic@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] block: bugfix for Amiga partition overflow check patch | expand |
On Tue, Jul 04, 2023 at 05:49:55PM +1200, Michael Schmitz 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. > > 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 > go on to create a filesystem with a size exceeding the > actual partition size. Now that the 32 bit overflow has been > 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. > > Reported-by: Christian Zigotzky <chzigotzky@xenosoft.de> > Fixes: b6f3f28f60 ("block: add overflow checks for Amiga partition support") That commit is not in: > Cc: <stable@vger.kernel.org> # 6.4 6.4. It's in Linus's tree only right now. But yes, it's tagged for 5.2 and older kernels to be added to the stable tree, so why is this one limited only to 6.4 and not also for 5.2 and newer? thanks, greg k-h
Hi MIchael, Thanks for your patch! On Tue, Jul 4, 2023 at 7:50 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. 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@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> # 6.4 > Link: https://lore.kernel.org/r/024ce4fa-cc6d-50a2-9aae-3701d0ebf668@xenosoft.de > Signed-off-by: Michael Schmitz <schmitzmic@gmail.com> > Tested-by: Christian Zigotzky <chzigotzky@xenosoft.de> > > -- > > 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@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
Michael Schmitz - 04.07.23, 07:49:55 CEST: > 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. > > 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 > go on to create a filesystem with a size exceeding the > actual partition size. Now that the 32 bit overflow has been > 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. > > 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> # 6.4 > Link: https://lore.kernel.org/r/024ce4fa-cc6d-50a2-9aae-3701d0ebf668@xenosoft.de > Signed-off-by: Michael Schmitz <schmitzmic@gmail.com> > Tested-by: Christian Zigotzky <chzigotzky@xenosoft.de> > > -- > > 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 > --- > 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", > Looks good. I do not consider myself a kernel developer, but patch description and patch itself make sense to me. Reviewed-By: Martin Steigerwald <martin@lichtvoll.de> Ciao,
Hi Geert, On 4/07/23 19:20, Geert Uytterhoeven wrote: > Hi MIchael, > > Thanks for your patch! > > On Tue, Jul 4, 2023 at 7:50 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. > 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. Yes, I realize that. I had hoped that by way of the Fixes: tag, people would be able to relate that comment to the correct commit. Might be a little circuitous ... > 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. Does lore pick up related patches through the In-Reply-To header? In that case it would be easiest for me to to put this comment in a cover letter to the bugfix patch. > >> 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? No, it was incorrect (though valid). On a 2 TB disk, a partition of 1.3 TB at the end of the disk (but not extending to the very end!) would trigger a overflow in the size calculation: sda: p4 size 18446744071956107760 extends beyond EOD, That's only noted somewhere inside put_partition. The effective partition size seen by the kernel and user tools is then that of a partition extending to EOD (in Christian's case a full 8 GB more than recorded in the partition table). >> 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? mkfs used what the old kernel code gave as partition size. That did 'seem' correct at that time, but after the overflow fixes (which prevent other partition miscalculations, which in Martin's case caused partitions to overlap), the partitions size is actually correct and smaller than the filesystem size. I have a hunch I don't explain myself very well. > >> 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? Yes, that's the easiest way to do it, but we don't yet know if gparted (for example) does allow to do that. Mucking around with hexedit (which is what I used to verify this change gives identical partition sizes for old and new kernels) isn't to everyone's taste. I haven't looked at amiga-fdisk - that one might be easiest to fix. Cheers, Michael > >> 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> # 6.4 >> Link: https://lore.kernel.org/r/024ce4fa-cc6d-50a2-9aae-3701d0ebf668@xenosoft.de >> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com> >> Tested-by: Christian Zigotzky <chzigotzky@xenosoft.de> >> >> -- >> >> 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@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
Hi Greg, On 4/07/23 18:54, Greg KH wrote: > On Tue, Jul 04, 2023 at 05:49:55PM +1200, Michael Schmitz 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. >> >> 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 >> go on to create a filesystem with a size exceeding the >> actual partition size. Now that the 32 bit overflow has been >> 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. >> >> Reported-by: Christian Zigotzky <chzigotzky@xenosoft.de> >> Fixes: b6f3f28f60 ("block: add overflow checks for Amiga partition support") > That commit is not in: > >> Cc: <stable@vger.kernel.org> # 6.4 > 6.4. It's in Linus's tree only right now. Sigh ... I should have followed that tree also. I had wondered why the patches hadn't shown up in Geert's tree yet. > > But yes, it's tagged for 5.2 and older kernels to be added to the stable > tree, so why is this one limited only to 6.4 and not also for 5.2 and > newer? Brain fade on my part, same day (and situation) as the botched Fixes: tag, sorry. I'll correct that, along with Geert's comment regarding the commit description. Cheers, Michael > > thanks, > > greg k-h
Hi Michael, On Tue, Jul 4, 2023 at 9:30 PM Michael Schmitz <schmitzmic@gmail.com> wrote: > On 4/07/23 19:20, Geert Uytterhoeven wrote: > > On Tue, Jul 4, 2023 at 7:50 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. > > 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. > > Yes, I realize that. I had hoped that by way of the Fixes: tag, people > would be able to relate that comment to the correct commit. Might be a > little circuitous ... > > > 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. > > Does lore pick up related patches through the In-Reply-To header? In > that case it would be easiest for me to to put this comment in a cover > letter to the bugfix patch. Lore does not do that (b4 (the tool to download patch series from lore) usually can link a series to its previous version, though). New replies sent to a patch submission do end up in the right thread, so any later comments (bug reports, Reviewed/Tested-by tags, ...) can be found easily by following the Link: tag in the commit. > >> 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? > > No, it was incorrect (though valid). > > On a 2 TB disk, a partition of 1.3 TB at the end of the disk (but not > extending to the very end!) would trigger a overflow in the size > calculation: > > sda: p4 size 18446744071956107760 extends beyond EOD, Oh, so they were not "truncated to the end of the disk"? > That's only noted somewhere inside put_partition. The effective > partition size seen by the kernel and user tools is then that of a > partition extending to EOD (in Christian's case a full 8 GB more than > recorded in the partition table). > > >> 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? > > mkfs used what the old kernel code gave as partition size. That did > 'seem' correct at that time, but after the overflow fixes (which prevent > other partition miscalculations, which in Martin's case caused > partitions to overlap), the partitions size is actually correct and > smaller than the filesystem size. > > I have a hunch I don't explain myself very well. ;-) Gr{oetje,eeting}s, Geert
Hi Geert, Am 05.07.2023 um 19:24 schrieb Geert Uytterhoeven: >>> 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. >> >> Does lore pick up related patches through the In-Reply-To header? In >> that case it would be easiest for me to to put this comment in a cover >> letter to the bugfix patch. > > Lore does not do that (b4 (the tool to download patch series from lore) > usually can link a series to its previous version, though). > New replies sent to a patch submission do end up in the right thread, > so any later comments (bug reports, Reviewed/Tested-by tags, ...) > can be found easily by following the Link: tag in the commit. OK, that's good enough for me. >>>> 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? >> >> No, it was incorrect (though valid). >> >> On a 2 TB disk, a partition of 1.3 TB at the end of the disk (but not >> extending to the very end!) would trigger a overflow in the size >> calculation: >> >> sda: p4 size 18446744071956107760 extends beyond EOD, > > Oh, so they were not "truncated to the end of the disk"? Not by the RDB parser, but truncation happens ultimately. I should have copied the second instance of that message from Christian's log: sda: p4 size 18446744071956107760 extends beyond EOD, truncated The core partition code later sanity checks the partition data and truncates (see block/partitions/core.c:blk_add_partition()) Cheers, Michael
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",
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. 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 go on to create a filesystem with a size exceeding the actual partition size. Now that the 32 bit overflow has been 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. 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> # 6.4 Link: https://lore.kernel.org/r/024ce4fa-cc6d-50a2-9aae-3701d0ebf668@xenosoft.de Signed-off-by: Michael Schmitz <schmitzmic@gmail.com> Tested-by: Christian Zigotzky <chzigotzky@xenosoft.de> -- 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 --- block/partitions/amiga.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)