Re: [PATCH v2 00/17] hw/ide: fix the logical CHS translation a guest selects

["Denis V. Lunev" <den@virtuozzo.com>]

On 8/20/26 12:08, Denis V. Lunev wrote:
> This email originated from an IP that might not be authorized by the domain it was sent from.
> Do not click links or open attachments unless it is an email you expected to receive.
> INITIALIZE DEVICE PARAMETERS lets a guest replace the logical CHS
> translation that turns the CHS registers into an LBA. cmd_specify() has
> implemented it since 176e4961bb33, released in v7.2, and the geometry it
> stores is mishandled four ways.
>
> It is not validated: zero sectors per logical track kills QEMU with SIGFPE
> in ide_set_sector(), which is issue 2399, and since handle_cmd() takes the
> count from a 16 bit field of the register FIS, an AHCI guest can drive
> ide_get_sector() into an int overflow as well. It is not reported:
> IDENTIFY DEVICE is built once and cached, so both the words describing the
> default geometry and the words describing the current one go wrong. It
> does not travel, leaving a migrated guest addressing the disk in a
> translation the destination does not have. And it does not revert on a
> hardware reset, which ATA-5 9.1 requires, so it outlives the reset of the
> machine it was selected on.
>
> The two subsections patches 10 and 11 add are the only change to the
> migration stream, and both are sent only when the guest replaced the
> default geometry. An unaffected guest migrates to an older QEMU as before;
> an affected one now fails the load on the unknown subsection instead of
> silently addressing the wrong sectors.
>
> Tested with ide-test (25 cases), ahci-test (74), a full make check, a
> clang build, and real guests on both controllers: FreeDOS reading a FAT16
> disk through INT 13h CHS, Red Hat Linux 9 whose 2.4 IDE driver prints the
> geometry it read, and Ubuntu 26.04 over libata, each with a CD and a disk,
> plus a machine reset in the middle.
>
> v1:
> https://lore.kernel.org/qemu-devel/20260817205242.1199851-1-den@openvz.org/
>
> Changes in v2
> -------------
>
>  - patch 1, patch 5: Cc qemu-stable. (Philippe)
>  - patch 6: new, the "XXX: retired, remove ?" on IDENTIFY DEVICE words 4,
>    5 and 20 is answered rather than carried. ATA-5 3.2.3.6 keeps the
>    ATA-1 meaning of a retired word that a device still fills in, so the
>    comments name that meaning; word 21 is a buffer size, not a cache
>    size. (Philippe)
>  - patch 7: new, splits the extraction of ide_identify_chs() out of what
>    is now patch 8, leaving that one with the two call sites that keep the
>    words in sync. (Philippe)
>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Cc: John Snow <jsnow@redhat.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Philippe Mathieu-Daudé <philmd@oss.qualcomm.com>
>
> Denis V. Lunev (17):
>   hw/ide: reject an unsupported CHS translation
>   tests/qtest/ide-test: cover a CHS translation with zero sectors
>   tests/qtest/libqos/ahci: allow a count and an expected error
>   tests/qtest/ahci: cover the sector count of INITIALIZE DEVICE
>     PARAMETERS
>   hw/ide: report the default CHS translation in IDENTIFY DEVICE
>   hw/ide: name the retired IDENTIFY DEVICE words the device fills in
>   hw/ide: factor out the IDENTIFY DEVICE current geometry words
>   hw/ide: keep the IDENTIFY DEVICE current geometry in sync
>   hw/ide: restore the power-on device state before loading
>   hw/ide: migrate the logical CHS translation
>   hw/ide: migrate the power-on defaults revert flag
>   tests/qtest/ide-test: cover the CHS translation across migration
>   tests/qtest/ide-test: cover a rejected CHS translation in the stream
>   tests/qtest/ide-test: cover the IDENTIFY DEVICE geometry words
>   hw/ide: revert the CHS translation on a hardware reset
>   tests/qtest/ide-test: cover the CHS translation across resets
>   hw/ide: drop a redundant interrupt from INITIALIZE DEVICE PARAMETERS
>
>  hw/ide/ahci.c             |  12 +-
>  hw/ide/cmd646.c           |   2 +-
>  hw/ide/core.c             | 147 ++++++++---
>  hw/ide/ide-internal.h     |   7 +-
>  hw/ide/isa.c              |   2 +-
>  hw/ide/macio.c            |   2 +-
>  hw/ide/mmio.c             |   2 +-
>  hw/ide/piix.c             |   2 +-
>  hw/ide/sii3112.c          |   6 +-
>  hw/ide/via.c              |   2 +-
>  tests/qtest/ahci-test.c   |  40 +++
>  tests/qtest/ide-test.c    | 499 ++++++++++++++++++++++++++++++++++++++
>  tests/qtest/libqos/ahci.c |  14 ++
>  tests/qtest/libqos/ahci.h |   6 +
>  14 files changed, 690 insertions(+), 53 deletions(-)
>
>
> base-commit: ae4f3443209ab154b48b706a146e5f557ab147cb
queued