Denis, would you be interested? Peter Maydell <[email protected]> writes:
> Ping^2 ? > > thanks > -- PMM > > On Mon, 20 Jul 2026 at 09:42, Peter Maydell <[email protected]> wrote: >> >> Ping -- would anybody on the qemu-block side like to review this? >> >> thanks >> -- PMM >> >> On Tue, 30 Jun 2026 at 11:42, Peter Maydell <[email protected]> wrote: >> > >> > In ide_set_sector() if we're using CHS addressing the way we update >> > the drive state involves dividing by (s->heads * s->sectors). These >> > values can be set by the guest using the INITIALIZE DRIVE PARAMETERS >> > command, which means that s->sectors can be 0. (s->heads can't be 0 >> > because the command passes a heads-1 value.) INITIALIZE DRIVE >> > PARAMETERS is specified to not check the input values for validity; >> > instead no error is posted until some other command makes an illegal >> > access. So we have to cope with a 0 divisor here. >> > >> > Special-case s->sectors being zero. >> > >> > Cc: [email protected] >> > Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/2399 >> > Signed-off-by: Peter Maydell <[email protected]> >> > --- >> > We don't seem to ever actually check that the guest's read/write ops >> > are within the CHS limits it sets with INITIALIZE DRIVE PARAMETERS -- >> > ide_sect_range_ok() only checks against the underlying block device >> > geometry. Maybe we should? Then a zero-sectors misconfig would >> > result in our reporting the error on the read/write as the spec >> > suggests we should. This patch fixes the division-by-zero, at least, >> > though. >> > --- >> > hw/ide/core.c | 28 +++++++++++++++++++++------- >> > 1 file changed, 21 insertions(+), 7 deletions(-) >> > >> > diff --git a/hw/ide/core.c b/hw/ide/core.c >> > index f78b00220b..b030531534 100644 >> > --- a/hw/ide/core.c >> > +++ b/hw/ide/core.c >> > @@ -646,13 +646,27 @@ void ide_set_sector(IDEState *s, int64_t sector_num) >> > } >> > } else { >> > /* CHS */ >> > - cyl = sector_num / (s->heads * s->sectors); >> > - r = sector_num % (s->heads * s->sectors); >> > - s->hcyl = cyl >> 8; >> > - s->lcyl = cyl; >> > - s->select = (s->select & ~(ATA_DEV_HS)) | >> > - ((r / s->sectors) & (ATA_DEV_HS)); >> > - s->sector = (r % s->sectors) + 1; >> > + if (s->sectors == 0) { >> > + /* >> > + * s->sectors is under guest control via INITIALIZE DRIVE >> > + * PARAMETERS and can be 0; this is an error but can't >> > + * be reported at INITIALIZE DRIVE PARAMETERS time so we >> > + * have to cope with the bad value here. >> > + * (s->heads is always at least 1.) >> > + */ >> > + s->hcyl = 0; >> > + s->lcyl = 0; >> > + s->select = s->select & ~(ATA_DEV_HS); >> > + s->sector = 1; >> > + } else { >> > + cyl = sector_num / (s->heads * s->sectors); >> > + r = sector_num % (s->heads * s->sectors); >> > + s->hcyl = cyl >> 8; >> > + s->lcyl = cyl; >> > + s->select = (s->select & ~(ATA_DEV_HS)) | >> > + ((r / s->sectors) & (ATA_DEV_HS)); >> > + s->sector = (r % s->sectors) + 1; >> > + } >> > } >> > }
