Re: [PATCH v2] hw/ide: check null block before _cancel_dma_sync

[P J P]

  Hello Li,

+-- On Fri, 18 Sep 2020, Li Qiang wrote --+
| P J P <ppandit@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