Re: [PATCH] hw/ide: Don't divide by zero if guest specifies 0 sectors

Sure. Thanks.

Please let me know if we have something else stalled.

Den

On 7/30/26 15:04, Markus Armbruster wrote:
> 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;
>>>> +        }
>>>>      }
>>>>  }