Re: [PATCH 01/15] hw/ide: reject an unsupported CHS translation

On 17/8/26 22:52, Denis V. Lunev wrote:
> From: Denis V. Lunev <[email protected]>
> 
> ide_set_sector() divides by (s->heads * s->sectors) when the drive is
> addressed in CHS mode. Both come from the guest via INITIALIZE DEVICE
> PARAMETERS, and cmd_specify() stored them without any check, so a guest
> asking for zero sectors per logical track killed QEMU with SIGFPE on the
> completion of the first CHS read or write. s->heads is safe, as the
> command passes a heads-1 value.
> 
> The count has an upper bound as well. The legacy sector count register is
> eight bits wide, but handle_cmd() takes the count from a 16 bit field of
> the register FIS, so an AHCI guest can ask for up to 65535 sectors per
> track, and the CHS branch of ide_get_sector() then overflows the int it
> multiplies cylinder, heads and sectors in.
> 
> ATA-5 6.2 numbers CHS sectors from one and ATA-2 D.2.8 limits IDENTIFY
> DEVICE word 56 to 1 through 255, so neither end is a translation a device
> may accept. ATA-5 8.16.6 requires an unsupported one to be reported as an
> aborted command: do that, leave the translation in effect alone, and
> refuse the value rather than checking it at every use.
> 
> Cc: John Snow <[email protected]>
> Cc: Peter Maydell <[email protected]>
> Fixes: 176e4961bb33 ("hw/ide/core.c: Implement ATA INITIALIZE_DEVICE_PARAMETERS command")
> Reported-by: Zheyu Ma <[email protected]>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2399
> Signed-off-by: Denis V. Lunev <[email protected]>
> ---
>   hw/ide/core.c | 17 ++++++++++++-----
>   1 file changed, 12 insertions(+), 5 deletions(-)

Cc: [email protected]
Reviewed-by: Philippe Mathieu-Daudé <[email protected]>