[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2] hw/ide: check null block before _cancel_dma_sync
From: |
P J P |
Subject: |
Re: [PATCH v2] hw/ide: check null block before _cancel_dma_sync |
Date: |
Tue, 29 Sep 2020 11:52:32 +0530 (IST) |
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