+-- On Tue, 29 Sep 2020, Li Qiang wrote --+ | P J P <ppan...@redhat.com> 于2020年9月29日周二 下午2:22写道: | > +-- On Fri, 18 Sep 2020, Li Qiang wrote --+ | > | P J P <ppan...@redhat.com> 于2020年9月18日周五 下午6:26写道: | > | > +-- 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) | | This is what I am worried, in the 'ide_ioport_write' set the 'bus->unit'. It | also change the 'buf->ifs[0].select'. Also there maybe some other corner | case that causes some inconsistent. And if we choice this method we need to | deep into the more ahci-spec to know how things really going. | | > === | > | > | 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(). | | I prefer the 'check the s->blk in the beginning of ide_cancel_dma_sync' method. | Some little different with your earlier patch. | | Anyway, let the maintainer do the choices. |
@John ...wdyt? Thank you. -- Prasad J Pandit / Red Hat Product Security Team 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D