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",
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
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
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
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
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?
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
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.
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
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?
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
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.
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=7eb1e47696aa231b...
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!
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=7eb1e47696aa231b...
which has the following manual edits:
- 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.
- Break lines at 72-74 chars, yours were very short.
Somehow a limit of 60 had stuck in my mind.
Drop message-id
Drop cc stable tag
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
linux-stable-mirror@lists.linaro.org
-
Geert Uytterhoeven -
Jens Axboe -
Martin Steigerwald -
Michael Schmitz