Hello Li, +-- On Fri, 18 Sep 2020, Li Qiang wrote --+ | P J P <ppan...@redhat.com> 篋\x8E2020綛\xB49\xE6\x9C\x8818\xE6\x97ュ\x91\xA8篋\x94 筝\x8B\xE5\x8D\x886:26\xE5\x86\x99\xE9\x81\x93鐚\x9A | > +-- On Fri, 18 Sep 2020, Li Qiang wrote --+ | > | Update v2: use an assert() call | > | ->https://lists.nongnu.org/archive/html/qemu-devel/2020-08/msg08336.html | | In 'ide_ioport_write' the guest can set 'bus->unit' to 0 or 1 by issue | 'ATA_IOPORT_WR_DEVICE_HEAD'. So this case the guest can set the active ifs. | If the guest set this to 1. | | Then in 'idebus_active_if' will return 'IDEBus.ifs[1]' and thus the 's->blk' | will be NULL.
Right, guest does select the drive via portio_write ->ide_ioport_write case ATA_IOPORT_WR_DEVICE_HEAD: /* FIXME: HOB readback uses bit 7 */ bus->ifs[0].select = (val & ~0x10) | 0xa0; bus->ifs[1].select = (val | 0x10) | 0xa0; /* select drive */ bus->unit = (val >> 4) & 1; <== set bus->unit=0x1 break; | So from your (Peter's) saying, we need to check the value in | 'ATA_IOPORT_WR_DEVICE_HEAD' handler. To say if the guest | set a valid 'bus->unit'. This can also work I think. Yes, with the following fix, an assert(3) in ide_cancel_dma_sync fails. === diff --git a/hw/ide/core.c b/hw/ide/core.c index f76f7e5234..cb55cc8b0f 100644 --- a/hw/ide/core.c +++ b/hw/ide/core.c @@ -1300,7 +1300,11 @@ void ide_ioport_write(void *opaque, uint32_t addr, uint_) bus->ifs[0].select = (val & ~0x10) | 0xa0; bus->ifs[1].select = (val | 0x10) | 0xa0; /* select drive */ + uint8_t bu = bus->unit; bus->unit = (val >> 4) & 1; + if (!bus->ifs[bus->unit].blk) { + bus->unit = bu; + } break; default: qemu-system-x86_64: ../hw/ide/core.c:724: ide_cancel_dma_sync: Assertion `s->bus->dma->aiocb == NULL' failed. Aborted (core dumped) === | As we the 'ide_exec_cmd' and other functions in 'hw/ide/core.c' check the | 's->blk' directly. I think we just check it in 'ide_cancel_dma_sync' is | enough and also this is more consistent with the other functions. | 'ide_cancel_dma_sync' is also called by 'cmd_device_reset' which is one of | the 'ide_cmd_table' handler. Yes, I'm okay with either approach. Earlier patch v1 checks 's->blk' in ide_cancel_dma_sync(). | BTW, where is the Peter's email saying this, just want to learn something, | :). -> https://lists.nongnu.org/archive/html/qemu-devel/2020-09/msg05820.html Thank you. -- Prasad J Pandit / Red Hat Product Security Team 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D